diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f8d10cea2e7..8e003950da6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1063,9 +1063,15 @@ class ApplicationController < ActionController::Base end end - def run_second_factor!(action_class, action_data = nil) - action = action_class.new(guardian, request, action_data) - manager = SecondFactor::AuthManager.new(guardian, action) + def run_second_factor!(action_class, action_data: nil, target_user: current_user) + if current_user && target_user != current_user + # Anon can run 2fa against another target, but logged-in users should not. + # This should be validated at the `run_second_factor!` call site. + raise "running 2fa against another user is not allowed" + end + + action = action_class.new(guardian, request, opts: action_data, target_user: target_user) + manager = SecondFactor::AuthManager.new(guardian, action, target_user: target_user) yield(manager) if block_given? result = manager.run!(request, params, secure_session) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index ece44871764..05afd6c2cf9 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -11,8 +11,6 @@ class SessionController < ApplicationController skip_before_action :check_xhr, only: %i[second_factor_auth_show] - requires_login only: %i[second_factor_auth_show second_factor_auth_perform] - allow_in_staff_writes_only_mode :create allow_in_staff_writes_only_mode :email_login @@ -47,8 +45,10 @@ class SessionController < ApplicationController result = run_second_factor!( SecondFactor::Actions::DiscourseConnectProvider, - payload: payload, - confirmed_2fa_during_login: confirmed_2fa_during_login, + action_data: { + payload: payload, + confirmed_2fa_during_login: confirmed_2fa_during_login, + }, ) if result.second_factor_auth_skipped? @@ -136,8 +136,10 @@ class SessionController < ApplicationController skip_before_action :check_xhr, only: :test_second_factor_restricted_route def test_second_factor_restricted_route + target_user = User.find_by_username(params[:username]) || current_user + raise "user required" if !target_user result = - run_second_factor!(TestSecondFactorAction) do |manager| + run_second_factor!(TestSecondFactorAction, target_user: target_user) do |manager| manager.allow_backup_codes! if params[:allow_backup_codes] end if result.no_second_factors_enabled? @@ -145,6 +147,15 @@ class SessionController < ApplicationController else render json: { result: "second_factor_auth_completed" } end + rescue StandardError => e + # Normally this would be checked by the consumer before calling `run_second_factor!` + # but since this is a test route, we allow passing a bad value into the API, catch the error + # and return a JSON response to assert against. + if e.message == "running 2fa against another user is not allowed" + render json: { result: "wrong user" }, status: 400 + else + raise e + end end end @@ -464,14 +475,18 @@ class SessionController < ApplicationController end def second_factor_auth_show - user = current_user - nonce = params.require(:nonce) challenge = nil error_key = nil + user = nil status_code = 200 begin - challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session) + challenge = + SecondFactor::AuthManager.find_second_factor_challenge( + nonce: nonce, + secure_session: secure_session, + target_user: current_user, + ) rescue SecondFactor::BadChallenge => exception error_key = exception.error_translation_key status_code = exception.status_code @@ -479,6 +494,7 @@ class SessionController < ApplicationController json = {} if challenge + user = User.find(challenge[:target_user_id]) json.merge!( totp_enabled: user.totp_enabled?, backup_enabled: user.backup_codes_enabled?, @@ -510,9 +526,16 @@ class SessionController < ApplicationController nonce = params.require(:nonce) challenge = nil error_key = nil + user = nil status_code = 200 begin - challenge = SecondFactor::AuthManager.find_second_factor_challenge(nonce, secure_session) + challenge = + SecondFactor::AuthManager.find_second_factor_challenge( + nonce: nonce, + secure_session: secure_session, + target_user: current_user, + ) + user = User.find(challenge[:target_user_id]) rescue SecondFactor::BadChallenge => exception error_key = exception.error_translation_key status_code = exception.status_code @@ -535,7 +558,7 @@ class SessionController < ApplicationController # they're redirected to the 2fa page and then uses the same method they've # disabled. second_factor_method = params[:second_factor_method].to_i - if !current_user.valid_second_factor_method_for_user?(second_factor_method) + if !user.valid_second_factor_method_for_user?(second_factor_method) raise Discourse::InvalidAccess.new end # and this happens if someone tries to use a 2FA method that's not accepted @@ -546,8 +569,8 @@ class SessionController < ApplicationController end if !challenge[:successful] - rate_limit_second_factor!(current_user) - second_factor_auth_result = current_user.authenticate_second_factor(params, secure_session) + rate_limit_second_factor!(user) + second_factor_auth_result = user.authenticate_second_factor(params, secure_session) if second_factor_auth_result.ok challenge[:successful] = true challenge[:generated_at] += 1.minute.to_i diff --git a/lib/second_factor/actions/base.rb b/lib/second_factor/actions/base.rb index d3caa729828..24ad12d8554 100644 --- a/lib/second_factor/actions/base.rb +++ b/lib/second_factor/actions/base.rb @@ -5,9 +5,10 @@ module SecondFactor::Actions include Rails.application.routes.url_helpers attr_reader :current_user, :guardian, :request - def initialize(guardian, request, opts = nil) + def initialize(guardian, request, target_user:, opts: nil) @guardian = guardian @current_user = guardian.user + @target_user = target_user @request = request @opts = HashWithIndifferentAccess.new(opts) end diff --git a/lib/second_factor/auth_manager.rb b/lib/second_factor/auth_manager.rb index a9ba5eec338..06151337f6c 100644 --- a/lib/second_factor/auth_manager.rb +++ b/lib/second_factor/auth_manager.rb @@ -120,7 +120,7 @@ class SecondFactor::AuthManager attr_reader :allowed_methods - def self.find_second_factor_challenge(nonce, secure_session) + def self.find_second_factor_challenge(nonce:, secure_session:, target_user:) challenge_json = secure_session["current_second_factor_auth_challenge"] if challenge_json.blank? raise SecondFactor::BadChallenge.new( @@ -137,16 +137,25 @@ class SecondFactor::AuthManager ) end + if target_user && (challenge[:target_user_id] != target_user.id) + raise SecondFactor::BadChallenge.new( + "second_factor_auth.challenge_not_found", + status_code: 404, + ) + end + generated_at = challenge[:generated_at] if generated_at < MAX_CHALLENGE_AGE.ago.to_i raise SecondFactor::BadChallenge.new("second_factor_auth.challenge_expired", status_code: 401) end + challenge end - def initialize(guardian, action) + def initialize(guardian, action, target_user:) @guardian = guardian @current_user = guardian.user + @target_user = target_user @action = action @allowed_methods = Set.new([UserSecondFactor.methods[:totp], UserSecondFactor.methods[:security_key]]).freeze @@ -163,7 +172,7 @@ class SecondFactor::AuthManager elsif @action.skip_second_factor_auth?(params) data = @action.second_factor_auth_skipped!(params) create_result(:second_factor_auth_skipped, data) - elsif !allowed_methods.any? { |m| @current_user.valid_second_factor_method_for_user?(m) } + elsif !allowed_methods.any? { |m| @target_user.valid_second_factor_method_for_user?(m) } data = @action.no_second_factors_enabled!(params) create_result(:no_second_factor, data) else @@ -185,6 +194,7 @@ class SecondFactor::AuthManager callback_params: callback_params, allowed_methods: allowed_methods.to_a, generated_at: Time.zone.now.to_i, + target_user_id: @target_user.id, } challenge[:description] = config[:description] if config[:description] challenge[:redirect_url] = config[:redirect_url] if config[:redirect_url].present? @@ -193,7 +203,12 @@ class SecondFactor::AuthManager end def verify_second_factor_auth_completed(nonce, secure_session) - challenge = self.class.find_second_factor_challenge(nonce, secure_session) + challenge = + self.class.find_second_factor_challenge( + nonce: nonce, + secure_session: secure_session, + target_user: @target_user, + ) if !challenge[:successful] raise SecondFactor::BadChallenge.new( "second_factor_auth.challenge_not_completed", diff --git a/spec/lib/second_factor/actions/discourse_connect_provider_spec.rb b/spec/lib/second_factor/actions/discourse_connect_provider_spec.rb index 74922ad6c7a..f619e89a5b0 100644 --- a/spec/lib/second_factor/actions/discourse_connect_provider_spec.rb +++ b/spec/lib/second_factor/actions/discourse_connect_provider_spec.rb @@ -33,7 +33,12 @@ RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do def create_instance(user, request = nil, opts = nil) request ||= create_request - SecondFactor::Actions::DiscourseConnectProvider.new(Guardian.new(user), request, opts) + SecondFactor::Actions::DiscourseConnectProvider.new( + Guardian.new(user), + request, + opts: opts, + target_user: user, + ) end describe "#skip_second_factor_auth?" do diff --git a/spec/lib/second_factor/actions/grant_admin_spec.rb b/spec/lib/second_factor/actions/grant_admin_spec.rb index 37b82c05b4f..0d51d993921 100644 --- a/spec/lib/second_factor/actions/grant_admin_spec.rb +++ b/spec/lib/second_factor/actions/grant_admin_spec.rb @@ -22,7 +22,7 @@ RSpec.describe SecondFactor::Actions::GrantAdmin do def create_instance(user, request = nil) request ||= create_request - SecondFactor::Actions::GrantAdmin.new(Guardian.new(user), request) + SecondFactor::Actions::GrantAdmin.new(Guardian.new(user), request, target_user: user) end describe "#no_second_factors_enabled!" do diff --git a/spec/lib/second_factor/auth_manager_spec.rb b/spec/lib/second_factor/auth_manager_spec.rb index fe42bfad3fc..a2b3da6d414 100644 --- a/spec/lib/second_factor/auth_manager_spec.rb +++ b/spec/lib/second_factor/auth_manager_spec.rb @@ -10,12 +10,12 @@ RSpec.describe SecondFactor::AuthManager do end def create_manager(action) - SecondFactor::AuthManager.new(guardian, action) + SecondFactor::AuthManager.new(guardian, action, target_user: user) end def create_action(request = nil) request ||= create_request - TestSecondFactorAction.new(guardian, request) + TestSecondFactorAction.new(guardian, request, target_user: user) end def stage_challenge(successful:) diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index fbcea95fa1f..f180efab1b0 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -2844,126 +2844,106 @@ RSpec.describe SessionController do describe "#second_factor_auth_show" do let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) } - before { sign_in(user) } + it "can work for anon" do + post "/session/2fa/test-action?username=#{user.username}", xhr: true + expect(response.status).to eq(403) - it "returns 404 if there is no challenge for the given nonce" do - get "/session/2fa.json", params: { nonce: "asdasdsadsad" } - expect(response.status).to eq(404) - expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_not_found")) - end - - it "returns 404 if the nonce does not match the challenge nonce" do - post "/session/2fa/test-action" - get "/session/2fa.json", params: { nonce: "wrongnonce" } - expect(response.status).to eq(404) - expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_not_found")) - end - - it "returns 401 if the challenge nonce has expired" do - post "/session/2fa/test-action", xhr: true nonce = response.parsed_body["second_factor_challenge_nonce"] get "/session/2fa.json", params: { nonce: nonce } expect(response.status).to eq(200) expect(response.parsed_body["error"]).not_to be_present - - freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now - get "/session/2fa.json", params: { nonce: nonce } - expect(response.status).to eq(401) - expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_expired")) end - it "responds with challenge data" do - post "/session/2fa/test-action", xhr: true - nonce = response.parsed_body["second_factor_challenge_nonce"] - get "/session/2fa.json", params: { nonce: nonce } - expect(response.status).to eq(200) - expect(response.parsed_body["error"]).not_to be_present - challenge_data = response.parsed_body - expect(challenge_data["totp_enabled"]).to eq(true) - expect(challenge_data["backup_enabled"]).to eq(false) - expect(challenge_data["security_keys_enabled"]).to eq(false) - expect(challenge_data["allowed_methods"]).to contain_exactly( - UserSecondFactor.methods[:totp], - UserSecondFactor.methods[:security_key], - ) - expect(challenge_data["description"]).to eq("this is description for test action") + it "throws an error if logged in to a different user" do + sign_in user + other_user = Fabricate(:user) + post "/session/2fa/test-action?username=#{other_user.username}", xhr: true - Fabricate( - :user_security_key_with_random_credential, - user: user, - name: "Enabled YubiKey", - enabled: true, - ) - Fabricate(:user_second_factor_backup, user: user) - post "/session/2fa/test-action", params: { allow_backup_codes: true }, xhr: true - nonce = response.parsed_body["second_factor_challenge_nonce"] - get "/session/2fa.json", params: { nonce: nonce } - expect(response.status).to eq(200) - expect(response.parsed_body["error"]).not_to be_present - challenge_data = response.parsed_body - expect(challenge_data["totp_enabled"]).to eq(true) - expect(challenge_data["backup_enabled"]).to eq(true) - expect(challenge_data["security_keys_enabled"]).to eq(true) - expect(challenge_data["allowed_credential_ids"]).to be_present - expect(challenge_data["challenge"]).to be_present - expect(challenge_data["allowed_methods"]).to contain_exactly( - UserSecondFactor.methods[:totp], - UserSecondFactor.methods[:security_key], - UserSecondFactor.methods[:backup_codes], - ) + expect(response.status).to eq(400) + expect(response.parsed_body["result"]).to eq("wrong user") + end + + context "when logged in" do + before { sign_in(user) } + + it "returns 404 if there is no challenge for the given nonce" do + get "/session/2fa.json", params: { nonce: "asdasdsadsad" } + expect(response.status).to eq(404) + expect(response.parsed_body["error"]).to eq( + I18n.t("second_factor_auth.challenge_not_found"), + ) + end + + it "returns 404 if the nonce does not match the challenge nonce" do + post "/session/2fa/test-action" + get "/session/2fa.json", params: { nonce: "wrongnonce" } + expect(response.status).to eq(404) + expect(response.parsed_body["error"]).to eq( + I18n.t("second_factor_auth.challenge_not_found"), + ) + end + + it "returns 401 if the challenge nonce has expired" do + post "/session/2fa/test-action", xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(200) + expect(response.parsed_body["error"]).not_to be_present + + freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(401) + expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_expired")) + end + + it "responds with challenge data" do + post "/session/2fa/test-action", xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(200) + expect(response.parsed_body["error"]).not_to be_present + challenge_data = response.parsed_body + expect(challenge_data["totp_enabled"]).to eq(true) + expect(challenge_data["backup_enabled"]).to eq(false) + expect(challenge_data["security_keys_enabled"]).to eq(false) + expect(challenge_data["allowed_methods"]).to contain_exactly( + UserSecondFactor.methods[:totp], + UserSecondFactor.methods[:security_key], + ) + expect(challenge_data["description"]).to eq("this is description for test action") + + Fabricate( + :user_security_key_with_random_credential, + user: user, + name: "Enabled YubiKey", + enabled: true, + ) + Fabricate(:user_second_factor_backup, user: user) + post "/session/2fa/test-action", params: { allow_backup_codes: true }, xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + get "/session/2fa.json", params: { nonce: nonce } + expect(response.status).to eq(200) + expect(response.parsed_body["error"]).not_to be_present + challenge_data = response.parsed_body + expect(challenge_data["totp_enabled"]).to eq(true) + expect(challenge_data["backup_enabled"]).to eq(true) + expect(challenge_data["security_keys_enabled"]).to eq(true) + expect(challenge_data["allowed_credential_ids"]).to be_present + expect(challenge_data["challenge"]).to be_present + expect(challenge_data["allowed_methods"]).to contain_exactly( + UserSecondFactor.methods[:totp], + UserSecondFactor.methods[:security_key], + UserSecondFactor.methods[:backup_codes], + ) + end end end describe "#second_factor_auth_perform" do let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) } - before { sign_in(user) } - - it "returns 401 if the challenge nonce has expired" do - post "/session/2fa/test-action", xhr: true - nonce = response.parsed_body["second_factor_challenge_nonce"] - - freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now - token = ROTP::TOTP.new(user_second_factor.data).now - post "/session/2fa.json", - params: { - nonce: nonce, - second_factor_method: UserSecondFactor.methods[:totp], - second_factor_token: token, - } - expect(response.status).to eq(401) - expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_expired")) - end - - it "returns 403 if the 2FA method is not allowed" do - Fabricate(:user_second_factor_backup, user: user) - post "/session/2fa/test-action", xhr: true - nonce = response.parsed_body["second_factor_challenge_nonce"] - post "/session/2fa.json", - params: { - nonce: nonce, - second_factor_method: UserSecondFactor.methods[:backup_codes], - second_factor_token: "iAmValidBackupCode", - } - expect(response.status).to eq(403) - end - - it "returns 403 if the user disables the 2FA method in the middle of the 2FA process" do - post "/session/2fa/test-action", xhr: true - nonce = response.parsed_body["second_factor_challenge_nonce"] - token = ROTP::TOTP.new(user_second_factor.data).now - user_second_factor.destroy! - post "/session/2fa.json", - params: { - nonce: nonce, - second_factor_method: UserSecondFactor.methods[:totp], - second_factor_token: token, - } - expect(response.status).to eq(403) - end - - it "marks the challenge as successful if the 2fa succeeds" do - post "/session/2fa/test-action", params: { redirect_url: "/ggg" }, xhr: true + it "works as anon" do + post "/session/2fa/test-action?username=#{user.username}", xhr: true nonce = response.parsed_body["second_factor_challenge_nonce"] token = ROTP::TOTP.new(user_second_factor.data).now @@ -2975,36 +2955,113 @@ RSpec.describe SessionController do } expect(response.status).to eq(200) expect(response.parsed_body["error"]).not_to be_present - expect(response.parsed_body["ok"]).to eq(true) - expect(response.parsed_body["callback_method"]).to eq("POST") - expect(response.parsed_body["callback_path"]).to eq("/session/2fa/test-action") - expect(response.parsed_body["redirect_url"]).to eq("/ggg") - post "/session/2fa/test-action", params: { second_factor_nonce: nonce } + post "/session/2fa/test-action?username=#{user.username}", + params: { + second_factor_nonce: nonce, + } expect(response.status).to eq(200) expect(response.parsed_body["error"]).not_to be_present expect(response.parsed_body["result"]).to eq("second_factor_auth_completed") end - it "does not mark the challenge as successful if the 2fa fails" do - post "/session/2fa/test-action", params: { redirect_url: "/ggg" }, xhr: true - nonce = response.parsed_body["second_factor_challenge_nonce"] + it "prevents use by different user" do + other_user = Fabricate(:user) - token = ROTP::TOTP.new(user_second_factor.data).now.to_i - token += token == 999_999 ? -1 : 1 - post "/session/2fa.json", - params: { - nonce: nonce, - second_factor_method: UserSecondFactor.methods[:totp], - second_factor_token: token.to_s, - } - expect(response.status).to eq(400) - expect(response.parsed_body["ok"]).to eq(false) - expect(response.parsed_body["reason"]).to eq("invalid_second_factor") - expect(response.parsed_body["error"]).to eq(I18n.t("login.invalid_second_factor_code")) + post "/session/2fa/test-action?username=#{user.username}", xhr: true + expect(response.status).to eq(403) + end - post "/session/2fa/test-action", params: { second_factor_nonce: nonce } - expect(response.status).to eq(401) + context "when signed in" do + before { sign_in(user) } + + it "returns 401 if the challenge nonce has expired" do + post "/session/2fa/test-action", xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + + freeze_time (SecondFactor::AuthManager::MAX_CHALLENGE_AGE + 1.minute).from_now + token = ROTP::TOTP.new(user_second_factor.data).now + post "/session/2fa.json", + params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token, + } + expect(response.status).to eq(401) + expect(response.parsed_body["error"]).to eq(I18n.t("second_factor_auth.challenge_expired")) + end + + it "returns 403 if the 2FA method is not allowed" do + Fabricate(:user_second_factor_backup, user: user) + post "/session/2fa/test-action", xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + post "/session/2fa.json", + params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:backup_codes], + second_factor_token: "iAmValidBackupCode", + } + expect(response.status).to eq(403) + end + + it "returns 403 if the user disables the 2FA method in the middle of the 2FA process" do + post "/session/2fa/test-action", xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + token = ROTP::TOTP.new(user_second_factor.data).now + user_second_factor.destroy! + post "/session/2fa.json", + params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token, + } + expect(response.status).to eq(403) + end + + it "marks the challenge as successful if the 2fa succeeds" do + post "/session/2fa/test-action", params: { redirect_url: "/ggg" }, xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + + token = ROTP::TOTP.new(user_second_factor.data).now + post "/session/2fa.json", + params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token, + } + expect(response.status).to eq(200) + expect(response.parsed_body["error"]).not_to be_present + expect(response.parsed_body["ok"]).to eq(true) + expect(response.parsed_body["callback_method"]).to eq("POST") + expect(response.parsed_body["callback_path"]).to eq("/session/2fa/test-action") + expect(response.parsed_body["redirect_url"]).to eq("/ggg") + + post "/session/2fa/test-action", params: { second_factor_nonce: nonce } + expect(response.status).to eq(200) + expect(response.parsed_body["error"]).not_to be_present + expect(response.parsed_body["result"]).to eq("second_factor_auth_completed") + end + + it "does not mark the challenge as successful if the 2fa fails" do + post "/session/2fa/test-action", params: { redirect_url: "/ggg" }, xhr: true + nonce = response.parsed_body["second_factor_challenge_nonce"] + + token = ROTP::TOTP.new(user_second_factor.data).now.to_i + token += token == 999_999 ? -1 : 1 + post "/session/2fa.json", + params: { + nonce: nonce, + second_factor_method: UserSecondFactor.methods[:totp], + second_factor_token: token.to_s, + } + expect(response.status).to eq(400) + expect(response.parsed_body["ok"]).to eq(false) + expect(response.parsed_body["reason"]).to eq("invalid_second_factor") + expect(response.parsed_body["error"]).to eq(I18n.t("login.invalid_second_factor_code")) + + post "/session/2fa/test-action", params: { second_factor_nonce: nonce } + expect(response.status).to eq(401) + end end end