Skip to content
Snippets Groups Projects

Allow top-level group owners to create SA

All threads resolved!

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.

  1. In a self-managed instance, add a non-admin user as owner to a top-level group
  2. Create a personal access token for the given owner user
  3. 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."
  1. 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 press Save 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"
  1. 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

#468806 (closed)

Feature Rollout Issue for GitLab.com Derisk

#482400 (closed)

Edited by Eren Akca

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Brian Williams approved this merge request

    approved this merge request

  • Brian Williams removed review request for @bwill

    removed review request for @bwill

  • Eren Akca added 1 commit

    added 1 commit

    • 31cc7737 - Remove boilerplate comments to keep the migrations lean

    Compare with previous version

  • Eren Akca reset approvals from @bwill by pushing to the branch

    reset approvals from @bwill by pushing to the branch

  • James Rushford approved this merge request

    approved this merge request

  • James Rushford requested review from @psimyn and removed review request for @jrushford

    requested review from @psimyn and removed review request for @jrushford

  • Eren Akca resolved all threads

    resolved all threads

  • Eren Akca requested review from @jarka

    requested review from @jarka

  • Lucas Charles approved this merge request

    approved this merge request

  • Lucas Charles
  • Lucas Charles
  • Lucas Charles
  • Lucas Charles requested review from @rshambhuni and @vshushlin and removed review request for @theoretick

    requested review from @rshambhuni and @vshushlin and removed review request for @theoretick

  • Simon Knox
  • Simon Knox approved this merge request

    approved this merge request

  • Jarka Košanová approved this merge request

    approved this merge request

  • added databaseapproved label and removed databasereviewed label

  • Eren Akca added 1 commit

    added 1 commit

    • 5015b0c4 - Apply suggestion for improving test names

    Compare with previous version

  • Eren Akca reset approvals from @theoretick, @jarka, and @psimyn by pushing to the branch

    reset approvals from @theoretick, @jarka, and @psimyn by pushing to the branch

  • Vladimir Shushlin requested review from @10io and removed review request for @vshushlin

    requested review from @10io and removed review request for @vshushlin

  • Jarka Košanová
  • David Fernandez removed review request for @10io

    removed review request for @10io

  • Eren Akca added 737 commits

    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

    Compare with previous version

  • Eren Akca added 1 commit

    added 1 commit

    • cb09f3df - Apply suggestions for group policy

    Compare with previous version

  • Simon Knox approved this merge request

    approved this merge request

  • mentioned in issue #482400 (closed)

  • Eren Akca added 1 commit

    added 1 commit

    • 0a19e33d - FF addition to derisk .com rollout

    Compare with previous version

  • Eren Akca reset approvals from @psimyn by pushing to the branch

    reset approvals from @psimyn by pushing to the branch

  • A deleted user added feature flag label

    added feature flag label

  • Eren Akca added 1 commit

    added 1 commit

    • 9b43a054 - FF addition to derisk .com rollout

    Compare with previous version

  • Eren Akca added 1 commit

    added 1 commit

    • 4d685b17 - FF addition to derisk .com rollout

    Compare with previous version

  • Eren Akca changed the description

    changed the description

  • Simon Knox approved this merge request

    approved this merge request

  • Rohit Shambhuni removed review request for @rshambhuni

    removed review request for @rshambhuni

  • Jarka Košanová approved this merge request

    approved this merge request

  • Eren Akca resolved all threads

    resolved all threads

  • mentioned in issue #468806 (closed)

  • Drew Blessing
  • Drew Blessing requested review from @dblessing

    requested review from @dblessing

  • Drew Blessing requested changes

    requested changes

  • Jarka Košanová removed review request for @jarka

    removed review request for @jarka

  • @eakca1 it seems Drew has some concerns here, I am unassigning myself until this is resolved.

  • Drew Blessing
  • Drew Blessing
  • Drew Blessing
  • Drew Blessing
    • 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?

  • Drew Blessing requested changes

    requested changes

  • Eren Akca changed the description

    changed the description

  • Eren Akca added 2088 commits

    added 2088 commits

    • 4d685b17...c12f7936 - 2087 commits from branch master
    • 4a426129 - Allow top level group owners to create Service Accounts in Self-Managed

    Compare with previous version

  • Eren Akca reset approvals from @jarka and @psimyn by pushing to the branch

    reset approvals from @jarka and @psimyn by pushing to the branch

  • Eren Akca added 218 commits

    added 218 commits

    • 4a426129...82f7566c - 217 commits from branch master
    • 06376573 - Allow top level group owners to create Service Accounts in Self-Managed

    Compare with previous version

  • Eren Akca added 123 commits

    added 123 commits

    • 06376573...524274ea - 122 commits from branch master
    • fc586da0 - Allow top level group owners to create Service Accounts in Self-Managed

    Compare with previous version

  • Eren Akca added 47 commits

    added 47 commits

    • fc586da0...7e97412f - 46 commits from branch master
    • 14be7e53 - Allow top level group owners to create Service Accounts in Self-Managed

    Compare with previous version

  • Eren Akca changed milestone to %17.5

    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

  • Simon Knox approved this merge request

    approved this merge request

  • Adil Farrukh requested review from @sgarg_gitlab

    requested review from @sgarg_gitlab

  • Smriti Garg
  • Smriti Garg
  • Smriti Garg added 1 commit

    added 1 commit

    • 3e1740c6 - Spec organization and restructuring

    Compare with previous version

  • Smriti Garg reset approvals from @psimyn by pushing to the branch

    reset approvals from @psimyn by pushing to the branch

  • Smriti Garg added 2752 commits

    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

    Compare with previous version

  • Smriti Garg requested review from @dblessing

    requested review from @dblessing

  • Smriti Garg removed review request for @sgarg_gitlab

    removed review request for @sgarg_gitlab

  • Eren Akca added 797 commits

    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

    Compare with previous version

  • Eren Akca requested review from @jglassman1

    requested review from @jglassman1

  • A deleted user added documentation label

    added documentation label

  • Eren Akca marked the checklist item Documentation Update MR to follow as completed

    marked the checklist item Documentation Update MR to follow as completed

  • Eren Akca added 1 commit

    added 1 commit

    • a1171d6e - Adding documentation and moving feature flag to right place

    Compare with previous version

  • Adil Farrukh
  • Adil Farrukh
  • Adil Farrukh
  • Adil Farrukh
  • Eren Akca added 136 commits

    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

    Compare with previous version

  • Eren Akca added 23 commits

    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

    Compare with previous version

  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Jon Glassman
  • Thank you @eakca1 I left some suggestions and questions.

  • Jon Glassman
    • Resolved by Eren Akca

      Thank you both for discussing and answering my questions. @eakca1 I think that, if you're happy to apply my suggestions where appropriate, I'll take another look and hopefully approve the docs changes.

  • Eren Akca added 112 commits

    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

    Compare with previous version

  • Eren Akca added 1 commit

    added 1 commit

    • 840e88cb - Fix doc links and minor improvements

    Compare with previous version

  • Eren Akca added 1 commit

    added 1 commit

    • 82d02ed3 - Fix doc links and minor improvements

    Compare with previous version

  • Jon Glassman approved this merge request

    approved this merge request

  • Hi @jglassman1 :wave:,

    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.

  • Jon Glassman removed review request for @jglassman1

    removed review request for @jglassman1

  • Adil Farrukh approved this merge request

    approved this merge request

  • Eren Akca requested review from @jarka

    requested review from @jarka

  • Author Developer

    @jarka May I ask for a final review and approval, if all looks good, from your side to this MR, as you already had a look to the changes. See also recent thread Thanks!

    Edited by Eren Akca
  • Author Developer

    @psimyn May I ask for a final review and approval, if all is good, from your side to this MR, as you already had a look to the changes. See also recent thread Thanks!

    Edited by Eren Akca
  • Simon Knox approved this merge request

    approved this merge request

  • Drew Blessing approved this merge request

    approved this merge request

  • Drew Blessing resolved all threads

    resolved all threads

  • Jarka Košanová approved this merge request

    approved this merge request

  • Jarka Košanová resolved all threads

    resolved all threads

  • Jarka Košanová resolved all threads

    resolved all threads

  • Jarka Košanová enabled automatic add to merge train when checks pass

    enabled automatic add to merge train when checks pass

  • Hello @eakca1 :wave:

    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! :heart:

    This message was generated automatically. You're welcome to improve it.

  • mentioned in commit c910aef5

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading