Skip to content
Snippets Groups Projects

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?

The development of this MR is sponsored by @ siemens (/cc @bufferoverflow).

Edited by Roger Meier

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Imre Farkas
  • 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. :thinking:

  • @ifarkas I continue the work from @rroger here. However, I wait for @jeremy to confirm first.

  • Roger Meier added 6253 commits

    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

    Compare with previous version

  • Roger Meier added 2 commits

    added 2 commits

    • 1e2118a2 - test: change group spec
    • 9d9d266a - tests: add test cases for group, subgroup, project and subgroup project members

    Compare with previous version

  • @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?

  • Imre Farkas
  • 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?

  • Roger Meier added 4 commits

    added 4 commits

    • 73e3a962 - refactor: use all_expanded_groups as initially
    • f2f3df5d - test: extend user_spec with shared project on 2fa group
    • a2f0d2d6 - docs(2fa): describe behavior in detail
    • c17568bb - refactor: group_spec with describe and before block

    Compare with previous version

  • Roger Meier marked the checklist item Documentation created/updated via this MR as completed

    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.

  • Roger Meier added 1 commit

    added 1 commit

    Compare with previous version

  • Imre Farkas
  • Thanks @bufferoverflow, one last comment. :thumbsup:

  • Roger Meier added 1 commit

    added 1 commit

    • 4f934dba - refactor: adopt group_spec structure

    Compare with previous version

  • 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?

  • Imre Farkas assigned to @eread and @tkuah and unassigned @ifarkas

    assigned to @eread and @tkuah and unassigned @ifarkas

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading