Follow-up from "API: Add endpoint to reset runner authentication token"
The following discussion from !69561 (merged) should be addressed:
-
@toupeira started a discussion: (+3 comments) note: If I'm following the code correctly, this exposes the token to:
- Admins: https://gitlab.com/gitlab-org/gitlab/blob/ebd9bd5acef38a8f48b179c370dcd427645669e5/lib/api/ci/runners.rb#L312
- Project maintainers: https://gitlab.com/gitlab-org/gitlab/blob/ebd9bd5acef38a8f48b179c370dcd427645669e5/app/models/user.rb#L1619
- Group owners: https://gitlab.com/gitlab-org/gitlab/blob/ebd9bd5acef38a8f48b179c370dcd427645669e5/app/models/user.rb#L1624
I'm not sure why we have this discrepancy between maintainers on projects vs. owners on groups
🤔 For the registration token I saw we have a custom permission
update_runners_registration_token
which is enabled for maintainers on both projects and groups. Maybe we want to do the same here and introduce a permissionupdate_runners_authentication_token
?
My reply from that thread
For the registration token I saw we have a custom permission
update_runners_registration_token
which is enabled for maintainers on both projects and groups. Maybe we want to do the same here and introduce a permissionupdate_runners_authentication_token
?Good catch and I wasn't rigorous enough in my review of the previous MR for the registration token reset. I checked that it matched the GraphQL API and allowing maintainers seemed to make sense to me, but all other permissions related to group runners require that a user
owns_runner
which, for group runners, requires Owner permissions (see also https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/32/diffs#4ed0c03b53dbc8320da88e7887465514cd9e90a4_1330_1330 (internal)).Seems like we introduced a discrepancy with c4aa70b9, let's more forward with this MR here as it's using the most strict access controls and I'll open a new issue / MR for this where we can discuss this specific point.
I'll do some more reading on all the MRs that lead to this to make sure I have all the context, but I'm tempted to just open an MR with
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 018ce4fb72a..98ce119faad 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -160,7 +160,6 @@ class GroupPolicy < BasePolicy
enable :admin_cluster
enable :read_deploy_token
enable :create_jira_connect_subscription
- enable :update_runners_registration_token
end
rule { owner }.policy do
@@ -176,6 +175,7 @@ class GroupPolicy < BasePolicy
enable :update_default_branch_protection
enable :create_deploy_token
enable :destroy_deploy_token
+ enable :update_runners_registration_token
end
rule { can?(:read_nested_project_resources) }.policy do