From 4db5525d2582a39d5b64885e77307224483dfd97 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Fri, 11 Nov 2022 13:00:06 +1100 Subject: [PATCH] FIX: do not lock account if backup codes are available (#18982) Currently, we have available three 2fa methods: - Token-Based Authenticators - Physical Security Keys - Two-Factor Backup Codes If the first two are deleted, user lose visibility of their backup codes, which suggests that 2fa is disabled. However, when they try to authenticate, the account is locked, and they have to ask admin to fix that problem. This PR is fixing the issue. User still sees backup codes in their panel and can use them to authenticate. In next PR, I will improve UI to clearly notify the user when 2fa is fully disabled and when it is still active. --- .../app/components/second-factor-form.js | 8 +++++--- .../discourse/app/controllers/login.js | 19 ++++++++++++++----- .../discourse/app/templates/modal/login.hbs | 2 +- .../acceptance/second-factor-auth-test.js | 14 ++++++++++++++ app/models/concerns/second_factor_manager.rb | 2 +- .../admin_detailed_user_serializer.rb | 2 +- app/serializers/current_user_serializer.rb | 2 +- app/serializers/user_serializer.rb | 2 +- .../admin_user_list_serializer_spec.rb | 12 ++++++++++++ .../current_user_serializer_spec.rb | 10 ++++++++++ spec/serializers/user_serializer_spec.rb | 10 ++++++++++ 11 files changed, 70 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/second-factor-form.js b/app/assets/javascripts/discourse/app/components/second-factor-form.js index e208a5f3145..9b23ed64ac3 100644 --- a/app/assets/javascripts/discourse/app/components/second-factor-form.js +++ b/app/assets/javascripts/discourse/app/components/second-factor-form.js @@ -42,10 +42,12 @@ export default Component.extend({ } }, - @discourseComputed("backupEnabled", "secondFactorMethod") - showToggleMethodLink(backupEnabled, secondFactorMethod) { + @discourseComputed("backupEnabled", "totpEnabled", "secondFactorMethod") + showToggleMethodLink(backupEnabled, totpEnabled, secondFactorMethod) { return ( - backupEnabled && secondFactorMethod !== SECOND_FACTOR_METHODS.SECURITY_KEY + backupEnabled && + totpEnabled && + secondFactorMethod !== SECOND_FACTOR_METHODS.SECURITY_KEY ); }, diff --git a/app/assets/javascripts/discourse/app/controllers/login.js b/app/assets/javascripts/discourse/app/controllers/login.js index 401c85f354a..482509d68a7 100644 --- a/app/assets/javascripts/discourse/app/controllers/login.js +++ b/app/assets/javascripts/discourse/app/controllers/login.js @@ -223,21 +223,30 @@ export default Controller.extend(ModalFunctionality, { this.clearFlash(); if ( - (result.security_key_enabled || result.totp_enabled) && + (result.security_key_enabled || + result.totp_enabled || + result.backup_enabled) && !this.secondFactorRequired ) { + let secondFactorMethod; + if (result.security_key_enabled) { + secondFactorMethod = SECOND_FACTOR_METHODS.SECURITY_KEY; + } else if (result.totp_enabled) { + secondFactorMethod = SECOND_FACTOR_METHODS.TOTP; + } else { + secondFactorMethod = SECOND_FACTOR_METHODS.BACKUP_CODE; + } this.setProperties({ otherMethodAllowed: result.multiple_second_factor_methods, secondFactorRequired: true, showLoginButtons: false, backupEnabled: result.backup_enabled, - showSecondFactor: result.totp_enabled, + totpEnabled: result.totp_enabled, + showSecondFactor: result.totp_enabled || result.backup_enabled, showSecurityKey: result.security_key_enabled, - secondFactorMethod: result.security_key_enabled - ? SECOND_FACTOR_METHODS.SECURITY_KEY - : SECOND_FACTOR_METHODS.TOTP, securityKeyChallenge: result.challenge, securityKeyAllowedCredentialIds: result.allowed_credential_ids, + secondFactorMethod, }); // only need to focus the 2FA input for TOTP diff --git a/app/assets/javascripts/discourse/app/templates/modal/login.hbs b/app/assets/javascripts/discourse/app/templates/modal/login.hbs index d7465134af5..39de9f023e9 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/login.hbs +++ b/app/assets/javascripts/discourse/app/templates/modal/login.hbs @@ -26,7 +26,7 @@
{{d-icon "exclamation-triangle"}} {{i18n "login.caps_lock_warning"}}
- + {{#if this.showSecurityKey}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js b/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js index 32b57a7e6d7..0d8c4cc0c65 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/second-factor-auth-test.js @@ -44,6 +44,12 @@ const RESPONSES = { security_keys_enabled: true, allowed_methods: [BACKUP_CODE], }, + ok010010: { + totp_enabled: false, + backup_enabled: true, + security_keys_enabled: false, + allowed_methods: [BACKUP_CODE], + }, }; Object.keys(RESPONSES).forEach((k) => { @@ -178,6 +184,14 @@ acceptance("Second Factor Auth Page", function (needs) { !exists(".toggle-second-factor-method"), "no alternative methods are shown if only 1 method is allowed" ); + + // only backup codes + await visit("/session/2fa?nonce=ok010010"); + assert.ok(exists("form.backup-code-token"), "backup code form is shown"); + assert.ok( + !exists(".toggle-second-factor-method"), + "no alternative methods are shown if only 1 method is allowed" + ); }); test("switching 2FA methods", async function (assert) { diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index e21957e75ff..4e212306af6 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/app/models/concerns/second_factor_manager.rb @@ -79,7 +79,7 @@ module SecondFactorManager end def has_any_second_factor_methods_enabled? - totp_enabled? || security_keys_enabled? + totp_enabled? || security_keys_enabled? || backup_codes_enabled? end def has_multiple_second_factor_methods? diff --git a/app/serializers/admin_detailed_user_serializer.rb b/app/serializers/admin_detailed_user_serializer.rb index aab939247c5..454fcf6cbd9 100644 --- a/app/serializers/admin_detailed_user_serializer.rb +++ b/app/serializers/admin_detailed_user_serializer.rb @@ -45,7 +45,7 @@ class AdminDetailedUserSerializer < AdminUserSerializer has_many :groups, embed: :object, serializer: BasicGroupSerializer def second_factor_enabled - object.totp_enabled? || object.security_keys_enabled? + object.totp_enabled? || object.security_keys_enabled? || object.backup_codes_enabled? end def can_disable_second_factor diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 9f53ff09376..70920d01a51 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -323,7 +323,7 @@ class CurrentUserSerializer < BasicUserSerializer end def second_factor_enabled - object.totp_enabled? || object.security_keys_enabled? + object.totp_enabled? || object.security_keys_enabled? || object.backup_codes_enabled? end def featured_topic diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 0e9f975032e..eb2db4e56ed 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -105,7 +105,7 @@ class UserSerializer < UserCardSerializer end def second_factor_enabled - object.totp_enabled? || object.security_keys_enabled? + object.totp_enabled? || object.security_keys_enabled? || object.backup_codes_enabled? end def include_second_factor_backup_enabled? diff --git a/spec/serializers/admin_user_list_serializer_spec.rb b/spec/serializers/admin_user_list_serializer_spec.rb index 955e726719e..2c1e3ebadb4 100644 --- a/spec/serializers/admin_user_list_serializer_spec.rb +++ b/spec/serializers/admin_user_list_serializer_spec.rb @@ -31,6 +31,18 @@ RSpec.describe AdminUserListSerializer do end end + context "when backup codes enabled" do + before do + Fabricate(:user_second_factor_backup, user: user) + end + + it "is true" do + json = serializer.as_json + + expect(json[:second_factor_enabled]).to eq(true) + end + end + describe "emails" do fab!(:admin) { Fabricate(:user, admin: true, email: "admin@email.com") } fab!(:moderator) { Fabricate(:user, moderator: true, email: "moderator@email.com") } diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index 3b6ce305831..b25f9c94e09 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -102,6 +102,16 @@ RSpec.describe CurrentUserSerializer do expect(json[:second_factor_enabled]).to eq(true) end end + + context "when backup codes enabled" do + before do + User.any_instance.stubs(:backup_codes_enabled?).returns(true) + end + + it "is true" do + expect(json[:second_factor_enabled]).to eq(true) + end + end end describe "#groups" do diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 05765453258..beb8637d244 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -250,6 +250,16 @@ RSpec.describe UserSerializer do expect(json[:second_factor_enabled]).to eq(true) end end + + context "when backup codes enabled" do + before do + User.any_instance.stubs(:backup_codes_enabled?).returns(true) + end + + it "is true" do + expect(json[:second_factor_enabled]).to eq(true) + end + end end describe "ignored and muted" do