Added Globus-Auth as an identity provider #1
Conversation
| return user_info | ||
|
|
||
|
|
||
| def get_user_id(remote, email): |
There was a problem hiding this comment.
Invenio binds user ids to usernames, so the Globus ID is also needed. This results in this second required call. The information is stored within the id token, but one of the advantages to hitting the REST APIs directly in this case is we don't require any other dependencies to look inside the OIDC token or the Globus API.
There was a problem hiding this comment.
Is it potentially problematic that Globus Auth explicitly states that the mapping between identity username and ID is not fixed over time?
https://docs.globus.org/api/auth/reference/#get_v2_api_identities_usernames_lt_list_of_identity_names_gt
There was a problem hiding this comment.
Also, are we guaranteed to get the primary identity at index zero of the list from v2/api/identities? it's not clear from the doc that order is guaranteed. It may be possible for a Globus Auth user to have more than one identity that uses the same email address, in which case, I'm not sure that this get_user_id function necessarily returns what we want.
There was a problem hiding this comment.
I didn't think it was possible for one identity username to map to multiple ids. The example shows calling in with multiple identity usernames and returning a bunch of corresponding ids to those usernames.
For mapping identity usernames to IDs? That's a good question. If the identity username were to change hands, would the ID also change to something else thereby invalidating the user account on Invenio? Something I'm trying to confirm is that users in Invenio are tracked by external provider id rather than by username.
| globus=globus.REMOTE_APP, | ||
| ), | ||
| GLOBUS_APP_CREDENTIALS=GLOBUS_APP_CREDENTIALS, | ||
| DEBUG=True, |
There was a problem hiding this comment.
Do we want DEBUG=True in general? Or is this leftover from development?
There was a problem hiding this comment.
I mostly followed the examples from all the other apps, which all set DEBUG to True.
| SECURITY_PASSWORD_SALT='security-password-salt', | ||
| MAIL_SUPPRESS_SEND=True, | ||
| TESTING=True, | ||
| USERPROFILES_EXTEND_SECURITY_FORMS=True, |
There was a problem hiding this comment.
A couple notes on Config options.
- Mail throws an error when a user logs in due to a bug in Flask-Security. However, Invenio is aware of the problem here. It's probably better to apply the fix in one of the dependent packages instead to avoid duplication.
- MAIL_SUPPRESS_SEND also throws an error if it's set to
False. This happens very rarely for logged in users and is probably better handled in downstream packages. I'm not completely sure why editing this value results in an error, but it seemed to be a caching error here - SQLALCHEMY_TRACK_MODIFICATIONS throws a bunch of warnings, but should also probably be left to dependent packages to configure for themselves. We could set this in tests to squelch the warning messages on tests.
|
|
||
| SPHINX-START | ||
|
|
||
| 1. Register a Globus application at `https://developers.globus.org/` with the |
There was a problem hiding this comment.
Add reference to the app registration instructions
There was a problem hiding this comment.
There is a link on line 32, should it be more obvious?
There was a problem hiding this comment.
I think it's better to reference the app registration portion directly by adding the #register-app to the URL. More broadly, we should think about whether this is really just a developer activity. Deploying something like Invenio is a sys admin task.
There was a problem hiding this comment.
Good point, I added links to those specific sections
| consumer_secret='changeme', | ||
| ) | ||
|
|
||
| 2. Register a Globus application at `https://developers.globus.org/` with the |
There was a problem hiding this comment.
Same here, reference the registration portion of the docs.
| @@ -88,6 +94,7 @@ def base_app(request): | |||
| DEBUG=False, | |||
| SECRET_KEY='TEST', | |||
| SECURITY_DEPRECATED_PASSWORD_SCHEMES=[], | |||
| SQLALCHEMY_TRACK_MODIFICATIONS=False, | |||
There was a problem hiding this comment.
I should note that this isn't part of Globus Auth, but tracking modifications in tests isn't needed and setting this variable prevents a large number of warnings produced every time a test is run.
|
Looks good to me, now. |
NickolausDS
force-pushed
the
globus-auth
branch
from
73133df to
c1e9a13
Compare
* Globus Login work for any provider Globus Auth supports * Added tests, coverage of new code is 100% * Added Sphinx docs
NickolausDS
force-pushed
the
globus-auth
branch
from
c1e9a13 to
0e52345
Compare
|
I went ahead and rebased on the latest upstream changes and squashed the commits into one, along with a few descriptive bullet points. This should be good to go. |
|
@ericblau @rpwagner @lukaszlacinski Closing this PR and opening up the official one. Thanks for all your feedback! |
No description provided.