Allow option to change permission levels of users when LDAP group sync is enabled

I was on the side of using ldap groups and not over-rides, however one of my users is on the other side of that fence.

@dstanley great picture!

I agree it should be an option. The inability to manage LDAP and non-LDAP members next to each other seems strange, same with the permission level.

I'm a little wary about implementing this quickly. I think this is a feature that is hard to understand and very impactful for our users and that is often used very differently between customers.

I think we should have a global and a group level configuration for this, where the global takes precedence. The checks would look like:

Global: Allow LDAP-synced groups to add, edit and remove non-LDAP members
Group level: Allow non-LDAP members in this group

@jacobvosmaer any thoughts about this? (assuming you know our LDAP implementation well)

cc @dzaporozhets

@JobV I like your 2 summary points. I think we have to include LDAP members in those as well. The other major use-case I've heard from users is that they have groups in LDAP representing a team. However, a handful of those team members need to have elevated privileges (master/owner) but it doesn't make sense to add a special LDAP group just to give them those privileges. So, for LDAP members default to group sync, but if a manual override has been put in place ignore that user on syncs from then on.

I might be misunderstanding what @JobV is proposing, but I don't think people should be able to add non-LDAP members in LDAP-sync'd groups. I think if we do that then there would be ever so much more confusion than there already is on how to control group memberships.

Based on the fixes I'm having to do, it seems most of our users were using this feature to promote certain individuals from the LDAP group to higher access levels, mainly Owner.

Thanks, @dstanley. Given that, I think that's the main use-case that I'm hearing from others as well.

@JobV Does it seem reasonable to take this one step first (allowing LDAP users to be promoted independent of group sync) and then address non-LDAP users at a later time, if necessary?

@dblessing @dstanley ah, thanks for clarifying. I'll edit the issue.

@dblessing yes I think so.

@dstanley @dblessing does the new description sound good?

Title changed from Allow option to manually manage some users when LDAP group sync is enabled to Allow option to change permission levels of users when LDAP group sync is enabled

@JobV @DouweM I think the way it is now (members managed by ldap completely or not at all) is much easier to understand, use and implement. Before we allowed it to be manually managed while ldap sync on we have quite a messy code when sometimes it was hard to predict what happens after next sync. At least if we are going to allow changing permissions it should work only via certain database flag when membership is marked as "don't touch this member during ldap sync". So in source code we do something like

def ldap_group_sync
  members_to_sync = ldap_group.members - group.locked_members
  members_to_sync.each ....
end

I think we should have a global and a group level configuration for this

I don't think its necessary. This "hiding" feature behind config cause situation when we have some features that nobody except few customers ever tried that we need to support from code perspective.

@dzaporozhets yeah I removed the necessity for both in the issue description, please refer to that for my latest opinion ;-)

It sounds like we're in agreement @dzaporozhets. Can you check and if necessary add to the description of the issue?

plus a new boolean on users

@JobV boolean flag on membership so sync does not touch it.

This level would always be greater than the original level

not necessary with boolean flag. When we mark membership as one that should not be affected by sync we don't care if its higher or lower. More it would bring extra complexity if we start checking was it higher or lower.

@JobV @DouweM my proposal is just boolean flag in membership table that allow certain membership to be ignored by ldap sync. This way you can set role for specific users without being worried of ldap sync overrride

boolean flag in membership table that allow certain membership to be ignored by ldap sync

+1

And that might also solve https://gitlab.com/gitlab-org/gitlab-ee/issues/325#note_3941234

Milestone changed to 8.8

We want to allow people to change permission levels of individual members from synced LDAP groups in groups.

Yes, better.

mentioned in issue #395 (closed)

I personally think having individuals have different access, which does not correspond with their LDAP group will only create a lot of confusion in the long run, even if it seems a convenient way to make exceptions now. "Fixing" this will not really solve any issue.

Creating separate groups for those users in LDAP is exactly what we have done, and while this might seem a bit cumbersome just to make an exception, it's clear now and will remain so for everybody as to why those users have those permissions. The alternative will undoubtedly result in a colleague in the future wondering why someone is master when their group indicates they should be reporter (for example). Or worse, a security issue where someone has more access than they should have after changing roles within an organisation.

