Add soft logout possibility for OIDC backchannel_logout

[hachem2001]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Pull request includes a sign off
• Code style is correct

• ISSUE : When doing a backchannel logout using an OIDC provider, a user may lose any non backed-up keys.

• PROPOSED FEATURE : Allow to set the backchannel logout induced by an OIDC to be a soft-logout, thus allowing the user to reconnect to the same device and recover his session if necessary.

• IMPLEMENTATION : Allow setting OIDC backchannel logout to be a soft-logout in OIDC provider configuration via backchannel_logout_is_soft which defaults to false.

Signed-off-by: OUERTANI Mohamed Hachem omhx21@gmail.com

[clokep]

For cross-linking, there was a thread on the original PR about this: #11414 (comment)

[clokep]

@sandhose Do you have any thoughts on if it makes sense for this to be configurable or not? (Previous conversation at #11414 (comment))

The backchannel-logout is used when a user manually logs out of an IdP? Or is it also used when a user's session expires with an IdP?

The former seems like it would make sense with the current (hard-)logout, while the latter should likely be a soft-logout.

[sandhose]

I think keeping the option to do both is good. Usually, a backchannel logout happens because the user expressed the intent of logging out, but it might also happen when for example an admin kicks out a user to force logging back in, and in this case you might want to do a soft_logout in clients.

[hachem2001]

It's indeed for the case where the user doesn't launch the backchannel logout on the server directly. Backchannel logout on synapse can occur without the user intentionally provoking it in some cases.

I made this as an option for safety upon upgrade, not to break any OIDC behavior servers currently rely on. Although I don't envision cases where a server would prefer a hard-logout for users, it's better to have the choice I suppose.

Another alternative would be to set backchannel_logout_is_soft to true by default, and in a further version eventually rule out the option and make it default behavior (basically an ephemeral temporary option). This is the best compromise in my opinion.

[sandhose]

I think it makes sense to have control over this, but I'm really curious in what kind of deployment this is really useful/needed?

[sandhose]

I think keeping the option to do both is good. Usually, a backchannel logout happens because the user expressed the intent of logging out, but it might also happen when for example an admin kicks out a user to force logging back in, and in this case you might want to do a soft_logout in clients.

[sandhose]

This feels like it should be instead a parameter on the revoke_sessions_for_provider_session_id method instead, because most of the logic here is duplicated

[hachem2001]

I agree, though I avoided that in case this redundancy improves visibility, and in case we don't want to change the behavior of one of the functions in the long-run.

But merging it with revoke_sessions_for_provider_session_id by adding a new parameter (and modifying all subsequent calls of the function) is possible.

[clokep]

I suspect it would improve readability if there's less duplicated code, so we can see what the differences are based on the flag.

[clokep]

@hachem2001 Do you have interest in making this change?

[hachem2001]

The github-ci tests don't seem to fail for me, I'm not sure how to fix that on here.
It seems every time the branch is not up to date with the latest commits on develop, the CI doesn't go through all the way.

[clokep]

It seems every time the branch is not up to date with the latest commits on develop, the CI doesn't go through all the way.

I don't think it has to do with being up-to-date -- there's been a few flakey tests in our CI recently. It is usually easier to re-run the single job instead of merging in develop (which then runs all the tests again).