Skip to content

Commit

Permalink
Minimal Fix auth code cleanup function (#2292)
Browse files Browse the repository at this point in the history
* 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 <bricard@vmware.com>

* 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 <yalicia@vmware.com>
Co-authored-by: Bruce Ricard <bricard@vmware.com>
  • Loading branch information
3 people committed May 3, 2023
1 parent 53d8340 commit 59fbf8e
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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};
}

0 comments on commit 59fbf8e

Please sign in to comment.