If people really don't want to create groups in LDAP for those exceptions my suggestion would be to add a (user)groups concept to GitLab, so those users can be put in non-LDAP group. And any admin can easily see which groups a user is in and as such where they'll have what access.

@boudekerk Thanks for your feedback. I agree that the best option is to create a new group in LDAP. Not all GitLab administrators have the ability to do this. Many large enterprises have separate teams that manage LDAP and do not want to add application specific groups to the directory. In cases like that, this is necessary.

I think we can ensure that it's clear when users are manually moved to a permission level that is different than LDAP. We can add a badge or some visual notation in the member list for it.

Added ~157393 label

Added ~182284 label

Milestone changed to Backlog

@dblessing The issue contained within your original post is exactly one I am facing. We have groups for each team in our organization but want to assign different levels of git privileges depending on the rank of the member of the group I.E Teamlead/Manager etc. I don't see any harm in implementing the ability to do this along side of the current methodology of controlling it all via ldap. Currently you can override for things such as Assigning admin privs to users even though they auth via ldap or limit them via the "external user" option. I guess what I am getting at is, it would be nice to be able to have both methods available. Not everyone's LDAP is structured the same.

Relevant Zendesk ticket: https://gitlab.zendesk.com/agent/tickets/26378

We are also trying to use LDAP to provide simpler user management. we have thousands of projects, hundreds of groups, and hundreds of users. managing this entirely in LDAP is suboptimal for us.

We would prefer that a user get the greater privilege of all the LDAP synced privileges, and any group or project privilege that has been manually set, except for "blocked"; and, non-existence in LDAP should not delete or block a user that exists in gitlab.

FYI there is a potential customer with 200 users that this is a partial blocker for their deployment, and this feature would be a huge help to them

mentioned in issue #666 (closed)

The current description doesn't specify what will happen in the event that multiple LDAP group sync rules apply to a given user.

If an existing gitlab group member already has Reporter, and is in an LDAP group whose sync rule would set him to Guest, What is his access planned to be after the sync?

If an existing gitlab group member already has Reporter, and is in an LDAP group whose sync rule would set him to Guest, and is in an LDAP group whose sync rule would set him to Developer, What is his access planned to be after the sync?

mentioned in issue #736 (closed)

mentioned in issue #738 (closed)

If an existing gitlab group member already has Reporter, and is in an LDAP group whose sync rule would set him to Guest, What is his access planned to be after the sync? If an existing gitlab group member already has Reporter, and is in an LDAP group whose sync rule would set him to Guest, and is in an LDAP group whose sync rule would set him to Developer, What is his access planned to be after the sync?

They should have the most-privileged access. This is pretty much a solved problem with ACL handling anywhere else; it's effectively a logical "OR" (union) of the permissions.

We need this feature as well. Everyone on our team should have Developer access by default, but some of those on the team need Master access above that. I like the LDAP group integration but if we have to turn it off and manage group membership by hand, I'll do it for now.

The entire reason our organization upgraded to Enterprise was for the LDAP group syncing feature as it worked before v7.14. I regret not being involved in the discussion that changed how this functionality works as I would have been a very vocal detractor. As others have pointed out on this issue, this flat LDAP sync permissions model is a departure from the richer and finer grained ACL model implemented elsewhere. If I were to turn this feature on for all of our groups, we would immediately blow away years of fine grained permissions after the first LDAP sync takes place. That would be catastrophic to our teams.

We have a 900+ user organization and we're up for renewal in mid-December. This is far and away the most important feature for us. As this issue has gotten very little traction for the last 5 months (Backlog??), I'm going to have to start the painful process of evaluating other solutions that offer a permissions model similar to how GitLab used to work before https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/123 was merged.

@pkill Hey Alex, I am sending you an email right now.

Added ~126459 label

Milestone changed to %8.12

@tauriedavis @hazelyang @cperessini @dimitrieh Could one of you please help with some UX on this issue?

The UI currently looks like the examples below when LDAP group sync is disabled. If you enable group sync, the edit button is removed from this view. As part of this issue we'll make it so project owners/masters can override LDAP permissions. The possible scenarios will be as follows:

When group sync is disabled the UI should be the same as it is now - allows an owner/master to change the access level.
When group sync is enabled and the global option is set to allow manual changes, an owner/master can change the access level. We may want to display some sort of message to indicate that they're overriding an LDAP permission. We may also want to have some indicator in the members list to show that this is an LDAP user and their access level has been overridden.
When group sync is enabled and the global option is set to disallow manual changes. Maybe this can be the same UI as current when LDAP group sync is enabled - just don't show the edit button.

