Add support for logging out of OIDC Identity Provider #9162

machristie · 2019-12-23T19:39:56Z

This PR adds support for logging out of an OIDC Identity Provider when the user logs out of Galaxy. This makes sense for some IDPs that exist primarily to provide authentication for Galaxy such as a private Keycloak server. Logging out of the IDP is controlled by a new configuration option in oidc_backends_config.xml called <enable_idp_logout>.

The implementation works by creating a cookie called oidc-provider when the user logs in through an OIDC IDP to record which provider was used. Then, on logout (menu.js), /authnz/logout is called to determine if IDP logout is supported and enabled for the IDP and if so a redirect URI is returned. The user's browser is redirect to this redirect URI which terminates the user's session in the IDP. The IDP then redirects back to the Galaxy home page.

Support for IDP logout is only implemented for Custos/Keycloak since the PSA based backends don't support logout (PSA only supports disconnecting, but logout support was added for SAML recently so it may someday be available for other backends).

nuwang · 2019-12-27T14:05:12Z

lib/galaxy/authnz/managers.py

@@ -126,7 +127,8 @@ def _parse_custos_config(self, config_xml):
            'client_id': config_xml.find('client_id').text,
            'client_secret': config_xml.find('client_secret').text,
            'redirect_uri': config_xml.find('redirect_uri').text,
-            'realm': config_xml.find('realm').text}
+            'realm': config_xml.find('realm').text,
+            'enable_idp_logout': config_xml.findtext('enable_idp_logout', 'false').lower() == "true"}


The galaxy.util.asbool function is a potential replacement for this:

galaxy/lib/galaxy/util/__init__.py

Line 925 in 0204c9b

def asbool(obj):

Good suggestion, will do.

nuwang · 2019-12-27T14:11:47Z

lib/galaxy/authnz/managers.py

+            # check if logout is enabled for this idp and return false if not
+            unified_provider_name = self._unify_provider_name(provider)
+            if self.oidc_backends_config[unified_provider_name]['enable_idp_logout'] is False:
+                return False, "IDP logout is not enabled for {}".format(provider), None


Are the return values for this documented somewhere?

Good catch, I'll document.

nuwang · 2019-12-27T14:13:30Z

lib/galaxy/webapps/galaxy/buildapp.py

@@ -72,6 +72,7 @@ def app_factory(global_conf, load_app_kwds={}, **kwargs):
    webapp.add_route('/authnz/{provider}/login', controller='authnz', action='login', provider=None)
    webapp.add_route('/authnz/{provider}/callback', controller='authnz', action='callback', provider=None)
    webapp.add_route('/authnz/{provider}/disconnect', controller='authnz', action='disconnect', provider=None)
+    webapp.add_route('/authnz/logout', controller='authnz', action='logout')


Should this be a per provider logout as well?

I think having per provider logout could work too, but this seemed more natural to me from the perspective of the UI. For login, the user has picked a specific provider to log into. However, on logout the user has only indicated that they want to logout. /authnz/logout figures out what provider was used to authenticate (using the identity-provider cookie) and logs the user out of that provider.

The UI perspective makes sense. What do you think about the API perspective? I feel as if having a per provider logout allows programmatic logout from specific providers, although I'm not sure there's a clear use case. Perhaps a programmatic global logout from a specific provider like keycloak only through the API perhaps? Is the only necessary change to read the cookie from the client side instead of the server? I also feel like authnz/logout should log the user out from all providers, not just the last logged in one? Just some thoughts, but no strong views on which may be better.

What your saying makes a lot of sense. I do like that per provider logout is consistent with the other provider methods.

Is the only necessary change to read the cookie from the client side instead of the server?

Yeah I think that's basically it. However, I think I would change it so that server side the cookie is used to determine the logout API URL and this is provided to the client. I'll work on updating the PR.

Hi @nuwang , I looked into passing a value to the UI, but it seemed simpler and more in keeping with the Galaxy client design to add an API method for getting the provider specific logout url, so I did that. The frontend now calls /authnz/logout which reads the oidc-provider cookie and if it exists, redirects to /authnz/{provider}/logout.

…ider

nuwang

@machristie Thanks for the changes, looks great to me!

dannon · 2020-01-28T16:30:25Z

I'll get keycloak configured again for testing this morning and will merge this today when it works.

Resolved conflicts with upstream, pushed the merge here.

dannon · 2020-01-28T20:36:02Z

This is working well for me so I'm going to go ahead and merge it when the tests finish this time. I do think we might want to address the potential refactoring @nuwang mentions, and we might want to drop get_logout_url since, with teh cookie, we actually have all the information we need to call the correct method from the client side from the start.

