diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index ba82b76bd5c6e4354591a24090cfbde60daae6bc..edf85d4bdf8b230c0ae16e1e48e1eb8aa215eba8 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -739,7 +739,8 @@ def self.kroki_formats_attributes jsonb_accessor :sign_in_restrictions, disable_password_authentication_for_users_with_sso_identities: [:boolean, { default: false }], root_moved_permanently_redirection: [:boolean, { default: false }], - session_expire_from_init: [:boolean, { default: false }] + session_expire_from_init: [:boolean, { default: false }], + require_minimum_email_based_otp_for_users_with_passwords: [:boolean, { default: false }] validates :sign_in_restrictions, json_schema: { filename: 'application_setting_sign_in_restrictions' } diff --git a/app/models/concerns/authn/email_otp_enrollment.rb b/app/models/concerns/authn/email_otp_enrollment.rb new file mode 100644 index 0000000000000000000000000000000000000000..7090bbf9135a644fdb59dbfce29bd8dd30ac363a --- /dev/null +++ b/app/models/concerns/authn/email_otp_enrollment.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +module Authn + # This module uses ActiveRecord callbacks to ensure that Two Factor + # Authentication methods align with requirements placed on the user, + # regardless of whether a user is created or updated directly, or via + # a Service. + module EmailOtpEnrollment + extend ActiveSupport::Concern + + def can_modify_email_otp_enrollment? + email_otp_enrollment_restriction.nil? + end + + def email_otp_enrollment_restriction + return :uses_external_authenticator if password_automatically_set? + return :group_2fa_enforcement if require_two_factor_authentication_from_group? + return :instance_2fa_enforcement if Gitlab::CurrentSettings.require_two_factor_authentication? + return :future_enforcement if email_otp_required_after&.future? + return :email_otp_required if must_require_email_otp? + + nil + end + + # Modifies email_otp_required_after to match the user's + # restrictions based on other enabled features. + # + # Disables Email OTP as a fallback when 2FA enabled and enforced by + # Group or Instance Policy, or enables it when required as a + # minimum. + # + # Does not save the record. + def set_email_otp_required_after_based_on_restrictions + return unless enrollment_check_required? + + if must_disable_email_otp? + self.email_otp_required_after = nil + elsif must_require_email_otp? && email_otp_required_after.blank? + # The user must be enrolled. We use, in order: + # - The old value via *_was if it's not nil + # - Now (instance enforcement) as the final option + # Any value, including a date in the future, is permitted. + self.email_otp_required_after = email_otp_required_after_was || Time.current + end + # If neither condition match, email_otp_required_after is not + # an invalid value. + end + + # Validates the presence or absence of email_otp_required_after + # depending on other enabled features. + # This method is the "validate" version of #set_email_otp_required_after_based_on_restrictions + # above. + def validate_email_otp_required_after + return unless enrollment_check_required? + + if two_factor_enabled? + if has_mandatory_two_factor_policy? && email_otp_required_after.present? + errors.add(:email_otp_required_after, + _('is not permitted when 2FA is enforced by a policy')) + end + elsif email_otp_required_after.blank? && must_require_email_otp? + errors.add(:email_otp_required_after, + _('is required when 2FA is disabled')) + end + # If neither condition match, the user is in a valid state. + end + + private + + def enrollment_check_required? + (otp_required_for_login_changed? || email_otp_required_after_changed?) && + Feature.enabled?(:email_based_mfa, self) + end + + def has_mandatory_two_factor_policy? + Gitlab::CurrentSettings.require_two_factor_authentication? || + require_two_factor_authentication_from_group? + end + + def must_disable_email_otp? + two_factor_enabled? && has_mandatory_two_factor_policy? + end + + def must_require_email_otp? + !password_automatically_set? && + !two_factor_enabled? && + Gitlab::CurrentSettings.require_minimum_email_based_otp_for_users_with_passwords? + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 664635aa0bc425409bed95048dae7e08e333da4f..e11a24be9e9d824a914237f9091dc72d58734bae 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -36,6 +36,7 @@ class User < ApplicationRecord include Gitlab::InternalEventsTracking include Ci::PipelineScheduleOwnershipValidator include Users::DependentAssociations + include Authn::EmailOtpEnrollment ignore_column %i[role skype], remove_after: '2025-09-18', remove_with: '18.4' @@ -352,6 +353,7 @@ def update_tracked_fields!(request) validate :commit_email_verified, if: :commit_email_changed? validate :email_allowed_by_restrictions, if: ->(user) { user.new_record? ? !user.created_by_id : user.email_changed? } validate :check_username_format, if: :username_changed? + validate :validate_email_otp_required_after validates :theme_id, allow_nil: true, inclusion: { in: Gitlab::Themes.valid_ids, message: ->(*) { _("%{placeholder} is not a valid theme") % { placeholder: '%{value}' } } } @@ -374,6 +376,7 @@ def update_tracked_fields!(request) after_initialize :build_default_user_detail, if: :new_record? before_validation :sanitize_attrs before_validation :ensure_namespace_correct + before_validation :set_email_otp_required_after_based_on_restrictions after_validation :set_username_errors before_save :ensure_incoming_email_token before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? } @@ -528,6 +531,7 @@ def should_use_flipped_dashboard_mapping_for_rollout? delegate :bot_namespace, :bot_namespace=, to: :user_detail, allow_nil: true delegate :email_otp, :email_otp=, to: :user_detail, allow_nil: true delegate :email_otp_required_after, :email_otp_required_after=, to: :user_detail, allow_nil: true + delegate :email_otp_required_after_changed?, :email_otp_required_after_was, to: :user_detail delegate :email_otp_last_sent_at, :email_otp_last_sent_at=, to: :user_detail, allow_nil: true delegate :email_otp_last_sent_to, :email_otp_last_sent_to=, to: :user_detail, allow_nil: true diff --git a/app/validators/json_schemas/application_setting_sign_in_restrictions.json b/app/validators/json_schemas/application_setting_sign_in_restrictions.json index 7ea5ce699180da1e3dcd8ef575d30869627aaea2..d4a4faf3c0713adb27e587529f11b873178a9ad7 100644 --- a/app/validators/json_schemas/application_setting_sign_in_restrictions.json +++ b/app/validators/json_schemas/application_setting_sign_in_restrictions.json @@ -15,6 +15,10 @@ "session_expire_from_init": { "type": "boolean", "description": "When enabled, sessions will expire a specific time after creation even if the session is active. Sessions cannot be extended." + }, + "require_minimum_email_based_otp_for_users_with_passwords": { + "type": "boolean", + "description": "(Experiment) When enabled, users who sign in with a password must use Email-based OTP at a minimum. When other forms of 2FA are enabled, Email-based OTP is optional for them." } } } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 77bcb15db4b60a4c71b5c5babd45815ae429e79c..400f2f99705867a8a12f9739ba29331b7d593c8b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -78650,6 +78650,9 @@ msgstr "" msgid "is not one of" msgstr "" +msgid "is not permitted when 2FA is enforced by a policy" +msgstr "" + msgid "is not valid." msgstr "" @@ -78665,6 +78668,9 @@ msgstr "" msgid "is read-only" msgstr "" +msgid "is required when 2FA is disabled" +msgstr "" + msgid "is reserved and cannot be used" msgstr "" diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 45e1f690fcbec8550a0374c1db9d6691783611ab..ebfdf645ac2b41908d893bd166362145416b60ef 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -242,6 +242,7 @@ security_approval_policies_limit: 5, session_expire_delay: Settings.gitlab['session_expire_delay'], session_expire_from_init: false, + require_minimum_email_based_otp_for_users_with_passwords: false, shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], shared_runners_text: nil, show_migrate_from_jenkins_banner: true, @@ -252,7 +253,8 @@ sign_in_restrictions: { 'disable_password_authentication_for_users_with_sso_identities' => false, 'root_moved_permanently_redirection' => false, - 'session_expire_from_init' => false + 'session_expire_from_init' => false, + 'require_minimum_email_based_otp_for_users_with_passwords' => false }, signup_enabled: Settings.gitlab['signup_enabled'], silent_admin_exports_enabled: false, diff --git a/spec/models/concerns/authn/email_otp_enrollment_spec.rb b/spec/models/concerns/authn/email_otp_enrollment_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4ac3a2c48e36aaa49b67fbb2d1907f17e3009132 --- /dev/null +++ b/spec/models/concerns/authn/email_otp_enrollment_spec.rb @@ -0,0 +1,449 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Authn::EmailOtpEnrollment, feature_category: :system_access do + let(:email_otp_required_after) { Time.current } + let(:user) { create(:user, email_otp_required_after: email_otp_required_after) } + let(:group_require_two_factor_authentication) { false } + let(:group) { create(:group, require_two_factor_authentication: group_require_two_factor_authentication) } + + before do + # Adding a user to a group with 2FA requirement triggers + # side-effects that we want to ensure are accounted for. + # Otherwise, we skip #reload for spec performance. + group.add_developer(user) + user.reload if group_require_two_factor_authentication + end + + describe '#can_modify_email_otp_enrollment?' do + subject { user.can_modify_email_otp_enrollment? } + + it 'returns true when no restriction exists' do + allow(user).to receive(:email_otp_enrollment_restriction).and_return(nil) + is_expected.to be true + end + + it 'returns false when restriction exists' do + allow(user).to receive(:email_otp_enrollment_restriction).and_return(:some_restriction) + is_expected.to be false + end + end + + describe '#email_otp_enrollment_restriction' do + subject { user.email_otp_enrollment_restriction } + + # By default the user is unrestricted and can opt in and out of + # Email-based OTP + it { is_expected.to be_nil } + + context 'when user has email OTP required and has other MFA enabled' do + before do + allow(user).to receive(:two_factor_enabled?).and_return(true) + end + + # No restrictions + it { is_expected.to be_nil } + end + + context 'when user uses an external authenticator and has no GitLab password' do + before do + user.update!(password_automatically_set: true) + end + + it { is_expected.to eq(:uses_external_authenticator) } + end + + context 'when group enforces 2FA', :sidekiq_inline do + let(:group_require_two_factor_authentication) { true } + + it { is_expected.to eq(:group_2fa_enforcement) } + end + + context 'when instance enforces 2FA' do + before do + stub_application_setting(require_two_factor_authentication: true) + end + + it { is_expected.to eq(:instance_2fa_enforcement) } + end + + context 'when email OTP enforcement is in the future' do + let(:email_otp_required_after) { 1.day.from_now } + + it { is_expected.to eq(:future_enforcement) } + end + + context 'when instance enforces email OTP as a minimum' do + before do + stub_application_setting(require_minimum_email_based_otp_for_users_with_passwords: true) + end + + it { is_expected.to eq(:email_otp_required) } + + context 'when user has 2FA' do + before do + allow(user).to receive(:two_factor_enabled?).and_return(true) + end + + it { is_expected.to be_nil } + end + end + end + + describe '#set_email_otp_required_after_based_on_restrictions' do + # otp_required_for_login maps to the use of App-based TOTP or + # WebAuthN + let(:new_otp_required_for_login) { false } + let(:new_email_otp_required_after) { email_otp_required_after } + + before do + user.otp_required_for_login = new_otp_required_for_login + user.email_otp_required_after = new_email_otp_required_after + end + + subject(:setter) do + user.set_email_otp_required_after_based_on_restrictions + end + + it 'is a no-op when otp_required_for_login and email_otp_required_after have not changed' do + expect { setter }.not_to change { user } + end + + context 'when user enrolls in 2FA' do + let(:new_otp_required_for_login) { true } + + context 'when FF is disabled' do + before do + stub_feature_flags(email_based_mfa: false) + end + + it 'user remains enrolled in email MFA' do + expect { setter }.not_to change { user } + expect(user.email_otp_required_after.past?).to be true + end + end + + context 'when user enables 2FA without mandatory policy' do + it 'user remains enrolled in email MFA' do + expect { setter }.not_to change { user } + expect(user.email_otp_required_after.past?).to be true + end + end + + context 'when user enables 2FA with mandatory group policy', :sidekiq_inline do + let(:group_require_two_factor_authentication) { true } + + it 'disables enrollment in email-based OTP' do + expect { setter }.to change { user.email_otp_required_after }.to nil + end + end + + context 'when user enables 2FA with mandatory instance policy' do + before do + stub_application_setting(require_two_factor_authentication: true) + allow(user).to receive(:two_factor_enabled?).and_return(true) + end + + it 'disables enrollment in email-based OTP' do + expect { setter }.to change { user.email_otp_required_after }.to nil + end + end + end + + context 'when user is enrolled in 2FA and disables it', :freeze_time do + let(:email_otp_required_after) { nil } + let(:user) { create(:user, :two_factor, email_otp_required_after: email_otp_required_after) } + + it 'allows user to remain opted out of email-based OTP' do + expect { setter }.not_to change { user.email_otp_required_after } + end + + context 'when email OTP is required as a minimum' do + before do + stub_application_setting(require_minimum_email_based_otp_for_users_with_passwords: true) + end + + it 'enrolls them in email-based OTP' do + expect { setter }.to change { user.email_otp_required_after }.to Time.current + end + end + end + + context 'when user self enrols in email-based OTP', :freeze_time do + let(:email_otp_required_after) { nil } + let(:new_email_otp_required_after) { Time.current } + + it 'does not prevent their enrolment' do + expect { setter }.not_to change { user.email_otp_required_after } + user.save! + user.reload + expect(user.email_otp_required_after).to eq(new_email_otp_required_after) + end + end + + context 'when user is not enrolled and sets a future-dated email-based OTP', :freeze_time do + let(:email_otp_required_after) { nil } + let(:new_email_otp_required_after) { 60.days.from_now } + + it 'does not prevent their future enrolment' do + expect { setter }.not_to change { user.email_otp_required_after } + user.save! + user.reload + expect(user.email_otp_required_after).to eq(new_email_otp_required_after) + end + end + + context 'when user self un-enrols in email-based OTP' do + let(:new_email_otp_required_after) { nil } + + it 'does not change their un-enrolment' do + expect { setter }.not_to change { user.email_otp_required_after } + end + + context 'when email OTP is required as a minimum' do + before do + stub_application_setting(require_minimum_email_based_otp_for_users_with_passwords: true) + end + + it 'does not allow it to be changed and keeps them enrolled in email-based OTP' do + expect { setter }.to change { user.email_otp_required_after }.to email_otp_required_after + end + end + end + + context 'when user enables 2FA AND email OTP at the same time under mandatory policy', :freeze_time do + let(:email_otp_required_after) { nil } + let(:new_otp_required_for_login) { true } + let(:new_email_otp_required_after) { Time.current } + + before do + stub_application_setting(require_two_factor_authentication: true) + end + + it 'disables email OTP due to mandatory policy taking precedence' do + expect { setter }.to change { user.email_otp_required_after }.to nil + end + end + + it 'does not persist changes to database' do + expect { setter }.not_to change { user.reload.email_otp_required_after } + end + + context 'with minimum Email OTP requirement' do + before do + allow(user).to receive(:must_require_email_otp?).and_return(true) + end + + context 'when user is not enrolled and sets a future-dated email-based OTP', :freeze_time do + let(:email_otp_required_after) { nil } + let(:new_email_otp_required_after) { 60.days.from_now } + + it 'does not prevent their future enrolment' do + expect { setter }.not_to change { user.email_otp_required_after } + user.save! + user.reload + expect(user.email_otp_required_after).to eq(new_email_otp_required_after) + end + end + end + end + + describe '#validate_email_otp_required_after' do + before do + allow(user).to receive(:has_mandatory_two_factor_policy?).and_call_original + allow(user).to receive(:must_require_email_otp?).and_call_original + allow(user).to receive(:two_factor_enabled?).and_call_original + end + + subject(:validation) { user.validate_email_otp_required_after } + + context 'when neither otp_required_for_login nor email_otp_required_after have changed' do + it 'returns early without validating' do + validation + expect(user).not_to have_received(:two_factor_enabled?) + end + end + + context 'when FF is disabled' do + before do + stub_feature_flags(email_based_mfa: false) + end + + it 'returns early without validating' do + user.otp_required_for_login = true + validation + expect(user).not_to have_received(:two_factor_enabled?) + end + end + + context 'with no restrictions' do + it 'allows enabling 2FA with Email OTP off' do + user.otp_required_for_login = true + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'allows enabling both 2FA and Email OTP' do + user.otp_required_for_login = true + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'allows enabling Email OTP without 2FA' do + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + context 'with 2FA already enabled' do + before do + user.update!(otp_required_for_login: true) + end + + it 'allows disabling 2FA with Email OTP off' do + user.otp_required_for_login = false + user.email_otp_required_after = nil + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'allows disabling 2FA with Email OTP on' do + user.otp_required_for_login = false + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + end + end + + context 'with mandatory 2FA policy' do + before do + allow(user).to receive(:has_mandatory_two_factor_policy?).and_return(true) + end + + it 'allows enabling 2FA with Email OTP off' do + user.otp_required_for_login = true + user.email_otp_required_after = nil + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'prevents Email OTP remaining enabled when 2FA is turned on' do + user.otp_required_for_login = true + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]) + .to include('is not permitted when 2FA is enforced by a policy') + end + + context 'with 2FA already enabled' do + let(:email_otp_required_after) { nil } + + before do + user.update!(otp_required_for_login: true) + end + + it 'prevents enabling Email OTP' do + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]) + .to include('is not permitted when 2FA is enforced by a policy') + end + end + end + + context 'with minimum Email OTP requirement' do + before do + allow(user).to receive(:must_require_email_otp?).and_return(true) + end + + it 'allows 2FA on with Email OTP on' do + user.otp_required_for_login = true + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'allows 2FA on with Email OTP off' do + user.otp_required_for_login = true + user.email_otp_required_after = nil + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'prevents disabling Email OTP without 2FA' do + user.email_otp_required_after = nil + + validation + expect(user.errors[:email_otp_required_after]) + .to include('is required when 2FA disabled') + end + + context 'with 2FA already enabled' do + before do + user.update!(otp_required_for_login: true) + end + + it 'allows 2FA off with Email OTP on' do + user.otp_required_for_login = false + user.email_otp_required_after = Time.current + + validation + expect(user.errors[:email_otp_required_after]).to be_empty + end + + it 'prevents 2FA off with Email OTP off' do + user.otp_required_for_login = false + user.email_otp_required_after = nil + + validation + expect(user.errors[:email_otp_required_after]) + .to include('is required when 2FA disabled') + end + end + end + end + + describe '#has_mandatory_two_factor_policy?' do + subject { user.send(:has_mandatory_two_factor_policy?) } + + it { is_expected.to be false } + + context 'when instance requires 2FA' do + before do + stub_application_setting(require_two_factor_authentication: true) + end + + it { is_expected.to be true } + end + + context 'when group requires 2FA', :sidekiq_inline do + before do + group.update!(require_two_factor_authentication: true) + user.reload + end + + it { is_expected.to be true } + end + + context 'when both instance and group require 2FA', :sidekiq_inline do + before do + stub_application_setting(require_two_factor_authentication: true) + group.update!(require_two_factor_authentication: true) + end + + it { is_expected.to be true } + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c5bbfeae2ab063c7fb581ae3a692a7feea2385cb..3f294a68ed632f51dc565839cdbaf1e7e428a51a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1298,6 +1298,16 @@ expect(user.errors[:composite_identity_enforced]).to include('is not included in the list') end end + + describe 'validate_email_otp_required_after' do + let(:user) { build_stubbed(:user) } + + # This method is tested in spec/models/concerns/authn/email_otp_enrollment_spec.rb + it 'calls validate_email_otp_required_after' do + expect(user).to receive(:validate_email_otp_required_after) + user.valid? + end + end end describe 'dashboard enum and methods' do @@ -2052,6 +2062,16 @@ expect(UserDetail.exists?(user.id)).to be(true) end + + describe 'set_email_otp_required_after_based_on_restrictions' do + let(:user) { build_stubbed(:user) } + + # This method is tested in spec/models/concerns/authn/email_otp_enrollment_spec.rb + it 'runs the validation' do + expect(user).to receive(:set_email_otp_required_after_based_on_restrictions) + user.valid? + end + end end describe 'before save hook' do