#25815 do not remove previous refresh token for federated identity #27455
Conversation
|
@geoffreyfourmis Better is if you squash the commits and sign-off a single commit. |
|
sorry not used to do that, I squashed both commit into one but it seems to be still there, looking at it right now... |
|
ok, should be good this time |
services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Geoffrey Fourmis <geoffrey.fourmis@gmail.com>
| // like in OIDCIdentityProvider.exchangeStoredToken() | ||
| // we shouldn't override the refresh token if it is null in the context and not null in the DB | ||
| // as for google IDP it will be lost forever | ||
| if (federatedIdentityModel.getToken() != null) { |
There was a problem hiding this comment.
Tests are failing because when the IDP is a SAML IDP, the response can't be read into an OAuth AccessTokenResponse.
Changing this line to
if (federatedIdentityModel.getToken() != null && !(context.getIdp() instanceof SAMLIdentityProvider)) {
Fixed the test failures for me.
There was a problem hiding this comment.
Also, I'm not sure this works with the Twitter provider because it sets some sort of a custom token in the broker context https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/social/twitter/TwitterIdentityProvider.java#L235-L244
I don't think this can be read into an AccessTokenResponse.
|
Closing in favor of #29109 which adds tests to the PR. |
Closes #25815
Like what is done in
OIDCIdentityProvider.exchangeStoredToken()we shouldn't remove previous refresh token inside federated identity token while updating the token insideIdentityBrokerService.updateToken().