Bugfix: allow Clever takeover if email is already taken #20519
Conversation
| user.email = auth.info.email unless user.user_type == 'student' && auth.provider == 'clever' | ||
|
|
||
| if auth.provider == 'clever' && User.find_by_email_or_hashed_email(user.email) | ||
| user.email = user.email + '.cleveremailalreadytaken' |
There was a problem hiding this comment.
Possible bad edge case, I'm not sure though.
If we're creating a new student account, line 625 is a no-op so user.email is nil or blank?
Then if find_by_email_or_hashed_email finds a user with a blank email (presumably possible?) then line 628 sets the email to nil.cleveremailalreadytaken or just .cleveremailalreadytaken. We'd also probably end up with lots of users with that email, which might cause validation errors/conflicts later?
There was a problem hiding this comment.
I just did a check on prod:
irb(main):013:0> User.find_by_email_or_hashed_email('')
=> nil
irb(main):014:0> User.find_by_email_or_hashed_email(nil)
=> nil
Seems like we might be okay?
There was a problem hiding this comment.
Will we stay okay though? Assuming 627 evaluates false we don't set user.email at all, so then in the future do we end up finding a user with an empty email?
| @@ -95,5 +95,6 @@ | |||
| cleverLinkFlag: "#{session['clever_link_flag']}", | |||
| userIDToMerge: "#{session['clever_takeover_id']}", | |||
| mergeAuthToken: "#{session['clever_takeover_token']}", | |||
| forceConnect: "#{session['force_clever_takeover']}", | |||
There was a problem hiding this comment.
Does this need to be a string, or can we convert it to a boolean here right where the value gets stored in JavaScript, instead of inside the React component?
There was a problem hiding this comment.
Good call, will make this change.
There was a problem hiding this comment.
Actually, I tried to fix this, and it seems to have seriously broken some stuff, so I'm going to put this off - ultimately this code will be going away, so it'll disappear at that point.
|
Going to go ahead and merge for bug bash, will revert if we find issues - the failing test seems unrelated to my changes. |
This should resolve a lot of the Zendesk tickets that we have, because we don't currently allow Clever account takeover when the email is already in use. Now there will be a flow that allows (and in the case of teachers, forces) people to connect if their email is already in our system.