Fix project member access for group links
What does this MR do?
Among other things, ensure that users who have access to a project through a group link can see confidential issues.
Are there points in the code the reviewer needs to double check?
I tried to keep the change as minimal as possible.
Why was this MR needed?
ProjectTeam#find_member doesn't take group links into account. It was
used in two places:
- An admin view - it can stay here.
-
ProjectTeam#member?, which is often used to decide if a user has access to view something.
This second part broke confidential issues viewing. IssuesFinder ends
up delegating to Project#authorized_for_user?, which does consider
group links, so users with access to the project via a group link could
see confidential issues on the index page. However, IssuesPolicy used
ProjectTeam#member?, so the same user couldn't view the issue when
going to it directly.
Does this MR meet the acceptance criteria?
- Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #23872 (closed).
Merge request reports
Activity
/cc @stanhu @yorickpeterse @eReGeBe - it looks like we have at least two optimised project access lookup methods:
It looks like the latter has slightly more efficient queries, but may have a bug (I haven't checked) if the user's access level is higher than the maximum access level for the group link. Should we create a TD issue to just pick one, make sure that works, and use that one?
Edited by Sean McGivernMentioned in issue #23101 (closed)
@smcgivern I'm not sure if I fully understand your question. Do you mean we should deprecate
Project#authorized_for_user?? If so, we can do that given there's an adequate replacement that does not need more queries thanProject#authorized_for_user?does.-
@yorickpeterse I think we should either:
- Replace all instances of
Project#authorized_for_user?withProjectTeam#member?. - Replace all instances of
ProjectTeam#member?withProject#authorized_for_user?.
This is under the assumption that both are intended to do the same thing, which it looks like they are. We should pick the faster implementation (or the fastest parts of each implementation), and just have that.
- Replace all instances of
-
@smcgivern
Project#authorized_for_user?is only used in a single place (app/models/issue.rb line 97), so I think it makes more sense to move that intoProjectTeamand optimize it. 
-
@smcgivern Looks awesome
❤ ! I just left a few comments related to tests. Reassigned to @smcgivern
125 125 max_member_access(user.id) == Gitlab::Access::MASTER 126 126 end 127 127 128 def member?(user, min_member_access = nil) 129 member = !!find_member(user.id) 130 131 if min_member_access 132 member && max_member_access(user.id) >= min_member_access 133 else 134 member 135 end 128 def member?(user, min_member_access = Gitlab::Access::GUEST) 129 max_member_access(user.id) >= min_member_access Added 24 commits:
-
a99aab4d...20a7db44 - 23 commits from branch
master - db9979bc - Fix project member access for group links
-
a99aab4d...20a7db44 - 23 commits from branch
-
@grzesiek changes made! I've created https://gitlab.com/gitlab-org/gitlab-ce/issues/23938 for the discussion in the first couple of comments.
Reassigned to @grzesiek
-
@smcgivern Thanks! LGTM
👍 Reassigned to @smcgivern
Reassigned to @rymai
Thanks @smcgivern! This looks very good, thanks for the tests and the followup (#23938 (closed)) but I don't see any tests for members via group links? We should probably test that since that was the original issue?
-
@rymai haha whoops! Yes, of course, I will add those now.
Reassigned to @smcgivern
-
@rymai added some!
Reassigned to @rymai
-
@rymai added some!
@smcgivern Perfect, thanks!
❤ Mentioned in commit 5742f4a6
Mentioned in issue #23827 (closed)
Mentioned in commit e35ecccb