So, you could check for and read oidc-provider, you'll know it's custos, and can then call authnz/custos/logout, right?

Anyway, thank you again for the enhancement.

machristie · 2020-01-30T02:22:42Z

Thanks @dannon

So, you could check for and read oidc-provider, you'll know it's custos, and can then call authnz/custos/logout, right?

Yes that's right. I went with the approach I took so that only the backend needs to know about cookie instead of the front end and back end needing to know about the cookie. That makes it easier to change the code around setting the cookie, if necessary, going forward. But the approach of the client reading the cookie I think would be fine too, especially if we don't anticipate change in the logic around setting the cookie. I hope that makes sense.

I can put together a PR to do that refactor, maybe this weekend.

nuwang · 2020-01-30T12:07:23Z

@machristie That makes sense. In any case, in future perhaps an enhancement may be to have the logout url log you out of all providers, and the provider specific logout url log you out of that provider only. For now, this seems like a pretty good way for things to work, and we can implement that pretty easily if we ever need it, so I think we may as well leave this as is if @dannon agrees.

Add support for logging out of OIDC Identity Provider

[WIP] Backporting PR #9162 into 20.01

Add support for logging out of OIDC Identity Provider

[20.01] Backporting PR #9162 into 20.01 branch

machristie added 2 commits December 22, 2019 13:54

Add idp logout support

f3aa649

Using constant for name of provider cookie

4f9fe14

galaxybot added the triage label Dec 23, 2019

galaxybot added this to the 20.05 milestone Dec 23, 2019

machristie added 2 commits December 24, 2019 11:01

Fixing JS formatting

9361f13

FIx py3 compat and add unit test

757cf8c

nuwang reviewed Dec 27, 2019

View reviewed changes

machristie added 6 commits December 30, 2019 16:42

Switch to asbool to convert str -> bool

5889294

Document AuthnzManager.logout

4d31f88

Adding missing brace

c9b9feb

Use separate route for getting the logout url for logged in oidc prov…

e2c1881

…ider

Merge remote-tracking branch 'upstream/dev' into idp-logout

0792bcd

Change post logout url to go to login page, matching normal logout flow

b1023cc

dannon self-requested a review January 15, 2020 15:30

nuwang approved these changes Jan 28, 2020

View reviewed changes

dannon added 2 commits January 28, 2020 14:01

Merge remote-tracking branch 'upstream/dev' into idp-logout

947b9ec

Update logout url.

5e84b8c

dannon added area/auth Authentication and authorization kind/enhancement and removed triage labels Jan 28, 2020

dannon approved these changes Jan 28, 2020

View reviewed changes

dannon merged commit 299a729 into galaxyproject:dev Jan 29, 2020

almahmoud pushed a commit to almahmoud/galaxy that referenced this pull request Mar 2, 2020

Merge pull request galaxyproject#9162 from machristie/idp-logout

9155c68

Add support for logging out of OIDC Identity Provider

dannon added a commit that referenced this pull request Mar 3, 2020

Merge pull request #9473 from almahmoud/idp-logout

6a61e7c

[WIP] Backporting PR #9162 into 20.01

almahmoud pushed a commit to almahmoud/galaxy that referenced this pull request Mar 4, 2020

Merge pull request galaxyproject#9162 from machristie/idp-logout

6abc96f

Add support for logging out of OIDC Identity Provider

mvdbeek added a commit that referenced this pull request Mar 4, 2020

Merge pull request #9488 from almahmoud/idp-logout

7f2686c

[20.01] Backporting PR #9162 into 20.01 branch

hexylena mentioned this pull request Jun 2, 2020

Release notes for 20.05 #9838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for logging out of OIDC Identity Provider #9162

Add support for logging out of OIDC Identity Provider #9162

machristie commented Dec 23, 2019

nuwang Dec 27, 2019

machristie Dec 30, 2019

nuwang Dec 27, 2019

machristie Dec 30, 2019

nuwang Dec 27, 2019

machristie Dec 30, 2019

nuwang Jan 2, 2020

machristie Jan 5, 2020

machristie Jan 12, 2020

nuwang left a comment

dannon commented Jan 28, 2020 •

edited

Loading

dannon commented Jan 28, 2020

machristie commented Jan 30, 2020

nuwang commented Jan 30, 2020

Add support for logging out of OIDC Identity Provider #9162

Add support for logging out of OIDC Identity Provider #9162

Conversation

machristie commented Dec 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuwang left a comment

Choose a reason for hiding this comment

dannon commented Jan 28, 2020 • edited Loading

dannon commented Jan 28, 2020

machristie commented Jan 30, 2020

nuwang commented Jan 30, 2020

dannon commented Jan 28, 2020 •

edited

Loading