[rootvector2]

AbstractOAuthDataProvider.doRefreshAccessToken builds the new token through the doCreateAccessToken overload that never copies the mTLS x5t#S256 thumbprint from the message context, so every refreshed access token is an unbound bearer token (no cnf claim, OAuthRequestFilter skips the certificate check) and a stolen one works without the client key; found while reviewing the RFC 8705 binding paths, the thumbprint is now applied on both paths and AbstractOAuthDataProviderTest covers the refresh case.

[utafrali]

The core fix is correct: extracting setCertificateThumbprintConfirmation and calling it from doRefreshAccessToken is the right approach to plug the RFC 8705 certificate-binding gap. The main issues are test isolation (the mock MessageContext and the registered client are not cleaned up) and a missing negative test case for the no-cert-on-refresh path.

[utafrali]

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:

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.

[rootvector2]

Each subclass builds a new provider in its @Before, so the mock MessageContext doesn't outlive the test. Client 101 is already removed by tearDownClients() in the base @After, same as the other tests here that use it.

[utafrali]

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.

[rootvector2]

A refresh without a cert gives an unbound token, which is exactly what doCreateAccessToken(AccessTokenRegistration) does for the initial grant. AbstractTokenService only puts x5t#S256 in the context when the client authenticates with its certificate, so the binding follows the cert on the current request. A tls_client_auth client can't refresh without it, and a confidential client's stolen refresh token is still useless without the client credentials. Rejecting an uncertified refresh of a bound token (the RFC 8705 section 4 public client case) needs the thumbprint persisted on RefreshToken, which I'd keep as a separate change.

[utafrali]

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.

[rootvector2]

doCreateNewRefreshToken doesn't copy the access token's extra properties, so a refresh token never carries x5t#S256 and there's nothing to overwrite. The create path uses the same order too (registration extra properties first, then the thumbprint from the current request), so I left it without a comment.

[coheigea]

Thanks, the fix looks right. Could you add a variant of testRefreshedAccessTokenKeepsCertificateBinding with setUseJwtFormatForAccessTokens(true) that checks the refreshed JWT carries the cnf/x5t#S256 claim? The binding has to be set before convertToJWTAccessToken, and right now nothing tests that. Optionally, a case with no thumbprint in the message context at refresh time would make the expected behaviour explicit.