RP initiated logout makes backchannel logout throw an HTTP/400 error

[Spitfireap]

How to use GitHub

• Please use the 👍 reaction to show that you are affected by the same issue.
• Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
• Subscribe to receive notifications on status change and new comments.

---

Steps to reproduce

1. Logout from the RP (Nextcloud) with backchannel logout enabled on the IdP
2. Agree to disconnect on the IdP interface
3. Backchannel logout will likely throw an error 400

Expected behaviour

• Do not throw an HTTP/400 error on non existent session as stated per the RFC

Actual behaviour

Backchannel logout will almost every time return an HTTP/400 error when doing a RP-initiated logout.

Explaination

The issue is that the error HTTP 400 will then be thrown to the IdP during the backchannel logout due to the session not being found since the session has been deleted here.
The IdP I am using (LemonLDAP) is registering the error and inform the user a possible error occured. Of course everything is fine :

The RFC does state that a success HTTP/200 should be returned :
If the identified End-User is already logged out at the RP when the logout request is received, the logout is considered to have succeeded.

Proposed fix

• Make the response an HTTP/200 if no session are found ;
• Optionally: have a debug checkbox in the provider option to enable this behavior but have it disabled by default. The checkbox could be general so it could be use for later use.

I can try to push a PR if you take somebody's first NC related PR :)

Server configuration

Web server: Nginx

Database: PostgreSQL

PHP version: 8.4

Nextcloud version: 33.0.2

[julien-nc]

Hi, thanks for reporting this. We happily take PRs, whether they are one's first or not 😁 .

You could add a responseStatus param to the LoginController::getBackchannelLogoutErrorResponse method and use Http::STATUS_OK when the session is not found (only after findSessionBySid or findSessionsBySubAndIss don't return any session).
Makes sense?

[Spitfireap]

Well it seems "someone" was quicker than me :)

[julien-nc]

It seems like I should not have added the "good first issue" tag 😁

[julien-nc]

Is #1431 more or less what you had in mind? It still returns an error code if there was a database error in findSessionsBySubAndIss.

[Spitfireap]

Yeah it fixes it up. The debug feature would be indeed be too much.
I didn't try the sub+iss path though. You're right it should be ignored as well I think.
And it would be great to refactor it to use the narrower DoesNotExist exception instead of the super one.

On top of that and while we're at it : I don't think this path is possible : we have a unique constraint on sid (or is just the index?) :
"user_oidc_sess_sid" UNIQUE, btree (sid)

Do we prompt it through the PR or should I open a new one ?

[Spitfireap]

I mean if that's ok, I'd like to open a new one ^^