Add global configuration option to restrict group owners to admins

Marked as confidential pending @jacobvosmaer-gitlab's verification that we aren't creating unnecessary user risk by outlining the current state.

Let's keep this confidential for now. I share your hesitation @dblessing

I prefer option 3 because it does not change what 'group owner' or 'ldap group sync' means.

Yeah, I'm with @jacobvosmaer-gitlab on this one.

@JobV Do you think this is a blocker/must be shipped alongside the override addition in 8.12?

@dblessing it's strongly preferable, seeing as one of the major customers will otherwise suffer with the consequences.

Milestone changed to %8.12

@dblessing on thinking about it, let's make this a blocker so we never ship something that will burn anyone. Let me know if you need any help in managing it.

@JobV If there is any dev that can pick this up for 8.12, it would be a big help.

@JobV What should we do here? The backend is in final review, and front end to follow closely. Basically, are we going to be able to ship in 8.12?

In my mind, adding an option like this is going to have lots of touch points.

Mentioned in issue #343 (closed)

Milestone changed to %8.13

Added ~481018 label

@DouweM Thanks for adding to the 8.13 milestone. What are the chances of this making it? It's a blocker for https://gitlab.com/gitlab-org/gitlab-ee/issues/343. Of which backend is done, frontend is in progress.

Added ~360297 label

Milestone changed to %8.14

Removed ~360297 label

Milestone changed to %16

Ping @DouweM @smcgivern. https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/822 should finally land for 8.15 (delayed 3 months already). This is important for another customer regardless of 822, but I hate to see this delay the new LDAP stuff. Please assign for 8.15 if you can.

@dblessing it's on my list for %8.15, I'm just trying to do this in a batch. Thanks for the ping, though, as you couldn't have known that!

Milestone changed to %8.15

Added Deliverable label

Which option has been decided on? Is it no. 3?

@dblessing can you give me some feedback on this? Thank you

Added ~126459 label

changed milestone to %8.16

@pedroms Yes, option 3

assigned to @kushalpandya

Hi @dblessing I have started working on this issue. I had a few questions around it.

How do we plan to expose this configuration? is it a just flag that can be enabled/disabled (i.e. Checkbox) which says Prevent changing group owner ? or any other form of input?
Where do we plan to show this option? I can think of new group page (/admin/groups/new) and group settings page (/groups/<group_name>/edit)

Let me know if I understood this issue correctly, I'm also following !822 (merged).

Ok I was wrong about the whole YML thing.

Had a quick chat with @DouweM and he said :

It seems like it’s only relevant when LDAP group sync is enabled Which already has a UI

So the UI could go there.

Also @tauriedavis or @awhildy would be good if you could chime in here.

But in the end we would need to wait for product to make a final decision here. @dblessing that's you.

No new settings should be added to gitlab.yml.

@kushalpandya The option should be something like 'Restrict group owners to administrators' (or '... global administrators'). I believe we decided it should be a global configuration ('admin/settings'?) rather than a group-level setting.

@jschatz1 I'll chime in

@dblessing @kushalpandya I was thinking about adding the setting in admin/application_settings between “Enabled Git access protocols” and the “Account and Limit Settings” section, as follows:

What do you think?

@pedroms Thanks for the mockup, and especially the config text! though I added this config under Account and Limit Settings in my WIP. I'll update it though!

Do we have config item for this feature in application_setting.rb and application_settings_controller.rb? I can't seem to find one

cc: @smcgivern @DouweM

@kushalpandya No problem, I'm glad it helps. I also thought about placing it under Account and Limit Settings but that section looks very specific to a user account in GitLab.

@pedroms @kushalpandya AFAIK, we do not want to make this setting explicitly about LDAP group links. It should be about restricting group owners to global admins, thereby restricting LDAP group link management.

@dblessing from my understanding, that's not what option 4 “SSOT” says. Can you please confirm?

@pedroms I don't know who added SSOT, but it wasn't me. I see no comments mentioning that this was the decision, either. Last we left off, we had agreed on option 3.

Maybe someone in this thread can shed some light on how we ended up with this mystery SSOT?

@JobV @jacobvosmaer-gitlab Do you still think the global option to restrict group owners to global admins is preferred over a specific LDAP-related configuration?

@dblessing I had discussion around this issue with @jschatz1 and based on comments, SSOT for option 3 was put, is it yet to be concluded which option to go with?

@kushalpandya Option 3 says:

Add a global configuration option to only allow GitLab administrators to be owners of groups.

Which does not match the SSOT. Maybe there was confusion when typing the SSOT?

