This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/oauth2-token-revocation in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 4913c7364830904eed7b68147d3a8c7b87ed15ba Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Mon May 11 14:55:54 2026 +0100 Make sure that users can't revoke other users tokens --- .../oauth2/provider/AbstractOAuthDataProvider.java | 31 +++++++++++- .../oauth2/provider/OAuthDataProvider.java | 21 ++++++++ .../oauth2/services/TokenRevocationService.java | 35 +++++++++++++- .../provider/AbstractOAuthDataProviderTest.java | 56 ++++++++++++++++++++++ 4 files changed, 140 insertions(+), 3 deletions(-) diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProvider.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProvider.java index a78f0d9df87..24b1a1e1477 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProvider.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProvider.java @@ -252,14 +252,20 @@ public abstract class AbstractOAuthDataProvider implements OAuthDataProvider, Cl @Override public void revokeToken(Client client, String tokenKey, String tokenTypeHint) throws OAuthServiceException { + revokeToken(client, null, tokenKey, tokenTypeHint); + } + + @Override + public void revokeToken(Client client, UserSubject callerSubject, String tokenKey, String tokenTypeHint) + throws OAuthServiceException { ServerAccessToken accessToken = null; if (!OAuthConstants.REFRESH_TOKEN.equals(tokenTypeHint)) { - accessToken = revokeAccessToken(client, tokenKey); + accessToken = revokeAccessToken(client, callerSubject, tokenKey); } if (accessToken != null) { handleLinkedRefreshToken(client, accessToken); } else if (!OAuthConstants.ACCESS_TOKEN.equals(tokenTypeHint)) { - RefreshToken currentRefreshToken = revokeRefreshToken(client, tokenKey); + RefreshToken currentRefreshToken = revokeRefreshToken(client, callerSubject, tokenKey); revokeAccessTokens(client, currentRefreshToken); } } @@ -568,21 +574,42 @@ public abstract class AbstractOAuthDataProvider implements OAuthDataProvider, Cl } protected ServerAccessToken revokeAccessToken(Client client, String accessTokenKey) { + return revokeAccessToken(client, null, accessTokenKey); + } + + protected ServerAccessToken revokeAccessToken(Client client, UserSubject callerSubject, String accessTokenKey) { ServerAccessToken at = getAccessToken(accessTokenKey); if (at != null) { if (!at.getClient().getClientId().equals(client.getClientId())) { throw new OAuthServiceException(OAuthConstants.INVALID_GRANT); } + // IDOR guard: if the revocation request carries a resource owner identity, + // it must match the subject bound to the token. This prevents User A from + // revoking User B's token when both share the same OAuth2 client. + if (callerSubject != null && at.getSubject() != null + && !callerSubject.getLogin().equals(at.getSubject().getLogin())) { + throw new OAuthServiceException(OAuthConstants.INVALID_GRANT); + } doRevokeAccessToken(at); } return at; } + protected RefreshToken revokeRefreshToken(Client client, String refreshTokenKey) { + return revokeRefreshToken(client, null, refreshTokenKey); + } + + protected RefreshToken revokeRefreshToken(Client client, UserSubject callerSubject, String refreshTokenKey) { RefreshToken refreshToken = getRefreshToken(refreshTokenKey); if (refreshToken != null) { if (!refreshToken.getClient().getClientId().equals(client.getClientId())) { throw new OAuthServiceException(OAuthConstants.INVALID_GRANT); } + // IDOR guard: same subject check as for access tokens. + if (callerSubject != null && refreshToken.getSubject() != null + && !callerSubject.getLogin().equals(refreshToken.getSubject().getLogin())) { + throw new OAuthServiceException(OAuthConstants.INVALID_GRANT); + } doRevokeRefreshToken(refreshToken); } return refreshToken; diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/OAuthDataProvider.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/OAuthDataProvider.java index 8178edd7f60..a01e28383c8 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/OAuthDataProvider.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/OAuthDataProvider.java @@ -113,6 +113,27 @@ public interface OAuthDataProvider { */ void revokeToken(Client client, String tokenId, String tokenTypeHint) throws OAuthServiceException; + /** + * Revokes a refresh or access token, verifying that the authenticated resource owner's + * identity ({@code callerSubject}) matches the subject bound to the token. + * This prevents one user from revoking another user's token when multiple resource + * owners share the same OAuth2 client (IDOR guard). + * <p> + * {@code callerSubject} may be {@code null} for machine-to-machine (client credentials) + * flows where no end-user principal is present; in that case only the client-level + * check is applied. + * + * @param client the authenticated client + * @param callerSubject the authenticated resource owner, or {@code null} for M2M flows + * @param tokenId token identifier + * @param tokenTypeHint can be access_token or refresh_token or null + * @throws OAuthServiceException + */ + default void revokeToken(Client client, UserSubject callerSubject, String tokenId, String tokenTypeHint) + throws OAuthServiceException { + revokeToken(client, tokenId, tokenTypeHint); + } + /** * Converts the requested scopes to the list of permissions. * The scopes are extracted from OAuth2 'scope' property which diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java index 98698f30762..967410e117e 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java @@ -25,7 +25,9 @@ import jakarta.ws.rs.Path; import jakarta.ws.rs.Produces; import jakarta.ws.rs.core.MultivaluedMap; import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.SecurityContext; import org.apache.cxf.rs.security.oauth2.common.Client; +import org.apache.cxf.rs.security.oauth2.common.UserSubject; import org.apache.cxf.rs.security.oauth2.provider.OAuthServiceException; import org.apache.cxf.rs.security.oauth2.utils.OAuthConstants; @@ -58,7 +60,7 @@ public class TokenRevocationService extends AbstractTokenService { return createErrorResponseFromErrorCode(OAuthConstants.UNSUPPORTED_TOKEN_TYPE); } try { - getDataProvider().revokeToken(client, token, tokenTypeHint); + getDataProvider().revokeToken(client, getCallerUserSubject(client), token, tokenTypeHint); } catch (OAuthServiceException ex) { // Spec: The authorization server responds with HTTP status code 200 if the // token has been revoked successfully or if the client submitted an @@ -66,4 +68,35 @@ public class TokenRevocationService extends AbstractTokenService { } return Response.ok().build(); } + + /** + * Resolves the resource owner identity from the JAX-RS security context. + * <p> + * In standard OAuth2 flows the revocation endpoint is authenticated by the + * <em>client</em> (HTTP Basic Auth, client_secret_post, mTLS, …), so + * {@code SecurityContext.getUserPrincipal()} normally carries the client's own + * name, not an end-user identity. We must not mistake the client principal for + * a resource-owner subject, or we would block every legitimate client-initiated + * revocation (e.g. the canonical authorization-code flow where the client + * revokes its own token on logout). + * <p> + * A non-null {@link UserSubject} is returned only when a distinct end-user + * principal is present — i.e. the principal name differs from the authenticated + * client's {@code clientId}. In that case the data provider will verify that + * the token belongs to that end-user (IDOR guard). When the caller is the + * client itself, {@code null} is returned and only the client-level check applies. + */ + private UserSubject getCallerUserSubject(Client client) { + SecurityContext sc = getMessageContext().getSecurityContext(); + java.security.Principal principal = sc.getUserPrincipal(); + if (principal == null || principal.getName() == null) { + return null; + } + // Principal name matches the client id → this is the client authenticating + // at the token endpoint, not an independent resource-owner identity. + if (principal.getName().equals(client.getClientId())) { + return null; + } + return new UserSubject(principal.getName()); + } } \ No newline at end of file diff --git a/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java index 439b9d69a93..0d160d352cd 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java +++ b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java @@ -47,6 +47,7 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; abstract class AbstractOAuthDataProviderTest { private static KeyPair keyPair; @@ -256,6 +257,59 @@ abstract class AbstractOAuthDataProviderTest { assertNotEquals(at.getSubject().getId(), at2.getSubject().getId()); } + @Test + public void testRevokeAccessTokenIdorAcrossUsersOfSharedClient() { + // A multi-user shared client (e.g. a public mobile app with a known client_id). + // Its resourceOwnerSubject is intentionally null — many users share this client. + Client sharedClient = new Client(); + sharedClient.setRedirectUris(Collections.singletonList("http://client/redirect";)); + sharedClient.setClientId("103"); + sharedClient.setClientSecret("secret"); + getProvider().setClient(sharedClient); + + // Alice obtains an access token via the shared client. + AccessTokenRegistration aliceReg = new AccessTokenRegistration(); + aliceReg.setClient(sharedClient); + aliceReg.setApprovedScope(Collections.singletonList("a")); + aliceReg.setSubject(new UserSubject("alice")); + ServerAccessToken aliceToken = getProvider().createAccessToken(aliceReg); + assertNotNull(getProvider().getAccessToken(aliceToken.getTokenKey())); + + // Bob obtains a separate access token via the same shared client. + AccessTokenRegistration bobReg = new AccessTokenRegistration(); + bobReg.setClient(sharedClient); + bobReg.setApprovedScope(Collections.singletonList("a")); + bobReg.setSubject(new UserSubject("bob")); + ServerAccessToken bobToken = getProvider().createAccessToken(bobReg); + assertNotNull(getProvider().getAccessToken(bobToken.getTokenKey())); + + // IDOR attempt: Bob (authenticated only as the shared client) submits Alice's token + // key to the revocation endpoint. The subject-aware revokeToken overload is called + // with Bob's identity, which must not match Alice's subject. + // The data provider must reject this with OAuthServiceException; the HTTP layer + // (TokenRevocationService) swallows it per RFC 7009 and returns 200 anyway. + try { + getProvider().revokeToken(sharedClient, new UserSubject("bob"), aliceToken.getTokenKey(), + OAuthConstants.ACCESS_TOKEN); + // Reaching here means the cross-user revocation was NOT blocked — fail the test. + fail( + "IDOR: revokeToken should have thrown OAuthServiceException when Bob tried to " + + "revoke Alice's token, but it succeeded silently" + ); + } catch (OAuthServiceException expected) { + // Expected: the subject mismatch guard correctly rejected the request. + } + + // Alice's token must still be intact after the blocked revocation attempt. + assertNotNull( + "Alice's token must survive a cross-user revocation attempt", + getProvider().getAccessToken(aliceToken.getTokenKey()) + ); + + // Bob's own token must remain unaffected regardless. + assertNotNull(getProvider().getAccessToken(bobToken.getTokenKey())); + } + @Test public void testAddGetDeleteRefreshToken() { Client c = addClient("101", "bob"); @@ -330,6 +384,8 @@ abstract class AbstractOAuthDataProviderTest { protected void tearDownClients() { tearDownClient("101"); + tearDownClient("102"); + tearDownClient("103"); tearDownClient("12345"); tearDownClient("56789"); tearDownClient("09876");
