SECURITY: 2FA with U2F / TOTP

spec/components/concern/second_factor_manager_spec.rb

@@ -3,8 +3,16 @@
 require 'rails_helper'
 
 RSpec.describe SecondFactorManager do
-  fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp) }
-  let(:user) { user_second_factor_totp.user }
+  fab!(:user) { Fabricate(:user) }
+  fab!(:user_second_factor_totp) { Fabricate(:user_second_factor_totp, user: user) }
+  fab!(:user_security_key) do
+    Fabricate(
+      :user_security_key,
+      user: user,
+      public_key: valid_security_key_data[:public_key],
+      credential_id: valid_security_key_data[:credential_id]
+    )
+  end
   fab!(:another_user) { Fabricate(:user) }
 
   fab!(:user_second_factor_backup) { Fabricate(:user_second_factor_backup) }
@@ -78,7 +86,7 @@ RSpec.describe SecondFactorManager do
 
     describe "when user's second factor record is disabled" do
       it 'should return false' do
-        user.user_second_factors.totps.first.update!(enabled: false)
+        disable_totp
         expect(user.totp_enabled?).to eq(false)
       end
     end
@@ -107,6 +115,252 @@ RSpec.describe SecondFactorManager do
     end
   end
 
+  describe "#has_multiple_second_factor_methods?" do
+    context "when security keys and totp are enabled" do
+      it "retrns true" do
+        expect(user.has_multiple_second_factor_methods?).to eq(true)
+      end
+    end
+
+    context "if the totp gets disabled" do
+      it "retrns false" do
+        disable_totp
+        expect(user.has_multiple_second_factor_methods?).to eq(false)
+      end
+    end
+
+    context "if the security key gets disabled" do
+      it "retrns false" do
+        disable_security_key
+        expect(user.has_multiple_second_factor_methods?).to eq(false)
+      end
+    end
+  end
+
+  describe "#only_security_keys_enabled?" do
+    it "returns true if totp disabled and security key enabled" do
+      disable_totp
+      expect(user.only_security_keys_enabled?).to eq(true)
+    end
+  end
+
+  describe "#only_totp_or_backup_codes_enabled?" do
+    it "returns true if totp enabled and security key disabled" do
+      disable_security_key
+      expect(user.only_totp_or_backup_codes_enabled?).to eq(true)
+    end
+  end
+
+  describe "#authenticate_second_factor" do
+    let(:params) { {} }
+    let(:secure_session) { {} }
+
+    context "when neither security keys nor totp/backup codes are enabled" do
+      before do
+        disable_security_key && disable_totp
+      end
+      it "returns OK, because it doesn't need to authenticate" do
+        expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+      end
+    end
+
+    context "when only security key is enabled" do
+      before do
+        disable_totp
+        simulate_localhost_webauthn_challenge
+        Webauthn.stage_challenge(user, secure_session)
+      end
+
+      context "when security key params are valid" do
+        let(:params) { { second_factor_token: valid_security_key_auth_post_data, second_factor_method: UserSecondFactor.methods[:security_key] } }
+        it "returns OK" do
+          expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+        end
+      end
+
+      context "when security key params are invalid" do
+        let(:params) do
+          {
+            second_factor_token: {
+              signature: 'bad',
+              clientData: 'bad',
+              authenticatorData: 'bad',
+              credentialId: 'bad'
+            },
+            second_factor_method: UserSecondFactor.methods[:security_key]
+          }
+        end
+        it "returns not OK" do
+          result = user.authenticate_second_factor(params, secure_session)
+          expect(result.ok).to eq(false)
+          expect(result.error).to eq(I18n.t("webauthn.validation.not_found_error"))
+        end
+      end
+    end
+
+    context "when only totp is enabled" do
+      before do
+        disable_security_key
+      end
+
+      context "when totp is valid" do
+        let(:params) do
+          {
+            second_factor_token: user.user_second_factors.totps.first.totp_object.now,
+            second_factor_method: UserSecondFactor.methods[:totp]
+          }
+        end
+        it "returns OK" do
+          expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+        end
+      end
+
+      context "when totp is invalid" do
+        let(:params) do
+          {
+            second_factor_token: "blah",
+            second_factor_method: UserSecondFactor.methods[:totp]
+          }
+        end
+        it "returns not OK" do
+          result = user.authenticate_second_factor(params, secure_session)
+          expect(result.ok).to eq(false)
+          expect(result.error).to eq(I18n.t("login.invalid_second_factor_code"))
+        end
+      end
+    end
+
+    context "when both security keys and totp are enabled" do
+      let(:invalid_method) { 4 }
+      let(:method) { invalid_method }
+
+      before do
+        simulate_localhost_webauthn_challenge
+        Webauthn.stage_challenge(user, secure_session)
+      end
+
+      context "when method selected is invalid" do
+        it "returns an error" do
+          result = user.authenticate_second_factor(params, secure_session)
+          expect(result.ok).to eq(false)
+          expect(result.error).to eq(I18n.t("login.invalid_second_factor_method"))
+        end
+      end
+
+      context "when method selected is TOTP" do
+        let(:method) { UserSecondFactor.methods[:totp] }
+        let(:token) { user.user_second_factors.totps.first.totp_object.now }
+
+        context "when totp params are provided" do
+          let(:params) do
+            {
+              second_factor_token: token,
+              second_factor_method: method
+            }
+          end
+
+          it "validates totp OK" do
+            expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+          end
+
+          context "when the user does not have TOTP enabled" do
+            let(:token) { 'test' }
+            before do
+              user.totps.destroy_all
+            end
+
+            it "returns an error" do
+              result = user.authenticate_second_factor(params, secure_session)
+              expect(result.ok).to eq(false)
+              expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method"))
+            end
+          end
+        end
+      end
+
+      context "when method selected is Security Keys" do
+        let(:method) { UserSecondFactor.methods[:security_key] }
+
+        before do
+          simulate_localhost_webauthn_challenge
+          Webauthn.stage_challenge(user, secure_session)
+        end
+
+        context "when security key params are valid" do
+          let(:params) { { second_factor_token: valid_security_key_auth_post_data, second_factor_method: method } }
+          it "returns OK" do
+            expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+          end
+
+          context "when the user does not have security keys enabled" do
+            before do
+              user.security_keys.destroy_all
+            end
+
+            it "returns an error" do
+              result = user.authenticate_second_factor(params, secure_session)
+              expect(result.ok).to eq(false)
+              expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method"))
+            end
+          end
+        end
+      end
+
+      context "when method selected is Backup Codes" do
+        let(:method) { UserSecondFactor.methods[:backup_codes] }
+        let!(:backup_code) { Fabricate(:user_second_factor_backup, user: user) }
+
+        context "when backup code params are provided" do
+          let(:params) do
+            {
+              second_factor_token: 'iAmValidBackupCode',
+              second_factor_method: method
+            }
+          end
+
+          context "when backup codes enabled" do
+            it "validates codes OK" do
+              expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+            end
+          end
+
+          context "when backup codes disabled" do
+            before do
+              user.user_second_factors.backup_codes.destroy_all
+            end
+
+            it "returns an error" do
+              result = user.authenticate_second_factor(params, secure_session)
+              expect(result.ok).to eq(false)
+              expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method"))
+            end
+          end
+        end
+      end
+
+      context "when no totp params are provided" do
+        let(:params) { { second_factor_token: valid_security_key_auth_post_data, second_factor_method: UserSecondFactor.methods[:security_key] } }
+
+        it "validates the security key OK" do
+          expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+        end
+      end
+
+      context "when totp params are provided" do
+        let(:params) do
+          {
+            second_factor_token: user.user_second_factors.totps.first.totp_object.now,
+            second_factor_method: UserSecondFactor.methods[:totp]
+          }
+        end
+
+        it "validates totp OK" do
+          expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true)
+        end
+      end
+    end
+  end
+
   context 'backup codes' do
     describe '#generate_backup_codes' do
       it 'should generate and store 10 backup codes' do
@@ -187,4 +441,12 @@ RSpec.describe SecondFactorManager do
       end
     end
   end
+
+  def disable_totp
+    user.user_second_factors.totps.first.update!(enabled: false)
+  end
+
+  def disable_security_key
+    user.security_keys.first.destroy!
+  end
 end

spec/lib/webauthn/security_key_authentication_service_spec.rb

@@ -62,6 +62,20 @@ describe Webauthn::SecurityKeyAuthenticationService do
     expect(security_key.reload.last_used).not_to eq(nil)
   end
 
+  context "when params is blank" do
+    let(:params) { nil }
+    it "returns false with no validation" do
+      expect(subject.authenticate_security_key).to eq(false)
+    end
+  end
+
+  context "when params is not blank and not a hash" do
+    let(:params) { 'test' }
+    it "returns false with no validation" do
+      expect(subject.authenticate_security_key).to eq(false)
+    end
+  end
+
   context 'when the credential ID does not match any user security key in the database' do
     let(:credential_id) { 'badid' }
 

spec/rails_helper.rb

@@ -160,7 +160,7 @@ RSpec.configure do |config|
   config.include MessageBus
   config.include RSpecHtmlMatchers
   config.include IntegrationHelpers, type: :request
-  config.include WebauthnIntegrationHelpers, type: :request
+  config.include WebauthnIntegrationHelpers
   config.include SiteSettingsHelpers
   config.mock_framework = :mocha
   config.order = 'random'

