Skip to content

Conversation

@julien-nc
Copy link
Member

@julien-nc julien-nc commented Aug 20, 2025

⚠️ This depends on nextcloud/server#54545 which will be included in Nextcloud 32.

When a user logs in twice (with user_oidc) and invalidates the other session in the security settings, for example, it does not kill the IdP session so, in the browser session that is revoked, it is possible to complete the login flow without authenticating again in the IdP.

This PR adds a listener on a new event dispatched when an auth token is invalidated: OCP\Authentication\Events\TokenInvalidatedEvent
If we have an Oidc session that matches this invalidated token, we call the end_session_endpoint to make sure the related IdP session is terminated.

We also cleanup the session entry (in a user_oidc table) when this happens.

New columns in the oc_user_oidc_session table:

  • user_id is used with authtoken_id to get the session when a token is invalidated
  • provider_id is necessary to get the client_id so we can send it as a param to the end_session_endpoint
  • id_token is the login ID token, we also need it as a param to the end_session_endpoint
  • idp_session_closed helps with backchannel logout (explained below)

To make sure we don't mess with the backchannel logout, there is a new idp_session_closed column in the oc_user_oidc_session table. When we receive a backchannel logout request, we know that the IdP is currently killing one of its sessions. We call authTokenProvider->invalidateTokenById to kill the related Nextcloud session. So we will receive the OCP\Authentication\Events\TokenInvalidatedEvent but this time we don't need to make a request to the end_session_endpoint, we know the IdP session is already terminated. So we store that information in the session in the idp_session_closed column so the listener can ignore the event related with this Oidc session.

closes #1159
closes nextcloud/server#53942

@julien-nc julien-nc requested a review from janepie August 20, 2025 15:38
@julien-nc julien-nc requested a review from juliusknorr as a code owner August 20, 2025 15:38
@julien-nc julien-nc added enhancement New feature or request security Pull requests that address a security vulnerability 3. to review labels Aug 20, 2025
@julien-nc julien-nc force-pushed the enh/noid/handle-session-token-revocation branch 4 times, most recently from f0b6e64 to 09c3ef1 Compare August 25, 2025 12:11
@julien-nc julien-nc force-pushed the enh/noid/handle-session-token-revocation branch from cde8a4e to a349040 Compare August 28, 2025 10:38
Copy link
Member

@janepie janepie left a comment

Choose a reason for hiding this comment

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

Nitpicks, looks good overall!

return;
}
// we have nothing to do if we know the idp session is already closed
if ($oidcSession->getIdpSessionClosed() !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not ==1? Can it be null for some reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a "notNull" column. See 'notnull' => true, in the migration.

'created_at' => $this->getCreatedAt(),
'user_id' => $this->getUserId(),
'provider_id' => $this->getProviderId(),
'idp_session_closed' => $this->getIdpSessionClosed() !== 0,
Copy link
Member

Choose a reason for hiding this comment

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

same here

…dP's end_session_endpoint to make sure it's not possible to login again from the invalidated browser session

Signed-off-by: Julien Veyssier <[email protected]>
…point, cleanup our own oidc session when a token has been invalidated

Signed-off-by: Julien Veyssier <[email protected]>
…ions row with the SID, update it

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc julien-nc force-pushed the enh/noid/handle-session-token-revocation branch from a349040 to 9795046 Compare August 29, 2025 15:10
@julien-nc julien-nc merged commit dc8e174 into main Aug 29, 2025
46 of 47 checks passed
@julien-nc julien-nc deleted the enh/noid/handle-session-token-revocation branch August 29, 2025 15:15
@julien-nc julien-nc mentioned this pull request Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement New feature or request security Pull requests that address a security vulnerability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Session revoking logic [Bug]: Session revoke is useless (no-op) with session authenticated via OIDC

3 participants