Inconsistent 2FA enforcement for subgroups

Summary

There's an inconsistency between turning on 2FA for a group vs adding a new group member.

Turning on 2FA for a group only considers direct members while adding a new group member will consider 2FA requirement from the whole group hierarchy, both approaches are wrong.
Turning on 2FA for a group in Group#update_two_factor_requirement should propagate to subgroups.
Adding a new group member in User#expanded_groups_requiring_two_factor_authentication should only consider ancestors, not subgroups.

Steps to reproduce (Bottom-up Propagation)

Create a group hierarchy:
- Create a top-level group (Group A).
- Create a subgroup (Group B) under Group A.
Set up 2FA for Group B:
- Go to Group B's settings and enable require_two_factor_authentication.
Add a new user to Group A (the parent group).
- Sign in with new user belonging to top-level group (Group A)
- Observe the 2FA requirement propagation behaviour from subgroup (Group B), the which forces 2FA requirement on the new user
  - The user should not inherit the 2FA requirement from Group B (the subgroup)

Example Spec

The following spec is currently failing:

it 'does not enable 2FA for ancestor group member' do
  ancestor_group = create(:group)
  group.update!(require_two_factor_authentication: true, parent: ancestor_group)

  expect { ancestor_group.add_user(user, GroupMember::OWNER) }.not_to change { user.reload.require_two_factor_authentication_from_group }.from(false)
end

Current Behaviour

If subgroup (Group B) has 2FA enabled and top-level group (Group A) does not, a new user added to Group A still inherits Group B's 2FA requirement due to the current subgroup membership inheritance works.

Expected Behaviour

When adding a new member to the top-level group (Group A), the 2FA enforcement should only consider top-level group (Group A) and its ancestors. The user should not inherit the 2FA requirement from subgroups like Group B.

Relevant Context

Fixing Group#update_two_factor_requirement is already tackled by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24965, we should also fix User#expanded_groups_requiring_two_factor_authentication and create a background migration to update 2FA requirement for all users.

/cc @jeremy @tkuah

Related Support Tickets (internal)

Edited Oct 18, 2024 by Hakeem Abdul-Razak