From 35d928c4a9fe7c8545f2ad1866a45ff28c1ef5d3 Mon Sep 17 00:00:00 2001 From: Roger Meier Date: Mon, 3 Jun 2019 06:15:53 +0200 Subject: [PATCH] refactor: apply "require 2FA" to all subgroup and ancestor group members, when changing --- app/models/group.rb | 2 +- app/models/user.rb | 3 +- ...-require-2fa-for-all-entities-in-group.yml | 5 +- doc/security/two_factor_authentication.md | 22 ++- spec/models/group_spec.rb | 129 +++++++++++------- spec/models/user_spec.rb | 25 +++- 6 files changed, 122 insertions(+), 64 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 5e58b48a366..ba9f6221567 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -423,7 +423,7 @@ class Group < Namespace def update_two_factor_requirement return unless saved_change_to_require_two_factor_authentication? || saved_change_to_two_factor_grace_period? - User.from_union([users_with_descendants, project_users_with_descendants]).find_each(&:update_two_factor_requirement) + direct_and_indirect_members.find_each(&:update_two_factor_requirement) end def path_changed_hook diff --git a/app/models/user.rb b/app/models/user.rb index 916a0aa74f0..2eb5c63a4cc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -728,8 +728,7 @@ class User < ApplicationRecord end def expanded_groups_requiring_two_factor_authentication - Group.from_union([all_expanded_groups.where(require_two_factor_authentication: true), - authorized_groups.where(require_two_factor_authentication: true)]) + all_expanded_groups.where(require_two_factor_authentication: true) end # rubocop: disable CodeReuse/ServiceClass diff --git a/changelogs/unreleased/feature-require-2fa-for-all-entities-in-group.yml b/changelogs/unreleased/feature-require-2fa-for-all-entities-in-group.yml index 650adcda26e..ff04f99c1bb 100644 --- a/changelogs/unreleased/feature-require-2fa-for-all-entities-in-group.yml +++ b/changelogs/unreleased/feature-require-2fa-for-all-entities-in-group.yml @@ -1,7 +1,4 @@ ---- -title: Apply the group setting "require 2FA" to be inherited across all subgroups - and descendant projects of the group where it was set. (This works only for postgresql - databases, not for mysql/mariadb) +title: Apply the group setting "require 2FA" across all subgroup and ancestor group members as well when changing the group setting merge_request: 24965 author: rroger type: changed diff --git a/doc/security/two_factor_authentication.md b/doc/security/two_factor_authentication.md index ad5daef805a..49dadd5abc2 100644 --- a/doc/security/two_factor_authentication.md +++ b/doc/security/two_factor_authentication.md @@ -39,8 +39,26 @@ If you want to enforce 2FA only for certain groups, you can: To change this setting, you need to be administrator or owner of the group. -If there are multiple 2FA requirements (i.e. group + all users, or multiple -groups) the shortest grace period will be used. +> [From](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24965) GitLab 12.0, 2FA settings for a group are also applied to subgroups. + +If you want to enforce 2FA only for certain groups, you can enable it in the +group settings and specify a grace period as above. To change this setting you +need to be administrator or owner of the group. + +The following are important notes about 2FA: + +- Projects belonging to a 2FA-enabled group that + [is shared](../user/project/members/share_project_with_groups.md) + with a 2FA-disabled group will *not* require members of the 2FA-disabled group to use + 2FA for the project. For example, if project *P* belongs to 2FA-enabled group *A* and + is shared with 2FA-disabled group *B*, members of group *B* can access project *P* + without 2FA. To ensure this scenario doesn't occur, + [prevent sharing of projects](../user/group/index.md#share-with-group-lock) + for the 2FA-enabled group. +- If you add additional members to a project within a group or subgroup that has + 2FA enabled, 2FA is **not** required for those individually added members. +- If there are multiple 2FA requirements (for example, group + all users, or multiple + groups) the shortest grace period will be used. ## Disabling 2FA for everyone diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index e1c5479851c..074aec3eb07 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -603,73 +603,96 @@ describe Group do describe '#update_two_factor_requirement' do let(:user) { create(:user) } - before do - group.add_user(user, GroupMember::OWNER) - end - - it 'is called when require_two_factor_authentication is changed' do - expect_any_instance_of(User).to receive(:update_two_factor_requirement) - - group.update!(require_two_factor_authentication: true) - end - - it 'is called when two_factor_grace_period is changed' do - expect_any_instance_of(User).to receive(:update_two_factor_requirement) - - group.update!(two_factor_grace_period: 23) - end - - it 'is not called when other attributes are changed' do - expect_any_instance_of(User).not_to receive(:update_two_factor_requirement) - - group.update!(description: 'foobar') - end - - def expects_other_user_to_require_two_factors(expected_calls_mysql_db = 1) - calls = 0 - allow_any_instance_of(User).to receive(:update_two_factor_requirement) do - calls += 1 + context 'group membership' do + before do + group.add_user(user, GroupMember::OWNER) end - group.update!(require_two_factor_authentication: true, two_factor_grace_period: 23) + it 'is called when require_two_factor_authentication is changed' do + expect_any_instance_of(User).to receive(:update_two_factor_requirement) + + group.update!(require_two_factor_authentication: true) + end + + it 'is called when two_factor_grace_period is changed' do + expect_any_instance_of(User).to receive(:update_two_factor_requirement) + + group.update!(two_factor_grace_period: 23) + end + + it 'is not called when other attributes are changed' do + expect_any_instance_of(User).not_to receive(:update_two_factor_requirement) + + group.update!(description: 'foobar') + end + + it 'calls #update_two_factor_requirement on each group member' do + other_user = create(:user) + group.add_user(other_user, GroupMember::OWNER) + + calls = 0 + allow_any_instance_of(User).to receive(:update_two_factor_requirement) do + calls += 1 + end + + group.update!(require_two_factor_authentication: true, two_factor_grace_period: 23) - if Group.supports_nested_objects? expect(calls).to eq 2 - else - expect(calls).to eq expected_calls_mysql_db end end - it 'calls #update_two_factor_requirement on each group member' do - other_user = create(:user) - group.add_user(other_user, GroupMember::OWNER) + context 'sub groups and projects', :nested_groups do + it 'enables two_factor_requirement for group member' do + group.add_user(user, GroupMember::OWNER) - expects_other_user_to_require_two_factors(2) - end + group.update!(require_two_factor_authentication: true) - it 'calls #update_two_factor_requirement on each subgroup member' do - subgroup = create(:group, :nested, parent: group) - subgroup_user = create(:user) - subgroup.add_user(subgroup_user, GroupMember::OWNER) + expect(user.reload.require_two_factor_authentication_from_group).to be_truthy + end - expects_other_user_to_require_two_factors - end + context 'expanded group members', :nested_groups do + let(:indirect_user) { create(:user) } - it 'calls #update_two_factor_requirement on each child project member' do - project = create(:project, group: group) - project_user = create(:user) - project.add_developer(project_user) + it 'enables two_factor_requirement for subgroup member' do + subgroup = create(:group, :nested, parent: group) + subgroup.add_user(indirect_user, GroupMember::OWNER) - expects_other_user_to_require_two_factors(2) - end + group.update!(require_two_factor_authentication: true) - it 'calls #update_two_factor_requirement on each subgroups child project member' do - subgroup = create(:group, :nested, parent: group) - project = create(:project, group: subgroup) - project_user = create(:user) - project.add_developer(project_user) + expect(indirect_user.reload.require_two_factor_authentication_from_group).to be_truthy + end - expects_other_user_to_require_two_factors + it 'enables two_factor_requirement for ancestor group member' do + ancestor_group = create(:group) + ancestor_group.add_user(indirect_user, GroupMember::OWNER) + group.update!(parent: ancestor_group) + + group.update!(require_two_factor_authentication: true) + + expect(indirect_user.reload.require_two_factor_authentication_from_group).to be_truthy + end + end + + context 'project members' do + it 'does not enable two_factor_requirement for child project member' do + project = create(:project, group: group) + project.add_maintainer(user) + + group.update!(require_two_factor_authentication: true) + + expect(user.reload.require_two_factor_authentication_from_group).to be_falsey + end + + it 'does not enable two_factor_requirement for subgroup child project member', :nested_groups do + subgroup = create(:group, :nested, parent: group) + project = create(:project, group: subgroup) + project.add_maintainer(user) + + group.update!(require_two_factor_authentication: true) + + expect(user.reload.require_two_factor_authentication_from_group).to be_falsey + end + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d1338e34bb8..c95bbb0b3f5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2655,9 +2655,9 @@ describe User do end end - context 'with 2FA requirement on nested parent group', :nested_groups do + context 'with 2FA requirement from expanded groups', :nested_groups do let!(:group1) { create :group, require_two_factor_authentication: true } - let!(:group1a) { create :group, require_two_factor_authentication: false, parent: group1 } + let!(:group1a) { create :group, parent: group1 } before do group1a.add_user(user, GroupMember::OWNER) @@ -2685,6 +2685,27 @@ describe User do end end + context "with 2FA requirement from shared project's group" do + let!(:group1) { create :group, require_two_factor_authentication: true } + let!(:group2) { create :group } + let(:shared_project) { create(:project, namespace: group1) } + + before do + shared_project.project_group_links.create!( + group: group2, + group_access: ProjectGroupLink.default_access + ) + + group2.add_user(user, GroupMember::OWNER) + end + + it 'does not require 2FA' do + user.update_two_factor_requirement + + expect(user.require_two_factor_authentication_from_group).to be false + end + end + context 'without 2FA requirement on groups' do let(:group) { create :group }