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

Allow user authentication from email links #858

Closed

Conversation

nicoolas25
Copy link
Collaborator

@nicoolas25 nicoolas25 commented May 23, 2021

Summary

As part of #829 I would like to authenticate users right from emails. This PR sets the foundations to do this.

So far we authenticated users using matches' tokens or slot_alerts's tokens.
Now we need users' tokens: we need communication unrelated to any of the above events.

Details

No functional changes should be part of this PR.

It adds a concern for controllers UserAuthenticationViaSignedId that complements the usual devise/warden authentication using an authentication_token based on globalid.

It creates a single partial to DRY those routes:

  • users#destroy,
  • matches/users#destroy, and
  • slot_alerts/users#destroy.

Also DRY those views:

  • slot_alerts/users#edit, and
  • matches/users#edit

... into users/_confirm_destroy_message.

The goal is to reuse confirm_destroy_profile_path from email and sign it with an authentication token.

This users#confirm_destroy action looks like this:

Screenshot from 2021-05-23 10-53-57

Poke @mininao as you authored the original issue.

The next steps are:

  1. Create the rake task to identify the appropriate users
  2. Create emails to send to those users, include a link to confirm_destroy_profile_path with an auth token

Comment on lines +71 to +72
current_user.anonymize!
sign_out current_user
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I inverted those two lines, is that okay?

Comment on lines +6 to +7
<% token = user.signed_id(purpose: "users.destroy", expires_in: 1.hour) %>
<%= button_to "Supprimer mon compte", profile_path(authentication_token: token),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is where the magic happens ✨

@nicoolas25 nicoolas25 force-pushed the unsubscribe-automated-emails branch from f2652bf to c95f44f Compare May 23, 2021 09:20
@nicoolas25
Copy link
Collaborator Author

nicoolas25 commented May 23, 2021

I had a flaky spec that I fixed in the last commit.

While running the spec in a loop I had intermittent failures:

Screenshot from 2021-05-23 12-17-46

Comment on lines +75 to +81
# This prevents flaky specs when the change in lat/lng is zero (1/200)
allow_any_instance_of(RandomizeCoordinatesService)
.to receive(:generate_random_delta)
.and_wrap_original do |method, *args|
original_result = method.call(*args)
original_result.zero? ? 0.001 : original_result
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another approach would be to update the randomness seed but it would impact the production code a bit too much so I went for this.

@carsso
Copy link
Member

carsso commented May 23, 2021

Je t'ai invité sur le repo.
Tu peux faire une branche directement sur le repo pour que les tests puissent tourner ?
généralement on préfixe nos branches avec le pseudo github

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants