From ecb2278f9c7970f7d29acd2f00d3e5ad5b94917f Mon Sep 17 00:00:00 2001 From: Andrei Rebegea Date: Thu, 11 Apr 2019 11:43:29 +0300 Subject: [PATCH] REPO-4309: fix the equals method for Ticket class and improve trace logging (#82) Merge from master (8.31) ACS 6.2 to ACS 6.1 support branch (8.25.N) (cherry picked from commit 2f212c9be58c78c5c9aa58d77f288949fe955485) --- .../InMemoryTicketComponentImpl.java | 127 ++++++++++++++---- .../InMemoryTicketComponentTest.java | 87 ++++++++++++ 2 files changed, 188 insertions(+), 26 deletions(-) create mode 100644 src/test/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentTest.java diff --git a/src/main/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentImpl.java b/src/main/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentImpl.java index a50002b00d..132ced6f69 100644 --- a/src/main/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentImpl.java +++ b/src/main/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentImpl.java @@ -40,6 +40,7 @@ import org.apache.commons.codec.binary.Hex; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.safehaus.uuid.UUIDGenerator; +import org.alfresco.util.ParameterCheck; /** * Store tickets in memory. They can be distributed in a cluster via the cache @@ -135,6 +136,32 @@ public class InMemoryTicketComponentImpl implements TicketComponent this.validDuration = new Duration(validDuration); } + /** + * All put operations into ticketsCache should go through this method, + * so we can debug/trace ticket problems easier from the logs + */ + private void putTicketIntoTicketsCache(Ticket ticket) + { + if (logger.isTraceEnabled()) + { + logger.trace("Putting into ticketsCache " + ticketsCache.toString() + " ticket: " + ticket); + } + ticketsCache.put(ticket.getTicketId(), ticket); + } + + /** + * All remove operations from ticketsCache should go through this method, + * so we can debug/trace ticket problems easier from the logs + */ + private void removeTicketFromTicketsCache(String ticketId) + { + if (logger.isTraceEnabled()) + { + logger.trace("Removing ticket from ticketsCache: " + ticketId); + } + ticketsCache.remove(ticketId); + } + @Override public String getNewTicket(String userName) throws AuthenticationException { @@ -152,7 +179,7 @@ public class InMemoryTicketComponentImpl implements TicketComponent expiryDate = Duration.add(new Date(), validDuration); } ticket = new Ticket(ticketsExpire ? expiryMode : ExpiryMode.DO_NOT_EXPIRE, expiryDate, userName, validDuration); - ticketsCache.put(ticket.getTicketId(), ticket); + putTicketIntoTicketsCache(ticket); } String ticketString = GRANTED_AUTHORITY_TICKET_PREFIX + ticket.getTicketId(); @@ -178,9 +205,9 @@ public class InMemoryTicketComponentImpl implements TicketComponent { if (newTicket != ticket) { - ticketsCache.put(key, newTicket); + putTicketIntoTicketsCache(newTicket); } - return ticket; + return newTicket; } } } @@ -219,11 +246,11 @@ public class InMemoryTicketComponentImpl implements TicketComponent if (oneOff) { //this feature is deprecated - ticketsCache.remove(ticketKey); + removeTicketFromTicketsCache(ticketKey); } else if (newTicket != ticket) { - ticketsCache.put(ticketKey, newTicket); + putTicketIntoTicketsCache(newTicket); } currentTicket.set(ticketString); if (logger.isTraceEnabled()) @@ -269,12 +296,8 @@ public class InMemoryTicketComponentImpl implements TicketComponent @Override public void invalidateTicketById(String ticketString) { - if (logger.isTraceEnabled()) - { - logger.trace("Invalidate ticket: " + ticketString); - } String key = ticketString.substring(GRANTED_AUTHORITY_TICKET_PREFIX.length()); - ticketsCache.remove(key); + removeTicketFromTicketsCache(key); } @Override @@ -331,6 +354,10 @@ public class InMemoryTicketComponentImpl implements TicketComponent if (!expiredOnly) { count = ticketsCache.getKeys().size(); + if (logger.isTraceEnabled()) + { + logger.trace("Clearing all tickets from the ticketsCache, that used to have size: " + count); + } ticketsCache.clear(); } else @@ -347,7 +374,7 @@ public class InMemoryTicketComponentImpl implements TicketComponent } for (String id : toRemove) { - ticketsCache.remove(id); + removeTicketFromTicketsCache(id); } } return count; @@ -374,11 +401,7 @@ public class InMemoryTicketComponentImpl implements TicketComponent for (String id : toRemove) { - ticketsCache.remove(id); - if (logger.isTraceEnabled()) - { - logger.trace("Invalidate ticket for username: " + AuthenticationUtil.maskUsername(userName) + " ticket: " + id); - } + removeTicketFromTicketsCache(id); } } @@ -432,9 +455,9 @@ public class InMemoryTicketComponentImpl implements TicketComponent private final Duration testDuration; - Ticket(ExpiryMode expires, Date expiryDate, String userName, Duration validDuration) { + checkValidTicketParameters(expires, expiryDate, userName, validDuration); this.expires = expires; this.expiryDate = expiryDate; this.userName = userName; @@ -442,7 +465,17 @@ public class InMemoryTicketComponentImpl implements TicketComponent this.testDuration = validDuration.divide(2); final String guid = UUIDGenerator.getInstance().generateRandomBasedUUID().toString(); - String encode = (expires.toString()) + ((expiryDate == null) ? new Date().toString() : expiryDate.toString()) + userName + guid; + this.ticketId = computeTicketId(expires, expiryDate, userName, guid); + + if (logger.isTraceEnabled()) + { + logger.trace("Creating new ticket for user: " + AuthenticationUtil.maskUsername(userName) + " with ticketId: " + this.ticketId); + } + } + + private static String computeTicketId(ExpiryMode expires, Date expiryDate, String userName, String guid) + { + String encode = expires.toString() + getExpireDateAsString(expiryDate) + userName + guid; MessageDigest digester; String ticketId; try @@ -473,15 +506,14 @@ public class InMemoryTicketComponentImpl implements TicketComponent ticketId = new String(Hex.encodeHex(bytes)); } } - this.ticketId = ticketId; - if (logger.isTraceEnabled()) - { - logger.trace("Creating new ticket for user:" + AuthenticationUtil.maskUsername(userName)); - } + return ticketId; } private Ticket(ExpiryMode expires, Date expiryDate, String userName, Duration validDuration, String ticketId) { + checkValidTicketParameters(expires, expiryDate, userName, validDuration); + ParameterCheck.mandatory("ticketId", ticketId); + this.expires = expires; this.expiryDate = expiryDate; this.userName = userName; @@ -489,12 +521,30 @@ public class InMemoryTicketComponentImpl implements TicketComponent Duration tenPercent = validDuration.divide(10); this.testDuration = validDuration.subtract(tenPercent); this.ticketId = ticketId; + if (logger.isTraceEnabled()) { - logger.trace("Creating new ticket for user:" + AuthenticationUtil.maskUsername(userName)); + logger.trace( + "Creating (cloning) new ticket for user: " + AuthenticationUtil.maskUsername(userName) + " with ticketId: " + this.ticketId); } } - + + private static void checkValidTicketParameters(ExpiryMode expires, Date expiryDate, String userName, Duration validDuration) + { + ParameterCheck.mandatory("expires mode", expires); + ParameterCheck.mandatoryString("userName", userName); + ParameterCheck.mandatory("validDuration", validDuration); + if (!ExpiryMode.DO_NOT_EXPIRE.equals(expires)) + { + ParameterCheck.mandatory("expiryDate", expiryDate); + } + } + + private static String getExpireDateAsString(Date expiryDate) + { + return (expiryDate == null) ? "" : expiryDate.toString(); + } + boolean hasExpired(Date now) { return ((expiryDate != null) && (expiryDate.compareTo(now) < 0)); @@ -554,7 +604,26 @@ public class InMemoryTicketComponentImpl implements TicketComponent return false; } Ticket t = (Ticket) o; - return (this.expires == t.expires) && this.expiryDate.equals(t.expiryDate) && this.userName.equals(t.userName) && this.ticketId.equals(t.ticketId); + + return this.ticketId.equals(t.ticketId) && this.userName.equals(t.userName) && this.expires.equals(t.expires) && areTheExpiryDatesEqual(t); + } + + private boolean areTheExpiryDatesEqual(Ticket t) + { + if (ExpiryMode.DO_NOT_EXPIRE.equals(this.expires)) + { + // if we have tickets that do not expire, we don't care about their expiration date + // this.expiryDate should be null + return true; + } + + // in this case this.expiryDate should not ever be null; There are checks in the constructors that validate this; + // but just in case, somehow it is null: print a warning, then fail + if (this.expiryDate == null) + { + logger.warn("expiryDate should not be null in this case"); + } + return this.expiryDate.equals(t.expiryDate); } public int hashCode() @@ -582,6 +651,12 @@ public class InMemoryTicketComponentImpl implements TicketComponent return userName; } + @Override + public String toString() + { + return "Ticket with ticketId: " + ticketId + " for user: " + AuthenticationUtil.maskUsername(userName) + " ExpiryMode: " + expires + + " expire date: " + expiryDate + " validDuration: " + validDuration; + } } @Override diff --git a/src/test/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentTest.java b/src/test/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentTest.java new file mode 100644 index 0000000000..f37f73dced --- /dev/null +++ b/src/test/java/org/alfresco/repo/security/authentication/InMemoryTicketComponentTest.java @@ -0,0 +1,87 @@ +package org.alfresco.repo.security.authentication; + +import org.alfresco.service.cmr.repository.datatype.Duration; +import org.junit.Test; + +import java.util.Date; +import java.util.HashSet; +import java.util.Set; + +import static org.junit.Assert.fail; + +/** + * This test class is not exhaustive. + * Please add here tests that are relevant for the + * org.alfresco.repo.security.authentication.InMemoryTicketComponentImpl and + * org.alfresco.repo.security.authentication.InMemoryTicketComponentImpl.Ticket classes + */ +public class InMemoryTicketComponentTest +{ + @Test + public void testTicketCollectionReadinessDoNotExpire() + { + final Duration validDuration = new Duration("PT1H"); + final InMemoryTicketComponentImpl.ExpiryMode expireMode = InMemoryTicketComponentImpl.ExpiryMode.DO_NOT_EXPIRE; + final String randomUserName = "someUserName"; + + final Date someDate = null; + + checkEqualsAndHashCode(validDuration, expireMode, someDate, randomUserName); + } + + @Test + public void testTicketCollectionReadinessAfterInactivity() + { + final Duration validDuration = new Duration("PT1H"); + final InMemoryTicketComponentImpl.ExpiryMode expireMode = InMemoryTicketComponentImpl.ExpiryMode.AFTER_INACTIVITY; + final String randomUserName = "someUserName"; + + final Date someDate = new Date(); + + checkEqualsAndHashCode(validDuration, expireMode, someDate, randomUserName); + + checkInvalidExpireDateParameter(validDuration, expireMode, randomUserName); + } + + @Test + public void testTicketCollectionReadinessAfterFixTime() + { + final Duration validDuration = new Duration("PT1H"); + final InMemoryTicketComponentImpl.ExpiryMode expireMode = InMemoryTicketComponentImpl.ExpiryMode.AFTER_FIXED_TIME; + final String randomUserName = "someUserName"; + + final Date someDate = new Date(); + + checkEqualsAndHashCode(validDuration, expireMode, someDate, randomUserName); + + checkInvalidExpireDateParameter(validDuration, expireMode, randomUserName); + } + + private void checkEqualsAndHashCode(Duration validDuration, InMemoryTicketComponentImpl.ExpiryMode expireMode, Date someDate, + String randomUserName) + { + InMemoryTicketComponentImpl.Ticket ticket1 = new InMemoryTicketComponentImpl.Ticket(expireMode, someDate, randomUserName, validDuration); + InMemoryTicketComponentImpl.Ticket ticket2 = new InMemoryTicketComponentImpl.Ticket(expireMode, someDate, randomUserName, validDuration); + ticket1.equals(ticket2); + + final Set aSet = new HashSet<>(); + for (int i = 0; i < 100000; ++i) + { + InMemoryTicketComponentImpl.Ticket ticket = new InMemoryTicketComponentImpl.Ticket(expireMode, someDate, randomUserName, validDuration); + aSet.add(ticket); + } + } + + private void checkInvalidExpireDateParameter(Duration validDuration, InMemoryTicketComponentImpl.ExpiryMode expireMode, String randomUserName) + { + try + { + checkEqualsAndHashCode(validDuration, expireMode, null, randomUserName); + fail("expire date should not be allowed as null in this expireMode case"); + } + catch (IllegalArgumentException e) + { + //we expect this, we sent the date to be null + } + } +}