Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let users sign in with a known provider before linking a new provider to the user #3670

Merged
merged 45 commits into from
Jun 28, 2022

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented May 30, 2022

This pull request forces a user to confirm their identity before linking their new sign in method to an existing user.

The steps followed in the default scenario are as followed:

  1. An existing users signs in using a new provider
  2. We search for an existing identity for this user. (None can be found because it is the first sign in of this user using this provider)
  3. Try to find an existing user in the same institution matching username or email (Which is found)
  4. Check if user does not already have an identity for this provider
  5. Save identity details and user id in session
  6. Ask user to confirm that they have access to this user by signing in using a previously used login method
  7. User signs in using known provider
  8. Check if identity in session matches current signed in user
  9. Add new identity to user
  10. Notify user

image
image

I also added the linked sign in methods to the user detail screen
image

Misc

  • I created a separate user_short partial for reuse (It is only used at a single place at the moment)
  • I removed a failing test which was made specifically for merging token users into real users. As discussed this usecase does not happen
  • flash messages can now contain multiple links instead of one

Closes #3668 .

@jorg-vr jorg-vr added the feature New feature or request label May 30, 2022
@jorg-vr jorg-vr self-assigned this May 30, 2022
@jorg-vr jorg-vr temporarily deployed to naos May 31, 2022 12:20 Inactive
@jorg-vr jorg-vr temporarily deployed to naos May 31, 2022 12:35 Inactive
@jorg-vr jorg-vr temporarily deployed to naos May 31, 2022 12:41 Inactive
@jorg-vr jorg-vr marked this pull request as ready for review May 31, 2022 14:44
@jorg-vr jorg-vr requested a review from a team as a code owner May 31, 2022 14:44
@jorg-vr jorg-vr requested review from niknetniko and chvp and removed request for a team May 31, 2022 14:44
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

I would prefer this to be on a separate page or at least a modal instead of a flash/alert. This is an important action for the user and it feels somewhat wrong to stash this away like this.

If we make linking more transparent like this, it might also be a good idea to list the linked providers on the profile page of a user?

@jorg-vr jorg-vr marked this pull request as draft June 7, 2022 08:05
@jorg-vr jorg-vr temporarily deployed to naos June 7, 2022 08:38 Inactive
@jorg-vr jorg-vr temporarily deployed to naos June 7, 2022 08:53 Inactive
@jorg-vr jorg-vr temporarily deployed to naos June 8, 2022 08:51 Inactive
@jorg-vr jorg-vr temporarily deployed to naos June 8, 2022 08:59 Inactive
@jorg-vr jorg-vr temporarily deployed to naos June 8, 2022 09:04 Inactive
@jorg-vr jorg-vr temporarily deployed to naos June 8, 2022 13:00 Inactive
@jorg-vr jorg-vr requested a review from niknetniko June 8, 2022 13:26
@jorg-vr jorg-vr temporarily deployed to naos June 8, 2022 13:39 Inactive
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Can you remove the drop shadow from the providers on the profile page? They look a lot like buttons now which they aren't. I think outlined-cards could work here, but I don't think I implemented them yet in the m3 refactor.

config/locales/views/auth/nl.yml Outdated Show resolved Hide resolved
Base automatically changed from merge-users-on-sign-in to develop June 28, 2022 06:56
jorg-vr and others added 2 commits June 28, 2022 08:57
Co-authored-by: Charlotte Van Petegem <[email protected]>
This was one of the things I used to test these changes, but no reason
to not commit it.
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Looks good. I added a test I used to review this, and there was no reason to not commit it.

@jorg-vr jorg-vr merged commit 5d50323 into develop Jun 28, 2022
@jorg-vr jorg-vr deleted the link-users-securely branch June 28, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't link a new user with an existing email address when they are from the same provider
5 participants