Skip to content

Added fixes to Token Handling#8

Open
NickolausDS wants to merge 3 commits intofair-research:masterfrom
NickolausDS:introspection-fixes
Open

Added fixes to Token Handling#8
NickolausDS wants to merge 3 commits intofair-research:masterfrom
NickolausDS:introspection-fixes

Conversation

@NickolausDS
Copy link
Copy Markdown

Previous implementation did not properly do token introspection on
incoming tokens due to them being the wrong token type. The new
implementation only accepts tokens from the minid defined scope.

Previous implementation did not properly do token introspection on
incoming tokens due to them being the wrong token type. The new
implementation only accepts tokens from the minid defined scope.
Comment thread README.rst
GLOBUS_AUTH_ENABLED = False

GLOBUS_CLIENT_ID = "a5cdede9-567f-4ae5-aba5-cb997d08693a"
GLOBUS_CLIENT_SECRET = ""
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The GLOBUS_AUTH_ENABLED config option was removed. Not including a Client ID or Secret does effectively the same thing.

Copy link
Copy Markdown

@ericblau ericblau left a comment

Choose a reason for hiding this comment

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

I find it slightly confusing in minid_server/api/utils.py that
when you do:
code = authorization_header.split()
the variable code actually contains the bearer token that you later introspect, doesn't it? Thus I'd prefer "token" as a variable name, or something similar.

@NickolausDS
Copy link
Copy Markdown
Author

the variable code actually contains the bearer token that you later introspect, doesn't it? Thus I'd prefer "token" as a variable name, or something similar.

Good point! I also notice that type is used as a variable name which also bothers me now that I look at it, I'll change both var names.

Comment thread minid_server/api/utils.py
'code.', user=email,
type='AuthorizationFailed')
ids = client.get_identities(ids=info.data['identity_set'])
linked_emails = [str(u_id['email'])
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Originally this was set to username, which isn't what we want. Sometimes usernames match up with emails, but not always. One example is Globus IDs, which could have a user with the id of malcomreynolds@globusid.org and email of malcomr@globus.org.

@NickolausDS
Copy link
Copy Markdown
Author

@kylechard Question for you: do you know how many users are using the old auth tokens? The NIH Commons project just got setup to use them, and I'm not sure how many more folks are also using the old system. For those users these changes are breaking (although the only thing they need to do is switch their servers to request the minid scope, and re-login with their minid clients).

An alternative is we still allow the old token functionality for the time being and have the minid client log a deprecation warning when someone tries to use it.

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.

2 participants