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

Password reset should confirm users email #94

Open
gobijan opened this issue Nov 20, 2023 · 5 comments
Open

Password reset should confirm users email #94

gobijan opened this issue Nov 20, 2023 · 5 comments

Comments

@gobijan
Copy link
Contributor

gobijan commented Nov 20, 2023

I had a look at:

#80

In case a user forgot his password but never received a confirmation mail (be it some junk filter or broken mailer) he is basically locked out. On my applications I change it to still send out a password recovery link and when the link is used the user is confirmed automatically. This makes sense in my opinion, what do you think @lazaronixon .

Let me know if I should send in a PR :)

@lazaronixon
Copy link
Owner

lazaronixon commented Nov 20, 2023

Authentication zero follows the hey.com workflow which means that:

1 - The session never expires, so you can resend the email confirmation again in the future.
2 - To verify your email you must be signed in.
3 - To receive a "password reset" you must verify your email account.

The only problem I see is that if a user signs up with the wrong email, another person can take control of your account, I don't want to change this behavior by now but it's up to you if you can live with that, no problem.

image

@gobijan
Copy link
Contributor Author

gobijan commented Nov 27, 2023

Valid point.
What would be the best approach to go for this deadlock situation:

  1. User signs up and uses service (usage is not prevented by default before email verification).
  2. Confirmation Mail never appeared in his inbox.
  3. Time passes.
  4. User wants to comes back on another system (or browser etc.) but forgot his password.
  5. Can't send recovery mail because he's not confirmed.

Then there is no way to retrigger the confirmation mail out of the box isn't it?
Should there then be a resend confirmation mail path?
What do you think?

CleanShot 2023-11-27 at 15 00 47

A resend of the confirmation email could be triggered here automatically?

@lazaronixon
Copy link
Owner

lazaronixon commented Nov 27, 2023

There's a link to resend email verifications, but the user must be signed in. In your case, the user can contact the support team and they can resend the confirmation email. Of course, there are other options:

1 - Remove the requirement to only send password resets from verified accounts.
2 - Remove the email confirmation process, it worked for Basecamp. it's so 2000's
3 - Prevent sign-in from unverified users as Devise does.
4 - A recurrent job to resend verification emails =/

@kirley
Copy link

kirley commented Mar 4, 2024

I ran into this lock scenario today while testing and I feel like the current functionality basically tells hackers what email addresses exist in the system and which ones don't.

This code generated by the gem feels like a security gap to me:
app/controllers/identity/password_resets_controller.rb

  def create
    if @user = User.find_by(email: params[:email], verified: true)
      send_password_reset_email
      redirect_to sign_in_path, notice: "Check your email for reset instructions"
    else
      redirect_to new_identity_password_reset_path, alert: "You can't reset your password until you verify your email"
    end
  end

My suggestion to fix this would be to allow password resets without email verification.
app/controllers/identity/password_resets_controller.rb

  def create
    if @user = User.find_by(email: params[:email])
      send_password_reset_email
    end
    redirect_to sign_in_path, notice: "Check your email for reset instructions"
  end

And then automatically re-send the email verification if a user resets their password on an account that isn't verified and remind the user to verify their email when the password reset is confirmed on the screen.

@lazaronixon
Copy link
Owner

lazaronixon commented Mar 5, 2024

@kirley You're not wrong, this is why we have a rate_limit on this endpoint.

<%- if options.lockable? -%>
before_action :require_lock, only: :create
<%- end -%>

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

No branches or pull requests

3 participants