Currently, project members have implicit direct membership in the parent group. They can access it and have limited permissions.
The behaviour for group members is different: we don't grant access to the parent group.
There are other edge-cases of this inconsistency caused by how it is implemented. Eg. user Bob has access to Group_Board in the following scenario:
graph Group --> Subgroup Subgroup --> Project Group --> Group_Board User:Bob -->|Developer| Subgroup
However, Bob does not have access to Group_Board when Subgroup has no projects:
graph Group --> Subgroup Group --> Group_Board User:Bob -->|Developer| Subgroup
Because of group / project consolidation, we need to resolve this difference one way or the other. Either we grant access to parent groups for direct group members (which might have security implications), or we stop granting it for project members. Both might confuse some users, and this is likely going to be a breaking change.
I think the simplest approach is to limit access according to membership, and allow access to only flow down in the hierarchy. In other words, we shouldn't give access to parent Namespaces.
@ifarkas That would be the ideal solution but it is a breaking change to existing customers. I think we would need to wait until a major release to make this change. I also wonder what the user experience would be like since you'd have clickable items in the breadcrumbs (as an example, there are probably more) that would now be broken. We'd have to do a full sweep to see what else breaks and how we resolve it .
I think the simplest approach is to limit access according to membership, and allow access to only flow down in the hierarchy. In other words, we shouldn't give access to parent Namespaces.
Yes, it would be best option. It has some implications though, off hand:
users wouldn't be able to assign some labels, milestones, epics... (these from ancestor groups if they don't have access to it) to issues/MRs and other resources.
if someone else (who can see these labels, milestones, epics...) assigns them to an issue, some other users may not see them -> we will have to present these as something like "set but not visible to you". I think this is something we will need for sharing resourcess across multiple groups anyway (discussed in depth in #296668).
for larger organizations which use sub-groups and sub-projects and give users access only to selective projects but at the same time they use epics or labels for a whole department, I can imagine this may cause a lot of pain, as users couldn't suddenly see these resources. I wonder if we can figure out an alternative solution for such use-case?
Even with these downsides I would prefer to rather make this change but its impact is quite big so it depends if it's acceptable by PM.
I was going to mention the above implications as well. I would like it from engineering perspective, makes things much clearer, and perhaps easier to understand for the users, but I am not sure we can actually do this, given the footprint of the breaking change.
Even if we limit access of the user according to membership, we still need to give a bit of extra, right ? E.g. in a GroupA -> GroupB -> ProjectC hierarchy, user with membership only in ProjectC would still need to be able to at least read paths of the GroupB and GroupA to see the breadcrums, or do we show no breadcrumbs? If user can see the breadcrumbs, do we let the user also see the tree structure of GroupA -> GroupB -> ProjectC, for instance let user navigate to the GroupA page and show the structure of the groups and projects they are part of?
Is this issue to discuss how we fix the specific case of Inconsistency in group / project member access to parent group or permissions in general? IMHO the fix with the smallest footprint for the Inconsistency in group / project member access to parent group would be to give User:Bob member of Subgroup read permission in Group.
users wouldn't be able to assign some labels, milestones, epics... (these from ancestor groups if they don't have access to it) to issues/MRs and other resources.
@jprovaznik, there's a caveat to that: if there's no project in the group hierarchy, the user does not have access to eg. epics in the ancestors groups.
user with membership only in ProjectC would still need to be able to at least read paths of the GroupB and GroupA to see the breadcrums
@acroitor, that's correct and the behaviour is consistent even in today. Project and group members see the full path and the breadcrumbs are also displayed correctly. The difference comes from:
let user navigate to the GroupA page and show the structure of the groups and projects they are part of
This is only possible when there's a project in the hierarhcy. Otherwise members are not able to navigate to the groups page.
Is this issue to discuss how we fix the specific case of Inconsistency in group / project member access to parent group or permissions in general?
My intent was to discuss this specific exception and I plan to create a separate issue for discussion group-level resource access in general.
@ifarkas@jprovaznik@acroitor From my vantage point, it's hard for me to decouple this proposal from the "group-level resource access in general" given what I discussed here -- #338819 (comment 667830288). If we solve that problem for allowing parents to "selectively disclose resources to children such that they are viewable within the child's context", then preventing a member of a child namespace (gitlab.com/parent/child) from clicking into a parent namespace (gitlab.com/parent) seems completely logical, but as @mushakov mentioned, it would, unfortunately, need to be treated as a breaking change.
it's hard for me to decouple this proposal from the "group-level resource access in general"
@gweaver, I think the level of coupling depends on the direction we take in each question. For example, considering the parent group access inconsistency, a solution could be to revoke the generic group access, but grant access to a few types of resources of the parent (like epics, milestones, labels). This way, we can avoid the implications @jprovaznik and @acroitor mentioned.
On the other hand, if we decide that all resources need to flow down, it might not make sense to decouple the generic group access permission from the read inheritable resources of the parent group permission. A group (in some sense) is the collection of its resources.
However, if this is the case, I am confused by the concept of membership inheritance. It seems users have the same access down the hierarchy as upward. But then why are "inherited" members only listed in descendants?
For example, considering the parent group access inconsistency, a solution could be to revoke the generic group access, but grant access to a few types of resources of the parent (like epics, milestones, labels).
@ifarkas this is a possible solution, and perhaps most feasible, though as you pointed out:
On the other hand, if we decide that all resources need to flow down, it might not make sense to decouple the generic group access permission from the read inheritable resources of the parent group permission. A group (in some sense) is the collection of its resources.
Yes, if we distinguish between inheritable and not-inheritable resources, what would be left in not-inheritable resources? If this set is "big enough", I think this might be a good solution.
However, if this is the case, I am confused by the concept of membership inheritance. It seems users have the same access down the hierarchy as upward. But then why are "inherited" members only listed in descendants?
only nit: It's not exactly true users have same access up/down. Users have only "guest" access upward, not matter what access they have downward - IOW they still can't see confidential issues/epics for example or manage resources in upward groups. This can be viewed as providing "necessary minimum" access. Not that I would agree this, just playing devil's advocate, I would still prefer to provide more restricted/selective access upward if necessary.
@jprovaznik@ifarkas The bit that confuses me about access back up the hierarchy is that you can't modify an epic right? But, you can modify the contents of the epic by adding/removing issues to/from the epic, which is effectively modifying the epic?
E.g. as a child, I don't have access to my parents bank account, but I can debit and credit the account.
I understand this may be questionable what permissions should be required when assigning an issue to an epic. Personally I agree with consensus there that it's less confusing to require just read_epic. For assigning e.g. milestone we don't require milestone admin permission either just to be able to use it. But I understand your point.
I guess we can break down permissions in our app into several classes:
CRUD permissions on the object itself.
Permissions on object's attributes(in bulk), I don't think we have per attribute permissions granularity and not sure we want it either But we do have examples where we have different permissions between some attributes for same user.
Permissions on object's relationships. Can an object be related to another object in general and by a given user in particular. This seems similar to permissions on attributes, but I think it is different because the permissions on the relationship also needs to be accounted for. It is a bi-directional permissions relation. Taking the milestone <-> issue example above(may not be the best one for this example ), some user may have permission to relate an issue to a milestone, however milestone permission also dictates if that specific milestone can be set.
Would be great to step back and think of how we can integrate these into a cohesive permission framework/engine as right now I don't think there is an easy way to get an overview of the permissions we have.
Does it make sense to think of a more explicit way to express permissions, with explicit defaults, that can be changed vs implicit behaviours(e.g. if one has read access they can assign object to another object and this cannot be changed). At the same time the more granularity and control, the more complex and complicated it becomes for the end user and also scaling a very granular permissions system can be a big problem.
@jprovaznik, not sure about that... For example, a guest has access to all projects within that group, which is not true in this case. This is still the most confusing part for me. I am not sure we can ultimately pinpoint a membership level.
Personally, I find this model more intuitive. This is also more in line with revoking read_group and granting a read_inheritable_resources which they can use in the scope of their actual membership.
Also, maybe because of the lack of clear definition, the code for querying authorized groups / projects inconsistently applies this rule. In Manage, we saw security issues because of that like https://gitlab.com/gitlab-org/gitlab/-/issues/238112.
E.g. as a child, I don't have access to my parents bank account, but I can debit and credit the account.
@alexpooley, I would prefer to drop this concept, but apparently this is a product requirement for many ( / all? ) group-level resources.
But we do have examples where we have different permissions between some attributes for same user.
@acroitor, That's interesting! Can you share some examples?
I am aware of one example, with issues. A non-member can set title, description, issue type and confidential flag when issue is created but not other attributes. More so when the same user tries to edit the issue they just created they'd only be able to change title and description and not confidentiality flag and not the issue type.
not sure about that... For example, a guest has access to all projects within that group, which is not true in this case. This is still the most confusing part for me. I am not sure we can ultimately pinpoint a membership level.
@ifarkas yes, it's not that user would have regular guest membership in ancestor groups, what I meant was that in ancestor groups user doesn't have the same level of access as when going down in hierarchy (#340421 (comment 674785207)). Thus I quoted "guest", as guest permissions are similarly limited but I see that was rather misleading.
It's quite surprising there is no clear mention about this behavior in the membership documentation. It would be cool to add more info about this - apparently this behavior is around for a long time (37c40143 adds rule { has_projects } .enable :read_group but it was only refactoring - so the behavior will be even older)
Can Alex add/remove issues from Epic ?
@alexpooley no - at the moment admin_epic permission on Epic is checked when adding/removing issues from an epic (#208425 (comment 489494307)). Note that when/if this will be changed to check only admin_issue, the answer will be yes ;).
As Guest, I was only added as a direct member with the Guest role to Project, I can navigate to any parent Group and see a list of the non-confidential epics there. I can open any epic and view the details. I can comment on the epic. If I update Guest to Reporter, I can click edit on an issue in Project and see a list of all epics from parents in the dropdown but I get an error when attempting to actually set the epic.
E.g. as a child, I don't have access to my parents bank account, but I can debit and credit the account.
Current behavior: As a child, I can see my parent's bank account and all transactions, but I cannot debit or credit the account.
If you must be Reporter+ in both Parent and Child in order to make an association, it greatly restricts the idea of "least privileged access". On the other end of the spectrum, if fewer people are members of both, then it is more of a burden on the few that are in order to set up the relationships.
Desired behavior:
As a child, I can see my parent's bank account numbers and deposit money from my bank account into one of theirs. I can also withdraw the money I deposited later, but I can't take more money out than I put in.
As a parent, I want to selectively decide which bank accounts my children can deposit money into.
As a child, I want to hide my bank accounts from parents with the role of Guest, because they aren't family members.
If I update Guest to Reporter, I can click edit on an issue in Project and see a list of all epics from parents in the dropdown but I get an error when attempting to actually set the epic.
If I update to Developer, can I set the epic on the issue? This is what I was getting at with my previous query.
@alexpooley I believe you only need to be a reporter to do this. In your previous query, I believe Alex can only remove issues from the epic via the issue UI, but I could be wrong?
@dzaporozhets, the historical context of this feature might be beneficial here. Accessing ancestor groups when membership inheritance only flows down the hierarchy is controversial. A few issues we discussed so far:
inconsistently implemented between groups and projects (as described in the description).
difference between having direct or inherited membership vs having access to ancestors is not obvious to users. They are confused why they are not authorized to do some actions when they don't have membership.
we couldn't find this mentioned in the documentation.
confusing what counts as authorized group or projects for a user. Some of our code considers only membership, while others include ancestor access too. This leads to various issues like https://gitlab.com/gitlab-org/gitlab/-/issues/238112.
hard to settle with any access level. Even the lowest GUESTS access level is too broad for the permissions given when accessing ancestors.
Do you have any thoughts on how to reconcile this feature?
Let's go this way. Its more secure and transparent. In the end we consolidate everything under namespace and this is existing behavior for group/subgroup.
the historical context of this feature might be beneficial here
Probably it was some "temporary" hack. Anyway its good opportunity to solve it now.
difference between having direct or inherited membership vs having access to ancestors is not obvious to users.
A large GitLab Ultimatecustomerreports internally that group members can no longer see the pages of applications that had been added to the group, after their upgrade from 14.9.2 to 15.0.5. Is that an unexpected side effect of the related changes here, or also part of the new, intended behaviour?
I'm tending towards a new issue like Allow read-only access to group settings for non-owners. WDYT, @lohrc?
Update 2022-10-07 (after this): The customer confirmed in the Rails console that their user account has max_member_access_for_group: 40 in both test instances. On %14.9 they can access /-/settings/applications, but in %15.0 they get 404. To me, this means they should never have had access, but due to another (as of yet unknown) problem before %15.0, they accidentally had.
@lohrc It seems like the smaller (?) problem of this being scoped to what is shown on the application page would be long to ~"group::authentication and authorization" , but if this is about larger inconsistencies between access to parent groups, I think that is ~"group::workspace"
Hi, I am a PM at Jihu. We want to contribute to a case for this issue, when the user only has access to subgroups, but not to the parent group, and there is no project in the subgroup; at this time the group's dashboard shows the parent group and returns 404 when accessing the group. But the parent group is accessible when the child group has projects
We think that in this case, the presentation of the parent group should be consistent, whether the child group has projects or not.
subgroup member's dashboard
When clicking "top-level group" should be able to see this
We found that the page of the parent group is determined by the following conditions:
Whether there are projects for the user under the group
Whether the user has permission to access the group
We want to add a determination
whether there are subgroups under the group that the user can access
If there are subgroups that can be accessed, then show the parent group, the same as if there were items in the subgroup.
when the user only has access to subgroups, but not to the parent group, and there is no project in the subgroup; at this time the group's dashboard shows the parent group and returns 404 when accessing the group.
...
If there are subgroups that can be accessed, then show the parent group, the same as if there were items in the subgroup.
Is it acceptable to add conditions like this?
@QiangGu0 This is a tricky one. Maybe @mnichols1 can help guide us a bit? There's two issue to me:
Can you navigate to a group/project when you don't really have access.
Can you see siblings/cousins in the tree.
I don't think we should allow (1). It's much cleaner to have a hard boundary between whether that entity is viewable or not. Given that we're dealing with fundamental security here I would favor a clean access mechanism.
The issue of (2) seems like a UI issue. It makes sense to make navigation easier. It does not make sense to me to have to visit a whole new page just to navigate the tree.
For reference, we've had similar discussions before which might be useful at some point here.
@alexpooley Thanks for the reply, permission related issues are tricky. The problem we want to deal with can be found here.
We believe that when a user in a subgroup when there is a project, can access the parent group. Then when there is no project, this behavior should be consistent as well. Otherwise, some confusion will arise.
@mnichols1 I think this example here is a case where we might need to distinguish between Access and Visibility. Currently we are binding Visibility to Access, it seems, so I think there are two options here:
Detach Visibility from Access in the way that everyone would have minimal access to be able to navigate the tree. In this case Visibility would then correspond to minimal (read only) access and Access would correspond to inherited membership. I'm afraid this would complicate the model again that we are trying to simplify.
Merge Visibility and Access so that you don't get visibility to things you don't have access to.
@ifarkas Do you have some background on why it was decided to give project members access to the ancestor groups, but group members would not get access to their ancestor groups?
Do you have some background on why it was decided to give project members access to the ancestor groups, but group members would not get access to their ancestor groups?
@ifarkas@alexpooley@manojmj Wow, this problem is going to school already . I have read through the discussions on the other issues, and I think we have two options:
Align towards the current behaviour for project members and give group members the same kind of visibility into ancestor groups.
Align towards the current behaviour for group members and restrict the visibility of project members more.
It seems that the latter would cause some problems for Plan objects. The root of the problem seems to be that we think of access cascading downwards in the same way as access rolling up. Could there be a solution that considers the direction? For instance, cascading down, you have access, but rolling up you only have visibility (I am aware that the difference might be difficult to understand for users).
I have been asking myself why a user should not be able to see the groups in their organizational hierarchy that are accessible to them? Why would we not allow them to discover public or internal groups easier? Private groups could be hidden. I just checked on Slack with a quick search over all the channels that exist, and private channels do not appear in the search results unless I am a member of the private channel. I would like to understand the difference better between access via inheritance and access (upwards) via project membership. What is unique for each of these types?
Currently, project members have implicit direct membership in the parent group. They can access it and have limited permissions.
The behaviour for group members is different: we don't grant access to the parent group.
from the description. For me it's clear that access to a Project should not provide access to it's parent group. This is especially true with our recent group+project consolidation move where every Project is now backed by a ProjectNamespace.
Align towards the current behaviour for project members and give group members the same kind of visibility into ancestor groups.
This would break the permission model. When we go down the hierarchy the user picks up the highest role (reporter, developer, maintainer, etc). What role does a user receive when they go up the hierarchy?
I also think the silo'ed aspect of nested groups has been a deep part of our system for too long and will be depended upon by too many customers. But that's not my domain so I'll steer clear
Align towards the current behaviour for group members and restrict the visibility of project members more.
I have been advocating for the removal of this exceptional rule for a long time now. It's only used in rare circumstances, it's an obvious rough edge in our access check code, and it's applied inconsistently. I think this behavior will be relied on less by customers right now because it's completely unexpected behavior.
What role does a user receive when they go up the hierarchy?
@lohrc, this is causing a lot of confusion. The effective role of a user determines the available actions. However, we are not able to determine that role. Despite that, we do grant some undefined access.
It seems that the latter would cause some problems for Plan objects
That's correct. But the problem is that access to these Plan objects only works if there's a project in the hierarchy. Access is broken without a project (see the example in the issue description).
I think it's a valid use case to access certain types of objects of the parent namespaces (eg. epics, or labels). But this doesn't mean we need to give blank access to the namespace itself. We only need access to specific objects.
I have been advocating for the removal of this exceptional rule for a long time now.
@ifarkas@alexpooley OK, that sounds like there is consensus to opt for aligning the behaviour with group members and restrict the visibility of projects more. Could you help me break down the work to accomplish this? I would like to see whether it is possible to work towards %16.0 to implement this change.
@QiangGu0 This means that it will be our team taking on the work on this item.
@lohrc I'm supportive of whatever direction ya'll go here but do know that changing the existing behavior, while necessary for consistency, will likely be seen as a "breaking change" and we should treat it accordingly so we have adequate time to gather feedback from the use cases that no longer work and hopefully figure out alternative solutions for them.
@lohrc maybe following the guidance here -- https://docs.gitlab.com/ee/development/deprecation_guidelines/ -- announcing it ASAP with a link to the breaking change proposal, including instructions for folks on how to jump into the conversation to share their concerns ahead of time. I would give enough lead time for the breaking change announcement to be included in multiple release posts at a minimum.
I think it would also be helpful to document what will no longer work depending on which path we go (ex: a project member who is not a member of the parent group can no longer view boards at the group level...or whatever the ramifications will be).
I would also like to understand how this will work...
I think it's a valid use case to access certain types of objects of the parent namespaces (eg. epics, or labels). But this doesn't mean we need to give blank access to the namespace itself. We only need access to specific objects.
...within the context of epics, milestones, labels, and so on. If I do not have access to the parent group, what is the mechanism and behavior of having access to certain items that belong to the group?
Could you help me break down the work to accomplish this? I would like to see whether it is possible to work towards %16.0 to implement this change.
@lohrc, besides it's probably late for the deprecation announcement, this requires a substantial amount of work. We could start with a POC to remove the has_parent condition from GroupPolicy to better understand which objects access need to be fixed. This might also require UX involvement to discuss how the access should be fixed.
@ifarkas I was already expecting this to become an epic eventually The approach via the POC sounds good. I will promote this issue to an epic and create a separate issue for the assessment.
@ifarkas There is no mention of this in the issue description, so I wanted to check: the problem described here is specific to private and internal groups, isn't it? I don't see a reason why this would happen for public groups.
@lohrc, that's correct, it doesn't happen for public groups. However, it's not because the has_parent policy works correctly for public groups, but rather because the read_group permission is always granted for this visibility level.
Christina Lohrchanged title from Inconsistency in group / project member access to parent group to Inconsistency in group/project member access to parent group
changed title from Inconsistency in group / project member access to parent group to Inconsistency in group/project member access to parent group