inherit require 2fa for all subgroups and projects
What does this MR do?
Me as a group owner, I want the require 2FA property to be inherited across all my subgroups and projects. So in this MR the set of users is extended to include all users of sub-groups and projects (of the group or subgroups).
If the decision was taken, that a group needs to have two factor authentication, this indicates that high security requirements are needed. as there is the possibility to have subgroups and projects in a group, it is quite plausible that this high security requirements also apply for this subgroups and projects. So it would be much more intuitive when the 2FA requirement is propagated to the subgroups and all projects. It seems wrong that people added in subgroups or on project level do not need tho have 2FA.
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
The development of this MR is sponsored by @ siemens (/cc @bufferoverflow).
Merge request reports
Activity
added 1 commit
- 337e5416 - first try: fix mysql problem (not all users found)
added 1 commit
- 9ec77650 - second try: fix mysql problem (not all users found)
added Community contribution label
added 2FA label
added customer label
removed customer label
cc/ @jeremy
added 1 commit
- a1729c04 - remove experiments for 2fa requirements and fix tests
@grzesiek could you please review this MR? (Gitlab suggested you as an approver, thanks in advance! :) )
- Resolved by Imre Farkas
@jeremy What is your opinion on this? Do you agree, that it would be more intuitive to propagate
require_two_factor_authentication: true
of a group to all its subgroups and projects?
marked the checklist item Changelog entry added, if necessary as completed
marked the checklist item Tests added for this feature/bug as completed
mentioned in issue #61091 (closed)
added Manage [DEPRECATED] devopsmanage labels
- Resolved by Imre Farkas
- Resolved by Imre Farkas
- Resolved by Imre Farkas
Thanks @rroger for the MR! Left a few comments.
I think this change makes sense but can you confirm @jeremy? (see https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24965?view=inline#note_143553613)
I am wondering if we need to indicate on the subgroup settings page where the 2FA requirement for their users is coming from. It might not be obvious for the subgroup admin.
mentioned in issue gitlab-com/www-gitlab-com#4230 (closed)
- Resolved by Thong Kuah
added 6253 commits
-
0b40da61...c18136ae - 6245 commits from branch
gitlab-org:master
- d0087653 - add tests for 2fa requirment for all sub-entities members (subgroup and projects)
- 85e17391 - require update_two_factor_requirement on all sub-entities users
- 7945fd17 - first try: fix mysql problem (not all users found)
- 8bb8eead - second try: fix mysql problem (not all users found)
- 396459a7 - remove experiments for 2fa requirements and fix tests
- 5c021563 - add changelog entry
- 3e761dab - fix tests for mysql db.
- cf88ac41 - test: add test for expanded_groups_requiring_two_factor_authentication
Toggle commit list-
0b40da61...c18136ae - 6245 commits from branch
@jeremy I've updated the specs and did a rebase to the current master. I would appreciate if you could have a look. Beside of this I figured out that we have some conceptual topics to be clarified in regard of project membership within groups that have 2fa enabled, the reworked specs fail because we are able to add project members and they are not forced to use 2fa. My current impression is that we have to extend
member.rb
, find group of project check if 2fa is enabled. WDYT?@ifarkas Could you give some feedback as well?
- Resolved by Imre Farkas
- Resolved by Imre Farkas
Thanks @bufferoverflow for the update! Added a few comments.
Following up on my previous comment on adding a notification, what do you think of updating the documentation in
doc/security/two_factor_authentication.md
?The pipeline is also failing, can you please take a look?
assigned to @bufferoverflow
marked the checklist item Documentation created/updated via this MR as completed
@ifarkas I've updated the docs and fixed the topics you've mentioned. I'm unable to resolve the discussions as I'm not the original author of this MR.
- Resolved by Imre Farkas
Thanks @bufferoverflow, one last comment.
Thanks @ifarkas , just adopted the structure of the spec accordingly.
assigned to @ifarkas
added customer label
Thanks @bufferoverflow for the update, looks good to me!
@eread, would you mind reviewing the doc change in
doc/security/two_factor_authentication.md
@tkuah, would you mind doing the maintainer review?