Issue: user changed google account and can't connect thru his profile

(Sam Saffron) #8

The workaround is simple, but I do want to sort out the root cause

./launcher enter app
rails c
GoogleUserInfo.where(user_id: 2512).destroy_all
1 Like

(Mykhailo Poliarush) #9

user error looks like this

0 Likes

(Catherine Zavala) #10

Hi, I have a related issue where we need to change the email for a user account in Discourse. I understand I can do this as Admin. This generates a confirmation email to the old email address. The problem is the old email address is no longer a valid address so we are getting Delivery Failure notices.

I saw this post by Kane York- part time developer on Jan '15
"I think you can change it at the external site, then have them log out/log in again? I think that will resync the email address"

Does this mean if I have the user on hand to log in as soon as I reset their password, we can bypass the email confirmation issue?

0 Likes

(Kane York) #11

That was in reference to native SSO, not OAuth logins. It sounds like there’s something different between the Github and Google authenticators causing this issue.

0 Likes

(Michael Brown) #12

We just had this happen again and I think I have a pretty good idea of how it happens:

  • user has email: user@gmail.com, google login: user@gmail.com
  • user changes email to user@discourse.org
  • user logs in with google account user@discourse.org
  • Discourse sees existing account with email user@discourse.org, tries to associate the login with existing (correct) user
  • fails as user already has google account user@gmail.com associated with the Discourse account
3 Likes

(Michael Brown) #14

For future reference, if a user runs into this problem when logging in on an iPad, they get a purely white screen with no error message or any sort of indication of a problem.

2 Likes

(Michael - DiscourseHosting.com) #15

We had a customer running into this as well.

  • User got deactivated in Discourse
  • User got deactivated in Google Apps
  • After some time the User got re-activated in Google Apps (different email address)
  • We reactivated the account on Discourse as well
  • User attempts to log in with Google SSO
  • The process fail with 500 Error

Seems to me that this is caused by blindly trying to create a new record, instead of updating an existing one if that is there (or maybe blindly deleting/creating will work as well)

Lines 24 and 33:

1 Like

(Jeff Atwood) #16

How do you figure, if records are keyed on email? If the email changed…

0 Likes

(Michael - DiscourseHosting.com) #17

Not entirely sure how that part works, but the main issue is that a GoogleUserInfo record is created for a certain user id, while such a record already exists, causing a unique key violation on user_id.

So instead of

::GoogleUserInfo.create({ user_id: result.user.id }.merge(google_hash))

this should be something like (update scenario)

user_info = ::GoogleUserInfo.find_by(user_id: result.user.id)
if user_info 
  # update the record, dunno how that would look like
else
  ::GoogleUserInfo.create({ user_id: result.user.id }.merge(google_hash))
end

or alternatively something like (blindly delete scenario)

user_info = ::GoogleUserInfo.find_by(user_id: result.user.id)
::GoogleUserInfo.delete({ user_id: result.user.id }) if user_info
::GoogleUserInfo.create({ user_id: result.user.id }.merge(google_hash))

I’m not very fluent with ActiveRecord, maybe there is a better way. But you get the idea :slight_smile:

2 Likes

(Michael Brown) #18

I think we should do two things here:

  • when the user changes their email, scrub their GoogleUserInfo record if it exists. This avoids the common scenario of “I changed my email and now I can’t login via Google”
  • when there is a conflict, report a proper error with a useful way forward

EDIT: after further consideration, if the user can login to the Google account then they have access to the account’s email and could perform a local password reset to gain access (if enabled). So I don’t think we’re introducing any problems by scrubbing any existing GoogleUserInfo record if BOTH the google_user_id and email address are different.

3 Likes

(Michael Brown) #19

Implemented as described - if the google account ID AND email changes, it’ll be replaced, but if only the account ID changes the user will get an error:

5 Likes

User has changed their Google account and cannot login
(Kane York) #20

You should probably create a canonical set of directions for how to resolve the error if you’re going to be pointing people to this topic in the error message.

3 Likes

(Michael Brown) #21

Absolutely… I thought of this as well but hadn’t yet actually done it…

Maybe a support:faq section.

EDIT: It’s a bit of a shift from our current direction, this notion of having an external “online help page” for this kind of situation.

It also doesn’t sit well for translating. But I don’t feel great about having reams of text in the error message itself.

Maybe we just Do It for now and worry about it when it never becomes a problem since it’s so infrequent (we’ve had one report?)

Added: https://meta.discourse.org/t/76575

4 Likes

(David Taylor) #22

@supermathie now that we’re migrating all the auth providers to use the same logic, this logic will need to be refactored / removed.

A lot has changed in the authentication system since 2017. The main thing is that users can connect/disconnect accounts whenever they want.

As I understand it, this change was added to protect against:

  • User connects google account 00001, with email user@example.com, to their discourse account

  • User tries to log into discourse using google account 00002, with email user@example.com

  • Error is shown, must be resolved on the console

Under the new “ManagedAuthenticator”, this would happen:

  • User connects google account 00001, with email user@example.com, to their discourse account

  • User tries to log into discourse using google account 00002, with email user@example.com

  • User logged in, reference to 00001 is scrubbed and replaced with 00002

I believe this is also the same method we follow with core Discourse SSO.

This seems fine to me. If a user is in control of a google account with a matching email address, then we should trust them. Was there a specific attack vector you were trying to protect against with this error message, or can we go ahead and make it match the other authenticators?

6 Likes

(Michael Brown) #23

There was, see:

Wearing my paranoid hat, I felt it was best to at least make someone look into this situation when it happens.

5 Likes

(Bas van Leeuwen) #24

But in that case they could just resend their password and log in that way instead of using OAuth?

This change feels user-hostile, in our corporate environment, the scenario (user leaves company, email is deactivated; user is re-hired and email is re-instanced) is relatively common when we hire freelancers.

0 Likes

(Michael Brown) #25

Fair enough, after some thought I don’t think there’s any benefit here. What do you think @david?

Prior to this change the result was “error”, functionality didn’t change.

If the email is deactivated, the Google account won’t change and you won’t run into this. Only if you delete the user.

3 Likes

(Bas van Leeuwen) #26

Fair enough :smiley:

“Deactivated, don’t know how our IT-department implements that, might be that they delete the google account”
We just encountered this yesterday and I found this recent thread, wanted to share my feedback :slight_smile:

1 Like

(David Taylor) #27

Yeah, the protection here is certainly more ‘cautious’, but as @bas says we put so much trust in email=identity elsewhere that I think it’s fine to clean things up automatically. I’ll be migrating Google over to the new system this week so will make this change.

Also, note that @supermathie’s change was made well over a year ago - I just bumped a very old topic. Things have changed a lot in our authentication system since then.

4 Likes

(Bas van Leeuwen) #28

I did not notice that, thank you :slight_smile:

3 Likes