Current UI

Reassigned to @tauriedavis

mentioned in issue #881 (closed)

I've created mockups for this based on the new design for member pages here: https://gitlab.com/gitlab-org/gitlab-ce/issues/19868#note_13316760

Non LDAP user list

LDAP user list - set to disallow changes (but still allow adding new members)

LDAP user list - set to allow changes

Editing LDAP user

After confirming change permission of LDAP user

Question: Do we want to add a way to revert a user back to the ldap group sync?

cc/ @hazelyang @dblessing

mentioned in issue #884 (closed)

@tauriedavis This looks amazing.

Yes, we want a way to revert back to LDAP. Also, I think if a person's access level is overridden that we should still have LDAP group sync remove them if they disappear from the LDAP group. How can we visualize this? @JobV @jacobvosmaer What do you think?

Based on the decision above, we need to update the warning message for overriding access.

@jschatz1 Will someone from frontend be available to help with this for 8.12? I will handle the backend.

Added ~127620 label

Will removing someone from LDAP group sync (because we changed their permissions) disable the LDAP auth integration for them as well? Or does it just remove them from the sync'ed group in Gitlab?

@gjunker No, definitely not. They will still authenticate via LDAP. In my comment above I question whether we should still allow group sync to remove the user if they are removed from the LDAP group (I think yes).

I think I see what you are saying @dblessing. What if we change the warning to state that changing the permissions will override the settings from the LDAP group sync. Then always show an ldap label next to the permission dropdown with a hover state showing the name of the LDAP group sync. This way it is clear that even though the user's permissions are overrode, they are still a part of the LDAP group so removing them would remove them from the LDAP group sync. There is also an option in the dropdown to revert them back to the LDAP group sync settings.

@tauriedavis Yes, that's better. LDAP members cannot be deleted (they'll just reappear). Should we grey the delete button or just remove it? Also, the tooltip is a neat idea but currently we don't store which LDAP groups the member is a part of. Maybe in the future we can do that.

@dblessing That makes sense. I removed the delete button for LDAP users.

Ah, okay. I thought maybe we did based on this video but maybe that can be an improvement in the future. Updated the mockups to not include the tooltip for now.

cc @hazelyang @cperessini

@tauriedavis Perfect. Let's ship it. On the ~Frontend and ~Backend

Assignee removed

Updated description with most recent mockups for desktop and mobile

@tauriedavis Great job on these!

I thought you could modify the LDAP tag when the permissions have been changed manually. Maybe making the tag hollow or greying it out. Just an idea, maybe it doesn't work.

Thanks @cperessini! The styling matches how we have labels throughout the site. Since the styling of the label doesn't change when the permissions have been changed manually, I'm not sure why it would look like it was editable then? It's confusing to me to gray it out when it the permission is now editable. What are your thoughts @hazelyang?

Agree with @tauriedavis.

Yeah, I thought the grey option might be problematic. No need to add this then!

mentioned in issue #918 (closed)

Hey, I heard you say you need somebody from the frontend team to help out. I'm willing to volunteer if you don't mind me asking a bunch of newbie questions along the way :)

@mikegreiling Of course, that'd be great. I'll answer any questions I can. I haven't actually done many joint development operations with Frontend so what is the preferable way to accomplish this? Should I go forward with the backend first?

Added ~481018 label

@dblessing let's get @iamphill on this one. Since @mikegreiling is super new.

@dblessing If you could start the backend & then I can make a frontend branch from yours

@iamphill Awesome, thanks. In the meantime, do you want to tackle https://gitlab.com/gitlab-org/gitlab-ce/issues/21320 first? The initial changes need to be done in CE first, in that issue. Then it'll be minor changes for EE.

@dblessing Working on it now

mentioned in merge request !717 (merged)

Mentioned in commit 299ec600

@iamphill What is the status of the frontend?

@dblessing Waiting on !6148 (merged) to be reviewed by @jschatz1

@iamphill This is only the CE stuff, right? We also have a sort of blocker on a config option described in https://gitlab.com/gitlab-org/gitlab-ee/issues/918. In that case, it's probably better to only ship what you have in gitlab-org/gitlab-ce!6148 for 8.12. We can make the EE changes for 8.13. How does that sound? cc/ @JobV

