Revert "Password reset session allows link to be loaded multiple times (

rubygems#4558)" (rubygems#4605)

This reverts commit 1d10606.

app/controllers/passwords_controller.rb

@@ -1,18 +1,17 @@
 class PasswordsController < ApplicationController
   include MfaExpiryMethods
   include WebauthnVerifiable
+  include SessionVerifiable
 
   before_action :ensure_email_present, only: %i[create]
-  before_action :clear_password_reset_session, only: %i[create edit]
 
-  before_action :no_referrer, only: %i[edit]
   before_action :validate_confirmation_token, only: %i[edit otp_edit webauthn_edit]
   before_action :session_expired_failure, only: %i[otp_edit webauthn_edit], unless: :session_active?
   before_action :webauthn_failure, only: %i[webauthn_edit], unless: :webauthn_credential_verified?
   before_action :otp_failure, only: %i[otp_edit], unless: :otp_edit_conditions_met?
   after_action :delete_mfa_expiry_session, only: %i[otp_edit webauthn_edit]
 
-  before_action :validate_password_reset_session, only: %i[update]
+  verify_session_before only: %i[update]
 
   def new
   end
@@ -26,7 +25,8 @@ def edit
 
       render template: "multifactor_auths/prompt"
     else
-      password_reset_verified
+      # When user doesn't have mfa, a valid token is a full "magic link" sign in.
+      verified_sign_in
       render :edit
     end
   end
@@ -43,30 +43,36 @@ def create
   end
 
   def update
-    if @user.update_password reset_params[:password]
-      @user.reset_api_key! if reset_params[:reset_api_key] == "true" # singular
-      @user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true" # plural
-      clear_password_reset_session
-      sign_in @user
+    if current_user.update_password reset_params[:password]
+      current_user.reset_api_key! if reset_params[:reset_api_key] == "true" # singular
+      current_user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true" # plural
       redirect_to dashboard_path
+      session[:password_reset_token] = nil
     else
       flash.now[:alert] = t(".failure")
       render :edit
     end
   end
 
   def otp_edit
-    password_reset_verified
+    verified_sign_in
     render :edit
   end
 
   def webauthn_edit
-    password_reset_verified
+    verified_sign_in
     render :edit
   end
 
   private
 
+  def verified_sign_in
+    sign_in @user
+    session_verified
+    @user.update!(confirmation_token: nil)
+    StatsD.increment "login.success"
+  end
+
   def reset_params
     params.fetch(:password_reset, {}).permit(:password, :reset_api_key, :reset_api_keys)
   end
@@ -80,52 +86,23 @@ def ensure_email_present
   end
 
   def validate_confirmation_token
-    confirmation_token = params[:token] || session[:password_reset_token]
-    @user = User.find_by(id: params[:user_id], confirmation_token:)
-    return token_failure(t("passwords.edit.token_failure")) unless @user&.valid_confirmation_token?
-    session[:password_reset_token] = confirmation_token
+    @user = User.find_by(id: params[:user_id], confirmation_token: params[:token].to_s)
+    redirect_to root_path, alert: t("passwords.edit.token_failure") unless @user&.valid_confirmation_token?
   end
 
   def otp_edit_conditions_met? = @user.mfa_enabled? && @user.ui_mfa_verified?(params[:otp]) && session_active?
 
-  def session_expired_failure = mfa_failure(t("multifactor_auths.session_expired"))
-  def webauthn_failure = mfa_failure(@webauthn_error)
-  def otp_failure = mfa_failure(t("multifactor_auths.incorrect_otp"))
+  def session_expired_failure = login_failure(t("multifactor_auths.session_expired"))
+  def webauthn_failure = login_failure(@webauthn_error)
+  def otp_failure = login_failure(t("multifactor_auths.incorrect_otp"))
 
-  def mfa_failure(message)
+  def login_failure(message)
     flash.now.alert = message
     render template: "multifactor_auths/prompt", status: :unauthorized
   end
 
