(PXP-7539): Fix/populate idp for user info #877
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Pull Request Test Coverage Report for Build 10506
💛 - Coveralls |
fence/auth.py
Outdated
|
|
||
| Args: | ||
| request (flask.request): not currently used by this function, TODO |
There was a problem hiding this comment.
I was wondering if there were any thoughts on removing request from the args? It’s not used in login_user, but somehow I’d still be cautious removing it given the number of places that supply it (I counted 7 across all of Fence).
There was a problem hiding this comment.
i'd definitely remove it to clean up the codebase - if something breaks, Jenkins will let us know
| idp = IdentityProvider(name=provider) | ||
| user.identity_provider = idp | ||
| current_session.add(user) | ||
| current_session.commit() |
There was a problem hiding this comment.
we only need to do this commit if a) the user is completely new OR B) the user existed but the idp was not previously there. e.g. doing it every time like this I don't think is necessary and will be a bit of a performance hit
There was a problem hiding this comment.
This should now be addressed above by
if user.identity_provider and user.identity_provider.name == provider:
set_flask_session_values(user)
return
johnfrancismccann
added
test-google-googleDataAccessTest
| if user.identity_provider and user.identity_provider.name == provider: | ||
| set_flask_session_values(user) | ||
| return | ||
| else: |
There was a problem hiding this comment.
you can actually remove this else statement and un-indent the next line, b/c you're doing an early return above
There was a problem hiding this comment.
This else is paired with if user:, so imagine a flow where a user does exist but doesn't have an identity provider. In that case we wouldn't necessarily want to overwrite the existing user with the un-indented user = User(username=username).
There was a problem hiding this comment.
🤦 you're right, I didn't look closely enough
PXP-7539
So that the
/userendpoint is populated with idp info, make modifications tologin_userfunction.Please see ticket above for more details.
Bug Fixes
/userendpoint so that idp field is populated for the user.Improvements
login_userfunction