changed title from Add configuration option to restrict overriding LDAP permissions? to Add global configuration option to restrict group owners to admins

Edited description so that option 3 is the SSOT (per https://gitlab.slack.com/archives/f_ldap/p1483647921000029)

@dblessing @kushalpandya @jschatz1 I was reviewing the issue description and restricting group owners to admins seems to be the same as restricting group creation/edition to admins. If the purpose of this issue is to restrict the management of LDAP group links and LDAP user permissions, I think this is approach is too restrictive. This would mean that only admins could create groups or edit them, which goes beyond LDAP group links. Am I missing something here?

@pedroms I believe master can also create and edit projects.

@dblessing Yes, you're right, I think a master user can do that. My point with the previous comment was to understand how option 3 caters to the issue purpose better than option 4. What are your thoughts?

@pedroms From @jacobvosmaer-gitlab comment:

I prefer option 3 because it does not change what 'group owner' or 'ldap group sync' means.

This is also more generic, which prevents us from getting in to a situation where we're basically implementing custom roles. Let's start with this and if it's too restrictive or some other issues comes up we can re-visit.

@dblessing @jacobvosmaer-gitlab do we have field for this option added in application_settings.rb (and also probably in application_settings_controller.rb) which can be used for form submission on saving?

This will not be retroactive so we will also have to create a Rake task or similar to demote all current group owners to masters and add GitLab administrators as group owners.

@dblessing @pedroms I think that we should add a note warning users that we will demote all current group owners to masters when this feature is turned on. Which GitLab administrators we should pick up as group owner when we have more than one?

@dbalexandre seems it is not happening this release right?

@jschatz1 Yes, I think that we don't have enough time to make this happens for this release. We'll need some backend work here.

This is also being requested by a customer at https://gitlab.zendesk.com/agent/tickets/56840

@kushalpandya can you confirm if this is coming up in 8.16?

Hi @balameb as @dbalexandre mentioned, there's pending work on backend for this feature, so I don't think it'll make it in the 8.16.

changed milestone to %8.17

Is there related backend issue for this?

Or if this is the only issue tracking both frontend and backend changes, I'd rather prefer to have it assigned to someone who is working on pending backend part of it (and remove ~Frontend label meanwhile).

cc: @dbalexandre

SSOT: Add a global configuration option to only allow GitLab administrators to be owners of groups.

If we add an admin area option with a label along those lines, I think it will not be clear that this implies that only admins can create groups.

The backend work required for option 3 seems to be:

Store the new admin area option
Don't award the :create_group ability to non-admins when the option is enabled.
Don't allow non-admins to be added as group members with owner access (in GroupMember.add_user and GroupMember.add_users_to_group)
Don't allow non-admins to have their group member access changed to owner (in the controller and the API
Clarify this in the UI, in the "Add user" form and elsewhere, and show errors when someone tries to give a non-admin owner access.

Personally, I would prefer option 4 which just affects the LdapGroupLinks page and API, and could be implemented easily by checking a new :admin_ldap_group_links ability, without touching all member management.

I don't think it "change[s] what 'group owner' or 'ldap group sync' means.", it just specifies that admins and admins alone are in charge of setting up the integration between GitLab and LDAP.

@regisF @jacobvosmaer-gitlab @dblessing @dbalexandre Thoughts?

@kushalpandya I'll unassign you while this waits for backend work. The frontend will be very minor anyway—backend devs should be capable of adding a single option to a form :)

removed assignee

@DouweM Even though option 3 is marked as the SSOT, I agree with you on option 4. It's simpler to implement, understand and impacts just what this issue was initially intended for: LDAP group links. Besides, @dbalexandre raised a good point about option 3:

Which GitLab administrators we should pick up as group owner when we have more than one?

@DouweM @kushalpandya admin_ldap_group_links doesn't quite cover it - it's also about restricting member overrides. @regisF Can you weigh in here?

I also dislike this very specific configuration because we're wandering in to the custom roles realm and instead of implementing true custom roles we're sidestepping and bolting on a feature in a weird location. This contributes to splintered ways to affect roles in various configuration. This is also sort of true for option 3 but at least it's at a way less granular level.

it's also about restricting member overrides

@dblessing I thought those overrides were already restricted?

I also dislike this very specific configuration because we're wandering in to the custom roles realm

For me, it doesn't feel like a custom role, but a "Admins are responsible for LDAP management" checkbox.

@DouweM they're restricted to masters. But some orgs want to make sure no one but admins can manage an override, or delete/add ldap group links. Both scenarios are covered by the option 3, but not by option 4.

I understand what you are saying @dblessing. But

adding a global configuration to only allow administrators to be owners of groups will change master to be custom and change depending on the context, which might be misleading.
this setting goes beyond LDAP, which is the main topic here.

Have you received demand for the need to restrict the management of groups to admins?

What I like with solution 4, without knowing the needs of the customers, is that it's restricted to LDAP specifically and can explained that way, under a specific LDAP section in the Admin interface.

@regisF The ultimate requirements are what I mentioned before:

Restrict management of LDAP group links
Restrict management of LDAP member overrides

The solution to this issue needs to address both things. I have some philosophical opinions on whether option 3 or option 4 are best, but at the end the above two things must be addressed. However we can move to that solution most quickly, let's do it.

Edited description: added clear requirements as specified in https://gitlab.com/gitlab-org/gitlab-ee/issues/918#note_21974842

@regisF @DouweM @dblessing Let's make a decision so that we can ship this in %8.17 and don't impact customers negatively. #343 (closed) was shipped in %8.15 and as @JobV said about this issue: “let's make this a blocker so we never ship something that will burn anyone”.

Even though you are all more experienced than me on this, I'd say option 4 addresses this issue better than other options.

@dblessing @DouweM @pedroms it seems that option 4 addresses both point.

Moreover, it doesn't require a new Rake task, making it a more boring solution.

Would this option be off by default for new setup?
What about existing installations? Off by default too?

@regisF Is there something different you would name the configuration for option 4? To me, if the configuration is Allow group owners to manage LDAP group links it would be surprising if it also restricted the ability to override membership levels. Perhaps `Allow group owners to manage LDAP-related group settings' or something similar so it's more inclusive.

I would definitely recommend this is off by default for new and existing setups.

@dblessing what about (imagine this being a UI element):

- [ ] Allow group owners to manage LDAP-related group settings
      If checked, group owners will be able to manage LDAP group links and LDAP members overrides. Read the documentation to know more.

@regisF Sounds great. Do you want to update the issue description?

@DouweM description updated.

Picking up on @regisF suggestion, here's a quick mockup of what it would look like (placing it under “Visibility and Access Controls”):

I've added the label “LDAP group settings” before the checkbox becaue “LDAP-related group settings” seemed a bit too long. Good to go?

@pedroms that depends whether we'll have documentation for this feature or not.

I think we will have a bit of documentation, right? If we do, the question mark is misleading because you expect a bubble, not the ability to click on it and direct the person to the documentation.

This comment was lost on Feb 1st DB issue

@regisF I think it could link to the documentation regarding LDAP group links and permission overrides:

Using the question mark icon to link to the documentation/help/information is a common pattern used throughout GitLab. It is used specifically on that settings page on numerous occasions. Have you seen it being used in a different way?

This comment by @regisF was lost on Feb 1st DB issue

@pedroms I thought (and it was wrong of me) that this wasn't the case.

Edited description: updated the Specification section with example screenshot and details about the documentation.

added ~127620 ~1268427 and removed ~126459 labels

@dbalexandre @kushalpandya are you comfortable with this being a Deliverable for %8.17 ?

@pedroms From UI perspective, changes aren't much once I have form field available that I can use in Admin settings form as mentioned in this comment.

This will not go into %8.17. I will move it to %9.0, but as Stretch, since breaking changes and other Deliverables take priority.

added Stretch label

changed milestone to %9.0

removed Deliverable label

changed milestone to %9.1

removed Stretch ~158101 labels

changed milestone to %16

Support has added a label to this, we will consider presenting this for the next release.

added ~1672340 label

@DouweM I'm cutting it to the wire here for next release , but support would like to request this for prioritization. I believe you are the right person to ask. If not, please tag them!

@lbot Correct person to ask is @mydigitalself :)

Customer has requested a follow-up:

ZD: https://gitlab.zendesk.com/agent/tickets/73544

Thanks!

added customer label

changed milestone to %9.3

@dblessing Does this issue still need to be confidential? We made some changes in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/822 ? - The customer wishes to follow this issue for updates?

@DouweM Removed the security label, are there still security concerns here? hence the "confidentiality"?

@MrChrisW It's security related, but not a vulnerability per se.

made the issue visible to everyone

@DouweM Thanks for making it visible

added Deliverable label

@mydigitalself has there been a backend developer assigned to this issue? I don't think I can get started without it.

@DouweM

@mikegreiling We usually don't assign developers directly to issues, except for really urgent/big ones. This issue will hopefully get picked up by one of them as it rises to the top of the prioritized issue list, but as you can see there is still a lot of other stuff in there.