utafrali commented on code in PR #3652:
URL: https://github.com/apache/cxf/pull/3652#discussion_r4416000141
##########
rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java:
##########
@@ -403,6 +407,26 @@ public void
testCrossClientRefreshTokenRejectedWhenRecycleDisabled() {
assertNotNull(getProvider().getAccessToken(atA.getTokenKey()));
}
+ @Test
+ public void testRefreshedAccessTokenKeepsCertificateBinding() {
+ MessageContext mc = mock(MessageContext.class);
+
when(mc.get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256)).thenReturn("thumbprint");
+ getProvider().setMessageContext(mc);
+
+ Client c = addClient("101", "bob");
+
+ AccessTokenRegistration atr = new AccessTokenRegistration();
+ atr.setClient(c);
+ atr.setApprovedScope(Arrays.asList("a", "refreshToken"));
+ atr.setSubject(c.getResourceOwnerSubject());
+
+ ServerAccessToken at = getProvider().createAccessToken(atr);
+ assertEquals("thumbprint",
at.getExtraProperties().get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256));
+
+ ServerAccessToken at2 = getProvider().refreshAccessToken(c,
at.getRefreshToken(), Collections.emptyList());
+ assertEquals("thumbprint",
at2.getExtraProperties().get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256));
+ }
+
protected Client addClient(String clientId, String userLogin) {
Review Comment:
The test only covers the happy path where the cert thumbprint is present in
the message context for both the initial request and the refresh. Consider
adding a second case: bind the initial token with a cert, then refresh with a
`MessageContext` that returns `null` for the thumbprint (simulating a
misconfiguration or a refresh request that arrives without the client
certificate). The expected behavior in that scenario (token becomes unbound vs.
an error is thrown) should be explicitly documented and tested, since a stolen
refresh token could otherwise be used to obtain an unbound access token.
##########
rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java:
##########
@@ -403,6 +407,26 @@ public void
testCrossClientRefreshTokenRejectedWhenRecycleDisabled() {
assertNotNull(getProvider().getAccessToken(atA.getTokenKey()));
}
+ @Test
+ public void testRefreshedAccessTokenKeepsCertificateBinding() {
Review Comment:
`getProvider().setMessageContext(mc)` is never reset after this test. If the
provider is shared across tests (typical for abstract base test classes with a
single `@Before` setup), every test that runs after this one will see the mock
`MessageContext` returning `"thumbprint"`, which will silently pollute those
tests. Reset it in a try-finally block or an `@After` method:
```java
try {
getProvider().setMessageContext(mc);
// ... assertions ...
} finally {
getProvider().setMessageContext(null);
}
```
Also, the client `"101"` registered here is never cleaned up via
`tearDownClient("101")`, which can leave stale state for other tests.
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProvider.java:
##########
@@ -475,6 +479,8 @@ protected ServerAccessToken doRefreshAccessToken(Client
client,
oldRefreshToken.getGrantType(), oldRefreshToken.getNonce(),
null, theNewScopes, oldRefreshToken.getSubject());
Review Comment:
`doRefreshAccessToken` passes `oldRefreshToken.getExtraProperties()` into
the lower-level `doCreateAccessToken` call (so the new token inherits those
properties), and then immediately calls `setCertificateThumbprintConfirmation`.
If any subclass or future code ever stores `x5t#S256` in the refresh token's
extra properties, the thumbprint would be silently overwritten only when
`messageContext` has one, and preserved otherwise. This creates an ambiguous
write-order. A short inline comment noting that
`setCertificateThumbprintConfirmation` intentionally supersedes any thumbprint
that may have been copied from `extraProperties` would clarify the intent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
