diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6 index 7fd028cced..5eabad52fb 100644 --- a/app/assets/javascripts/discourse/controllers/login.js.es6 +++ b/app/assets/javascripts/discourse/controllers/login.js.es6 @@ -141,6 +141,7 @@ export default Controller.extend(ModalFunctionality, { document.getElementById("modal-alert").style.display = "none"; this.setProperties({ + otherMethodAllowed: result.multiple_second_factor_methods, secondFactorRequired: true, showLoginButtons: false, backupEnabled: result.backup_enabled, diff --git a/app/assets/javascripts/discourse/templates/mobile/modal/login.hbs b/app/assets/javascripts/discourse/templates/mobile/modal/login.hbs index b2d65fbe87..08e111e52a 100644 --- a/app/assets/javascripts/discourse/templates/mobile/modal/login.hbs +++ b/app/assets/javascripts/discourse/templates/mobile/modal/login.hbs @@ -49,7 +49,7 @@ showSecurityKey=showSecurityKey showSecondFactor=showSecondFactor secondFactorMethod=secondFactorMethod - otherMethodAllowed=secondFactorRequired + otherMethodAllowed=otherMethodAllowed action=(action "authenticateSecurityKey")}} {{/security-key-form}} {{else}} diff --git a/app/assets/javascripts/discourse/templates/modal/login.hbs b/app/assets/javascripts/discourse/templates/modal/login.hbs index 84f79f8eb7..d76add4d57 100644 --- a/app/assets/javascripts/discourse/templates/modal/login.hbs +++ b/app/assets/javascripts/discourse/templates/modal/login.hbs @@ -35,7 +35,7 @@ showSecurityKey=showSecurityKey showSecondFactor=showSecondFactor secondFactorMethod=secondFactorMethod - otherMethodAllowed=secondFactorRequired + otherMethodAllowed=otherMethodAllowed action=(action "authenticateSecurityKey")}} {{/security-key-form}} {{else}} diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 8b51a339c2..b7a3fcba17 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -295,22 +295,36 @@ class SessionController < ApplicationController if payload = login_error_check(user) render json: payload else - if (params[:second_factor_token].blank?) - security_key_valid = ::Webauthn::SecurityKeyAuthenticationService.new(user, params[:security_key_credential], - challenge: secure_session["staged-webauthn-challenge-#{user.id}"], - rp_id: secure_session["staged-webauthn-rp-id-#{user.id}"], + if user.security_keys_enabled? && params[:second_factor_token].blank? + security_key_valid = ::Webauthn::SecurityKeyAuthenticationService.new( + user, + params[:security_key_credential], + challenge: Webauthn.challenge(user, secure_session), + rp_id: Webauthn.rp_id(user, secure_session), origin: Discourse.base_url ).authenticate_security_key - return invalid_security_key(user) if user.security_keys_enabled? && !security_key_valid + return invalid_security_key(user) if !security_key_valid + return (user.active && user.email_confirmed?) ? login(user) : not_activated(user) end - if user.totp_enabled? && \ - !user.authenticate_second_factor(params[:second_factor_token], params[:second_factor_method].to_i) && - !params[:security_key_credential].present? + if user.totp_enabled? + invalid_second_factor = !user.authenticate_second_factor(params[:second_factor_token], params[:second_factor_method].to_i) + if (params[:security_key_credential].blank? || !user.security_keys_enabled?) && invalid_second_factor + return render json: failed_json.merge( + error: I18n.t("login.invalid_second_factor_code"), + reason: "invalid_second_factor", + backup_enabled: user.backup_codes_enabled?, + multiple_second_factor_methods: user.has_multiple_second_factor_methods? + ) + end + elsif user.security_keys_enabled? + # if we have gotten this far then the user has provided the totp + # params for a security-key-only account return render json: failed_json.merge( error: I18n.t("login.invalid_second_factor_code"), reason: "invalid_second_factor", - backup_enabled: user.backup_codes_enabled? + backup_enabled: user.backup_codes_enabled?, + multiple_second_factor_methods: user.has_multiple_second_factor_methods? ) end @@ -321,12 +335,13 @@ class SessionController < ApplicationController end def invalid_security_key(user, err_message = nil) - stage_webauthn_security_key_challenge(user) if !params[:security_key_credential] + Webauthn.stage_challenge(user, secure_session) if !params[:security_key_credential] render json: failed_json.merge( error: err_message || I18n.t("login.invalid_security_key"), reason: "invalid_security_key", - backup_enabled: user.backup_codes_enabled? - ).merge(webauthn_security_key_challenge_and_allowed_credentials(user)) + backup_enabled: user.backup_codes_enabled?, + multiple_second_factor_methods: user.has_multiple_second_factor_methods? + ).merge(Webauthn.allowed_credentials(user, secure_session)) end def email_login_info @@ -351,9 +366,9 @@ class SessionController < ApplicationController end if matched_user&.security_keys_enabled? - stage_webauthn_security_key_challenge(matched_user) + Webauthn.stage_challenge(matched_user, secure_session) response.merge!( - webauthn_security_key_challenge_and_allowed_credentials(matched_user).merge(security_key_required: true) + Webauthn.allowed_credentials(matched_user, secure_session).merge(security_key_required: true) ) end @@ -376,9 +391,11 @@ class SessionController < ApplicationController if security_key_credential.present? if matched_token&.user&.security_keys_enabled? - security_key_valid = ::Webauthn::SecurityKeyAuthenticationService.new(matched_token&.user, params[:security_key_credential], - challenge: secure_session["staged-webauthn-challenge-#{matched_token&.user&.id}"], - rp_id: secure_session["staged-webauthn-rp-id-#{matched_token&.user&.id}"], + security_key_valid = ::Webauthn::SecurityKeyAuthenticationService.new( + matched_token&.user, + params[:security_key_credential], + challenge: Webauthn.challenge(matched_token&.user, secure_session), + rp_id: Webauthn.rp_id(matched_token&.user, secure_session), origin: Discourse.base_url ).authenticate_security_key return invalid_security_key(matched_token&.user) if !security_key_valid @@ -391,6 +408,10 @@ class SessionController < ApplicationController RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed! return render json: { error: I18n.t('login.invalid_second_factor_code') } end + elsif matched_token&.user&.security_keys_enabled? + # this means the user only has security key enabled + # but has not provided credentials + return render json: { error: I18n.t('login.invalid_second_factor_code') } end end @@ -407,7 +428,7 @@ class SessionController < ApplicationController render json: { error: I18n.t('email_login.invalid_token') } rescue ::Webauthn::SecurityKeyError => err - invalid_security_key(user, err.message) + invalid_security_key(matched_token&.user, err.message) end def one_time_password @@ -577,21 +598,4 @@ class SessionController < ApplicationController def sso_url(sso) sso.to_url end - - def stage_webauthn_security_key_challenge(user) - challenge = SecureRandom.hex(30) - secure_session["staged-webauthn-challenge-#{user.id}"] = challenge - secure_session["staged-webauthn-rp-id-#{user.id}"] = Discourse.current_hostname - end - - def webauthn_security_key_challenge_and_allowed_credentials(user) - return {} if !user.security_keys_enabled? - credential_ids = user.security_keys.select(:credential_id) - .where(factor_type: UserSecurityKey.factor_types[:second_factor]) - .pluck(:credential_id) - { - allowed_credential_ids: credential_ids, - challenge: secure_session["staged-webauthn-challenge-#{user.id}"] - } - end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 07afe1a40d..b4a6515432 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class UsersController < ApplicationController - skip_before_action :authorize_mini_profiler, only: [:avatar] requires_login only: [ @@ -536,8 +535,8 @@ class UsersController < ApplicationController security_key_authenticated = ::Webauthn::SecurityKeyAuthenticationService.new( @user, security_key_credential, - challenge: secure_session["staged-webauthn-challenge-#{@user.id}"], - rp_id: secure_session["staged-webauthn-rp-id-#{@user.id}"], + challenge: Webauthn.challenge(@user, secure_session), + rp_id: Webauthn.rp_id(@user, secure_session), origin: Discourse.base_url ).authenticate_security_key end @@ -561,6 +560,9 @@ class UsersController < ApplicationController if !valid_second_factor @user.errors.add(:user_second_factors, :invalid) @error = I18n.t('login.invalid_second_factor_code') + elsif !valid_security_key + @user.errors.add(:security_keys, :invalid) + @error = I18n.t('login.invalid_security_key') elsif @invalid_password = params[:password].blank? || params[:password].size > User.max_password_length @user.errors.add(:password, :invalid) else @@ -586,7 +588,7 @@ class UsersController < ApplicationController if @error render layout: 'no_ember' else - stage_webauthn_security_key_challenge(@user) + Webauthn.stage_challenge(@user, secure_session) store_preloaded( "password_reset", MultiJson.dump( @@ -596,7 +598,7 @@ class UsersController < ApplicationController second_factor_required: !valid_second_factor, security_key_required: !valid_security_key, backup_enabled: @user.backup_codes_enabled? - }.merge(webauthn_security_key_challenge_and_allowed_credentials(@user)) + }.merge(Webauthn.allowed_credentials(@user, secure_session)) ) ) end @@ -629,14 +631,14 @@ class UsersController < ApplicationController errors: @user&.errors&.to_hash } else - stage_webauthn_security_key_challenge(@user) if !valid_security_key && !security_key_credential.present? + Webauthn.stage_challenge(@user, secure_session) if !valid_security_key && !security_key_credential.present? render json: { is_developer: UsernameCheckerService.is_developer?(@user.email), admin: @user.admin?, second_factor_required: !valid_second_factor, security_key_required: !valid_security_key, backup_enabled: @user.backup_codes_enabled? - }.merge(webauthn_security_key_challenge_and_allowed_credentials(@user)) + }.merge(Webauthn.allowed_credentials(@user, secure_session)) end end end @@ -699,8 +701,8 @@ class UsersController < ApplicationController @security_key_required = security_keys_enabled if security_keys_enabled && params[:security_key_credential].blank? - stage_webauthn_security_key_challenge(email_token_user) - challenge_and_credentials = webauthn_security_key_challenge_and_allowed_credentials(email_token_user) + Webauthn.stage_challenge(email_token_user, secure_session) + challenge_and_credentials = Webauthn.allowed_credentials(email_token_user, secure_session) @security_key_challenge = challenge_and_credentials[:challenge] @security_key_allowed_credential_ids = challenge_and_credentials[:allowed_credential_ids].join(",") end @@ -708,9 +710,11 @@ class UsersController < ApplicationController if security_keys_enabled && params[:security_key_credential].present? credential = JSON.parse(params[:security_key_credential]).with_indifferent_access - confirm_email = ::Webauthn::SecurityKeyAuthenticationService.new(email_token_user, credential, - challenge: secure_session["staged-webauthn-challenge-#{email_token_user&.id}"], - rp_id: secure_session["staged-webauthn-rp-id-#{email_token_user&.id}"], + confirm_email = ::Webauthn::SecurityKeyAuthenticationService.new( + email_token_user, + credential, + challenge: Webauthn.challenge(email_token_user, secure_session), + rp_id: Webauthn.rp_id(email_token_user, secure_session), origin: Discourse.base_url ).authenticate_security_key @message = I18n.t('login.security_key_invalid') if !confirm_email @@ -1191,10 +1195,10 @@ class UsersController < ApplicationController error: I18n.t("login.incorrect_password") ) end - secure_session["confirmed-password-#{current_user.id}"] = "true" + confirm_secure_session end - if secure_session["confirmed-password-#{current_user.id}"] == "true" + if secure_session_confirmed? totp_second_factors = current_user.totps .select(:id, :name, :last_used, :created_at, :method) .where(enabled: true).order(:created_at) @@ -1238,15 +1242,11 @@ class UsersController < ApplicationController end def create_second_factor_security_key - challenge = SecureRandom.hex(30) - secure_session["staged-webauthn-challenge-#{current_user.id}"] = challenge - secure_session["staged-webauthn-rp-id-#{current_user.id}"] = Discourse.current_hostname - secure_session["staged-webauthn-rp-name-#{current_user.id}"] = SiteSetting.title - + challenge_session = Webauthn.stage_challenge(current_user, secure_session) render json: success_json.merge( - challenge: challenge, - rp_id: Discourse.current_hostname, - rp_name: SiteSetting.title, + challenge: challenge_session.challenge, + rp_id: challenge_session.rp_id, + rp_name: challenge_session.rp_name, supported_algoriths: ::Webauthn::SUPPORTED_ALGORITHMS, user_secure_id: current_user.create_or_fetch_secure_identifier, existing_active_credential_ids: current_user.second_factor_security_key_credential_ids @@ -1261,15 +1261,13 @@ class UsersController < ApplicationController ::Webauthn::SecurityKeyRegistrationService.new( current_user, params, - challenge: secure_session["staged-webauthn-challenge-#{current_user.id}"], - rp_id: secure_session["staged-webauthn-rp-id-#{current_user.id}"], + challenge: Webauthn.challenge(current_user, secure_session), + rp_id: Webauthn.rp_id(current_user, secure_session), origin: Discourse.base_url ).register_second_factor_security_key render json: success_json rescue ::Webauthn::SecurityKeyError => err - render json: failed_json.merge( - error: err.message - ) + render json: failed_json.merge(error: err.message) end def update_security_key @@ -1358,7 +1356,7 @@ class UsersController < ApplicationController def second_factor_check_confirmed_password raise Discourse::NotFound if SiteSetting.enable_sso || !SiteSetting.enable_local_logins - raise Discourse::InvalidAccess.new unless current_user && secure_session["confirmed-password-#{current_user.id}"] == "true" + raise Discourse::InvalidAccess.new unless current_user && secure_session_confirmed? end def revoke_account @@ -1538,19 +1536,11 @@ class UsersController < ApplicationController end end - def stage_webauthn_security_key_challenge(user) - challenge = SecureRandom.hex(30) - secure_session["staged-webauthn-challenge-#{user.id}"] = challenge - secure_session["staged-webauthn-rp-id-#{user.id}"] = Discourse.current_hostname + def confirm_secure_session + secure_session["confirmed-password-#{current_user.id}"] = "true" end - def webauthn_security_key_challenge_and_allowed_credentials(user) - return {} if !user.security_keys_enabled? - credential_ids = user.second_factor_security_key_credential_ids - { - allowed_credential_ids: credential_ids, - challenge: secure_session["staged-webauthn-challenge-#{user.id}"] - } + def secure_session_confirmed? + secure_session["confirmed-password-#{current_user.id}"] == "true" end - end diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index 4fde41c08b..235242339f 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/app/models/concerns/second_factor_manager.rb @@ -67,6 +67,10 @@ module SecondFactorManager self&.security_keys.where(factor_type: UserSecurityKey.factor_types[:second_factor], enabled: true).exists? end + def has_multiple_second_factor_methods? + security_keys_enabled? && (totp_enabled? || backup_codes_enabled?) + end + def remaining_backup_codes self&.user_second_factors&.backup_codes&.count end @@ -76,6 +80,11 @@ module SecondFactorManager authenticate_totp(token) elsif second_factor_method == UserSecondFactor.methods[:backup_codes] authenticate_backup_code(token) + elsif second_factor_method == UserSecondFactor.methods[:security_key] + # some craziness has happened if we have gotten here...like the user + # switching around their second factor types then continuing an already + # started login attempt + false end end diff --git a/app/models/user_second_factor.rb b/app/models/user_second_factor.rb index a5e34216dc..7846f27ad8 100644 --- a/app/models/user_second_factor.rb +++ b/app/models/user_second_factor.rb @@ -20,6 +20,7 @@ class UserSecondFactor < ActiveRecord::Base @methods ||= Enum.new( totp: 1, backup_codes: 2, + security_key: 3, ) end diff --git a/lib/webauthn.rb b/lib/webauthn.rb index cd4ecb917f..56705cfa23 100644 --- a/lib/webauthn.rb +++ b/lib/webauthn.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true +require 'webauthn/challenge_generator' require 'webauthn/security_key_base_validation_service' require 'webauthn/security_key_registration_service' require 'webauthn/security_key_authentication_service' @@ -27,4 +28,38 @@ module Webauthn class OwnershipError < SecurityKeyError; end class PublicKeyError < SecurityKeyError; end class UnknownCOSEAlgorithmError < SecurityKeyError; end + + ## + # Usage: + # + # These methods should be used in controllers where we + # are challenging the user that has a security key, and + # they must respond with a valid webauthn response and + # credentials. + def self.stage_challenge(user, secure_session) + ::Webauthn::ChallengeGenerator.generate.commit_to_session(secure_session, user) + end + + def self.allowed_credentials(user, secure_session) + return {} if !user.security_keys_enabled? + credential_ids = user.second_factor_security_key_credential_ids + { + allowed_credential_ids: credential_ids, + challenge: secure_session[ + Webauthn::ChallengeGenerator::ChallengeSession.session_challenge_key(user) + ] + } + end + + def self.rp_id(user, secure_session) + secure_session[Webauthn::ChallengeGenerator::ChallengeSession.session_rp_id_key(user)] + end + + def self.rp_name(user, secure_session) + secure_session[Webauthn::ChallengeGenerator::ChallengeSession.session_rp_name_key(user)] + end + + def self.challenge(user, secure_session) + secure_session[Webauthn::ChallengeGenerator::ChallengeSession.session_challenge_key(user)] + end end diff --git a/lib/webauthn/challenge_generator.rb b/lib/webauthn/challenge_generator.rb new file mode 100644 index 0000000000..b35c95d5cf --- /dev/null +++ b/lib/webauthn/challenge_generator.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true +module Webauthn + class ChallengeGenerator + class ChallengeSession + attr_reader :challenge, :rp_id, :rp_name + + def initialize(params) + @challenge = params[:challenge] + @rp_id = params[:rp_id] + @rp_name = params[:rp_name] + end + + def commit_to_session(secure_session, user) + secure_session[self.class.session_challenge_key(user)] = @challenge + secure_session[self.class.session_rp_id_key(user)] = @rp_id + secure_session[self.class.session_rp_name_key(user)] = @rp_name + + self + end + + def self.session_challenge_key(user) + "staged-webauthn-challenge-#{user&.id}" + end + + def self.session_rp_id_key(user) + "staged-webauthn-rp-id-#{user&.id}" + end + + def self.session_rp_name_key(user) + "staged-webauthn-rp-name-#{user&.id}" + end + end + + def self.generate + ChallengeSession.new( + challenge: SecureRandom.hex(30), + rp_id: Discourse.current_hostname, + rp_name: SiteSetting.title + ) + end + end +end diff --git a/spec/lib/webauthn/challenge_generator_spec.rb b/spec/lib/webauthn/challenge_generator_spec.rb new file mode 100644 index 0000000000..a0b7609ecf --- /dev/null +++ b/spec/lib/webauthn/challenge_generator_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Webauthn::ChallengeGenerator do + it "generates a Webauthn::ChallengeGenerator::ChallengeSession with correct params" do + session = Webauthn::ChallengeGenerator.generate + expect(session).to be_a(Webauthn::ChallengeGenerator::ChallengeSession) + expect(session.challenge).not_to eq(nil) + expect(session.rp_id).to eq(Discourse.current_hostname) + expect(session.rp_name).to eq(SiteSetting.title) + end + + describe "ChallengeSession" do + describe "#commit_to_session" do + let(:user) { Fabricate(:user) } + + it "stores the challenge, rpid, and name in the provided session object" do + secure_session = {} + generated_session = Webauthn::ChallengeGenerator.generate + generated_session.commit_to_session(secure_session, user) + + expect(secure_session["staged-webauthn-challenge-#{user&.id}"]).to eq(generated_session.challenge) + expect(secure_session["staged-webauthn-rp-id-#{user&.id}"]).to eq(generated_session.rp_id) + expect(secure_session["staged-webauthn-rp-name-#{user&.id}"]).to eq(generated_session.rp_name) + end + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 5f8d9f5005..5497386178 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -160,6 +160,7 @@ RSpec.configure do |config| config.include MessageBus config.include RSpecHtmlMatchers config.include IntegrationHelpers, type: :request + config.include WebauthnIntegrationHelpers, type: :request config.include SiteSettingsHelpers config.mock_framework = :mocha config.order = 'random' diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 9cb114679c..3b83d08724 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -61,9 +61,10 @@ RSpec.describe SessionController do it "includes that information in the response" do get "/session/email-login/#{email_token.token}.json" - expect(JSON.parse(response.body)["can_login"]).to eq(true) - expect(JSON.parse(response.body)["second_factor_required"]).to eq(true) - expect(JSON.parse(response.body)["backup_codes_enabled"]).to eq(true) + response_body_parsed = JSON.parse(response.body) + expect(response_body_parsed["can_login"]).to eq(true) + expect(response_body_parsed["second_factor_required"]).to eq(true) + expect(response_body_parsed["backup_codes_enabled"]).to eq(true) end end @@ -73,14 +74,15 @@ RSpec.describe SessionController do it "includes that information in the response" do get "/session/email-login/#{email_token.token}.json" - expect(JSON.parse(response.body)["can_login"]).to eq(true) - expect(JSON.parse(response.body)["security_key_required"]).to eq(true) - expect(JSON.parse(response.body)["second_factor_required"]).to eq(nil) - expect(JSON.parse(response.body)["backup_codes_enabled"]).to eq(nil) - expect(JSON.parse(response.body)["allowed_credential_ids"]).to eq([user_security_key.credential_id]) + response_body_parsed = JSON.parse(response.body) + expect(response_body_parsed["can_login"]).to eq(true) + expect(response_body_parsed["security_key_required"]).to eq(true) + expect(response_body_parsed["second_factor_required"]).to eq(nil) + expect(response_body_parsed["backup_codes_enabled"]).to eq(nil) + expect(response_body_parsed["allowed_credential_ids"]).to eq([user_security_key.credential_id]) secure_session = SecureSession.new(session["secure_session_id"]) - expect(JSON.parse(response.body)["challenge"]).to eq(secure_session["staged-webauthn-challenge-#{user.id}"]) - expect(secure_session["staged-webauthn-rp-id-#{user.id}"]).to eq(Discourse.current_hostname) + expect(response_body_parsed["challenge"]).to eq(Webauthn.challenge(user, secure_session)) + expect(Webauthn.rp_id(user, secure_session)).to eq(Discourse.current_hostname) end end end @@ -284,6 +286,80 @@ RSpec.describe SessionController do end end end + + context "user has only security key 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 + + before do + simulate_localhost_webauthn_challenge + + # store challenge in secure session by visiting the email login page + get "/session/email-login/#{email_token.token}.json" + end + + context "when the security key params are blank and a random second factor token is provided" do + it "shows an error message and denies login" do + + post "/session/email-login/#{email_token.token}.json", params: { + second_factor_token: "XXXXXXX", + second_factor_method: UserSecondFactor.methods[:totp] + } + + 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 + context "when the security key params are invalid" do + it" shows an error message and denies login" do + + post "/session/email-login/#{email_token.token}.json", params: { + security_key_credential: { + signature: 'bad_sig', + clientData: 'bad_clientData', + credentialId: 'bad_credential_id', + authenticatorData: 'bad_authenticator_data' + }, + second_factor_method: UserSecondFactor.methods[:security_key] + } + + expect(response.status).to eq(200) + expect(session[:current_user_id]).to eq(nil) + response_body = JSON.parse(response.body) + expect(response_body["failed"]).to eq("FAILED") + expect(response_body['error']).to eq(I18n.t( + 'webauthn.validation.not_found_error' + )) + end + end + context "when the security key params are valid" do + it "logs the user in" 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_method: UserSecondFactor.methods[:security_key] + } + + expect(response.status).to eq(200) + user.reload + + expect(session[:current_user_id]).to eq(user.id) + expect(user.user_auth_tokens.count).to eq(1) + end + end + end end end @@ -1061,7 +1137,114 @@ RSpec.describe SessionController do end end - context 'when user has 2-factor logins' do + context "when a user has security key-only 2FA login" 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 + + before do + simulate_localhost_webauthn_challenge + + # store challenge in secure session by failing login once + post "/session.json", params: { + login: user.username, + password: 'myawesomepassword' + } + end + + context "when the security key params are blank and a random second factor token is provided" do + it "shows an error message and denies login" do + + post "/session.json", params: { + login: user.username, + password: 'myawesomepassword', + security_key_credential: {}, + second_factor_token: '99999999', + second_factor_method: UserSecondFactor.methods[:totp] + } + + expect(response.status).to eq(200) + expect(session[:current_user_id]).to eq(nil) + 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' + )) + end + end + context "when the security key params are invalid" do + it" shows an error message and denies login" do + + post "/session.json", params: { + login: user.username, + password: 'myawesomepassword', + security_key_credential: { + signature: 'bad_sig', + clientData: 'bad_clientData', + credentialId: 'bad_credential_id', + authenticatorData: 'bad_authenticator_data' + }, + second_factor_method: UserSecondFactor.methods[:security_key] + } + + expect(response.status).to eq(200) + expect(session[:current_user_id]).to eq(nil) + response_body = JSON.parse(response.body) + expect(response_body["failed"]).to eq("FAILED") + expect(response_body['error']).to eq(I18n.t( + 'webauthn.validation.not_found_error' + )) + end + end + context "when the security key params are valid" do + it "logs the user in" do + + post "/session.json", params: { + login: user.username, + password: 'myawesomepassword', + security_key_credential: valid_security_key_auth_post_data, + second_factor_method: UserSecondFactor.methods[:security_key] + } + + expect(response.status).to eq(200) + user.reload + + expect(session[:current_user_id]).to eq(user.id) + expect(user.user_auth_tokens.count).to eq(1) + end + end + context "when the security key is disabled in the background by the user and TOTP is enabled" do + before do + user_security_key.destroy! + Fabricate(:user_second_factor_totp, user: user) + end + + it "shows an error message and denies login" do + + post "/session.json", params: { + login: user.username, + password: 'myawesomepassword', + security_key_credential: valid_security_key_auth_post_data, + second_factor_method: UserSecondFactor.methods[:security_key] + } + + expect(response.status).to eq(200) + expect(session[:current_user_id]).to eq(nil) + 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' + )) + end + end + end + + context 'when user has TOTP-only 2FA login' do let!(:user_second_factor) { Fabricate(:user_second_factor_totp, user: user) } let!(:user_second_factor_backup) { Fabricate(:user_second_factor_backup, user: user) } diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index c7898ab7e0..ecfdb6aa79 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -384,47 +384,43 @@ describe UsersController do end context 'security key authentication required' do - let!(:security_key) { Fabricate(:user_security_key, user: user, factor_type: UserSecurityKey.factor_types[:second_factor]) } + 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(:token) { user.email_tokens.create!(email: user.email).token } + + before do + simulate_localhost_webauthn_challenge + + # store challenge in secure session by visiting the email login page + get "/u/password-reset/#{token}" + end it 'preloads with a security key challenge and allowed credential ids' do - token = user.email_tokens.create!(email: user.email).token - - get "/u/password-reset/#{token}" - expect(response.body).to have_tag("div#data-preloaded") do |element| json = JSON.parse(element.current_scope.attribute('data-preloaded').value) password_reset = JSON.parse(json['password_reset']) expect(password_reset['challenge']).not_to eq(nil) - expect(password_reset['allowed_credential_ids']).to eq([security_key.credential_id]) + expect(password_reset['allowed_credential_ids']).to eq([user_security_key.credential_id]) expect(password_reset['security_key_required']).to eq(true) end end it 'stages a webauthn challenge and rp-id for the user' do - token = user.email_tokens.create!(email: user.email).token - - get "/u/password-reset/#{token}" - secure_session = SecureSession.new(session["secure_session_id"]) - expect(secure_session["staged-webauthn-challenge-#{user.id}"]).not_to eq(nil) - expect(secure_session["staged-webauthn-rp-id-#{user.id}"]).to eq(Discourse.current_hostname) + expect(Webauthn.challenge(user, secure_session)).not_to eq(nil) + expect(Webauthn.rp_id(user, secure_session)).to eq(Discourse.current_hostname) end it 'changes password with valid security key challenge and authentication' do - token = user.email_tokens.create(email: user.email).token - - get "/u/password-reset/#{token}" - - ::Webauthn::SecurityKeyAuthenticationService.any_instance.stubs(:authenticate_security_key).returns(true) - - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{token}.json", params: { password: 'hg9ow8yHG32O', - security_key_credential: { - signature: 'test', - clientData: 'test', - authenticatorData: 'test', - credentialId: 'test' - }, + security_key_credential: valid_security_key_auth_post_data, second_factor_method: UserSecondFactor.methods[:security_key] } @@ -433,6 +429,26 @@ describe UsersController do expect(user.confirm_password?('hg9ow8yHG32O')).to eq(true) expect(user.user_auth_tokens.count).to eq(1) 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: { + signature: 'bad', + clientData: 'bad', + authenticatorData: 'bad', + credentialId: 'bad' + }, + second_factor_method: UserSecondFactor.methods[:security_key] + } + + 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")) + end + end end end @@ -590,12 +606,24 @@ describe UsersController do end describe 'when security key authentication required' do - fab!(:security_key) { Fabricate(:user_security_key, user: admin) } fab!(:email_token) { Fabricate(:email_token, user: admin) } + let!(:security_key) do + Fabricate( + :user_security_key, + user: admin, + credential_id: valid_security_key_data[:credential_id], + public_key: valid_security_key_data[:public_key] + ) + end + + before do + simulate_localhost_webauthn_challenge + + # store challenge in secure session by visiting the admin login page + get "/u/admin-login/#{email_token.token}" + end it 'does not log in when token required' do - security_key - get "/u/admin-login/#{email_token.token}" expect(response).not_to redirect_to('/') expect(session[:current_user_id]).not_to eq(admin.id) expect(response.body).to include(I18n.t('login.security_key_authenticate')) @@ -603,33 +631,24 @@ describe UsersController do describe 'invalid security key' do it 'should display the right error' do - ::Webauthn::SecurityKeyAuthenticationService.any_instance.stubs(:authenticate_security_key).returns(false) - put "/u/admin-login/#{email_token.token}", params: { security_key_credential: { - signature: 'test', - clientData: 'test', - authenticatorData: 'test', - credentialId: 'test' + signature: 'bad', + clientData: 'bad', + authenticatorData: 'bad', + credentialId: 'bad' }.to_json, second_factor_method: UserSecondFactor.methods[:security_key] } expect(response.status).to eq(200) - expect(response.body).to include(I18n.t('login.security_key_invalid')) + expect(response.body).to include(I18n.t('webauthn.validation.not_found_error')) end end it 'logs in when a valid security key is given' do - ::Webauthn::SecurityKeyAuthenticationService.any_instance.stubs(:authenticate_security_key).returns(true) - put "/u/admin-login/#{email_token.token}", params: { - security_key_credential: { - signature: 'test', - clientData: 'test', - authenticatorData: 'test', - credentialId: 'test' - }.to_json, + security_key_credential: valid_security_key_auth_post_data.to_json, second_factor_method: UserSecondFactor.methods[:security_key] } @@ -3642,6 +3661,77 @@ describe UsersController do end end + describe "#create_second_factor_security_key" do + it "stores the challenge in the session and returns challenge data, user id, and supported algorithms" do + create_second_factor_security_key + secure_session = read_secure_session + response_parsed = JSON.parse(response.body) + expect(response_parsed["challenge"]).to eq( + Webauthn.challenge(user, secure_session) + ) + expect(response_parsed["rp_id"]).to eq( + Webauthn.rp_id(user, secure_session) + ) + expect(response_parsed["rp_name"]).to eq( + Webauthn.rp_name(user, secure_session) + ) + expect(response_parsed["user_secure_id"]).to eq( + user.reload.create_or_fetch_secure_identifier + ) + expect(response_parsed["supported_algoriths"]).to eq( + ::Webauthn::SUPPORTED_ALGORITHMS + ) + end + + context "if the user has security key credentials already" do + let!(:user_security_key) { Fabricate(:user_security_key_with_random_credential, user: user) } + + it "returns those existing active credentials" do + create_second_factor_security_key + response_parsed = JSON.parse(response.body) + expect(response_parsed["existing_active_credential_ids"]).to eq( + [user_security_key.credential_id] + ) + end + end + end + + describe "#register_second_factor_security_key" do + context "when creation parameters are valid" do + it "creates a security key for the user" do + simulate_localhost_webauthn_challenge + create_second_factor_security_key + response_parsed = JSON.parse(response.body) + + post "/u/register_second_factor_security_key.json", params: valid_security_key_create_post_data + + expect(user.security_keys.count).to eq(1) + expect(user.security_keys.last.credential_id).to eq(valid_security_key_create_post_data[:rawId]) + expect(user.security_keys.last.name).to eq(valid_security_key_create_post_data[:name]) + end + end + + context "when the creation parameters are invalid" do + it "shows a security key error and does not create a key" do + stub_as_dev_localhost + create_second_factor_security_key + response_parsed = JSON.parse(response.body) + + post "/u/register_second_factor_security_key.json", params: { + id: "bad id", + rawId: "bad rawId", + type: "public-key", + attestation: "bad attestation", + clientData: Base64.encode64('{"bad": "json"}'), + name: "My Bad Key" + } + + expect(user.security_keys.count).to eq(0) + expect(JSON.parse(response.body)["error"]).to eq(I18n.t("webauthn.validation.invalid_type_error")) + end + end + end + describe '#revoke_account' do fab!(:other_user) { Fabricate(:user) } it 'errors for unauthorised users' do @@ -3949,4 +4039,10 @@ describe UsersController do expect(user.user_profile.featured_topic).to eq nil end end + + def create_second_factor_security_key + sign_in(user) + UsersController.any_instance.stubs(:secure_session_confirmed?).returns(true) + post "/u/create_second_factor_security_key.json" + end end diff --git a/spec/support/integration_helpers.rb b/spec/support/integration_helpers.rb index 381fedf2d5..d44adc333b 100644 --- a/spec/support/integration_helpers.rb +++ b/spec/support/integration_helpers.rb @@ -29,4 +29,8 @@ module IntegrationHelpers get "/session/#{user.username}/become" user end + + def read_secure_session + SecureSession.new(session[:secure_session_id]) + end end diff --git a/spec/support/webauthn_integration_helpers.rb b/spec/support/webauthn_integration_helpers.rb new file mode 100644 index 0000000000..d6a32ffd35 --- /dev/null +++ b/spec/support/webauthn_integration_helpers.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module WebauthnIntegrationHelpers + ## + # Usage notes: + # + # The valid_security_key_auth_post_data is derived from an actual YubiKey login + # attempt that is successful. No security risk is posed by this; this YubiKey + # has only ever been used for local credentials. + # + # To make this all work together you need to + # create a UserSecurityKey for a user using valid_security_key_data, + # and you override Webauthn::ChallengeGenerator.generate to return + # a Webauthn::ChallengeGenerator::ChallengeSession object using + # valid_security_key_challenge_data. + # + # This is because the challenge is embedded + # in the post data's authenticatorData and must match up. See + # simulate_localhost_webautn_challenge for a real example. + def valid_security_key_data + { + credential_id: "9GiFosW50+s+juyJlyxKEVAsk3gZLo9XWIhX47eC4gHfDsldF3TWR43Tcl/+3gLTL5t1TjpmcbKA2DUV2eKrBw==".freeze, + public_key: "pQECAyYgASFYIPMGM1OpSuCU5uks+BulAdfVxdlJiYcgGac5Y+LnLXC9Ilgghy0BKvRvptmQdtWz33Jjnf8Y6+HD85XdRiqmo1KMGPE=".freeze + } + end + + def valid_security_key_auth_post_data + { + signature: "MEYCIQC5xyUQvF4qTPZ2yX7crp/IEs1E/4wqhXgxC1EVAumhfgIhAIC/7w4BVEy+ew6vMYISahtnnIqbqsPZosBeTUSI8Y4j".freeze, + clientData: "eyJjaGFsbGVuZ2UiOiJOR1UzWW1Zek0yWTBNelkyWkdFM05EVTNZak5qWldVNFpUWTNOakJoTm1NMFlqVTVORFptTlRrd016Vm1ZMlZpTURVd01UZzJOemcxTW1RMSIsIm9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzAwMCIsInR5cGUiOiJ3ZWJhdXRobi5nZXQifQ==".freeze, + authenticatorData: "SZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzzuoMdl2MBAAAA2Q==".freeze, + credentialId: valid_security_key_data[:credential_id] + } + end + + def valid_security_key_challenge_data + { + challenge: "4e7bf33f4366da7457b3cee8e6760a6c4b5946f59035fceb0501867852d5".freeze + } + end + + def valid_security_key_create_post_data + { + id: "hg7Ojg9H4urf9UlT99T2yr-FQtEGCWnRNdkI5QKEqDxlSjsLHhUcQxeTPelC26cy9XQ_qIg1Nq88PNVDlZvxHA", + rawId: "hg7Ojg9H4urf9UlT99T2yr+FQtEGCWnRNdkI5QKEqDxlSjsLHhUcQxeTPelC26cy9XQ/qIg1Nq88PNVDlZvxHA==", + type: "public-key", + attestation: "o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVjESZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzzuoMdl2NBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQIYOzo4PR+Lq3/VJU/fU9sq/hULRBglp0TXZCOUChKg8ZUo7Cx4VHEMXkz3pQtunMvV0P6iINTavPDzVQ5Wb8RylAQIDJiABIVggJI3i7Svv1+Hu8pGYIQ6XEIeWHxjr+qKVXPmXSQswGysiWCDs0ZRoPXkajl+Mpvc16BPVFrKRxl06V+XTKdKffiMzZQ==", + clientData: "eyJjaGFsbGVuZ2UiOiJOR1UzWW1Zek0yWTBNelkyWkdFM05EVTNZak5qWldVNFpUWTNOakJoTm1NMFlqVTVORFptTlRrd016Vm1ZMlZpTURVd01UZzJOemcxTW1RMSIsIm9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzAwMCIsInR5cGUiOiJ3ZWJhdXRobi5jcmVhdGUifQ==", + name: "My Security Key" + } + end + + # all of the valid security key data is sourced from a localhost + # login, if this is not set the specs for webauthn WILL NOT WORK + def stub_as_dev_localhost + Discourse.stubs(:current_hostname).returns('localhost') + Discourse.stubs(:base_url).returns('http://localhost:3000') + end + + def simulate_localhost_webauthn_challenge + stub_as_dev_localhost + Webauthn::ChallengeGenerator.stubs(:generate).returns( + Webauthn::ChallengeGenerator::ChallengeSession.new( + challenge: valid_security_key_challenge_data[:challenge], + rp_id: Discourse.current_hostname + ) + ) + end +end