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 re-sending of magic link emails #137

Conversation

DaanVanVugt
Copy link

When the config setting Passwordless.allow_token_resend = true a visit to /users/sign_in/TOKEN_URL for an expired or already claimed user will return a page with a button which can be used to resend an email with this token. Extra care is taken to record a query parameter destination_path into the current session, so opening the new email immediately from the same browser redirects you to the proper location.

When the config setting Passwordless.allow_token_resend = true a visit
to /users/sign_in/TOKEN_URL for an expired or already claimed user will
return a page with a button which can be used to resend an email with
this token. Extra care is taken to record a query parameter
`destination_path` into the current session, so opening the new email
immediately from the same browser redirects you to the proper location.
@DaanVanVugt DaanVanVugt force-pushed the feature/magic-link-resend-button branch from 15fa930 to 1237a8b Compare April 14, 2023 07:49
@mikker
Copy link
Owner

mikker commented Jun 17, 2023

I think this solution complicates things too much. This problem could be solved, slightly worse but way simpler, with just providing a description and link to do the sign in from the start again.

@mikker mikker closed this Jun 17, 2023
@DaanVanVugt
Copy link
Author

The reason that was not enough for us, is that we send transactional emails containing magic links with destinations (an update on a ticket, for instance). Just re-requesting through the login page would drop the location information.

@mikker
Copy link
Owner

mikker commented Jun 19, 2023

That use case makes sense. Could you not create a new session for each notification?

@DaanVanVugt
Copy link
Author

We do, like this (in ApplicationMailer, to save as @magic_link and use in templates)

  def magic_link(user, object)
    session = Passwordless::Session.new({
                                          authenticatable: user,
                                          user_agent: 'ApplicationMailer',
                                          remote_addr: 'unknown'
                                        })
    session.save!
    @magic_link = send(Passwordless.mounted_as).token_sign_in_url(session.token)
    @magic_link = "#{@magic_link}?destination_path=#{polymorphic_url(object)}" if object
  end
  • If the magic link is valid, the user gets redirected.
  • If expired, the user needs to request a new magic link. But, the sent email needs to contain a destination_path, or we should read it from the rails session storage.
    This path was not yet saved to the session, since we only saved that on access denied pages.

@mikker
Copy link
Owner

mikker commented Jun 19, 2023

Two ideas:

  1. Make the session's expiry super long, 3 months / 1 year / whatever. This makes them unlikely to have expired and problem solved without this.
  2. Build something custom, a new controller, maybe a db model too, that handles all your logic for this. It's fairly easy to still use Passwordless with Passwordless::ControllerHelpers. Maybe outright copy Passwordless' SessionsController to use as a starting point.

I'm not convinced this is common enough (?) to include as an option in Passwordless.

@mikker
Copy link
Owner

mikker commented Jun 19, 2023

Just my opinion of course, but, your example from above seems to go out of its way to use Passwordless' bundled functionality. I think you could have something simpler by only using Passwordless when necessary, like the call to sign_in and handling the session going forward and all that.

Like, there's no reason to do send(Passwordless.mounted_as).token_sign_in_url in your app. You know what it's mounted as.

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.

None yet

2 participants