From 59fbf8e5629142a58ae3fbb59fc16a89413a41f0 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Wed, 3 May 2023 21:26:25 +0200 Subject: [PATCH] Minimal Fix auth code cleanup function (#2292) * fix: incorrect elapsed time determination * A recent previous refactor commit was using the `getNano()` method, thinking that it would return the total number of nanoseconds in the duration, which is does not. It returns the number of nanoseconds within one second, which is not at all what we wanted. * We decided to go ahead and just use the `getSeconds` method, which this time actually returns the total number of seconds in the duration object. We understand that previously the precision of this was to the millisecond level. We believe that it's OK if we change to the second level. The cleanup will still happen every n minutes, just not at the precise millisecond. Co-Authored-by: Bruce Ricard * update the lastClean instant and adapt tests * update the lastClean instant and adapt tests * initial setting of lastClean * fix expected value * change new lastClean to now as discussed with Bruce in weekly meeting * create mock for Duration and remove the Sleep * reduce the fix to the minimum needed * add SQL counting test * simplify test and verification run 10 times, without extra call SQL itself not needed to be verified, but check on usage of getConnection * Update UaaTokenStoreTests.java --------- Co-authored-by: Alicia Yingling Co-authored-by: Bruce Ricard --- .../identity/uaa/oauth/UaaTokenStore.java | 2 +- .../uaa/oauth/UaaTokenStoreTests.java | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStore.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStore.java index 84a609fc186..fa2fa9ffe92 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStore.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStore.java @@ -216,7 +216,7 @@ protected void performExpirationClean() { Instant now = Instant.now(); if (enoughTimeHasPassedSinceLastExpirationClean(last, now)) { //avoid concurrent deletes from the same UAA - performance improvement - if (lastClean.compareAndSet(last, last.plus(getExpirationTime()))) { + if (lastClean.compareAndSet(last, now)) { try { JdbcTemplate template = new JdbcTemplate(dataSource); int expired = template.update(SQL_EXPIRE_STATEMENT, now.toEpochMilli()); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java index fd7e0e7b064..7e5043ef8af 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java @@ -33,9 +33,11 @@ import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.PreparedStatement; +import java.sql.SQLException; import java.sql.Timestamp; import java.time.Duration; import java.time.Instant; +import java.time.temporal.Temporal; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -54,6 +56,11 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atMost; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; @WithDatabaseContext class UaaTokenStoreTests { @@ -207,6 +214,7 @@ void retrieveNonExistentToken() { @Test void cleanUpExpiredTokensBasedOnExpiresField() { int count = 10; + store = new UaaTokenStore(dataSource, givenMockedExpiration()); String lastCode = null; for (int i = 0; i < count; i++) { lastCode = store.createAuthorizationCode(clientAuthentication); @@ -224,6 +232,7 @@ void cleanUpExpiredTokensBasedOnExpiresField() { void cleanUpLegacyCodesCodesWithoutExpiresAtAfter3Days() { int count = 10; long oneday = 1000 * 60 * 60 * 24; + store = new UaaTokenStore(dataSource, givenMockedExpiration()); for (int i = 0; i < count; i++) { legacyCodeServices.createAuthorizationCode(clientAuthentication); } @@ -332,6 +341,29 @@ void beOAuth2StandardCompliant() { assertTrue(code.length() >= 32); } + @Test + void testCountingTheExecutedSqlDeleteStatements() throws SQLException { + // Given, mocked data source to count how often it is used, call performExpirationClean 10 times. + DataSource mockedDataSource = mock(DataSource.class); + Instant before = Instant.now(); + store = new UaaTokenStore(mockedDataSource); + // When + for (int i = 0; i < 10; i++) { + try { + store.performExpirationClean(); + } catch (Exception sqlException) { + // ignore + } + } + // Then + Instant after = Instant.now(); + assertTrue(after.isAfter(before)); + // Expect less than 5 minutes between the start and end of the tests + assertTrue(after.compareTo(before) < Duration.ofMinutes(5).toNanos()); + // Expect us to call the DB only once within 5 minutes. Check this when using the data source object + verify(mockedDataSource, atMost(1)).getConnection(); + } + public static class SameConnectionDataSource implements DataSource { private final Connection con; @@ -450,5 +482,11 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl } } + private static Duration givenMockedExpiration() { + Duration durationMock = mock(Duration.class); + doReturn(Instant.now().plus(UaaTokenStore.DEFAULT_EXPIRATION_TIME)).when(durationMock).addTo(any(Temporal.class)); + return durationMock; + } + private static final byte[] UAA_AUTHENTICATION_DATA_OLD_STYLE = new byte[]{123, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 114, 101, 115, 112, 111, 110, 115, 101, 84, 121, 112, 101, 115, 34, 58, 91, 93, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 114, 101, 115, 111, 117, 114, 99, 101, 73, 100, 115, 34, 58, 91, 93, 44, 34, 117, 115, 101, 114, 65, 117, 116, 104, 101, 110, 116, 105, 99, 97, 116, 105, 111, 110, 46, 117, 97, 97, 80, 114, 105, 110, 99, 105, 112, 97, 108, 34, 58, 34, 123, 92, 34, 105, 100, 92, 34, 58, 92, 34, 117, 115, 101, 114, 105, 100, 92, 34, 44, 92, 34, 110, 97, 109, 101, 92, 34, 58, 92, 34, 117, 115, 101, 114, 110, 97, 109, 101, 92, 34, 44, 92, 34, 101, 109, 97, 105, 108, 92, 34, 58, 92, 34, 117, 115, 101, 114, 110, 97, 109, 101, 64, 116, 101, 115, 116, 46, 111, 114, 103, 92, 34, 44, 92, 34, 111, 114, 105, 103, 105, 110, 92, 34, 58, 92, 34, 117, 97, 97, 92, 34, 44, 92, 34, 101, 120, 116, 101, 114, 110, 97, 108, 73, 100, 92, 34, 58, 110, 117, 108, 108, 44, 92, 34, 122, 111, 110, 101, 73, 100, 92, 34, 58, 92, 34, 117, 97, 97, 92, 34, 125, 34, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 114, 101, 113, 117, 101, 115, 116, 80, 97, 114, 97, 109, 101, 116, 101, 114, 115, 34, 58, 123, 34, 103, 114, 97, 110, 116, 95, 116, 121, 112, 101, 34, 58, 34, 112, 97, 115, 115, 119, 111, 114, 100, 34, 44, 34, 99, 108, 105, 101, 110, 116, 95, 105, 100, 34, 58, 34, 99, 108, 105, 101, 110, 116, 105, 100, 34, 44, 34, 115, 99, 111, 112, 101, 34, 58, 34, 111, 112, 101, 110, 105, 100, 34, 125, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 114, 101, 100, 105, 114, 101, 99, 116, 85, 114, 105, 34, 58, 110, 117, 108, 108, 44, 34, 117, 115, 101, 114, 65, 117, 116, 104, 101, 110, 116, 105, 99, 97, 116, 105, 111, 110, 46, 97, 117, 116, 104, 111, 114, 105, 116, 105, 101, 115, 34, 58, 91, 34, 111, 112, 101, 110, 105, 100, 34, 93, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 97, 117, 116, 104, 111, 114, 105, 116, 105, 101, 115, 34, 58, 91, 34, 111, 97, 117, 116, 104, 46, 108, 111, 103, 105, 110, 34, 93, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 99, 108, 105, 101, 110, 116, 73, 100, 34, 58, 34, 99, 108, 105, 101, 110, 116, 105, 100, 34, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 97, 112, 112, 114, 111, 118, 101, 100, 34, 58, 116, 114, 117, 101, 44, 34, 111, 97, 117, 116, 104, 50, 82, 101, 113, 117, 101, 115, 116, 46, 115, 99, 111, 112, 101, 34, 58, 91, 34, 111, 112, 101, 110, 105, 100, 34, 93, 125}; }