search

BookStackApp / BookStack

A platform to create documentation/wiki content built with PHP & Laravel
https://www.bookstackapp.com/
MIT License
14.67k stars 1.85k forks source link

Logout of OIDC provider when logging out of Bookstack #3715

Closed hadiahmed098 closed 8 months ago

hadiahmed098 commented 1 year ago

Describe the feature you'd like

When I press "Logout" on Bookstack, I only get logged out of Bookstack. If the authentication is through an OIDC provider, I'd want to be logged out of the provider as well.

There is an end_session_endpoint given in the OIDC discovery endpoint, so this shouldn't be too difficult to add.

Describe the benefits this would bring to existing BookStack users

This feature is how the SAML method works, and is how SSO is supposed to work - single sign on, single logout.

If another person wants to log onto the Bookstack service and we are using OIDC, I need to logout of Bookstack and manually clear my OIDC tokens before they can log on.

Can the goal of this request already be achieved via other means?

Yes? Technically, using SAML would allow this to work, but I was unable to successfully get SAML setup, whereas the OIDC setup was quick and painless.

Have you searched for an existing open/closed issue?

How long have you been using BookStack?

0 to 6 months

Additional context

No response

ssddanbrown commented 1 year ago

@hadiahmed098 Thanks for the suggestion. As usual with auth features, I'm hesitant to grow the scope.

Relevant specs:

All in implementers draft standard, support will vary across identity providers. Existing OIDC work has been done to final spec standards. Not really sure on the general support expectation in regards to depth of support and support of the different specs above. From a glance of the specs, back-channel looks quite technically ideal and compatible with BookStack's server model, but would require a fair amount of extra logic and looks like maybe it's a younger/lesser-used spec. Has the ability for the auth provide to logout all active RP sessions. Session spec looks more established, but it has an iframe-session-polling element which looks generally not required but somewhat expected for a useful implementation, which is not ideal.

hadiahmed098 commented 1 year ago

I found this spec on the OpenID website, it is also in implementers draft.

(Note that the OpenID specification list doesn't include it under implementers draft, but this press release mentions it.

The RP-initiated logout would be much simpler to implement, and is exactly what I had in mind. Keycloak, a fairly popular SSO IdP does support this, and since it's just a GET or POST request, it should be simple to ignore if the IdP does not support it.

I would work on a PR, but my PHP is rather weak and I don't know how much help I could be. What do you think?

hadiahmed098 commented 1 year ago

The voting period to finalize the specification closes on Monday, so there should be an update on the specification after Tuesday, September 13. I'll keep an eye on it.

hadiahmed098 commented 1 year ago

@ssddanbrown The RP-Initiated logout procedure was promoted to a final specification.

https://openid.net/specs/openid-connect-rpinitiated-1_0.html

nurradityam commented 1 year ago

hi, i just recently tried OIDC method, then found this logout problem too.

keycloak 22.0.0 bookstack 23.06.1

ssddanbrown commented 11 months ago

@hadiahmed098 Do you still desire this feature? If so, Would you be able to confirm the software/service you're using for OIDC? Just to help me understand the OIDC platform landscape, and logout support, of those that desire this.

fuchsi3010 commented 11 months ago

hi, i'd desire this feature. i use a selfhosted keycloak instance for auth management.

hadiahmed098 commented 11 months ago

@ssddanbrown yes, this feature is still desired. I am running a self hosted Keycloak server v19.0.3 with Bookstack 23.06.1. I have successfully used the RP-initiated logout functionality across other self hosted sites.

ssddanbrown commented 10 months ago

Okay, so only really demanded for keycloak so far. Felt nervous with only a single platform in the mix, but checking support (list below) on some of my test OIDC provider systems makes me feel more comfortable, think it'd be fine to move forward with, especially as I should have a range of platforms to validate/test against.

Follow up question to all for feedback:

I'm thinking we'd enable logout automatically if the auth provider indicates support from autodiscovery. There will still be the option to disable this (or define endpoint via setting if autodiscovery is not active). There would still be an upgrade advisory in our update notes since it's a change to auth functionality.

Does that sound sensible? Or could you see that being a nuisance for new and existing setups using OIDC? Or could there be security implications of enabling by default?

Sounds okay to me but I don't live in the realm of enterprise administration.

Platform Support Checks

Likely RP-Initiated logout support based upon end_session_endpoint existing via discovery:

Note: It could be there are in-auth-platform settings which can affect this.

thespad commented 10 months ago

Would be useful for Authentik as well.

hadiahmed098 commented 10 months ago

Sorry for my late response, your plan sounds sensible.

Getting the endpoint via autodiscovery with options to define/disable this makes sense to me, and keeps the flow of the other OIDC settings. I can't imagine there are any security implications with this, as long as the Bookstack session is ended before we send the request to the IdP. As for nuisances, I can't speak to that unless admins/users were relying on staying logged into an IdP while logging out of Bookstack.

Also, after looking into it a bit Auth0 does support RP initiated endpoints as well.

ssddanbrown commented 8 months ago

This functionality has now been added via @joancyho in PR #4467, with further changes via PR #4714. This will be part of the next feature release.

In contrary my previous comment, this will not be enabled by default via auto-discovery. While testing I found that, as per the spec, some auth systems will be strict on the return path, which often requires configuration on the auth system side. We could omit that, but then the user journey presents quite a break relative to existing use. Therefore, to avoid issues, this will be inactive by default but with option to load via autodiscovery, or specify a URL.

I successfully tested the added functionality across the following auth systems:

Thanks again @hadiahmed098 for the original request and for pointing me to (and watching progress of) the spec.