Honour issue and merge request visibility in their respective finders
This MR fixes a security issue with the IssuesFinder and MergeRequestFinder where they would return items the user did not have permission to see. This was most visible on the issue and merge requests page for a group containing projects that had set their issues or merge requests to "private".
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/22481
See merge request !2000
- Group / project members cannot request access
- Group members cannot request access to a group's project
This addresses an issue where project owners could request access
to their own project, leading to UI inconsistency where their requester
status would replace their owner status.
Signed-off-by: Rémy Coutable <remy@rymai.me>
Exclude requesters from Project#members, Group#members and User#members
## What does this MR do?
It excludes requesters from the `Project#members`, `Group#members` and `User#members` associations, and adds new `Project#requesters` and `Group#requesters` associations.
## Are there points in the code the reviewer needs to double check?
No.
## Why was this MR needed?
Without this, if you call `project.members`, requesters are included in the results! This is at best misleading, and at worst can lead to security issues. By excluding requesters from the `#members` associations, we avoid introducing security inadvertently since you have to call the `#requesters` association explicitly to get requesters.
## What are the relevant issue numbers?
This is something I realized while fixing the security issue #19102.
## Does this MR meet the acceptance criteria?
- [x] I don't think this needs a CHANGELOG since this is an internal change
- Tests
- [x] Added for this feature/bug
- [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4946
The issue was with the `User#groups` and `User#projects` associations
which goes through the `User#group_members` and `User#project_members`.
Initially I chose to use a secure approach by storing the requester's
user ID in `Member#created_by_id` instead of `Member#user_id` because I
was aware that there was a security risk since I didn't know the
codebase well enough.
Then during the review, we decided to change that and directly store the
requester's user ID into `Member#user_id` (for the sake of simplifying
the code I believe), meaning that every `group_members` / `project_members`
association would include the requesters by default...
My bad for not checking that all the `group_members` / `project_members`
associations and the ones that go through them (e.g. `Group#users` and
`Project#users`) were made safe with the `where(requested_at: nil)` /
`where(members: { requested_at: nil })` scopes.
Now they are all secure.
Signed-off-by: Rémy Coutable <remy@rymai.me>
Display group/project access requesters separately in admin
## What does this MR do?
It displays the access requesters in a separate list in group & project members pages.
It also harmonize the members counter UI to use `%span.badge` everywhere (in the admin & non-admin members views).
## Are there points in the code the reviewer needs to double check?
No.
## Why was this MR needed?
To not confuse access requesters with actual members.
## What are the relevant issue numbers?
Closes#18871.
## Screenshots
### Group members
| Before | After |
| --------- | ---- |
|  |  |
### Project members
| Before | After |
| --------- | ---- |
|  |  |
### Admin group members
| Before | After |
| --------- | ---- |
|  |  |
### Admin project members
| Before | After |
| --------- | ---- |
|  |  |
## Does this MR meet the acceptance criteria?
- [x] No CHANGELOG since this is related to the original "request access" MR.
- [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4798
+ Move 'Edit Project/Group' out of membership-related partial
+ Show the access request buttons only to logged-in users
+ Put the request access buttons out of in a more visible button
+ Improve the copy in the #remove_member_message helper
Signed-off-by: Rémy Coutable <remy@rymai.me>
Douwe Maan