Fix authentication failures after going halfway through a sign-in attempt (#16607)

* Add tests

* Add security-related tests

My first (unpublished) attempt at fixing the issues introduced (extremely
hard-to-exploit) security vulnerabilities, addressing them in a test.

* Fix authentication failures after going halfway through a sign-in attempt

* Refactor `authenticate_with_sign_in_token` and `authenticate_with_two_factor` to make the two authentication steps more obvious

1 files changed, 109 insertions, 0 deletions

diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb
index d03ae51e8..051a0807d 100644
--- a/spec/controllers/auth/sessions_controller_spec.rb
+++ b/spec/controllers/auth/sessions_controller_spec.rb
@@ -206,6 +206,38 @@ RSpec.describe Auth::SessionsController, type: :controller do
           end
         end
 
+        context 'using email and password after an unfinished log-in attempt to a 2FA-protected account' do
+          let!(:other_user) do
+            Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32))
+          end
+
+          before do
+            post :create, params: { user: { email: other_user.email, password: other_user.password } }
+            post :create, params: { user: { email: user.email, password: user.password } }
+          end
+
+          it 'renders two factor authentication page' do
+            expect(controller).to render_template("two_factor")
+            expect(controller).to render_template(partial: "_otp_authentication_form")
+          end
+        end
+
+        context 'using email and password after an unfinished log-in attempt with a sign-in token challenge' do
+          let!(:other_user) do
+            Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago)
+          end
+
+          before do
+            post :create, params: { user: { email: other_user.email, password: other_user.password } }
+            post :create, params: { user: { email: user.email, password: user.password } }
+          end
+
+          it 'renders two factor authentication page' do
+            expect(controller).to render_template("two_factor")
+            expect(controller).to render_template(partial: "_otp_authentication_form")
+          end
+        end
+
         context 'using upcase email and password' do
           before do
             post :create, params: { user: { email: user.email.upcase, password: user.password } }
@@ -231,6 +263,21 @@ RSpec.describe Auth::SessionsController, type: :controller do
           end
         end
 
+        context 'using a valid OTP, attempting to leverage previous half-login to bypass password auth' do
+          let!(:other_user) do
+            Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago)
+          end
+
+          before do
+            post :create, params: { user: { email: other_user.email, password: other_user.password } }
+            post :create, params: { user: { email: user.email, otp_attempt: user.current_otp } }, session: { attempt_user_updated_at: user.updated_at.to_s }
+          end
+
+          it "doesn't log the user in" do
+            expect(controller.current_user).to be_nil
+          end
+        end
+
         context 'when the server has an decryption error' do
           before do
             allow_any_instance_of(User).to receive(:validate_and_consume_otp!).and_raise(OpenSSL::Cipher::CipherError)
@@ -380,6 +427,52 @@ RSpec.describe Auth::SessionsController, type: :controller do
         end
       end
 
+      context 'using email and password after an unfinished log-in attempt to a 2FA-protected account' do
+        let!(:other_user) do
+          Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32))
+        end
+
+        before do
+          post :create, params: { user: { email: other_user.email, password: other_user.password } }
+          post :create, params: { user: { email: user.email, password: user.password } }
+        end
+
+        it 'renders sign in token authentication page' do
+          expect(controller).to render_template("sign_in_token")
+        end
+
+        it 'generates sign in token' do
+          expect(user.reload.sign_in_token).to_not be_nil
+        end
+
+        it 'sends sign in token e-mail' do
+          expect(UserMailer).to have_received(:sign_in_token)
+        end
+      end
+
+      context 'using email and password after an unfinished log-in attempt with a sign-in token challenge' do
+        let!(:other_user) do
+          Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago)
+        end
+
+        before do
+          post :create, params: { user: { email: other_user.email, password: other_user.password } }
+          post :create, params: { user: { email: user.email, password: user.password } }
+        end
+
+        it 'renders sign in token authentication page' do
+          expect(controller).to render_template("sign_in_token")
+        end
+
+        it 'generates sign in token' do
+          expect(user.reload.sign_in_token).to_not be_nil
+        end
+
+        it 'sends sign in token e-mail' do
+          expect(UserMailer).to have_received(:sign_in_token).with(user, any_args)
+        end
+      end
+
       context 'using a valid sign in token' do
         before do
           user.generate_sign_in_token && user.save
@@ -395,6 +488,22 @@ RSpec.describe Auth::SessionsController, type: :controller do
         end
       end
 
+      context 'using a valid sign in token, attempting to leverage previous half-login to bypass password auth' do
+        let!(:other_user) do
+          Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago)
+        end
+
+        before do
+          user.generate_sign_in_token && user.save
+          post :create, params: { user: { email: other_user.email, password: other_user.password } }
+          post :create, params: { user: { email: user.email, sign_in_token_attempt: user.sign_in_token } }, session: { attempt_user_updated_at: user.updated_at.to_s }
+        end
+
+        it "doesn't log the user in" do
+          expect(controller.current_user).to be_nil
+        end
+      end
+
       context 'using an invalid sign in token' do
         before do
           post :create, params: { user: { sign_in_token_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }