feat(email): populate email from IdPs, update deprecated .warn to .wa… #934
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 11228
💛 - Coveralls |
fence/auth.py
Outdated
| f"Updating username {user.username}'s email from {user.email} to {email}" | ||
| ) | ||
| user.email = email | ||
|
|
There was a problem hiding this comment.
If a user already exists in the db and they already have a linked matching identity provider (so, I think, for most cases where a user had logged in before this fence commit), this code won't run, even if the email and the user.email don't match--right? 🧐
There was a problem hiding this comment.
AhH! 😅 You're right, I missed the early return, will adjust. Good eye! 🕵️
There was a problem hiding this comment.
Yay! One more thing, I think with the new call to _update_users_email up top, this second call is unnecessary
|
I deployed this branch to https://fence.planx-pla.net for testing, will let you know if it's successful. |
| @@ -97,8 +100,9 @@ def get(self): | |||
| code = flask.request.args.get("code") | |||
| result = self.client.get_user_id(code) | |||
| username = result.get(self.username_field) | |||
| email = result.get(self.email_field) | |||
There was a problem hiding this comment.
So if I'm reading correctly, this happens to work in the google case because the result happens to contain email -- because when idp is google we use email as username. But in general if the get_user_id code for a given client is not changed so that the returned result has the email field, this part will not work? So for example RAS logins will still not populate the email? I might just be confused on the intended behavior.
There was a problem hiding this comment.
🤦 Good thing you looked this over, me hacking this together resulted in some silly mistakes, thanks for wading through this. Debating whether or not to rename get_user_id something like get_user_info... might just leave it and add the logic to clients
fence/auth.py
Outdated
| """ | ||
| Update email if provided and doesn't match db entry. | ||
|
|
||
| NOTE: This does NOT commit to the db, do so outside this function |
There was a problem hiding this comment.
Oof I think in the case where a user already exists, the early return will now prevent the db commit from happening 😅
fence/auth.py
Outdated
| f"Updating username {user.username}'s email from {user.email} to {email}" | ||
| ) | ||
| user.email = email | ||
|
|
There was a problem hiding this comment.
Yay! One more thing, I think with the new call to _update_users_email up top, this second call is unnecessary
…rning
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes