Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contrib: logout redirection to CERN auth #226

Closed
wants to merge 1 commit into from

Conversation

kpsherva
Copy link
Contributor

No description provided.

@cern_oauth_blueprint.route('/cern_openid/logout/')
def logout_redirect():
"""Redirect to the CERN OpenID logout."""
return redirect(current_app.config['OAUTH_REMOTE_REST_APP']['logout_url'])
Copy link
Member

Choose a reason for hiding this comment

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

What is the code that will return?

Suggested change
return redirect(current_app.config['OAUTH_REMOTE_REST_APP']['logout_url'])
return redirect(current_app.config['OAUTH_REMOTE_REST_APP']['logout_url'], code=302)

Copy link
Member

Choose a reason for hiding this comment

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

I think is the case already when you return the redirect :)

Copy link
Member

Choose a reason for hiding this comment

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

@topless Actually, you might be right 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

It is 302 by default... but yeah better safe than sorry :)

@@ -90,20 +94,24 @@
OAUTHCLIENT_CERN_OPENID_ALLOWED_ROLES = ["cern_user"]
"""CERN OAuth application role values that are allowed to be used."""

_ACCOUNTS_REST_AUTH_VIEWS.update(
logout="invenio_oauthclient.contrib.cern_openid:CERNOpenIDLogoutView")
ACCOUNTS_REST_AUTH_VIEWS = _ACCOUNTS_REST_AUTH_VIEWS
Copy link
Member

Choose a reason for hiding this comment

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

Can we just skip the whole _ACCOUNTS_REST_AUTH_VIEWS? I think it is just an alias, so we could just

from invenio_accounts.config import ACCOUNTS_REST_AUTH_VIEWS
    
ACCOUNTS_REST_AUTH_VIEWS.update(
    logout="invenio_oauthclient.contrib.cern_openid:CERNOpenIDLogoutView")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had problems with updating the config variables in this way in the past, they were not overwritten correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be a problem here: it looks like that ACCOUNTS_REST_AUTH_VIEWS is global. So if you have configured multiple login method, e.g. local accounts, GitHub and CERN, they will all use this class.
@zzacharo can you maybe give your advice?
No idea how to solve it at the moment...

Copy link
Member

Choose a reason for hiding this comment

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

Why don't you decouple this behaviour in the UI. For example:

  • SPA logout from accounts
  • SPA logout from OAuth

@cern_oauth_blueprint.route('/cern_openid/logout/')
def logout_redirect():
"""Redirect to the CERN OpenID logout."""
return redirect(current_app.config['OAUTH_REMOTE_REST_APP']['logout_url'])
Copy link
Member

Choose a reason for hiding this comment

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

@kprzerwa So, after the redirect to the logout page of OAuth, are you staying on that page, i.e not returning back to the SPA?

@kpsherva kpsherva closed this Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants