Auditor user with read-only access

@DouweM would this be a lot of work?

@JobV Not a lot, weighted at 3.

Milestone changed to gitlab-ce%29

Added gitlab-ce~480950 label

Another term could be 'audit' user.

Added gitlab-ce~113220 label

Similar request : https://gitlab.zendesk.com/agent/tickets/34621

Hey!

I've started an MR for an Audit role feature. Although the complexity of the concept wasn't overly involved what I underestimated was the reach of the changes.

Moving from the #admin boolean on User to an enum type affected a non-trivial amount of tests due to admin related setups.
The admin interface only authorized on whether a user was or was not an admin. Introducing a role that gave access to the admin interface but required restricting actions like, changing app settings, adding users, & ssh keys, etc involves a lot of UX component hiding of those types of actions throughout the entire admin interface.

I just wanted to check in to see if this was the expected behaviour, or if I might have stumbled in the wrong direction? As it stands it would still require more changes in the api, additional tests, and broken spec fixes.

Let me know if I've gone of course, or if I should keep heading down this path. /cc @DouweM

@brianp maybe just restrict access to the admin panel all together. cc @DouweM

Hey @JobV, thanks for the reply!

Restricting Admin access completely would simplify the issue, a lot.

I think my mis-step was to have the ability to audit the GitLab usage, not just the repos managed in GitLab.

I'll make the appropriate changes and ready it for a review.

@brianp you're a champ!

So I believe this is ready for review.

I made an additional change for discoverability. Once someone is an Audit user they have access to view all projects but didn't have any way to discover them without the admin interface or having a direct link. The explore tab was limited to projects the user was associated with. I changed the scope of projects and groups in the explore tab for Auditors. On the projects explore and groups explore pages Auditors can see all projects and groups.

Added gitlab-ce~164274 label

Customer request: https://gitlab.zendesk.com/agent/tickets/41716

Also it will be great if this gets implemented with GitLab-API too.

Milestone changed to gitlab-ce%31

Reassigned to @godfat

Milestone removed

Assignee removed

Reassigned to @smcgivern

Does this need gitlab-ce~~127616 and/or gitlab-ce~~19492 work?

Added gitlab-ce~~127616 gitlab-ce~~19492 labels

Milestone changed to gitlab-ce%32

Added gitlab-ce~992791 label

@awhildy It currently adds an option in the Admin User form, and a tab and badge on the Admin User index, both in line with existing styling on those pages. I don't think it needs actual UX work, other than a sign-off.

Could I get a concise description of what an Audit user is to place by the admin setting? Opened gitlab-ce!7769

@lbennett I saw you already have some work done, looks great!

Correct me if I'm wrong, but I was thinking that you probably can't be an Admin and and Auditor at the same time, so we should probably turn these options into radio buttons. This also gives us the opportunity to include some explanation of what each option means, like we do for "External".

I didn't combine the External option with the other three because I think you can be External + Regular/Auditor, but I'm not sure if you can be External + Admin.

cc. @DouweM

@cperessini See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7769#note_19207160 ;)

@DouweM You said in your comment that the placement of Admin/Audit/External didn't make sense to you in the current frontend implementation. Do you have the same problem with the design I posted? I'm trying to understand which part is not working for you

@cperessini I was mostly referring to the tabs in the admin user index, which had "Audit" by "External" instead of by "Admins". There is no similarity between External and Auditor, but there is between Admin and Auditor, as your mockup reflects.

removed assignee

We aren't going to get to this in 8.15, unfortunately. We will make it a priority for 8.16.

changed milestone to gitlab-ce%33

removed gitlab-ce~19492 label

Shouldn't this be EE? WDYT @victorwu @regisF?

moved from gitlab-ce#19737

Thanks @victorwu for moving the issue.

changed title from Allow global read-only user (like admin) to Auditor user

changed title from Auditor user to Auditor user with read-only access

@dblessing : Can you please review the updated the description and see if that makes sense.

@victorwu The auditor can access admin area and settings for projects and groups, but cannot modify them. Basically, they can see all the same things that a global admin can, but they cannot modify any of it.

This user cannot access the Admin Area.
The user cannot access any page that the cog menu of a project navigates to (i.e. Menu, Groups, Deploy Key, ..., Edit Project). The cog menu shouldn't be available.

Thanks @dblessing. I wanted to separate what's the intended requirements versus what may or may not be easier to build, and finally scope something small we can ship quickly. Updating the description so that the negative is not strict, but just a non-requirement, i.e.:

(1) It is required that the auditor has read access to all projects and groups.

(2) Why does the auditor need read access to the admin area? What are they auditing in that case? Why do they need to know how GitLab has been configured? I don't see that explicitly being requested in the ZD tickets.

(3) Why does the auditor need read access to the cog menu settings? I also don't see that being requested in the ZD tickets.

How about we do this: Make sure that we first implement (1) for sure as an MVP. As for (2) + (3), we may get them for free as part of the MVP, depending on how the implementation turns out. However, they are not strictly in scope and we should definitely exclude them if they add any more complexity to the implementation. We can use subsequent issues to discuss why (2) + (3) are necessary/requested/important.

Do you let me know if that's not right and I'm missing something.

@victorwu The customer specifically requested "read-only access to all projects in the same vein like admin but just read-only." This tells me they want access to project and group settings by this user. They also requested access to view all users. They had another ticket where we helped them write a small Ruby script to export all groups and projects and their owner.

We can ask them for clarification if you'd like. Let's not proceed with something that doesn't work for them. If we need to split this in two and let them know it will be delivered it two chunks, that's fine.

Keep in mind that this has been mostly completed twice and was unfortunately lost. Several people like @smcgivern and maybe @DouweM should have a pretty good model in their head on how this looked and how much work it is. Backend can help us scope it out and let us know if one route is more or less work than another.

Thanks @dblessing for the clarification. That's exactly what I needed so that we can better plan and avoid doing work on this multiple times without shipping anything.

I'll definitely work with @smcgivern to ensure we work on the smallest thing possible. And we'll make sure that we don't implement something that puts us into a corner where we cannot easily get the expanded functionality as you explained. If there is a major tradeoff that needs to be considered, let's get back to the customer. But nothing for now. Thanks!

assigned to @timothyandrew

removed milestone

changed milestone to %8.15

changed milestone to %8.16

mentioned in merge request !998 (merged)

I think that the simplest possible approach here is to not conflate the idea of an "admin" user and a "read-only" user. We can have an admin+read-only user or a non-admin+read-only user. By doing it this way, we only need to make a few small changes to the way abilities are compiled. The admin area is a special case, but even that can be tackled fairly easily. I've created a quick PoC of this approach in !998 (merged). Here's my commit message from that implementation, which should provide more details:

Implement backend for a read-only "Auditor" user

The simplest way to implement this seems to be in a dynamic fashion. We achieve
read-only access without changing too much code by:

1. Add a `read_only` boolean to `users` so we can check if a user is `read_only?`
2. When building the list of permissions/abilities for a `read_only?` user,
   strip all abilities that _don't_ start with `read_`. This is not perfect, but
   can be fine-tuned by manually adding in a list of extra abilities that the
   read-only user needs to have (like changing their password, for example).

The "Admin Area" needs to be tackled separately, since the authorization
there doesn't hook into the policies/abilities that the rest of the system
uses. The authorization code previously only checked if a user was `admin?`.
If so, the user can do anything in the "Admin Area". This commit implements
read-only access to the "Admin Area" by:

1. Split all admin actions into two types - `read_admin_area` and
   `modify_admin_area`. For now, this partition is fairly crude; all `index` and
   `show` actions are `read_admin_area`, and everything else is
   `modify_admin_area`. The current user (in addition to being an admin), needs
   one/both of these permissions to access pages in the "Admin Area".

2. Individual pages that don't neatly fall into this split can be fine-tuned
   individually. 

3. Create an `AdminPolicy` that gives all admins both the `read_admin_area` and
   `modify_admin_area` permissions. As per the previous section, all permissions
   that _don't_ start with `read_` will be stripped out for read-only users. And
   so, such users will only have the `read_admin_area` ability.

Note: If we implement read-only access for the "Admin Area" this way, there's one problem: none of the action buttons will be hidden. For example, the read-only admin user will see the "New User" button, even though they aren't allowed to create a new user (clicking the button leads to an error page). I discussed this with @victorwu today - we agreed that getting the backend permissions right is the first priority here, and we can make the UI more intuitive (remove the buttons that don't do anything for the read-only user) in a subsequent iteration.

@DouweM @smcgivern: Do you have any thoughts about this approach? I've only been looking at this issue for a few hours, so I'd be surprised if I'm not missing something.

@timothyandrew thanks! That's a great explanation, and I really like how you've listed the benefits and drawbacks.

I think I find the semantics of this quite confusing: an admin flag on a user allows them to access all projects, and the admin area; a read-only flag on a user doesn't allow them to do anything. This is most awkward when it comes to the admin area, but I think this is a little odd too:

When building the list of permissions/abilities for a read_only? user, strip all abilities that don't start with read_.

I also don't understand how a non-admin read-only user helps anyone. I don't think the customer needs it, and I'm not sure what it would be used for?

None of this is to say that we shouldn't do it this way, just that in my head I had the 'auditor / admin / regular user' roles as a single dimension, and I'm still getting my head around this way

@victorwu what's the state of this issue? Will we be separating "Read only" from "Admin" or will we still have "Auditor" users? I'm wondering if new UX work needs to be done, specifically for the Admin screen where you edit a user's permission level

@cperessini : The latest is what @timothyandrew wrote in the comment above: https://gitlab.com/gitlab-org/gitlab-ee/issues/1439#note_20441951.

He should be coming back from time off and starting on this next Monday.

We are currently leaning toward just getting the BE permissions right as the first iteration, and we should do whatever is easiest from a BE-perspective just to get something shipped and in the hands of customers. Then we can iterate further and see if we need to add anymore fine-grained functionality.

Feel free to add to the comments that Tim and Sean have made above. I agree with them and don't have anything else to add. Just wanted to hear what Tim has to say when he returns before we make any final decisions.

I agree with @smcgivern that admin+read-only is an odd paradigm. It will be confusing for our users, IMO. Can we retain this concept on the backend but still present to the user as a single dimension? On the backend you can take admin permissions first and stripping every permission that doesn't start with read_. But in the edit user screen a person can choose regular, admin or read-only, but not multiple.

Tim should be returning to this first week of 2017 after time off. Per the Discussion meeting on Dec 30, @smcgivern + @DouweM will sync up with him then and figure out next steps.

@timothyandrew : Any thoughts on what @smcgivern mentioned above?

I don't mind having a non-admin read-only scenario if it's the easiest and most straightforward way to implement what we want here.

I think I find the semantics of this quite confusing: an admin flag on a user allows them to access all projects, and the admin area; a read-only flag on a user doesn't allow them to do anything. This is most awkward when it comes to the admin area, but I think this is a little odd too.

@smcgivern: I think of it like this:

An admin user has access to everything
An admin+read-only user has read-only access to everything

It makes sense to me that the read-only modifier is provided by a flag. If we instead have separate admin and auditor user types, we'd have to give the auditor user access to all the pages/actions that an admin user can access, which would be a lot of work. Strictly speaking, that would be a cleaner approach, but it would involve a lot of ability/policy fine-tuning, and will be harder to get right, as far as I can tell. The read-only approach is better for a first iteration.

I also don't understand how a non-admin read-only user helps anyone. I don't think the customer needs it, and I'm not sure what it would be used for?

@smcgivern @dblessing: If we don't need a read-only+non-admin user, I agree that we can make things less confusing by changing the UX of the settings page (/cc @cperessini).

@timothyandrew @dblessing I think that the UI not matching the data model is an indication that the data model might not be correct! If we have a read-only / non-admin user in the DB, how would we display that in the UI?

In general, I'll leave this up to @DouweM!

assigned to @DouweM

@DouweM @smcgivern: To avoid rework, I'll wait for a consensus here before building this out fully. /cc @victorwu

I think the single dimension "user_type" regular/auditor/admin makes most sense.

If we instead have separate admin and auditor user types, we'd have to give the auditor user access to all the pages/actions that an admin user can access, which would be a lot of work.

@timothyandrew I don't think this is true. The "pages/actions that [only] an admin user can access" are all write actions as opposed to read, so their access logic don't need to be changed for auditors. As far as read permissions go, the only difference between admins and non-admins is that admins have access to all groups and projects as if they were members of those projects. Extending that from admins to admins + readonly-users would mostly come down to finding if user.admin? in policies and finders and replacing it by if user.admin? || user.auditor? (or some new user.can_see_all_the_things? convenience method).

When building the list of permissions/abilities for a read_only? user, strip all abilities that don't start with read_.

@timothyandrew What do you think about reusing the anonymous_rules list on policies, that has already been filtered to just have permissions that are relevant to users who cannot perform write actions?

The customer specifically requested "read-only access to all projects in the same vein like admin but just read-only." This tells me they want access to project and group settings by this user.

@dblessing I'm not reading that request the same. I think they want the ability to read all project data (repo, issues, etc) " in the same vein like admin", not their settings. I think it would be useful to clarify this with the customer, because allowing read-only access to the project settings and admin area would require adding tons of checks to disable inputs etc.

@mydigitalself : This one @timothyandrew already has been working on in recent weeks. @dblessing has been interacting with the customer and you can review the ZD ticket as week. @mydigitalself is our new platform PM, and I believe this would be classified as a platform issue right?

I believe this would be classified as a platform issue right?

@victorwu @mydigitalself I would agree!

Just so I'm clear, is this an EE feature or will it be going into CE too?

@mydigitalself One of our EE customers asked for this, and it's very specific to auditing requirements. So I don't think anyone doubted that it should be EE-only. (So that's why it's here in the gitlab-org/gitlab-ee project.) GitLab has a nice way to move an issue from one project to another all with one click if you ever need it.

@regisF + @mydigitalself : I removed myself from the description as the PM driving this. I had some initial chat with @timothyandrew . But nothing major. All the relevant details are in the thread here. Let's make sure we can at least get clear PMs for issues to drive them forward.

@DouweM

I think the single dimension "user_type" regular/auditor/admin makes most sense.

Alright, that works. /cc @smcgivern

Extending that from admins to admins + readonly-users would mostly come down to finding if user.admin? in policies and finders and replacing it by if user.admin? || user.auditor? (or some new user.can_see_all_the_things? convenience method).

I agree, but I worry that this might be more error-prone than the alternative. We'd also need to create a number of policies for the admin area. Do you think it's worth the (probably small) risk so we can keep things simple?

What do you think about reusing the anonymous_rules list on policies, that has already been filtered to just have permissions that are relevant to users who cannot perform write actions?

I'm not reading that request the same. I think they want the ability to read all project data (repo, issues, etc) " in the same vein like admin", not their settings. I think it would be useful to clarify this with the customer, because allowing read-only access to the project settings and admin area would require adding tons of checks to disable inputs etc.

We could use anonymous_rules if the only difference between the auditor and an anonymous user is the scope of projects/issues they see (auditor sees all projects, anonymous user sees public projects). This is only true if the project settings pages are inaccessible. If the settings pages need to be accessible, anonymous_rules won't be sufficient.

We'd also need to create a number of policies for the admin area.

[...]

This is only true if the project settings pages are inaccessible.

@timothyandrew Right, my suggestions were made under the assumption that the user doesn't care about those settings pages, just read access to all groups and projects. Let's get that cleared up from Product / @dblessing before we move forward.

added ~333913 audit events labels

@timothyandrew @DouweM Let's go with that assumption for now. We can revisit as a second iteration if the customer requires settings page access. At first, I thought they did, but re-reading their request it seems to be related to content.

removed ~333913 label

We should be careful with this. This can result in a UX mess. I think a user should be admin or auditor, not both. If we introduce the notion of admin+auditor, this will be confusing. Even I need to think of what it represents.

An auditor shouldn't be able to access the settings
A user can be an admin, or an auditor, not both (at least in the UI - perhaps they are similar under the hood).

As for accessing the list of users, how would they do that? See each group individually? @dblessing

@regisF All great questions. Ultimately, we'll probably have to answer them. For now, it's good enough to ship with access to all projects/groups but not settings/admin area. I agree that admin and auditor are exclusive and cannot be both.

my suggestions were made under the assumption that the user doesn't care about those settings pages, just read access to all groups and projects. Let's get that cleared up from Product / @dblessing before we move forward.

Let's go with that assumption for now.

@DouweM /cc @dblessing: Alright, that simplifies things! In this situation, I agree that it's better not to conflate the roles of auditor and admin. I'll get started on this right away.

assigned to @timothyandrew

changed milestone to %8.17

removed audit events label

I've read through the latest comments and I think the UX proposal from https://gitlab.com/gitlab-org/gitlab-ee/issues/1439#note_20341265 represents the functionality we're going to offer. Is that correct?

Should we disable the External checkbox if a user is Admin or Auditor? Since those types of users can see all projects, I think it only makes sense to enable the checkbox for Regular users.

Have we stalled here? Is there a SSOT on here yet? I think if @jivanvl is going to get this done we need to make a decision

@jschatz1 @cperessini is it a case of moving the mockup from https://gitlab.com/gitlab-org/gitlab-ee/issues/1439#note_20341265 into the description of the issue?

If we want to arrange a quick session today to clarify any outstandings, happy to help facilitate.

@mydigitalself It may be. I was holding off on doing that because I wanted to confirm that the design represented the way the feature is supposed to work.

added ~333913 label

closed via merge request !998 (merged)

added ~139512 label

added SP1 label

Auditor user with read-only access

Resources

Requirements

Design

Original description

Designs

Child items ...

Activity

Admin message

Admin message

Auditor user with read-only access

Resources

Requirements

Design

Original description

Activity