From cc4c19968028479c6d6603a49488e87bcd6b0053 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Wed, 19 Jun 2024 09:32:30 +1000 Subject: [PATCH] FEATURE: optional 2FA enforcement (#27506) A new admin setting called `enforce_second_factor_on_external_auth`. It allows users to authenticate using external providers even when 2FA is forced with `enforce_second_factor` site setting. --- .../controllers/preferences/second-factor.js | 26 +++++++++++++++++-- .../templates/preferences-second-factor.hbs | 12 ++++++--- .../users/omniauth_callbacks_controller.rb | 3 ++- config/locales/client.en.yml | 1 + config/locales/server.en.yml | 4 +-- config/site_settings.yml | 3 +++ lib/site_settings/validations.rb | 9 ------- spec/lib/site_settings/validations_spec.rb | 20 -------------- .../omniauth_callbacks_controller_spec.rb | 15 +++++++++++ 9 files changed, 56 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js b/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js index 3cc5abfebfe..a4be7cb75e0 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js @@ -20,6 +20,7 @@ import I18n from "discourse-i18n"; export default Controller.extend(CanCheckEmails, { dialog: service(), modal: service(), + siteSettings: service(), loading: false, dirty: false, errorMessage: null, @@ -34,13 +35,34 @@ export default Controller.extend(CanCheckEmails, { }, @discourseComputed - displayOAuthWarning() { + hasOAuth() { return findAll().length > 0; }, + @discourseComputed + displayOAuthWarning() { + return ( + this.hasOAuth && this.siteSettings.enforce_second_factor_on_external_auth + ); + }, + + @discourseComputed("currentUser") + showEnforcedWithOAuthNotice(user) { + return ( + user && + user.enforcedSecondFactor && + this.hasOAuth && + !this.siteSettings.enforce_second_factor_on_external_auth + ); + }, + @discourseComputed("currentUser") showEnforcedNotice(user) { - return user && user.enforcedSecondFactor; + return ( + user && + user.enforcedSecondFactor && + this.siteSettings.enforce_second_factor_on_external_auth + ); }, @discourseComputed("totps", "security_keys") diff --git a/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs b/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs index 5a4e4d81a15..c8a90d0d9e6 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs @@ -4,9 +4,15 @@
{{#if this.showEnforcedNotice}} -
{{i18n - "user.second_factor.enforced_notice" - }}
+
+ {{i18n "user.second_factor.enforced_notice"}} +
+ {{/if}} + + {{#if this.showEnforcedWithOAuthNotice}} +
+ {{i18n "user.second_factor.enforced_with_oauth_notice"}} +
{{/if}} {{#if this.displayOAuthWarning}} diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 673db129c48..7a181a6ef9a 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -143,7 +143,8 @@ class Users::OmniauthCallbacksController < ApplicationController end def user_found(user) - if user.has_any_second_factor_methods_enabled? + if user.has_any_second_factor_methods_enabled? && + SiteSetting.enforce_second_factor_on_external_auth @auth_result.omniauth_disallow_totp = true @auth_result.email = user.email return diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d6ef527f7c4..5e0ce5edf49 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1539,6 +1539,7 @@ en: Two-factor authentication adds extra security to your account by requiring a one-time token in addition to your password. Tokens can be generated on Android and iOS devices. oauth_enabled_warning: "Please note that social logins will be disabled once two-factor authentication has been enabled on your account." use: "Use Authenticator app" + enforced_with_oauth_notice: "You are required to enable two-factor authentication. You will only be prompted to use this when logging in with a password, not with external authentication or social login methods." enforced_notice: "You are required to enable two-factor authentication before accessing this site." disable: "Disable" disable_confirm: "Are you sure you want to disable two-factor authentication?" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index fe9dbcd334a..533e6ef6a5b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -348,7 +348,6 @@ en: secure_uploads_requirements: "S3 uploads and S3 ACLs must be enabled before enabling secure uploads." s3_use_acls_requirements: "You must have S3 ACLs enabled when secure uploads are enabled." share_quote_facebook_requirements: "You must set a Facebook app id to enable quote sharing for Facebook." - second_factor_cannot_enforce_with_socials: "You cannot enforce 2FA with social logins enabled. You must first disable login via: %{auth_provider_names}" second_factor_cannot_be_enforced_with_disabled_local_login: "You cannot enforce 2FA if local logins are disabled." second_factor_cannot_be_enforced_with_discourse_connect_enabled: "You cannot enforce 2FA if DiscourseConnect is enabled." local_login_cannot_be_disabled_if_second_factor_enforced: "You cannot disable local login if 2FA is enforced. Disable enforced 2FA before disabling local logins." @@ -1762,7 +1761,8 @@ en: email_custom_headers: "A pipe-delimited list of custom email headers" email_subject: "Customizable subject format for standard emails. See https://meta.discourse.org/t/customize-subject-format-for-standard-emails/20801" detailed_404: "Provides more details to users about why they can’t access a particular topic. Note: This is less secure because users will know if a URL links to a valid topic." - enforce_second_factor: "Require users to enable two-factor authentication before they can access the Discourse UI. Select 'all' to enforce it to all users. Select 'staff' to enforce it to staff users only. This setting does not affect API or 'DiscourseConnect provider' authentication." + enforce_second_factor: "Require users to enable two-factor authentication before they can access the Discourse UI. This setting does not affect API or 'DiscourseConnect provider' authentication. If enforce_second_factor_on_external_auth is enabled, users will not be able to log in with external authentication providers after they set up two-factor authentication." + enforce_second_factor_on_external_auth: "Require users to use two-factor authentication at all times. When enabled this will prevent users logging in with external authentication methods like social logins if they have two-factor authentication enabled. When disabled users will only need to confirm their two-factor authentication when logging in with a username and password. Also see the `enforce_second_factor` setting." force_https: "Force your site to use HTTPS only. WARNING: do NOT enable this until you verify HTTPS is fully set up and working absolutely everywhere! Did you check your CDN, all social logins, and any external logos / dependencies to make sure they are all HTTPS compatible, too?" summary_score_threshold: "The minimum score required for a post to be included in 'Summarize This Topic'" diff --git a/config/site_settings.yml b/config/site_settings.yml index 58510c4f7ee..cd693788114 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1913,6 +1913,9 @@ trust: security: detailed_404: false + enforce_second_factor_on_external_auth: + client: true + default: true enforce_second_factor: client: true type: enum diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index 828537c5439..dbaac48c006 100644 --- a/lib/site_settings/validations.rb +++ b/lib/site_settings/validations.rb @@ -219,15 +219,6 @@ module SiteSettings::Validations if new_val != "no" && SiteSetting.enable_discourse_connect? return validate_error :second_factor_cannot_be_enforced_with_discourse_connect_enabled end - if new_val == "all" && Discourse.enabled_auth_providers.count > 0 - auth_provider_names = Discourse.enabled_auth_providers.map(&:name).join(", ") - return( - validate_error( - :second_factor_cannot_enforce_with_socials, - auth_provider_names: auth_provider_names, - ) - ) - end return if SiteSetting.enable_local_logins return if new_val == "no" validate_error :second_factor_cannot_be_enforced_with_disabled_local_login diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb index 792a003915a..43a901175d8 100644 --- a/spec/lib/site_settings/validations_spec.rb +++ b/spec/lib/site_settings/validations_spec.rb @@ -159,26 +159,6 @@ RSpec.describe SiteSettings::Validations do end end - context "when social logins are enabled" do - let(:error_message) do - I18n.t( - "errors.site_settings.second_factor_cannot_enforce_with_socials", - auth_provider_names: "facebook, github", - ) - end - before do - SiteSetting.enable_facebook_logins = true - SiteSetting.enable_github_logins = true - end - - it "raises and error, and specifies the auth providers" do - expect { validations.validate_enforce_second_factor("all") }.to raise_error( - Discourse::InvalidParameters, - error_message, - ) - end - end - context "when SSO is enabled" do let(:error_message) do I18n.t( diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index a69a2c6845a..acd38e37fec 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -631,6 +631,21 @@ RSpec.describe Users::OmniauthCallbacksController do end end + context "when user has TOTP enabled but enforce_second_factor_on_external_auth is false" do + before { user.create_totp(enabled: true) } + + it "should return the right response" do + SiteSetting.enforce_second_factor_on_external_auth = false + get "/auth/google_oauth2/callback.json" + + expect(response.status).to eq(302) + + data = JSON.parse(cookies[:authentication_data]) + + expect(data["authenticated"]).to eq(true) + end + end + context "when user has security key enabled" do before { Fabricate(:user_security_key_with_random_credential, user: user) }