Skip to content

Commit

Permalink
UaaTokenStore: Don't fall over when failing to expire oauth codes
Browse files Browse the repository at this point in the history 
MySQL does row-level locking, which means when we delete expired oauth
codes in parallel we can sometimes hit a harmless deadlock (where the
two delete updates execute across the rows in different order
simultaneously).  This is not an issue as the winning update will expire
tokens as appropriate, plus the tokens are checked for expiry on use.
  • Loading branch information
@mook-as
mook-as committed Feb 16, 2018
1 parent a4132cb commit 332d119
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 1 deletion.
5 changes: 4 additions & 1 deletion server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.cloudfoundry.identity.uaa.util.UaaStringUtils;
import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.dao.DeadlockLoserDataAccessException;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
Expand Down Expand Up @@ -215,12 +216,14 @@ protected void performExpirationClean() {
//check if we should expire again
if ((System.currentTimeMillis()-last) > getExpirationTime()) {
//avoid concurrent deletes from the same UAA - performance improvement
if (lastClean.compareAndSet(last, last+getExpirationTime())) {
if (lastClean.compareAndSet(last, last+getExpirationTime())) try {
JdbcTemplate template = new JdbcTemplate(dataSource);
int expired = template.update(SQL_EXPIRE_STATEMENT, System.currentTimeMillis());
logger.debug("[oauth_code] Removed "+expired+" expired entries.");
expired = template.update(SQL_CLEAN_STATEMENT, new Timestamp(System.currentTimeMillis()-LEGACY_CODE_EXPIRATION_TIME));
logger.debug("[oauth_code] Removed "+expired+" old entries.");
} catch (DeadlockLoserDataAccessException e) {
logger.debug("[oauth code] Deadlock trying to expire entries, ignored.");
}
}

Expand Down
70 changes: 70 additions & 0 deletions server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.cloudfoundry.identity.uaa.zone.IdentityZone;
import org.junit.Before;
import org.junit.Test;
import org.springframework.dao.DeadlockLoserDataAccessException;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
Expand All @@ -44,6 +45,7 @@
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
import java.sql.Timestamp;
Expand Down Expand Up @@ -314,6 +316,27 @@ public void testCleanUpUnusedOldTokens_MySQL_In_Another_Timezone() throws Except
}
}

@Test
public void testCleanUpExpiredTokensDeadlockLoser() throws Exception {
Connection con = dataSource.getConnection();
try {
Connection expirationLoser = (Connection) Proxy.newProxyInstance(getClass().getClassLoader(),
new Class[]{Connection.class},
new ExpirationLoserConnection(con));

SameConnectionDataSource sameConnectionDataSource = new SameConnectionDataSource(expirationLoser);

store = new UaaTokenStore(sameConnectionDataSource, 1);
int count = 10;
for (int i=0; i<count; i++) {
String code = store.createAuthorizationCode(clientAuthentication);
try { store.consumeAuthorizationCode(code); } catch (InvalidGrantException e) {}
}
} finally {
con.close();
store = new UaaTokenStore(dataSource);
}
}


public class SameConnectionDataSource implements DataSource {
Expand Down Expand Up @@ -387,5 +410,52 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
}
}

public class ExpirationLoserConnection implements InvocationHandler {
static final String CLOSE_VAL = "close";
static final String PREPARE_VAL = "prepareStatement";
private final Connection con;

protected class ExpirationLoserPreparedStatement implements InvocationHandler {
static final String UPDATE_VAL = "executeUpdate";
private final PreparedStatement stmt;

ExpirationLoserPreparedStatement(PreparedStatement stmt) {
this.stmt = stmt;
}

@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
if (UPDATE_VAL.equals(method.getName())) {
throw new DeadlockLoserDataAccessException("Deadlock in update (emulated)", null);
}
return method.invoke(stmt, args);
}
}

ExpirationLoserConnection(Connection con) {
this.con = con;
}

@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
switch (method.getName()) {
case CLOSE_VAL:
// This breaks things
return null;
case PREPARE_VAL:
if (args.length > 0) {
String sql = (String) args[0];
if (sql.startsWith("delete from oauth_code where expiresat ")) {
PreparedStatement stmt = (PreparedStatement) method.invoke(con, args);
return Proxy.newProxyInstance(getClass().getClassLoader(),
new Class[]{PreparedStatement.class},
new ExpirationLoserPreparedStatement(stmt));
}
}
}
return method.invoke(con, args);
}
}

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 332d119

Please sign in to comment.