spec/requests/application_controller_spec.rb

@@ -153,6 +153,42 @@ RSpec.describe ApplicationController do
       get "/"
       expect(response.status).to eq(200)
     end
+
+    context "when enforcing second factor for staff" do
+      before do
+        SiteSetting.enforce_second_factor = "staff"
+        sign_in(admin)
+      end
+
+      context "when the staff member has not enabled TOTP or security keys" do
+        it "redirects the staff to the second factor preferences" do
+          get "/"
+          expect(response).to redirect_to("/u/#{admin.username}/preferences/second-factor")
+        end
+      end
+
+      context "when the staff member has enabled TOTP" do
+        before do
+          Fabricate(:user_second_factor_totp, user: admin)
+        end
+
+        it "does not redirects the staff to set up 2FA" do
+          get "/"
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context "when the staff member has enabled security keys" do
+        before do
+          Fabricate(:user_security_key_with_random_credential, user: admin)
+        end
+
+        it "does not redirects the staff to set up 2FA" do
+          get "/"
+          expect(response.status).to eq(200)
+        end
+      end
+    end
   end
 
   describe 'invalid request params' do

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -323,7 +323,7 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(user.confirm_password?("securepassword")).to eq(false)
       end
 
-      context 'when user has second factor enabled' do
+      context 'when user has TOTP enabled' do
         before do
           user.create_totp(enabled: true)
         end
@@ -346,6 +346,29 @@ RSpec.describe Users::OmniauthCallbacksController do
         end
       end
 
+      context 'when user has security key enabled' do
+        before do
+          Fabricate(:user_security_key_with_random_credential, user: user)
+        end
+
+        it 'should return the right response' do
+          get "/auth/google_oauth2/callback.json"
+
+          expect(response.status).to eq(302)
+
+          data = JSON.parse(cookies[:authentication_data])
+
+          expect(data["email"]).to eq(user.email)
+          expect(data["omniauth_disallow_totp"]).to eq(true)
+
+          user.update!(email: 'different@user.email')
+          get "/auth/google_oauth2/callback.json"
+
+          expect(response.status).to eq(302)
+          expect(JSON.parse(cookies[:authentication_data])["email"]).to eq(user.email)
+        end
+      end
+
       context 'when sso_payload cookie exist' do
         before do
           SiteSetting.enable_sso_provider = true

