client: add logout blueprint #304
Conversation
jrcastro2
commented
May 26, 2023
jrcastro2
commented
- closes [DEPENDS ON #invenio-app-rdm/2186] CERN e-groups integration CERNDocumentServer/cds-rdm#16
jrcastro2
force-pushed
the
add-logout-blueprint
branch
2 times, most recently
from
b050e25 to
bb41165
Compare
invenio_oauthclient/handlers/base.py
Outdated
|
|
||
| # We set the remote in the session to be aware of which one is being used and, on log out redirect to | ||
| # the correct URL set in the OAUTHCLIENT_REMOTE_APPS for each remote | ||
| session["remote_name"] = remote.name |
There was a problem hiding this comment.
does it make sense to namespace this, with a prefix?
See for example current_app.config["OAUTHCLIENT_SESSION_KEY_PREFIX"], I don't know if it makes much sense.
There was a problem hiding this comment.
Agree on this one... Also, what happens e.g. when you connect multiple OAuth providers to your account? Would it be only the last one that ends up in the session variable? Would it make sense to keep a list/set of remotes like so:
session.setdefault("remote_names", set()) # not sure if set is supported
session["remote_name"].add(remote.name)Then you could in logout check which of the remotes has a logout_url specified and again make an arbitrary choice, e.g. the first one...
This sounds a bit overcomplicated, and I'm not sure if it makes perfect sense though.
There was a problem hiding this comment.
Prefixed as requested.
I am not sure if multiple accounts connected affects this, the remote_name is set on loging and although your account might be connected to multiple OAuth providers you always log in with only one of the, or am I wrong?
However, if I am wrong, looks to me that could be done in another issue as an improvement (?)
| @@ -271,3 +271,19 @@ def rest_disconnect(remote_app): | |||
| return _disconnect(remote_app) | |||
| except OAuthRemoteNotFound: | |||
| abort(404) | |||
|
|
|||
|
|
|||
| @blueprint.route("/logout") | |||
There was a problem hiding this comment.
minor: does it make sense to also have a REST API version, i.e. under rest_blueprint, for this view similar to the others above?
There was a problem hiding this comment.
If we decide to got for this I think we should split between logout_url´ and rest_logout_url` because I guess while using the rest you would like to redirect to rest endpoint as well ?
I think this could be done as an improvement, WDYT?
There was a problem hiding this comment.
lets do as an improvement and ship this one ( I have already created a ticket )
invenio_oauthclient/handlers/base.py
Outdated
|
|
||
| # We set the remote in the session to be aware of which one is being used and, on log out redirect to | ||
| # the correct URL set in the OAUTHCLIENT_REMOTE_APPS for each remote | ||
| session["remote_name"] = remote.name |
There was a problem hiding this comment.
Agree on this one... Also, what happens e.g. when you connect multiple OAuth providers to your account? Would it be only the last one that ends up in the session variable? Would it make sense to keep a list/set of remotes like so:
session.setdefault("remote_names", set()) # not sure if set is supported
session["remote_name"].add(remote.name)Then you could in logout check which of the remotes has a logout_url specified and again make an arbitrary choice, e.g. the first one...
This sounds a bit overcomplicated, and I'm not sure if it makes perfect sense though.
jrcastro2
force-pushed
the
add-logout-blueprint
branch
from
bb41165 to
d159d6e
Compare
