summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorThong Kuah <tkuah@gitlab.com>2019-06-14 00:46:30 +0000
committerThong Kuah <tkuah@gitlab.com>2019-06-14 00:46:30 +0000
commit6de8cb7e9cf715e720fab9e7359ddb63c992af3b (patch)
treedb642e4210cdfb6bf125c78ebbf0e883eac276ad
parent7808259922906877f866d4b28fcd6994790ad8dd (diff)
parent7bfc4f999231385f2dc24cbb2d34b09cf5ae96c1 (diff)
downloadgitlab-ce-6de8cb7e9cf715e720fab9e7359ddb63c992af3b.tar.gz
Merge branch 'feature/require-2fa-for-all-entities-in-group' into 'master'
inherit require 2fa for all subgroups and projects See merge request gitlab-org/gitlab-ce!24965
-rw-r--r--app/models/group.rb2
-rw-r--r--changelogs/unreleased/feature-require-2fa-for-all-entities-in-group.yml4
-rw-r--r--doc/security/two_factor_authentication.md22
-rw-r--r--spec/models/group_spec.rb100
-rw-r--r--spec/models/user_spec.rb25
5 files changed, 126 insertions, 27 deletions
diff --git a/app/models/group.rb b/app/models/group.rb
index cdb4e6e87f6..dbec211935d 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?
- users.find_each(&:update_two_factor_requirement)
+ members_with_descendants.find_each(&:update_two_factor_requirement)
end
def path_changed_hook
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
new file mode 100644
index 00000000000..0abe777fb69
--- /dev/null
+++ b/changelogs/unreleased/feature-require-2fa-for-all-entities-in-group.yml
@@ -0,0 +1,4 @@
+title: Apply the group setting "require 2FA" across all subgroup 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 e6e7298a043..d7accbef6bd 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -603,40 +603,96 @@ describe Group do
describe '#update_two_factor_requirement' do
let(:user) { create(:user) }
- before do
- group.add_user(user, GroupMember::OWNER)
- end
+ context 'group membership' do
+ 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)
+ 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
+ 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)
+ 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
+ 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)
+ 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')
+ 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)
+
+ expect(calls).to eq 2
+ 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)
- calls = 0
- allow_any_instance_of(User).to receive(:update_two_factor_requirement) do
- calls += 1
+ group.update!(require_two_factor_authentication: true)
+
+ expect(user.reload.require_two_factor_authentication_from_group).to be_truthy
end
- group.update!(require_two_factor_authentication: true, two_factor_grace_period: 23)
+ context 'expanded group members', :nested_groups do
+ let(:indirect_user) { create(:user) }
+
+ it 'enables two_factor_requirement for subgroup member' do
+ subgroup = create(:group, :nested, parent: group)
+ subgroup.add_user(indirect_user, GroupMember::OWNER)
- expect(calls).to eq 2
+ group.update!(require_two_factor_authentication: true)
+
+ expect(indirect_user.reload.require_two_factor_authentication_from_group).to be_truthy
+ end
+
+ it 'does not enable 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_falsey
+ 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 }