keycloak: do not invalidate session without refresh_token #704
Conversation
michalskrivanek
requested review from
ahadas,
bennyz,
emesika,
mwperina,
oliel,
sgratch,
smelamud,
didib and
ljelinkova
as code owners
|
/ost basic-suite-master el9stream |
| log.debug("Existing Session found for token: {}, invalidating session on external OP", | ||
| ssoSession.getAccessToken()); | ||
| ExternalOIDCService.logout(ssoContext, refreshToken); | ||
| log.debug("invalidating session on external OP, refreshToken: {}", refreshToken); |
There was a problem hiding this comment.
Please use:
log.debug("Invalidating session on external OIDC, refreshToken: {}", refreshToken);
and move the log into if section
There was a problem hiding this comment.
This patch seems more like covering the fact that the logout does not work then actually solving the issue. If we place both logging the refresh token and the call to external service into the if, there will be no evidence in the engine.log why the user was not attempted to logout. Maybe we could add else with warn log that the user was not attempted to logout from Keycloak because the refresh token was not specified?
There was a problem hiding this comment.
This patch seems more like covering the fact that the logout does not work then actually solving the issue. If we place both logging the refresh token and the call to external service into the if, there will be no evidence in the engine.log why the user was not attempted to logout. Maybe we could add
elsewith warn log that the user was not attempted to logout from Keycloak because the refresh token was not specified?
it's not about logout really, it's about the fact that we have a single keycloak "session" for mutliple engine sessions for the same user. We cannot invalidate keycloak side when there is another engine session "holding" the keycloak one (having refresh_token).
So it's not really an error/warning that we're not logging out from keycloak, we're not supposed to. It will happen once the original/first engine session is invalidated
There was a problem hiding this comment.
re logging - i want to be able to see if it was actually called or not. two logs is fine, but single lock with a simple "null" or "something" makes it clear enough too...
michalskrivanek
force-pushed
the
refresh-token
branch
from
28a45b1
to
3b94700
Compare
if there are multiple sessions for the same user (i.e. with different scopes) we only get the refresh_token for the first established one and we cannot invalidate it on first logout, so let's keep keycloak session active until the one that established it first logs out
michalskrivanek
force-pushed
the
refresh-token
branch
from
3b94700
to
72cfdb8
Compare
if there are multiple sessions for the same user (i.e. with different
scopes) we only get the refresh_token for the first established one and
we cannot invalidate it on first logout, so let's keep keycloak session
active until the one that established it first logs out