spec/requests/session_controller_spec.rb

@@ -312,6 +312,22 @@ RSpec.describe SessionController do
             end
           end
         end
+
+        context "if the security_key_param is provided but only TOTP is enabled" do
+          it "does not log in the user" do
+            post "/session/email-login/#{email_token.token}.json", params: {
+              second_factor_token: 'foo',
+              second_factor_method: UserSecondFactor.methods[:totp]
+            }
+
+            expect(response.status).to eq(200)
+
+            expect(JSON.parse(response.body)["error"]).to eq(
+              I18n.t("login.invalid_second_factor_code")
+            )
+            expect(session[:current_user_id]).to eq(nil)
+          end
+        end
       end
 
       context "user has only security key enabled" do
@@ -343,7 +359,7 @@ RSpec.describe SessionController do
             expect(session[:current_user_id]).to eq(nil)
             response_body = JSON.parse(response.body)
             expect(response_body['error']).to eq(I18n.t(
-              'login.invalid_second_factor_code'
+              'login.not_enabled_second_factor_method'
             ))
           end
         end
@@ -351,7 +367,7 @@ RSpec.describe SessionController do
           it" shows an error message and denies login" do
 
             post "/session/email-login/#{email_token.token}.json", params: {
-              security_key_credential: {
+              second_factor_token: {
                 signature: 'bad_sig',
                 clientData: 'bad_clientData',
                 credentialId: 'bad_credential_id',
@@ -375,7 +391,7 @@ RSpec.describe SessionController do
             post "/session/email-login/#{email_token.token}.json", params: {
               login: user.username,
               password: 'myawesomepassword',
-              security_key_credential: valid_security_key_auth_post_data,
+              second_factor_token: valid_security_key_auth_post_data,
               second_factor_method: UserSecondFactor.methods[:security_key]
             }
 
@@ -387,6 +403,46 @@ RSpec.describe SessionController do
           end
         end
       end
+
+      context "user has security key and totp enabled" do
+        let!(:user_security_key) do
+          Fabricate(
+            :user_security_key,
+            user: user,
+            credential_id: valid_security_key_data[:credential_id],
+            public_key: valid_security_key_data[:public_key]
+          )
+        end
+        let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) }
+
+        it "doesnt allow logging in if the 2fa params are garbled" do
+          post "/session/email-login/#{email_token.token}.json", params: {
+            second_factor_method: UserSecondFactor.methods[:totp],
+            second_factor_token: "blah"
+          }
+
+          expect(response.status).to eq(200)
+          expect(session[:current_user_id]).to eq(nil)
+          response_body = JSON.parse(response.body)
+          expect(response_body['error']).to eq(I18n.t(
+            'login.invalid_second_factor_code'
+          ))
+        end
+
+        it "doesnt allow login if both of the 2fa params are blank" do
+          post "/session/email-login/#{email_token.token}.json", params: {
+            second_factor_method: UserSecondFactor.methods[:totp],
+            second_factor_token: ""
+          }
+
+          expect(response.status).to eq(200)
+          expect(session[:current_user_id]).to eq(nil)
+          response_body = JSON.parse(response.body)
+          expect(response_body['error']).to eq(I18n.t(
+            'login.invalid_second_factor_code'
+          ))
+        end
+      end
     end
   end
 
@@ -1190,9 +1246,8 @@ RSpec.describe SessionController do
             post "/session.json", params: {
               login: user.username,
               password: 'myawesomepassword',
-              security_key_credential: {},
               second_factor_token: '99999999',
-              second_factor_method: UserSecondFactor.methods[:totp]
+              second_factor_method: UserSecondFactor.methods[:security_key]
             }
 
             expect(response.status).to eq(200)
@@ -1200,7 +1255,7 @@ RSpec.describe SessionController do
             response_body = JSON.parse(response.body)
             expect(response_body["failed"]).to eq("FAILED")
             expect(response_body['error']).to eq(I18n.t(
-              'login.invalid_second_factor_code'
+              'login.invalid_security_key'
             ))
           end
         end
@@ -1210,7 +1265,7 @@ RSpec.describe SessionController do
             post "/session.json", params: {
               login: user.username,
               password: 'myawesomepassword',
-              security_key_credential: {
+              second_factor_token: {
                 signature: 'bad_sig',
                 clientData: 'bad_clientData',
                 credentialId: 'bad_credential_id',
@@ -1234,7 +1289,7 @@ RSpec.describe SessionController do
             post "/session.json", params: {
               login: user.username,
               password: 'myawesomepassword',
-              security_key_credential: valid_security_key_auth_post_data,
+              second_factor_token: valid_security_key_auth_post_data,
               second_factor_method: UserSecondFactor.methods[:security_key]
             }
 
@@ -1256,7 +1311,7 @@ RSpec.describe SessionController do
             post "/session.json", params: {
               login: user.username,
               password: 'myawesomepassword',
-              security_key_credential: valid_security_key_auth_post_data,
+              second_factor_token: valid_security_key_auth_post_data,
               second_factor_method: UserSecondFactor.methods[:security_key]
             }
 
@@ -1265,7 +1320,7 @@ RSpec.describe SessionController do
             response_body = JSON.parse(response.body)
             expect(response_body["failed"]).to eq("FAILED")
             expect(JSON.parse(response.body)['error']).to eq(I18n.t(
-              'login.invalid_second_factor_code'
+              'login.not_enabled_second_factor_method'
             ))
           end
         end
@@ -1279,12 +1334,12 @@ RSpec.describe SessionController do
           it 'should return the right response' do
             post "/session.json", params: {
               login: user.username,
-              password: 'myawesomepassword',
+              password: 'myawesomepassword'
             }
 
             expect(response.status).to eq(200)
             expect(JSON.parse(response.body)['error']).to eq(I18n.t(
-              'login.invalid_second_factor_code'
+              'login.invalid_second_factor_method'
             ))
           end
         end

spec/requests/users_controller_spec.rb

@@ -236,7 +236,7 @@ describe UsersController do
         expect(response.status).to eq(200)
         expect(response.body).to have_tag("div#data-preloaded") do |element|
           json = JSON.parse(element.current_scope.attribute('data-preloaded').value)
-          expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":false,"security_key_required":false,"backup_enabled":false}')
+          expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":false,"security_key_required":false,"backup_enabled":false,"multiple_second_factor_methods":false}')
         end
 
         expect(session["password-#{token}"]).to be_blank
@@ -349,7 +349,7 @@ describe UsersController do
 
           expect(response.body).to have_tag("div#data-preloaded") do |element|
             json = JSON.parse(element.current_scope.attribute('data-preloaded').value)
-            expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":true,"security_key_required":false,"backup_enabled":false}')
+            expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":true,"security_key_required":false,"backup_enabled":false,"multiple_second_factor_methods":false}')
           end
 
           put "/u/password-reset/#{token}", params: {
@@ -420,21 +420,34 @@ describe UsersController do
         it 'changes password with valid security key challenge and authentication' do
           put "/u/password-reset/#{token}.json", params: {
             password: 'hg9ow8yHG32O',
-            security_key_credential: valid_security_key_auth_post_data,
+            second_factor_token: valid_security_key_auth_post_data,
             second_factor_method: UserSecondFactor.methods[:security_key]
           }
 
           user.reload
           expect(response.status).to eq(200)
+
           expect(user.confirm_password?('hg9ow8yHG32O')).to eq(true)
           expect(user.user_auth_tokens.count).to eq(1)
         end
 
+        it "does not change a password if a fake TOTP token is provided" do
+          put "/u/password-reset/#{token}.json", params: {
+            password: 'hg9ow8yHG32O',
+            second_factor_token: 'blah',
+            second_factor_method: UserSecondFactor.methods[:security_key]
+          }
+
+          user.reload
+          expect(response.status).to eq(200)
+          expect(user.confirm_password?('hg9ow8yHG32O')).to eq(false)
+        end
+
         context "when security key authentication fails" do
           it 'shows an error message and does not change password' do
             put "/u/password-reset/#{token}", params: {
               password: 'hg9ow8yHG32O',
-              security_key_credential: {
+              second_factor_token: {
                 signature: 'bad',
                 clientData: 'bad',
                 authenticatorData: 'bad',
@@ -446,7 +459,7 @@ describe UsersController do
             user.reload
             expect(user.confirm_password?('hg9ow8yHG32O')).to eq(false)
             expect(response.status).to eq(200)
-            expect(JSON.parse(response.body)['errors']).to include(I18n.t("webauthn.validation.not_found_error"))
+            expect(response.body).to include(I18n.t("webauthn.validation.not_found_error"))
           end
         end
       end

spec/requests/users_email_controller_spec.rb

@@ -115,6 +115,88 @@ describe UsersEmailController do
           expect(user.email).to eq("new.n.cool@example.com")
         end
       end
+
+      context "security key required" do
+        fab!(:user_security_key) do
+          Fabricate(
+            :user_security_key,
+            user: user,
+            credential_id: valid_security_key_data[:credential_id],
+            public_key: valid_security_key_data[:public_key]
+          )
+        end
+
+        before do
+          simulate_localhost_webauthn_challenge
+        end
+
+        it 'requires a security key' do
+          get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+
+          expect(response.status).to eq(200)
+
+          response_body = response.body
+
+          expect(response_body).to include(I18n.t("login.security_key_authenticate"))
+          expect(response_body).to include(I18n.t("login.security_key_description"))
+        end
+
+        context "if the user has a TOTP enabled and wants to use that instead" do
+          before do
+            Fabricate(:user_second_factor_totp, user: user)
+          end
+
+          it 'allows entering the totp code instead' do
+            get "/u/confirm-new-email/#{user.email_tokens.last.token}?show_totp=true"
+
+            expect(response.status).to eq(200)
+
+            response_body = response.body
+
+            expect(response_body).to include(I18n.t("login.second_factor_title"))
+            expect(response_body).not_to include(I18n.t("login.security_key_authenticate"))
+          end
+        end
+
+        it 'adds an error on a security key attempt' do
+          get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+          put "/u/confirm-new-email", params: {
+            token: user.email_tokens.last.token,
+            second_factor_token: "{}",
+            second_factor_method: UserSecondFactor.methods[:security_key]
+          }
+
+          expect(response.status).to eq(302)
+          expect(flash[:invalid_second_factor]).to eq(true)
+        end
+
+        it 'confirms with a correct security key token' do
+          get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+          put "/u/confirm-new-email", params: {
+            second_factor_token: valid_security_key_auth_post_data.to_json,
+            second_factor_method: UserSecondFactor.methods[:security_key],
+            token: user.email_tokens.last.token
+          }
+
+          expect(response.status).to eq(302)
+
+          user.reload
+          expect(user.email).to eq("new.n.cool@example.com")
+        end
+
+        context "if the security key data JSON is garbled" do
+          it "raises an invalid parameters error" do
+            get "/u/confirm-new-email/#{user.email_tokens.last.token}"
+            put "/u/confirm-new-email", params: {
+              second_factor_token: "{someweird: 8notjson}",
+              second_factor_method: UserSecondFactor.methods[:security_key],
+              token: user.email_tokens.last.token
+            }
+
+            expect(response.status).to eq(400)
+          end
+        end
+      end
     end
   end