Skip to content
Snippets Groups Projects

WIP: Resolve "Allow global read-only user (like admin)"

Closed WIP: Resolve "Allow global read-only user (like admin)"
brianp/gitlab-ce:19737-read-only-auditor into master
Closed username-removed-417096 requested to merge brianp/gitlab-ce:19737-read-only-auditor into master

What does this MR do?

  • Enhances the management of Admins to offer a "Read Only" type role that allows a user full read only access to all resources.
  • Removes the admin boolean from User.
  • Adds a role_type enum on User that accepts Default, Admin, & Auditor
  • Adds a filter in the admin interface for filtering users based on Auditor role.
  • Policy changes to allow read only access to projects & related resources, and groups.
  • Project and Group explore pages are expanded to show Auditors all available projects and groups.

Are there points in the code the reviewer needs to double check?

  • Migrations should be double checked.
  • Didn't find information about backwards compatibility requirements in the API. API may require review as changes have been made.

Why was this MR needed?

The compliance department of a customer wants to run tests against the entire GitLab base to ensure users are complying with password, credit card, and other sensitive data policies. Without giving them full admin privileges.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

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
  • Douwe Maan
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • app/policies/project_policy.rb
    205 207 disabled_features!
    206 208 end
    207 209
    210 # This is similar to anonymous but doesn't check to see if the repo or builds
    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      Could we reduce that duplication somehow?

    • username-removed-417096
      username-removed-417096 @brianp ·
      Author

      So I thought about this a lot when I created the duplication, and more since your question.

      Yes, we could reduce the duplication.

      But every form I've thought to do it in I didn't consider better than having the duplication. What we gain in the extraction of commonalities, we lose in simplicity of having a single place to find what the permissions are for read_only. The way anonymous_rules works isn't quite the same as any other rule set in the policy due to it's un-natural return in the first line. It's not something that we can tier in the same way as the team rules get applied, in it's current form at least. We could tier it, but it would require additional refactoring in the way anonymous_rules are applied at the caller in BasePolicy#anonymous_abilities, which is outside the scope of this issue.

      Did you have something in mind that would reduce the duplication and keep it simple?

    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      @brianp The main thing I want to prevent is someone adding an ability to anonymous_rules, and forgetting to add it to read_only_access!, or the other way around.

      What do you think about having anonymous_rules call out to read_only_access!?

    • username-removed-417096
      username-removed-417096 @brianp ·
      Author

      I see your concern about adding some new rules in one and forgetting it in the other, I think it's completely valid. I considered that as well. But your idea still won't work. There are already differences between read_only_access and anonymous_rules which would give anonmous users access they should not have. I was also concerned about when that would happen. They would diverge it was only a matter of time, and it happened already.

      It really should be tiered, anonymous_rules being brought in to read_only_access, but like I mentioned, would require additional refactoring.

           # Abilities anonymous_rules lack
     can! :read_environment
     can! :read_deployment
    • Please register or sign in to reply
    Please register or sign in to reply