Skip to content

Fix some atomicity issues in removeCodeGrant #3358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
coheigea wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix some atomicity issues in removeCodeGrant #3358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion ...2/src/main/java/org/apache/cxf/rs/security/oauth2/grants/code/JPACMTCodeDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
package org.apache.cxf.rs.security.oauth2.grants.code;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import jakarta.persistence.EntityManager;
import jakarta.persistence.EntityTransaction;
import jakarta.persistence.LockModeType;
import org.apache.cxf.rs.security.oauth2.common.ServerAccessToken;
import org.apache.cxf.rs.security.oauth2.provider.OAuthServiceException;
import org.apache.cxf.rs.security.oauth2.tokens.refresh.RefreshToken;

/**
Expand Down Expand Up @@ -79,6 +81,7 @@ public void setEntityManager(EntityManager entityManager) {
public class JPACMTCodeDataProvider extends JPACodeDataProvider {

private static final int DEFAULT_PESSIMISTIC_LOCK_TIMEOUT = 10000;
private static final String JPA_LOCK_TIMEOUT_HINT = "jakarta.persistence.lock.timeout";

private int pessimisticLockTimeout = DEFAULT_PESSIMISTIC_LOCK_TIMEOUT;
private boolean useJpaLockForExistingRefreshToken = true;
Expand Down Expand Up @@ -129,12 +132,29 @@ protected RefreshToken updateExistingRefreshToken(RefreshToken rt, ServerAccessT
return super.updateExistingRefreshToken(rt, at);
}

@Override
protected ServerAuthorizationCodeGrant removeCodeGrant(String code, EntityManager em,

@reta reta Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the check + removal should happen in the same transaction scope (executeInTransaction), fe doRemoveClient / removeClientCodeGrants are good references

LockModeType lockModeType)
throws OAuthServiceException {
final Map<String, Object> options = new HashMap<>();
options.put(JPA_LOCK_TIMEOUT_HINT, pessimisticLockTimeout);
ServerAuthorizationCodeGrant grant =
em.find(ServerAuthorizationCodeGrant.class, code, LockModeType.PESSIMISTIC_WRITE, options);
try {
if (grant != null) {
em.remove(grant);
}
} catch (jakarta.persistence.EntityNotFoundException e) {
}
return grant;
}

protected void lockRefreshTokenForUpdate(final RefreshToken refreshToken) {
try {
execute(em -> {
final Map<String, Object> options;
if (pessimisticLockTimeout > 0) {
options = Collections.singletonMap("jakarta.persistence.lock.timeout", pessimisticLockTimeout);
options = Collections.singletonMap(JPA_LOCK_TIMEOUT_HINT, pessimisticLockTimeout);
} else {
options = Collections.emptyMap();
}
Expand Down
8 changes: 7 additions & 1 deletion ...uth2/src/main/java/org/apache/cxf/rs/security/oauth2/grants/code/JPACodeDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import jakarta.persistence.EntityManager;
import jakarta.persistence.EntityNotFoundException;
import jakarta.persistence.LockModeType;
import jakarta.persistence.TypedQuery;
import org.apache.cxf.rs.security.oauth2.common.Client;
import org.apache.cxf.rs.security.oauth2.common.UserSubject;
Expand Down Expand Up @@ -96,7 +97,12 @@ public ServerAuthorizationCodeGrant removeCodeGrant(final String code) throws OA
}

private ServerAuthorizationCodeGrant removeCodeGrant(String code, EntityManager em) throws OAuthServiceException {
ServerAuthorizationCodeGrant grant = em.find(ServerAuthorizationCodeGrant.class, code);
return removeCodeGrant(code, em, LockModeType.PESSIMISTIC_WRITE);
}

protected ServerAuthorizationCodeGrant removeCodeGrant(String code, EntityManager em,
LockModeType lockModeType) throws OAuthServiceException {
ServerAuthorizationCodeGrant grant = em.find(ServerAuthorizationCodeGrant.class, code, lockModeType);
try {
if (grant != null) {
em.remove(grant);
Expand Down
25 changes: 25 additions & 0 deletions .../src/test/java/org/apache/cxf/rs/security/oauth2/grants/code/JPACodeDataProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.fail;

public class JPACodeDataProviderTest {
Expand Down Expand Up @@ -102,6 +103,30 @@ public void testAddGetDeleteCodeGrants() {
assertEquals(0, grants.size());
}

@Test
public void testRemoveCodeGrantTwiceReturnsNullOnSecondCall() {
Client c = addClient("222", "alice");

AuthorizationCodeRegistration atr = new AuthorizationCodeRegistration();
atr.setClient(c);
atr.setApprovedScope(Collections.singletonList("a"));
atr.setSubject(c.getResourceOwnerSubject());

try {
ServerAuthorizationCodeGrant grant = getProvider().createCodeGrant(atr);

ServerAuthorizationCodeGrant first = getProvider().removeCodeGrant(grant.getCode());
assertNotNull(first);
assertEquals(grant.getCode(), first.getCode());

// second remove must not return the grant (single-use enforcement)
ServerAuthorizationCodeGrant second = getProvider().removeCodeGrant(grant.getCode());
assertNull(second);
} finally {
getProvider().removeClient(c.getClientId());
}
}

@Test
public void testResetClient() {
Client c = addClient("111", "bob");
Expand Down
Loading