-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add ability to resend instructions #52
Comments
@francirp, I definitely agree with this feature request. I'm surprised we missed it (or maybe we talked about it and punted for a later version, I don't remember). I don't have any questions about the However, for the |
@inveterateliterate well, if you send a new token, it effectively disables prior instructions that are already in the user's inbox. The thought is that by extending the token, you could send another email which would just have the same token. Thus both instructions would be valid rather than just one. I'm sure there is a reason why this is an anti-pattern that should be avoided, but just putting it out there. |
It does feel like an anti-pattern to me, I feel like the standard is to send a new token and you have to use that email. Feels like a security thing? @dpikt any thoughts? |
I don't know enough about token auth to say whether it's a security concern. I do think generating a new token for each email is pretty standard though. |
Sounds good. For now let's just focus on the "resend_x_instructions" concept and we can always circle back to the "extend token" idea in the future if it comes back up. |
My thought process is that For reference, the |
send_x_instructions!
errs out (intentionally) when the instructions have already been sent. However, we commonly need to implement a "resend invitation" feature since recipients often miss the invitation the first time around.I can think of two approaches to this "resending" functionality:
x_created_at
toTime.now
Perhaps we actually implement the following two methods:
Thoughts @inveterateliterate?
The text was updated successfully, but these errors were encountered: