Custom abilities for group memberships
What does this MR do and why?
- This enables
read_vulnerability
andadmin_vulnerability
for groups when a custom role exists on the group membership. - #411851 (closed)
Database
See related MR where preloader was added for project custom roles: !100466 (merged)
SQL query
SQL generated by preloader:
::Preloaders::UserMemberRolesInGroupsPreloader.new(
groups: groups,
user: User.first
).execute
And then using some group IDs from production (test groups with custom roles and the gitlab-org
group and my prod User ID)
SELECT
namespace_ids.namespace_id,
Bool_or(custom_permissions.read_vulnerability) AS read_vulnerability,
Bool_or(custom_permissions.admin_vulnerability) AS admin_vulnerability
FROM (
VALUES (60357594, ARRAY[60357594]::integer[]),
(65094811, ARRAY[65094811]::integer[]),
(60357923, ARRAY[60357923]::integer[]),
(9970, ARRAY[9970]::integer[])) AS namespace_ids (namespace_id, namespace_ids),
LATERAL ((
SELECT
read_vulnerability, admin_vulnerability
FROM
"members"
LEFT OUTER JOIN "member_roles" ON "member_roles"."id" = "members"."member_role_id"
WHERE (members.source_type = 'Namespace'
AND members.source_id = namespace_ids.namespace_id)
AND "members"."user_id" = 11997412
AND (member_roles.read_vulnerability = TRUE
OR member_roles.admin_vulnerability = TRUE)
LIMIT 1)
UNION ALL (
SELECT
read_vulnerability,
admin_vulnerability
FROM
"members"
LEFT OUTER JOIN "member_roles" ON "member_roles"."id" = "members"."member_role_id"
WHERE (members.source_type = 'Namespace'
AND members.source_id IN (
SELECT
unnest(namespace_ids) AS ids))
AND "members"."user_id" = 11997412
AND (member_roles.read_vulnerability = TRUE
OR member_roles.admin_vulnerability = TRUE)
LIMIT 1)
UNION ALL (
SELECT
FALSE AS read_vulnerability,
FALSE AS admin_vulnerability)
LIMIT 1) AS custom_permissions
GROUP BY
namespace_ids.namespace_id;
Explain plan for query
https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/21161/commands/69050
Screenshots or screen recordings
Guest viewing group security dashboard in "Before"
Custom Guest with admin_vulnerability: true
and read_vulnerability: true
viewing group security dashboard in "After"
Before | After |
---|---|
![]() |
![]() |
How to set up and validate locally
(These instructions assume you are running GDK with an Ultimate license. It uses the seeded flightjs
group but you can use another group if you like)
- As instance admin, make group Ultimate by visiting https://gdk.test:3443/admin/groups/flightjs as an admin
- As instance admin or group owner, add another user as a regular guest https://gdk.test:3443/groups/flightjs/-/group_members
Confirm "before" behavior:
- Log in as guest
- Visit https://gdk.test:3443/groups/flightjs/-/security/dashboard
- There should be nothing visible
Create "after" behavior:
- In a rails console, make the custom role and assign it to the guest user you just added:
Feature.enable(:custom_roles_on_groups) group = Group.find_by_name("Flightjs") MemberRole.create!(read_vulnerability: true, namespace: group, base_access_level: Gitlab::Access::GUEST) m = Member.last m.update!(member_role: MemberRole.last)
- Custom guest can now view https://gdk.test:3443/groups/flightjs/-/security/vulnerabilities and it shows vulnerabilities
- In a rails console, update the custom role so that it also enables the custom guest to admin vulnerabilities (usually custom roles cannot be updated after they have been assigned so we need to skip validations here)
mr = MemberRole.last mr.admin_vulnerability = true mr.save!(validate: false)
- Custom guest can now view https://gdk.test:3443/groups/flightjs/-/security/vulnerabilities and it shows vulnerabilities checkboxes to allow management of vulnerabilities.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %16.2
assigned to @jessieay
3 Warnings This merge request is quite big (544 lines changed), please consider splitting it into multiple merge requests. e29c231b: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. Please add a merge request subtype to this merge request. 1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Michael Becker (
@wandering_person
) (UTC-7, 15 hours behind@jessieay
)Felipe Artur (
@felipe_artur
) (UTC-3, 11 hours behind@jessieay
)database Vitali Tatarintev (
@ck3g
) (UTC+2, 6 hours behind@jessieay
)Leonardo da Rosa (
@l.rosa
) (UTC-3, 11 hours behind@jessieay
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for e29c231bexpand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Verify | 8 | 0 | 0 | 2 | 8 | ❗ | | Govern | 34 | 0 | 0 | 7 | 34 | ❗ | | Manage | 13 | 0 | 1 | 12 | 14 | ❗ | | Create | 38 | 0 | 0 | 10 | 38 | ❗ | | Data Stores | 20 | 0 | 0 | 8 | 20 | ❗ | | Plan | 47 | 0 | 0 | 6 | 47 | ❗ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 160 | 0 | 1 | 45 | 161 | ❗ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for e29c231bexpand test summary
+-------------------------------------------------------------+ | suites summary | +--------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +--------+--------+--------+---------+-------+-------+--------+ | Manage | 66 | 0 | 6 | 6 | 72 | ❗ | | Create | 0 | 0 | 10 | 0 | 10 | ➖ | +--------+--------+--------+---------+-------+-------+--------+ | Total | 66 | 0 | 16 | 6 | 82 | ❗ | +--------+--------+--------+---------+-------+-------+--------+
- A deleted user
added feature flag label
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@5ef33517
mentioned in issue #388156 (closed)
@jessieay, please can you answer the question: Should this have a feature flag? to help with code review for the Authentication and Authorization group.This nudge was added by this triage-ops policy.
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@9bead28f
added 1 commit
- 2a68b43f - Ensure that member role can only be created on root group
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@4c324952
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
- Resolved by Jessie Young
- Resolved by Jessie Young
- Resolved by Jessie Young
- Resolved by Jessie Young
Hi @tachyons-gitlab - can you review this for ~"group::authentication and authorization" ?