@dblessing That works for me I'll make sure this is my top priority for %8.13

Thanks @iamphill By the way, the new project members UI looks good! I'm glad that improvement will likely ship in 8.12.

Milestone changed to %8.13

Mentioned in issue #918 (closed)

@iamphill I'm adding this to the plans for 8.13. If that won't work, please let me know.

Mentioned in issue #666 (closed)

@dblessing Just to let you know - still waiting for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6148 to be merged

Milestone changed to %16

Any reason why this is now a 'backlog' item? we were waiting on this feature for the 8.13 release.

@aboccia it was moved from %8.12 to %8.13, and didn't get finished in either. I'm sorry about that, but we're trying to be more honest about what we're actually going to ship in each milestone. That it was moved twice suggests we might not have finished it in %8.14 as well, if we continued to overschedule our releases.

@dblessing could this go into %8.15?

@smcgivern @dblessing we have had customers waiting on this one for quite a while. And this customer specifically https://gitlab.zendesk.com/agent/tickets/45812 has been watching the issue and looking for it every time he upgraded. It would be really great if we can get this done for 8.14 or if for some reason we have already booked up what is going into 8.14 and adding this is not realistic, to prioritize it for 8.15 and make sure it makes it in.

@dewetblomerus sure thing! The support team provide a list of issues to be prioritised in each release, just make sure this gets added to that list for 8.15.

@dewetblomerus @smcgivern i think it is mainly frontend work left for this so I am trying my best to make sure this is top of my list!

Thanks @smcgivern and @iamphill

@dblessing would you please consider making this one of our priority requests for 8.15?

Mentioned in commit e7ca83a2

Mentioned in merge request !822 (merged)

Mentioned in commit fb02e15d

Removing UX label as UX work is done. Please readd if something comes up!

Removed ~126459 label

Milestone changed to %8.15

Added Deliverable label

mentioned in commit 9e9d9358

mentioned in commit fa81f866

mentioned in commit 6ffc1900

mentioned in commit 67522fc6

I have a question about this feature. Does it cover the following case?:

A user is granted access to the group and its projects via a LDAP group (e.g., a user is "developer" in all projects)
For a specific GitLab project I want him/her to be "master".

Currently, I could just add him/her separately to the GitLab project with the "master" role. But my problem is that everyone else registered in GitLab could be added to this project as well. But I want (for policy reasons) that a user needs to have access to GitLab group before he/she can be granted access to a GitLab project within this group. Is this already possible? Is this another feature request?

@schlauch This does not cover the use-case you described. That sounds like a very complex use-case.

@dblessing: Maybe it just sounds complicated.

In a first place, I think about an option on group-level similar to the existing "Member lock: Prevent adding new members to project membership within this group". But instead it should prevent adding members without access on group-level. As a consequence, in the "Members" section of groups, not all users can be selected but only those with an access on group level. It is a kind of filter for the available list of users.

@schlauch Please create a new issue since it's out of scope for this issue.

@dblessing: here its: #1419

closed via merge request !822 (merged)

@dblessing We've been waiting for this feature in this form for 2 years and it finally became a reality in v8.15. Thank you for staying on top of this and thanks to the rest of the team who worked on this feature and its associated tasks. It's going to make our engineering organization more collaborative and it's going to make administration of GitLab much easier because we can decommission a lot of the hacks we had in place as workarounds. Most importantly it's going to save a lot of time because there is no longer a need to constantly grant one-off access. Thank you for listening to your customers, keep up the good work.

Thanks @pkill. Glad to hear it's helping you.

mentioned in issue #2773 (closed)

mentioned in issue #4875 (closed)

added Enterprise Edition label

Allow option to change permission levels of users when LDAP group sync is enabled

description

Mockups

Editing LDAP user

Drop down options w/ option to revert back to LDAP

After editing permission, user still have label indicating they are still a part of the LDAP group

LDAP mobile view

Editing LDAP mobile view

original issue

Designs

Child items ...

Activity

Current UI

Non LDAP user list

LDAP user list - set to disallow changes (but still allow adding new members)

LDAP user list - set to allow changes

Editing LDAP user

After confirming change permission of LDAP user