-  def token_failure(message)
-    clear_password_reset_session
-    redirect_to root_path, alert: message
-  end
-
-  # password reset session is a short lived session that can only perform password reset.
-  def password_reset_verified
-    clear_password_reset_session
-    session[:password_reset_user] = @user.id
-    session[:password_reset_verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
-  end
-
-  def validate_password_reset_session
-    return token_failure(t("passwords.edit.session_expired")) unless password_reset_session_active?
-    @user = User.find(session[:password_reset_user])
-  end
-
-  def password_reset_session_active?
-    session[:password_reset_verification] && session[:password_reset_verification] > Time.current
-  end
-
-  def clear_password_reset_session
-    session.delete(:password_reset_token)
-    session.delete(:password_reset_user)
-    session.delete(:password_reset_verification)
-  end
-
-  # Avoid leaking token in referrer header (OWASP: forgot password)
-  def no_referrer
-    headers["Referrer-Policy"] = "no-referrer"
+  def redirect_to_verify
+    session[:redirect_uri] = verify_session_redirect_path
+    redirect_to verify_session_path, alert: t("verification_expired")
   end
 end

app/models/user.rb

@@ -207,7 +207,7 @@ def confirm_email!
     update!(email_confirmed: true, confirmation_token: nil)
   end
 
-  # confirmation token expires after Gemcutter::EMAIL_TOKEN_EXPIRES_AFTER
+  # confirmation token expires after 15 minutes
   def valid_confirmation_token?
     token_expires_at > Time.zone.now
   end

app/views/password_mailer/change_password.html.erb

@@ -29,7 +29,7 @@
 </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href="<%= edit_user_password_url(@user, token: @user.confirmation_token.html_safe) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">CHANGE PASSWORD</span></a>
+                          <a href=<%= edit_user_password_url(@user, token: @user.confirmation_token.html_safe) %> target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">CHANGE PASSWORD</span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

app/views/passwords/edit.html.erb

@@ -1,9 +1,9 @@
 <% @title = t('.title') %>
 
 <%= form_for(:password_reset,
-             :url => user_password_path(@user),
+             :url => user_password_path(current_user),
              :html => { :method => :put }) do |form| %>
-  <%= error_messages_for @user %>
+  <%= error_messages_for current_user %>
   <div class="password_field">
     <%= form.label :password, "Password", :class => 'form__label' %>
     <%= form.password_field :password, autocomplete: 'new-password', class: 'form__input' %>

config/locales/de.yml

@@ -421,7 +421,6 @@ de:
       submit: Speichern dieses Passworts
       title: Zurücksetzen des Passworts
       token_failure:
-      session_expired:
     new:
       submit: Zurücksetzen des Passworts
       title: Ändere dein Passwort

config/locales/en.yml

@@ -402,7 +402,6 @@ en:
       submit: Save this password
       title: Reset password
       token_failure: Please double check the URL or try submitting a new password reset.
-      session_expired: Your password reset session has expired.
     new:
       submit: Reset password
       title: Change your password

config/locales/es.yml

@@ -455,7 +455,6 @@ es:
       submit: Guardar esta contraseña
       title: Restablecer contraseña
       token_failure: Por favor verifica la URL o inténtalo nuevamente.
-      session_expired:
     new:
       submit: Restablecer contraseña
       title: Cambiar tu contraseña

config/locales/fr.yml

@@ -421,7 +421,6 @@ fr:
       submit: Enregistrer ce mot de passe
       title: Nouveau mot de passe
       token_failure: Veuillez vérifier l'URL ou réessayer.
-      session_expired:
     new:
       submit: Réinitialisez votre mot de passe
       title: Changement de mot de passe

config/locales/ja.yml

@@ -401,7 +401,6 @@ ja:
       submit: このパスワードを保存する
       title: パスワードをリセット
       token_failure: URLを見返すか、再度送信してみてください。
-      session_expired:
     new:
       submit: パスワードをリセット
       title: パスワードを変更する

config/locales/nl.yml

@@ -407,7 +407,6 @@ nl:
       submit: Sla dit wachtwoord op
       title: reset wachtwoord
       token_failure: Controleer het webadres, en probeer het opnieuw.
-      session_expired:
     new:
       submit: Wachtwoord wijzigen
       title: Wachtwoord wijzigen

config/locales/pt-BR.yml

@@ -418,7 +418,6 @@ pt-BR:
       submit: Salvar senha
       title: Resetar senha
       token_failure: Por favor, confira a URL ou tente submetê-la novamente.
-      session_expired:
     new:
       submit: Resetar senha
       title: Alterar senha

config/locales/zh-CN.yml

@@ -409,7 +409,6 @@ zh-CN:
       submit: 保存该密码
       title: 重置密码
       token_failure: 请再次确认 URL 或尝试重新提交
-      session_expired:
     new:
       submit: 重置密码
       title: 修改您的密码

config/locales/zh-TW.yml

@@ -391,7 +391,6 @@ zh-TW:
       submit: 更新密碼
       title: 重設密碼
       token_failure: 請確認 URL 或再次提交
-      session_expired:
     new:
       submit: 更新密碼
       title: 修改密碼

test/functional/passwords_controller_test.rb

@@ -44,35 +44,19 @@ class PasswordsControllerTest < ActionController::TestCase
       end
     end
 
-    context "with incorrect token" do
-      setup do
-        get :edit, params: { token: @user.confirmation_token.succ, user_id: @user.id }
-      end
-
-      should redirect_to("the home page") { root_path }
-
-      should "not sign in the user" do
-        refute_predicate @controller.request.env[:clearance], :signed_in?
-      end
-
-      should "warn about invalid url" do
-        assert_equal "Please double check the URL or try submitting a new password reset.", flash[:alert]
-      end
-    end
-
     context "with valid confirmation_token" do
       setup do
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should respond_with :success
 
-      should "not sign in the user" do
-        refute_predicate @controller.request.env[:clearance], :signed_in?
+      should "sign in the user" do
+        assert_predicate @controller.request.env[:clearance], :signed_in?
       end
 
-      should "not invalidate the confirmation_token" do
-        refute_nil @user.reload.confirmation_token
+      should "invalidate the confirmation_token" do
+        assert_nil @user.reload.confirmation_token
       end
 
       should "display edit form" do
@@ -202,12 +186,12 @@ class PasswordsControllerTest < ActionController::TestCase
 
         should respond_with :success
 
-        should "not sign in the user" do
-          refute_predicate @controller.request.env[:clearance], :signed_in?
+        should "sign in the user" do
+          assert_predicate @controller.request.env[:clearance], :signed_in?
         end
 
-        should "not invalidate the confirmation_token" do
-          refute_nil @user.reload.confirmation_token
+        should "invalidate the confirmation_token" do
+          assert_nil @user.reload.confirmation_token
         end
 
         should "display edit form" do
@@ -295,8 +279,12 @@ class PasswordsControllerTest < ActionController::TestCase
 
       should respond_with :success
 
-      should "not sign in the user" do
-        refute_predicate @controller.request.env[:clearance], :signed_in?
+      should "sign in the user" do
+        assert_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
+      should "invalidate the confirmation_token" do
+        assert_nil @user.reload.confirmation_token
       end
 
       should "display edit form" do
@@ -424,17 +412,15 @@ class PasswordsControllerTest < ActionController::TestCase
       @old_encrypted_password = @user.encrypted_password
     end
 
-    context "when not verified" do
+    context "when not signed in" do
       setup do
-        session[:password_reset_verification] = nil
-        session[:password_reset_user] = nil
         put :update, params: {
           user_id: @user.id,
           password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
         }
       end
 
-      should redirect_to("the home page") { root_path }
+      should redirect_to("the sign in page") { sign_in_path }
 
       should "not change api_key" do
         assert_equal(@user.reload.api_key, @api_key)
@@ -453,8 +439,9 @@ class PasswordsControllerTest < ActionController::TestCase
         @other_api_key = @other_user.api_key
         @other_new_api_key = create(:api_key, owner: @other_user, key: "rubygems_otheruserkey")
         @other_old_encrypted_password = @other_user.encrypted_password
-        session[:password_reset_verification] = 10.minutes.from_now
-        session[:password_reset_user] = @other_user.id
+        sign_in_as @other_user
+        session[:verification] = 10.minutes.from_now
+        session[:verified_user] = @other_user.id
 
         put :update, params: {
           user_id: @user.id,
@@ -463,8 +450,8 @@ class PasswordsControllerTest < ActionController::TestCase
       end
 
       teardown do
-        session[:password_reset_verification] = nil
-        session[:password_reset_user] = nil
+        session[:verification] = nil
+        session[:verified_user] = nil
       end
 
       should redirect_to("the dashboard") { dashboard_path }
@@ -498,7 +485,7 @@ class PasswordsControllerTest < ActionController::TestCase
         }
       end
 
-      should redirect_to("the home page") { root_path }
+      should redirect_to("the verification page") { verify_session_path }
 
       should "not change api_key" do
         assert_equal(@user.reload.api_key, @api_key)
@@ -508,16 +495,16 @@ class PasswordsControllerTest < ActionController::TestCase
       end
     end
 
-    context "when verified" do
+    context "when signed in" do
       setup do
         sign_in_as @user
-        session[:password_reset_verification] = 10.minutes.from_now
-        session[:password_reset_user] = @user.id
+        session[:verification] = 10.minutes.from_now
+        session[:verified_user] = @user.id
       end
 
       teardown do
-        session[:password_reset_verification] = nil
-        session[:password_reset_user] = nil
+        session[:verification] = nil
+        session[:verified_user] = nil
       end
 
       context "with invalid password" do
@@ -553,7 +540,7 @@ class PasswordsControllerTest < ActionController::TestCase
           end
 
           should set_flash[:alert]
-          should redirect_to("the home page") { root_path }
+          should redirect_to("the verification page") { verify_session_path }
 
           should "not sign the user out" do
             assert_predicate @controller.request.env[:clearance], :signed_in?