Allow top-level group owners to create SA
What does this MR do and why?
This MR implements the Setting to allow group owners to create Service Accounts on Self-Managed, so that top-level group owners can create Service Accounts on SM instances. A new Application Setting ie Admin Setting is created called "Allow top-level group owners to create Service Accounts" which is by default false. When activated, top-level group owners can create Service Accounts under Service Account Creation endpoint in Groups API and also delete Service Accounts that were created under same Group Endpoint.
With that the same settings also gets introduced in GitLab.com. Moving forward GitLab.com will have "Allow top-level group owners to create Service Accounts" setting by default enabled, whilist Self-Managed will have this setting by default disabled and give the admins the chance to decide. As a result a feature flag to derisk the change in GitLab.com gets introduced. Upon activation of the FF, application setting will take effect in GitLab.com, which would enable to cleanup later special GitLab.com checks in code together with the FF
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
-
Documentation Update MR to follow
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
Screen_Recording_2024-08-26_at_08.48.05 |
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
You need an admin user and a non-admin user to test.
- In a self-managed instance, add a non-admin user as owner to a top-level group
- Create a personal access token for the given owner user
- Try creating a service account with access token of user using the groups endpoint and confirm it returns
400 Bad request
. Here is the curl command and response example.
curl --request POST --header "PRIVATE-TOKEN: $TOKEN" "http://$GITLAB_URL/api/v4/groups/$GROUP_ID/service_accounts"
{"message":"400 Bad request - User does not have permission to create a service account in this namespace."
- With an admin user go to Settings -> General -> Account and Limit -> Service Account creation and check
Allow top-level group owners to create Service accounts.
and pressSave changes
. Alternatively update the application settings with an admin token like
curl --request PUT --header "PRIVATE-TOKEN: $ADMIN_TOKEN" "http://$GITLAB_URL/api/v4/application/settings?allow_top_level_group_owners_to_create_service_accounts=true"
- Try recreating a service account with same operation and access token from step 3 and verify that it works
curl --request POST --header "PRIVATE-TOKEN: $TOKEN"" "http://$GITLAB_URL/api/v4/groups/$GROUP_ID/service_accounts"
{"id":72,"username":"service_account_group_33_556ae685b8ecb97bf23fcc8155cb7697","name":"Service account user"}
Related Issues
Feature Rollout Issue for GitLab.com Derisk
Merge request reports
Activity
assigned to @eakca1
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels
- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@eakca1
- please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
- A deleted user
added backend database databasereview pending frontend labels
3 Warnings This merge request is quite big (937 lines changed), please consider splitting it into multiple merge requests. 1e324eb0: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 75ad88f0: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
-
doc/administration/settings/account_and_limit_settings.md
(Link to current live version) -
doc/security/hardening_nist_800_53.md
(Link to current live version) -
doc/user/profile/service_accounts.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Category Reviewer Maintainer backend @jwanjohi
(UTC+1)
@alipniagov
(UTC+2)
database @alexpooley
(UTC+8)
@Quintasan
(UTC+2)
frontend @nradina
(UTC+2)
@shampton
(UTC-7)
groupauthorization Reviewer review is optional for groupauthorization @alexbuijs
(UTC+2)
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger-
added typefeature label
added groupauthentication label
added devopsgovern sectionsec labels
added 1466 commits
-
b517bcdc...534fde78 - 1465 commits from branch
master
- 5247b11f - Rebase db migration
-
b517bcdc...534fde78 - 1465 commits from branch
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 82d02ed3expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Create | 34 | 0 | 2 | 0 | 36 | ✅ | | Plan | 38 | 0 | 0 | 0 | 38 | ✅ | | Govern | 32 | 0 | 0 | 0 | 32 | ✅ | | Data Stores | 2 | 0 | 0 | 0 | 2 | ✅ | | Verify | 4 | 0 | 0 | 0 | 4 | ✅ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Secure | 4 | 0 | 0 | 0 | 4 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 118 | 0 | 2 | 0 | 120 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for 5247b11fexpand test summary
+-------------------------------------------------------------+ | suites summary | +--------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +--------+--------+--------+---------+-------+-------+--------+ | Govern | 107 | 0 | 6 | 0 | 113 | ✅ | | Create | 270 | 0 | 34 | 0 | 304 | ✅ | +--------+--------+--------+---------+-------+-------+--------+ | Total | 377 | 0 | 40 | 0 | 417 | ✅ | +--------+--------+--------+---------+-------+-------+--------+
removed pipeline:run-e2e-omnibus-once label
added 1 commit
- 5cf4dcea - Allow top level group owners to create SA in SM
added 1 commit
- 9653cc25 - Allow top level group owners to create SA in SM
added 1 commit
- 99bc283c - Allow top level group owners to create SA in SM
added 1 commit
- b8350f3a - Allow top level group owners to create SA in SM
added 176 commits
-
b8350f3a...94f71cc5 - 175 commits from branch
master
- 124fb5a3 - Allow top level group owners to create SA in SM
-
b8350f3a...94f71cc5 - 175 commits from branch
Database migrations (on the main database)
Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).
Migration Type Total runtime Result DB size change 20240816061320 - AllowTopLevelGroupOwnersToCreateServiceAccounts Regular 5.2 s +0.00 B Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 4 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20240816061320 - AllowTopLevelGroupOwnersToCreateServiceAccounts
- Type: Regular
- Duration: 5.2 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 10.5 ms 10.5 ms 10.5 ms 0 ALTER TABLE "application_settings" ADD "allow_top_level_group_owners_to_create_service_accounts" boolean DEFAULT FALSE NOT NULL
1 1.5 ms 1.5 ms 1.5 ms 1 SELECT "feature_gates"."key", "feature_gates"."value" FROM "feature_gates" WHERE "feature_gates"."feature_key" = $1
1 0.0 ms 0.0 ms 0.0 ms 1 SELECT $1::regtype::oid
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AllowTopLevelGroupOwnersToCreateServiceAccounts
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 4 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Other information
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change 20240819123915 - AddIndexVulnerabilityOccurrencesToSupportSbomServices Post deploy 1030.7 s +1.41 GiB 20240820085247 - BackfillCatalogResourceVersionsPublishedById Post deploy 22.1 s +2.14 MiB Clone details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-3639655-15129074-main
2024-08-26T07:10:01Z 2024-08-26T04:09:55Z 2024-08-26 19:36:26 +0000 database-testing-3639655-15129074-ci
2024-08-26T07:10:01Z 2024-08-26T04:45:10Z 2024-08-26 19:36:26 +0000 Database migrations (on the ci database)
Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).
Migration Type Total runtime Result DB size change 20240816061320 - AllowTopLevelGroupOwnersToCreateServiceAccounts Regular 7.0 s +0.00 B Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 3 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20240816061320 - AllowTopLevelGroupOwnersToCreateServiceAccounts
- Type: Regular
- Duration: 7.0 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 14.2 ms 14.2 ms 14.2 ms 0 ALTER TABLE "application_settings" ADD "allow_top_level_group_owners_to_create_service_accounts" boolean DEFAULT FALSE NOT NULL
1 0.0 ms 0.0 ms 0.0 ms 1 SELECT $1::regtype::oid
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AllowTopLevelGroupOwnersToCreateServiceAccounts
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 3 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Other information
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change 20240819123915 - AddIndexVulnerabilityOccurrencesToSupportSbomServices Post deploy 8.0 s +8.00 KiB [note] 20240820085247 - BackfillCatalogResourceVersionsPublishedById Post deploy 6.0 s +0.00 B Clone details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-3639655-15129074-main
2024-08-26T07:10:01Z 2024-08-26T04:09:55Z 2024-08-26 19:36:26 +0000 database-testing-3639655-15129074-ci
2024-08-26T07:10:01Z 2024-08-26T04:45:10Z 2024-08-26 19:36:26 +0000
Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic
Edited by ****added database-testing-automation label
added 1 commit
- 3c0a7e56 - Allow top level group owners to create SA in SM
- Resolved by Eren Akca
Hi @adil.farrukh @hsutor ,
Here is the MR for mentioned setting to control service account creation on self-managed for top-level group owners. It would be great if your team can have a look and let me know, if this goes in the right direction, before I ask for additional reviews. Thanks!
added 1 commit
- 6cf5f1b7 - Allow top level group owners to create Service Accounts in Self-Managed
added 1 commit
- 84dd690c - Allow top level group owners to create Service Accounts in Self-Managed
changed milestone to %17.4
requested review from @theoretick, @bwill, and @jrushford
- Resolved by Adil Farrukh
@theoretick Could you please review this MR for backend review? Thanks so much!
- Resolved by Eren Akca
@jrushford Could you please review this MR for frontend review? Added a new checkbox in Admin UI under Settings -> General -> Account and Limit. Thanks so much!
Edited by Eren Akca
mentioned in merge request !164036 (merged)
- Resolved by Eren Akca
added databasereviewed label and removed databasereview pending label
removed review request for @bwill
added pipeline:mr-approved label
added 1 commit
- 31cc7737 - Remove boilerplate comments to keep the migrations lean
reset approvals from @bwill by pushing to the branch
requested review from @psimyn and removed review request for @jrushford
requested review from @jarka
- Resolved by Jarka Košanová
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
requested review from @rshambhuni and @vshushlin and removed review request for @theoretick
- Resolved by Eren Akca
added databaseapproved label and removed databasereviewed label
reset approvals from @theoretick, @jarka, and @psimyn by pushing to the branch
- Resolved by Eren Akca
Hi @eakca1, I arrived here from gitlab-com/www-gitlab-com!135999 (merged). Just wanted to say, please tag me for review when you add the documentation for this change. I'm also happy to review any UI text, and to provide any other support as needed.
requested review from @10io and removed review request for @vshushlin
- Resolved by Eren Akca
removed review request for @10io
added 737 commits
-
5015b0c4...695058b8 - 733 commits from branch
master
- 4bc3d3cf - Allow top level group owners to create Service Accounts in Self-Managed
- 2e168dd4 - Remove boilerplate comments to keep the migrations lean
- 45989023 - Apply suggestion for improving test names
- 5bb8a92f - Apply suggestions for group policy
Toggle commit list-
5015b0c4...695058b8 - 733 commits from branch
mentioned in issue #482400 (closed)
reset approvals from @psimyn by pushing to the branch
- A deleted user
added feature flag label
removed review request for @rshambhuni
mentioned in issue #468806 (closed)
- Resolved by Eren Akca
requested review from @dblessing
removed review request for @jarka
@eakca1 it seems Drew has some concerns here, I am unassigning myself until this is resolved.
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
Thanks for your patience @eakca1. It took me a while to really wrap my head around the final goal here. Now it makes a lot more sense.
I suspect one of the issues with the tests is things evolved throughout the review cycles but didn't quite come together as expected.
Given the goal of this change is to unify behavior of self-managed and .com, I think we can clean the tests up quite a bit where right now there's a lot of disparate test contexts. The best course might be to have a groupauthentication person team up and help with the specs.
@adil.farrukh Is there someone you could recommend to help out so we can get this in for %17.4?
added 2088 commits
-
4d685b17...c12f7936 - 2087 commits from branch
master
- 4a426129 - Allow top level group owners to create Service Accounts in Self-Managed
-
4d685b17...c12f7936 - 2087 commits from branch
added 218 commits
-
4a426129...82f7566c - 217 commits from branch
master
- 06376573 - Allow top level group owners to create Service Accounts in Self-Managed
-
4a426129...82f7566c - 217 commits from branch
added 123 commits
-
06376573...524274ea - 122 commits from branch
master
- fc586da0 - Allow top level group owners to create Service Accounts in Self-Managed
-
06376573...524274ea - 122 commits from branch
added 47 commits
-
fc586da0...7e97412f - 46 commits from branch
master
- 14be7e53 - Allow top level group owners to create Service Accounts in Self-Managed
-
fc586da0...7e97412f - 46 commits from branch
mentioned in merge request gitlab-com/www-gitlab-com!135999 (merged)
changed milestone to %17.5
- Resolved by Drew Blessing
Hi Team!
May we be able to get this MR through for our customer?
Can we specify any timeline and steps that have to be done before we can merge this?
It would be super helpful to have some basics I can share with the customer to avoid concerns and set real expectations!
Thanks for your help!
CC: @adil.farrukh @hsutor
requested review from @sgarg_gitlab
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Eren Akca
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Smriti Garg
- Resolved by Smriti Garg
reset approvals from @psimyn by pushing to the branch
added 2752 commits
-
3e1740c6...86eeeb8e - 2750 commits from branch
master
- e345e796 - Allow top level group owners to create Service Accounts in Self-Managed
- cade94c9 - Spec organization and restructuring
-
3e1740c6...86eeeb8e - 2750 commits from branch
assigned to @sgarg_gitlab
requested review from @dblessing
removed review request for @sgarg_gitlab
added 797 commits
-
cade94c9...9bad0825 - 794 commits from branch
master
- 2a425650 - Allow top level group owners to create Service Accounts in Self-Managed
- 7a0bbfb5 - Spec organization and restructuring
- fd722b0e - Adding documentation and moving feature flag to right place
Toggle commit list-
cade94c9...9bad0825 - 794 commits from branch
requested review from @jglassman1
- A deleted user
added documentation label
added 1 commit
- a1171d6e - Adding documentation and moving feature flag to right place
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
added 136 commits
-
a1171d6e...b585d8d0 - 133 commits from branch
master
- 2fedb41c - Allow top level group owners to create Service Accounts in Self-Managed
- 405d4b95 - Spec organization and restructuring
- f3df67ad - Adding documentation and moving feature flag to right place
Toggle commit list-
a1171d6e...b585d8d0 - 133 commits from branch
added 23 commits
-
f3df67ad...3005b5ab - 20 commits from branch
master
- 00be9dde - Allow top level group owners to create Service Accounts in Self-Managed
- b339dcb1 - Spec organization and restructuring
- f606fa69 - Adding documentation and moving feature flag to right place
Toggle commit list-
f3df67ad...3005b5ab - 20 commits from branch
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Eren Akca
- Resolved by Jon Glassman
Thank you @eakca1 I left some suggestions and questions.
- Resolved by Eren Akca
added 112 commits
-
f606fa69...3845c5a9 - 108 commits from branch
master
- 6a2c73d3 - Allow top level group owners to create Service Accounts in Self-Managed
- 75ad88f0 - Spec organization and restructuring
- 1e324eb0 - Adding documentation and moving feature flag to right place
- 6124fb59 - Doc corrections Co-Authored-With: @jglassman1
Toggle commit list-
f606fa69...3845c5a9 - 108 commits from branch
Hi
@jglassman1
,GitLab Bot has added the Technical Writing label because a Technical Writer has approved or merged this MR.
This message was generated automatically. You're welcome to improve it.
added Technical Writing label
removed review request for @jglassman1
requested review from @jarka
- Resolved by Jarka Košanová
Thanks @eakca1 @sgarg_gitlab , approved and setting MWPS
mentioned in incident gitlab-org/quality/engineering-productivity/approved-mr-pipeline-incidents#1172 (closed)
mentioned in incident gitlab-org/quality/engineering-productivity/approved-mr-pipeline-incidents#1178 (closed)
mentioned in incident gitlab-org/quality/engineering-productivity/approved-mr-pipeline-incidents#1179 (closed)
started a merge train
Hello @eakca1
The database team is looking for ways to improve the database review process and we would love your help!
If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:
@gitlab-org/database-team
And someone will be by shortly!
Thanks for your help!
This message was generated automatically. You're welcome to improve it.
mentioned in commit c910aef5
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!3429 (merged)
mentioned in issue gitlab-org/quality/triage-reports#20596 (closed)