From 248c186136faa0b7036131adf15c414233f1ae12 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 20 Jun 2023 11:42:49 +0200 Subject: [PATCH 1/7] Extract runners endpoint into separate file This will help with adding different scope requirement for endpoint --- lib/api/api.rb | 1 + lib/api/user_runners.rb | 70 ++++++++++ lib/api/users.rb | 59 +-------- spec/requests/api/user_runners_spec.rb | 173 +++++++++++++++++++++++++ spec/requests/api/users_spec.rb | 165 ----------------------- 5 files changed, 245 insertions(+), 223 deletions(-) create mode 100644 lib/api/user_runners.rb create mode 100644 spec/requests/api/user_runners_spec.rb diff --git a/lib/api/api.rb b/lib/api/api.rb index 090fbaa7f931f3..7da5f21b21ff9e 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -316,6 +316,7 @@ class API < ::API::Base mount ::API::UsageDataQueries mount ::API::Users mount ::API::UserCounts + mount ::API::UserRunners mount ::API::Wikis add_open_api_documentation! diff --git a/lib/api/user_runners.rb b/lib/api/user_runners.rb new file mode 100644 index 00000000000000..1a4b589afbbfc4 --- /dev/null +++ b/lib/api/user_runners.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module API + class UserRunners < ::API::Base + include APIGuard + + resource :user do + before do + authenticate! + end + + desc 'Create a runner owned by currently authenticated user' do + detail 'Create a new runner' + success Entities::Ci::RunnerRegistrationDetails + failure [[400, 'Bad Request'], [403, 'Forbidden']] + tags %w[user runners] + end + params do + requires :runner_type, type: String, values: ::Ci::Runner.runner_types.keys, + desc: %q(Specifies the scope of the runner) + given runner_type: ->(runner_type) { runner_type == 'group_type' } do + requires :group_id, type: Integer, + desc: 'The ID of the group that the runner is created in', + documentation: { example: 1 } + end + given runner_type: ->(runner_type) { runner_type == 'project_type' } do + requires :project_id, type: Integer, + desc: 'The ID of the project that the runner is created in', + documentation: { example: 1 } + end + optional :description, type: String, desc: %q(Description of the runner) + optional :maintenance_note, type: String, + desc: %q(Free-form maintenance notes for the runner (1024 characters)) + optional :paused, type: Boolean, desc: 'Specifies if the runner should ignore new jobs (defaults to false)' + optional :locked, type: Boolean, + desc: 'Specifies if the runner should be locked for the current project (defaults to false)' + optional :access_level, type: String, values: ::Ci::Runner.access_levels.keys, + desc: 'The access level of the runner' + optional :run_untagged, type: Boolean, + desc: 'Specifies if the runner should handle untagged jobs (defaults to true)' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, + desc: %q(A list of runner tags) + optional :maximum_timeout, type: Integer, + desc: 'Maximum timeout that limits the amount of time (in seconds) that runners can run jobs' + end + post 'runners', urgency: :low, feature_category: :runner_fleet do + attributes = attributes_for_keys( + %i[runner_type group_id project_id description maintenance_note paused locked run_untagged tag_list + access_level maximum_timeout] + ) + + case attributes[:runner_type] + when 'group_type' + attributes[:scope] = ::Group.find_by_id(attributes.delete(:group_id)) + when 'project_type' + attributes[:scope] = ::Project.find_by_id(attributes.delete(:project_id)) + end + + result = ::Ci::Runners::CreateRunnerService.new(user: current_user, params: attributes).execute + if result.error? + message = result.errors.to_sentence + forbidden!(message) if result.reason == :forbidden + bad_request!(message) + end + + present result.payload[:runner], with: Entities::Ci::RunnerRegistrationDetails + end + end + end +end diff --git a/lib/api/users.rb b/lib/api/users.rb index ff36a4cfe9580e..e390654107d3cd 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -6,7 +6,7 @@ class Users < ::API::Base include APIGuard include Helpers::CustomAttributes - allow_access_with_scope :read_user, if: -> (request) { request.get? || request.head? } + allow_access_with_scope :read_user, if: ->(request) { request.get? || request.head? } feature_category :user_profile, %w[ @@ -1369,63 +1369,6 @@ def set_user_status(include_missing_params:) get 'status', feature_category: :user_profile do present current_user.status || {}, with: Entities::UserStatus end - - desc 'Create a runner owned by currently authenticated user' do - detail 'Create a new runner' - success Entities::Ci::RunnerRegistrationDetails - failure [[400, 'Bad Request'], [403, 'Forbidden']] - tags %w[user runners] - end - params do - requires :runner_type, type: String, values: ::Ci::Runner.runner_types.keys, - desc: %q(Specifies the scope of the runner) - given runner_type: ->(runner_type) { runner_type == 'group_type' } do - requires :group_id, type: Integer, - desc: 'The ID of the group that the runner is created in', - documentation: { example: 1 } - end - given runner_type: ->(runner_type) { runner_type == 'project_type' } do - requires :project_id, type: Integer, - desc: 'The ID of the project that the runner is created in', - documentation: { example: 1 } - end - optional :description, type: String, desc: %q(Description of the runner) - optional :maintenance_note, type: String, - desc: %q(Free-form maintenance notes for the runner (1024 characters)) - optional :paused, type: Boolean, desc: 'Specifies if the runner should ignore new jobs (defaults to false)' - optional :locked, type: Boolean, - desc: 'Specifies if the runner should be locked for the current project (defaults to false)' - optional :access_level, type: String, values: ::Ci::Runner.access_levels.keys, - desc: 'The access level of the runner' - optional :run_untagged, type: Boolean, - desc: 'Specifies if the runner should handle untagged jobs (defaults to true)' - optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, - desc: %q(A list of runner tags) - optional :maximum_timeout, type: Integer, - desc: 'Maximum timeout that limits the amount of time (in seconds) that runners can run jobs' - end - post 'runners', urgency: :low, feature_category: :runner_fleet do - attributes = attributes_for_keys( - %i[runner_type group_id project_id description maintenance_note paused locked run_untagged tag_list - access_level maximum_timeout] - ) - - case attributes[:runner_type] - when 'group_type' - attributes[:scope] = ::Group.find_by_id(attributes.delete(:group_id)) - when 'project_type' - attributes[:scope] = ::Project.find_by_id(attributes.delete(:project_id)) - end - - result = ::Ci::Runners::CreateRunnerService.new(user: current_user, params: attributes).execute - if result.error? - message = result.errors.to_sentence - forbidden!(message) if result.reason == :forbidden - bad_request!(message) - end - - present result.payload[:runner], with: Entities::Ci::RunnerRegistrationDetails - end end end end diff --git a/spec/requests/api/user_runners_spec.rb b/spec/requests/api/user_runners_spec.rb new file mode 100644 index 00000000000000..fd55249ff5fb7a --- /dev/null +++ b/spec/requests/api/user_runners_spec.rb @@ -0,0 +1,173 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::UserRunners, :aggregate_failures, feature_category: :runner_fleet do + let_it_be(:admin) { create(:admin) } + let_it_be(:user, reload: true) { create(:user, username: 'user.withdot') } + + describe 'POST /user/runners' do + subject(:request) { post api(path, current_user, **post_args), params: runner_attrs } + + let_it_be(:group_owner) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, namespace: group) } + + let(:post_args) { { admin_mode: true } } + let(:runner_attrs) { { runner_type: 'instance_type' } } + let(:path) { '/user/runners' } + + before do + group.add_owner(group_owner) + end + + shared_context 'when user does not have sufficient permissions returns forbidden' do + let(:current_user) { admin } + let(:post_args) { { admin_mode: false } } + + it 'does not create a runner' do + expect do + request + + expect(response).to have_gitlab_http_status(:forbidden) + end.not_to change { Ci::Runner.count } + end + end + + shared_examples 'creates a runner' do + it 'creates a runner' do + expect do + request + + expect(response).to have_gitlab_http_status(:created) + end.to change { Ci::Runner.count }.by(1) + end + end + + shared_examples 'fails to create runner with :bad_request' do + it 'does not create runner' do + expect do + request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to include(expected_error) + end.not_to change { Ci::Runner.count } + end + end + + context 'when runner_type is :instance_type' do + let(:runner_attrs) { { runner_type: 'instance_type' } } + + context 'when user has sufficient permissions' do + let(:current_user) { admin } + + it_behaves_like 'creates a runner' + end + + it_behaves_like 'when user does not have sufficient permissions returns forbidden' + + context 'when model validation fails' do + let(:runner_attrs) { { runner_type: 'instance_type', run_untagged: false, tag_list: [] } } + let(:current_user) { admin } + + it_behaves_like 'fails to create runner with :bad_request' do + let(:expected_error) { 'Tags list can not be empty' } + end + end + end + + context 'when runner_type is :group_type' do + let(:post_args) { {} } + + context 'when group_id is specified' do + let(:runner_attrs) { { runner_type: 'group_type', group_id: group.id } } + + context 'when user has sufficient permissions' do + let(:current_user) { group_owner } + + it_behaves_like 'creates a runner' + end + + it_behaves_like 'when user does not have sufficient permissions returns forbidden' + end + + context 'when group_id is not specified' do + let(:runner_attrs) { { runner_type: 'group_type' } } + let(:current_user) { group_owner } + + it 'fails to create runner with :bad_request' do + expect do + request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to include('group_id is missing') + end.not_to change { Ci::Runner.count } + end + end + end + + context 'when runner_type is :project_type' do + let(:post_args) { {} } + + context 'when project_id is specified' do + let(:runner_attrs) { { runner_type: 'project_type', project_id: project.id } } + + context 'when user has sufficient permissions' do + let(:current_user) { group_owner } + + it_behaves_like 'creates a runner' + end + + it_behaves_like 'when user does not have sufficient permissions returns forbidden' + end + + context 'when project_id is not specified' do + let(:runner_attrs) { { runner_type: 'project_type' } } + let(:current_user) { group_owner } + + it 'fails to create runner with :bad_request' do + expect do + request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to include('project_id is missing') + end.not_to change { Ci::Runner.count } + end + end + end + + context 'with missing runner_type' do + let(:runner_attrs) { {} } + let(:current_user) { admin } + + it 'fails to create runner with :bad_request' do + expect do + request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('runner_type is missing, runner_type does not have a valid value') + end.not_to change { Ci::Runner.count } + end + end + + context 'with unknown runner_type' do + let(:runner_attrs) { { runner_type: 'unknown' } } + let(:current_user) { admin } + + it 'fails to create runner with :bad_request' do + expect do + request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('runner_type does not have a valid value') + end.not_to change { Ci::Runner.count } + end + end + + it 'returns a 401 error if unauthorized' do + post api(path), params: runner_attrs + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end +end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 3737c91adbc97d..2bbcf6b3f38769 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -4851,169 +4851,4 @@ def update_password(user, admin, password = User.random_password) let(:attributable) { user } let(:other_attributable) { admin } end - - describe 'POST /user/runners', feature_category: :runner_fleet do - subject(:request) { post api(path, current_user, **post_args), params: runner_attrs } - - let_it_be(:group_owner) { create(:user) } - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, namespace: group) } - - let(:post_args) { { admin_mode: true } } - let(:runner_attrs) { { runner_type: 'instance_type' } } - let(:path) { '/user/runners' } - - before do - group.add_owner(group_owner) - end - - shared_context 'returns forbidden when user does not have sufficient permissions' do - let(:current_user) { admin } - let(:post_args) { { admin_mode: false } } - - it 'does not create a runner' do - expect do - request - - expect(response).to have_gitlab_http_status(:forbidden) - end.not_to change { Ci::Runner.count } - end - end - - shared_examples 'creates a runner' do - it 'creates a runner' do - expect do - request - - expect(response).to have_gitlab_http_status(:created) - end.to change { Ci::Runner.count }.by(1) - end - end - - shared_examples 'fails to create runner with :bad_request' do - it 'does not create runner' do - expect do - request - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to include(expected_error) - end.not_to change { Ci::Runner.count } - end - end - - context 'when runner_type is :instance_type' do - let(:runner_attrs) { { runner_type: 'instance_type' } } - - context 'when user has sufficient permissions' do - let(:current_user) { admin } - - it_behaves_like 'creates a runner' - end - - it_behaves_like 'returns forbidden when user does not have sufficient permissions' - - context 'when model validation fails' do - let(:runner_attrs) { { runner_type: 'instance_type', run_untagged: false, tag_list: [] } } - let(:current_user) { admin } - - it_behaves_like 'fails to create runner with :bad_request' do - let(:expected_error) { 'Tags list can not be empty' } - end - end - end - - context 'when runner_type is :group_type' do - let(:post_args) { {} } - - context 'when group_id is specified' do - let(:runner_attrs) { { runner_type: 'group_type', group_id: group.id } } - - context 'when user has sufficient permissions' do - let(:current_user) { group_owner } - - it_behaves_like 'creates a runner' - end - - it_behaves_like 'returns forbidden when user does not have sufficient permissions' - end - - context 'when group_id is not specified' do - let(:runner_attrs) { { runner_type: 'group_type' } } - let(:current_user) { group_owner } - - it 'fails to create runner with :bad_request' do - expect do - request - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to include('group_id is missing') - end.not_to change { Ci::Runner.count } - end - end - end - - context 'when runner_type is :project_type' do - let(:post_args) { {} } - - context 'when project_id is specified' do - let(:runner_attrs) { { runner_type: 'project_type', project_id: project.id } } - - context 'when user has sufficient permissions' do - let(:current_user) { group_owner } - - it_behaves_like 'creates a runner' - end - - it_behaves_like 'returns forbidden when user does not have sufficient permissions' - end - - context 'when project_id is not specified' do - let(:runner_attrs) { { runner_type: 'project_type' } } - let(:current_user) { group_owner } - - it 'fails to create runner with :bad_request' do - expect do - request - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to include('project_id is missing') - end.not_to change { Ci::Runner.count } - end - end - end - - context 'with missing runner_type' do - let(:runner_attrs) { {} } - let(:current_user) { admin } - - it 'fails to create runner with :bad_request' do - expect do - request - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq('runner_type is missing, runner_type does not have a valid value') - end.not_to change { Ci::Runner.count } - end - end - - context 'with unknown runner_type' do - let(:runner_attrs) { { runner_type: 'unknown' } } - let(:current_user) { admin } - - it 'fails to create runner with :bad_request' do - expect do - request - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq('runner_type does not have a valid value') - end.not_to change { Ci::Runner.count } - end - end - - it 'returns a 401 error if unauthorized' do - post api(path), params: runner_attrs - - expect(response).to have_gitlab_http_status(:unauthorized) - end - end end -- GitLab From e251008430c7e75cf06deb6bd79c3aeffc3248ad Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 20 Jun 2023 11:57:39 +0200 Subject: [PATCH 2/7] Add :create_runner access token scope Changelog: added --- config/locales/doorkeeper.en.yml | 5 +++++ lib/gitlab/auth.rb | 6 ++++-- spec/lib/gitlab/auth_spec.rb | 31 +++++++++++++++++----------- spec/requests/openid_connect_spec.rb | 11 ++++++++-- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index 63bbab67039eb8..b571801c006df4 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -76,6 +76,7 @@ en: profile: Allows read-only access to the user's personal information using OpenID Connect email: Allows read-only access to the user's primary email address using OpenID Connect admin_mode: Admin Mode is a functionality designed to limit the access level of administrator's personal access tokens. + create_runner: Grants create access to the runners scope_desc: api: Grants complete read/write access to the API, including all groups and projects, the container registry, and the package registry. @@ -105,6 +106,8 @@ en: Grants read-only access to the user's primary email address using OpenID Connect. admin_mode: Grants permission to perform API actions as an administrator, when Admin Mode is enabled. + create_runner: + Grants create access to the runners. project_access_token_scope_desc: api: Grants complete read and write access to the scoped project API, including the Package Registry. @@ -118,6 +121,8 @@ en: Grants read access (pull) to the Container Registry images if a project is private and authorization is required. write_registry: Grants write access (push) to the Container Registry. + create_runner: + Grants create access to the runners. flash: applications: create: diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 83d94d168a0904..3ff9e22dd2550f 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -9,7 +9,8 @@ module Auth API_SCOPE = :api READ_API_SCOPE = :read_api READ_USER_SCOPE = :read_user - API_SCOPES = [API_SCOPE, READ_API_SCOPE, READ_USER_SCOPE].freeze + CREATE_RUNNER_SCOPE = :create_runner + API_SCOPES = [API_SCOPE, READ_API_SCOPE, READ_USER_SCOPE, CREATE_RUNNER_SCOPE].freeze PROFILE_SCOPE = :profile EMAIL_SCOPE = :email @@ -251,7 +252,8 @@ def abilities_for_scopes(scopes) read_registry: [:read_container_image], write_registry: [:create_container_image], read_repository: [:download_code], - write_repository: [:download_code, :push_code] + write_repository: [:download_code, :push_code], + create_runner: [:create_instance_runner, :create_runner] } scopes.flat_map do |scope| diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index b864dba58deda7..603609e5e62bee 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -10,7 +10,7 @@ describe 'constants' do it 'API_SCOPES contains all scopes for API access' do - expect(subject::API_SCOPES).to match_array %i[api read_user read_api] + expect(subject::API_SCOPES).to match_array %i[api read_user read_api create_runner] end it 'ADMIN_SCOPES contains all scopes for ADMIN access' do @@ -40,29 +40,29 @@ end it 'contains all non-default scopes' do - expect(subject.all_available_scopes).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode read_observability write_observability] + expect(subject.all_available_scopes).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode read_observability write_observability create_runner] end it 'contains for non-admin user all non-default scopes without ADMIN access and without observability scopes' do user = build_stubbed(:user, admin: false) - expect(subject.available_scopes_for(user)).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry] + expect(subject.available_scopes_for(user)).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry create_runner] end it 'contains for admin user all non-default scopes with ADMIN access and without observability scopes' do user = build_stubbed(:user, admin: true) - expect(subject.available_scopes_for(user)).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode] + expect(subject.available_scopes_for(user)).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode create_runner] end it 'contains for project all resource bot scopes without observability scopes' do - expect(subject.available_scopes_for(project)).to match_array %i[api read_api read_repository write_repository read_registry write_registry] + expect(subject.available_scopes_for(project)).to match_array %i[api read_api read_repository write_repository read_registry write_registry create_runner] end it 'contains for group all resource bot scopes' do group = build_stubbed(:group) - expect(subject.available_scopes_for(group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry read_observability write_observability] + expect(subject.available_scopes_for(group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry read_observability write_observability create_runner] end it 'contains for unsupported type no scopes' do @@ -70,7 +70,7 @@ end it 'optional_scopes contains all non-default scopes' do - expect(subject.optional_scopes).to match_array %i[read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode openid profile email read_observability write_observability] + expect(subject.optional_scopes).to match_array %i[read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode openid profile email read_observability write_observability create_runner] end context 'with observability_group_tab feature flag' do @@ -82,7 +82,7 @@ it 'contains for group all resource bot scopes without observability scopes' do group = build_stubbed(:group) - expect(subject.available_scopes_for(group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry] + expect(subject.available_scopes_for(group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry create_runner] end end @@ -94,23 +94,23 @@ end it 'contains for other group all resource bot scopes including observability scopes' do - expect(subject.available_scopes_for(group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry read_observability write_observability] + expect(subject.available_scopes_for(group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry read_observability write_observability create_runner] end it 'contains for admin user all non-default scopes with ADMIN access and without observability scopes' do user = build_stubbed(:user, admin: true) - expect(subject.available_scopes_for(user)).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode] + expect(subject.available_scopes_for(user)).to match_array %i[api read_user read_api read_repository write_repository read_registry write_registry sudo admin_mode create_runner] end it 'contains for project all resource bot scopes without observability scopes' do - expect(subject.available_scopes_for(project)).to match_array %i[api read_api read_repository write_repository read_registry write_registry] + expect(subject.available_scopes_for(project)).to match_array %i[api read_api read_repository write_repository read_registry write_registry create_runner] end it 'contains for other group all resource bot scopes without observability scopes' do other_group = build_stubbed(:group) - expect(subject.available_scopes_for(other_group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry] + expect(subject.available_scopes_for(other_group)).to match_array %i[api read_api read_repository write_repository read_registry write_registry create_runner] end end end @@ -351,6 +351,7 @@ def operation 'read_api' | described_class.read_only_authentication_abilities 'read_repository' | [:download_code] 'write_repository' | [:download_code, :push_code] + 'create_runner' | [:create_instance_runner, :create_runner] 'read_user' | [] 'sudo' | [] 'openid' | [] @@ -412,6 +413,12 @@ def operation expect_results_with_abilities(personal_access_token, [:download_code, :push_code]) end + it 'succeeds for personal access tokens with the `create_runner` scope' do + personal_access_token = create(:personal_access_token, scopes: ['create_runner']) + + expect_results_with_abilities(personal_access_token, [:create_instance_runner, :create_runner]) + end + context 'when registry is enabled' do before do stub_container_registry_config(enabled: true) diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb index 82f972e7f944ab..217241200ff744 100644 --- a/spec/requests/openid_connect_spec.rb +++ b/spec/requests/openid_connect_spec.rb @@ -270,13 +270,20 @@ def request_user_info! end context 'OpenID configuration information' do + let(:expected_scopes) do + %w[ + admin_mode api read_user read_api read_repository write_repository sudo openid profile email + read_observability write_observability create_runner + ] + end + it 'correctly returns the configuration' do get '/.well-known/openid-configuration' expect(response).to have_gitlab_http_status(:ok) expect(json_response['issuer']).to eq('http://localhost') expect(json_response['jwks_uri']).to eq('http://www.example.com/oauth/discovery/keys') - expect(json_response['scopes_supported']).to match_array %w[admin_mode api read_user read_api read_repository write_repository sudo openid profile email read_observability write_observability] + expect(json_response['scopes_supported']).to match_array expected_scopes end context 'with a cross-origin request' do @@ -286,7 +293,7 @@ def request_user_info! expect(response).to have_gitlab_http_status(:ok) expect(json_response['issuer']).to eq('http://localhost') expect(json_response['jwks_uri']).to eq('http://www.example.com/oauth/discovery/keys') - expect(json_response['scopes_supported']).to match_array %w[admin_mode api read_user read_api read_repository write_repository sudo openid profile email read_observability write_observability] + expect(json_response['scopes_supported']).to match_array expected_scopes end it_behaves_like 'cross-origin GET request' -- GitLab From 91562071d2e6544435c7a5cfc200f60e4198c49c Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 20 Jun 2023 11:58:45 +0200 Subject: [PATCH 3/7] REST: Require only :create_runner token scope for /user/runners endpoint Changelog: changed --- doc/api/users.md | 1 + lib/api/user_runners.rb | 2 + spec/requests/api/user_runners_spec.rb | 52 +++++++++++++++++++++++--- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/doc/api/users.md b/doc/api/users.md index ed788600e62b29..4ef8e5d69f5a88 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -2244,6 +2244,7 @@ Prerequisites: - You must be an administrator or have the Owner role of the target namespace or project. - For `instance_type`, you must be an administrator of the GitLab instance. +- An access token with the `create_runner` scope. Be sure to copy or save the `token` in the response, the value cannot be retrieved again. diff --git a/lib/api/user_runners.rb b/lib/api/user_runners.rb index 1a4b589afbbfc4..edbd0214bb8118 100644 --- a/lib/api/user_runners.rb +++ b/lib/api/user_runners.rb @@ -9,6 +9,8 @@ class UserRunners < ::API::Base authenticate! end + allow_access_with_scope :create_runner, if: ->(request) { request.post? } + desc 'Create a runner owned by currently authenticated user' do detail 'Create a new runner' success Entities::Ci::RunnerRegistrationDetails diff --git a/spec/requests/api/user_runners_spec.rb b/spec/requests/api/user_runners_spec.rb index fd55249ff5fb7a..513cfcf3659435 100644 --- a/spec/requests/api/user_runners_spec.rb +++ b/spec/requests/api/user_runners_spec.rb @@ -44,17 +44,44 @@ end end - shared_examples 'fails to create runner with :bad_request' do + shared_examples 'fails to create runner with expected_status_code' do + let(:expected_message) { nil } + let(:expected_error) { nil } + it 'does not create runner' do expect do request - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to include(expected_error) + expect(response).to have_gitlab_http_status(expected_status_code) + expect(json_response['message']).to include(expected_message) if expected_message + expect(json_response['error']).to include(expected_error) if expected_error end.not_to change { Ci::Runner.count } end end + shared_context 'with request authorized with access token' do + let(:current_user) { nil } + let(:pat) { create(:personal_access_token, user: token_user, scopes: [scope]) } + let(:path) { "/user/runners?private_token=#{pat.token}" } + + %i[create_runner api].each do |scope| + context "with #{scope} scope" do + let(:scope) { scope } + + it_behaves_like 'creates a runner' + end + end + + context 'with read_api scope' do + let(:scope) { :read_api } + + it_behaves_like 'fails to create runner with expected_status_code' do + let(:expected_status_code) { :forbidden } + let(:expected_error) { 'insufficient_scope' } + end + end + end + context 'when runner_type is :instance_type' do let(:runner_attrs) { { runner_type: 'instance_type' } } @@ -64,14 +91,21 @@ it_behaves_like 'creates a runner' end + context 'with admin mode enabled', :enable_admin_mode do + let(:token_user) { admin } + + it_behaves_like 'with request authorized with access token' + end + it_behaves_like 'when user does not have sufficient permissions returns forbidden' context 'when model validation fails' do let(:runner_attrs) { { runner_type: 'instance_type', run_untagged: false, tag_list: [] } } let(:current_user) { admin } - it_behaves_like 'fails to create runner with :bad_request' do - let(:expected_error) { 'Tags list can not be empty' } + it_behaves_like 'fails to create runner with expected_status_code' do + let(:expected_status_code) { :bad_request } + let(:expected_message) { 'Tags list can not be empty' } end end end @@ -88,6 +122,10 @@ it_behaves_like 'creates a runner' end + it_behaves_like 'with request authorized with access token' do + let(:token_user) { group_owner } + end + it_behaves_like 'when user does not have sufficient permissions returns forbidden' end @@ -118,6 +156,10 @@ it_behaves_like 'creates a runner' end + it_behaves_like 'with request authorized with access token' do + let(:token_user) { group_owner } + end + it_behaves_like 'when user does not have sufficient permissions returns forbidden' end -- GitLab From 7ab8e877b28968973115cfeb0b3dac1a2db16ab9 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 27 Jun 2023 09:36:17 +0200 Subject: [PATCH 4/7] Test addition scenarios for runner creation --- spec/requests/api/user_runners_spec.rb | 46 +++++++++++++++++++++----- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/spec/requests/api/user_runners_spec.rb b/spec/requests/api/user_runners_spec.rb index 513cfcf3659435..0e40dcade19495 100644 --- a/spec/requests/api/user_runners_spec.rb +++ b/spec/requests/api/user_runners_spec.rb @@ -9,22 +9,17 @@ describe 'POST /user/runners' do subject(:request) { post api(path, current_user, **post_args), params: runner_attrs } - let_it_be(:group_owner) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:group_owner) { create(:user).tap { |user| group.add_owner(user) } } + let_it_be(:group_maintainer) { create(:user).tap { |user| group.add_maintainer(user) } } + let_it_be(:project_developer) { create(:user).tap { |user| project.add_developer(user) } } let(:post_args) { { admin_mode: true } } let(:runner_attrs) { { runner_type: 'instance_type' } } let(:path) { '/user/runners' } - before do - group.add_owner(group_owner) - end - - shared_context 'when user does not have sufficient permissions returns forbidden' do - let(:current_user) { admin } - let(:post_args) { { admin_mode: false } } - + shared_examples 'when runner creation fails due to authorization' do it 'does not create a runner' do expect do request @@ -34,6 +29,21 @@ end end + shared_context 'when user does not have sufficient permissions returns forbidden' do + context 'when user is admin and admin mode is disabled' do + let(:current_user) { admin } + let(:post_args) { { admin_mode: false } } + + it_behaves_like 'when runner creation fails due to authorization' + end + + context 'when user is not an admin or a member of the namespace' do + let(:current_user) { user } + + it_behaves_like 'when runner creation fails due to authorization' + end + end + shared_examples 'creates a runner' do it 'creates a runner' do expect do @@ -99,6 +109,12 @@ it_behaves_like 'when user does not have sufficient permissions returns forbidden' + context 'when user is not an admin' do + let(:current_user) { user } + + it_behaves_like 'when runner creation fails due to authorization' + end + context 'when model validation fails' do let(:runner_attrs) { { runner_type: 'instance_type', run_untagged: false, tag_list: [] } } let(:current_user) { admin } @@ -127,6 +143,12 @@ end it_behaves_like 'when user does not have sufficient permissions returns forbidden' + + context 'when user is a maintainer' do + let(:current_user) { group_maintainer } + + it_behaves_like 'when runner creation fails due to authorization' + end end context 'when group_id is not specified' do @@ -161,6 +183,12 @@ end it_behaves_like 'when user does not have sufficient permissions returns forbidden' + + context 'when user is a developer' do + let(:current_user) { project_developer } + + it_behaves_like 'when runner creation fails due to authorization' + end end context 'when project_id is not specified' do -- GitLab From 9a25c959588795255eaeb264672b0aad08867c30 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 27 Jun 2023 09:36:41 +0200 Subject: [PATCH 5/7] Minor cleanup --- app/models/ci/runner.rb | 2 +- app/models/ci/runner_manager.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6319163b0d7893..c7896bb873dbc8 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -456,7 +456,7 @@ def heartbeat(values, update_contacted_at: true) end new_version = values[:version] - schedule_runner_version_update(new_version) if new_version && values[:version] != version + schedule_runner_version_update(new_version) if new_version && new_version != version merge_cache_attributes(values) diff --git a/app/models/ci/runner_manager.rb b/app/models/ci/runner_manager.rb index 57c33d8a30837c..3a3f95a8c699ee 100644 --- a/app/models/ci/runner_manager.rb +++ b/app/models/ci/runner_manager.rb @@ -77,7 +77,7 @@ def heartbeat(values, update_contacted_at: true) end new_version = values[:version] - schedule_runner_version_update(new_version) if new_version && values[:version] != version + schedule_runner_version_update(new_version) if new_version && new_version != version merge_cache_attributes(values) -- GitLab From 6c0c8aadba1acde2df4f9841f0215ba741210972 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 27 Jun 2023 09:52:15 +0200 Subject: [PATCH 6/7] Add create_runner scope to documentation --- doc/integration/oauth_provider.md | 1 + doc/user/group/settings/group_access_tokens.md | 1 + doc/user/profile/personal_access_tokens.md | 3 ++- doc/user/project/settings/project_access_tokens.md | 13 +++++++------ 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/doc/integration/oauth_provider.md b/doc/integration/oauth_provider.md index 6d08af225db49c..5cc706aafe1374 100644 --- a/doc/integration/oauth_provider.md +++ b/doc/integration/oauth_provider.md @@ -106,6 +106,7 @@ different actions. See the following table for all available scopes. | `openid` | Grants permission to authenticate with GitLab using [OpenID Connect](openid_connect_provider.md). Also gives read-only access to the user's profile and group memberships. | | `profile` | Grants read-only access to the user's profile data using [OpenID Connect](openid_connect_provider.md). | | `email` | Grants read-only access to the user's primary email address using [OpenID Connect](openid_connect_provider.md). | +| `create_runner` | Grants permission to create runners. | At any time you can revoke any access by selecting **Revoke**. diff --git a/doc/user/group/settings/group_access_tokens.md b/doc/user/group/settings/group_access_tokens.md index e264778062b288..4cf95a46c335ec 100644 --- a/doc/user/group/settings/group_access_tokens.md +++ b/doc/user/group/settings/group_access_tokens.md @@ -148,6 +148,7 @@ The scope determines the actions you can perform when you authenticate with a gr | `write_registry` | Grants write access (push) to the [Container Registry](../../packages/container_registry/index.md). | | `read_repository` | Grants read access (pull) to all repositories within a group. | | `write_repository` | Grants read and write access (pull and push) to all repositories within a group. | +| `create_runner` | Grants permission to create runners within a group. | ## Enable or disable group access token creation diff --git a/doc/user/profile/personal_access_tokens.md b/doc/user/profile/personal_access_tokens.md index 39f0d2e21df07f..48a34f78a395ba 100644 --- a/doc/user/profile/personal_access_tokens.md +++ b/doc/user/profile/personal_access_tokens.md @@ -119,7 +119,8 @@ A personal access token can perform actions based on the assigned scopes. | `read_registry` | Grants read-only (pull) access to a [Container Registry](../packages/container_registry/index.md) images if a project is private and authorization is required. Available only when the Container Registry is enabled. | | `write_registry` | Grants read-write (push) access to a [Container Registry](../packages/container_registry/index.md) images if a project is private and authorization is required. Available only when the Container Registry is enabled. ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28958) in GitLab 12.10.) | | `sudo` | Grants permission to perform API actions as any user in the system, when authenticated as an administrator. | -| `admin_mode` | Grants permission to perform API actions as an administrator, when Admin Mode is enabled. ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107875) in GitLab 15.8.) | +| `admin_mode` | Grants permission to perform API actions as an administrator, when Admin Mode is enabled. ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107875) in GitLab 15.8.) | +| `create_runner` | Grants permission to create runners. | WARNING: If you enabled [external authorization](../admin_area/settings/external_authorization.md), personal access tokens cannot access container or package registries. If you use personal access tokens to access these registries, this measure breaks this use of these tokens. Disable external authorization to use personal access tokens with container or package registries. diff --git a/doc/user/project/settings/project_access_tokens.md b/doc/user/project/settings/project_access_tokens.md index 7fd8fdf3a00dac..5528ceb08ec7b7 100644 --- a/doc/user/project/settings/project_access_tokens.md +++ b/doc/user/project/settings/project_access_tokens.md @@ -81,14 +81,15 @@ To revoke a project access token: The scope determines the actions you can perform when you authenticate with a project access token. -| Scope | Description | -|:-------------------|:------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `api` | Grants complete read and write access to the scoped project API, including the [Package Registry](../../packages/package_registry/index.md). | -| `read_api` | Grants read access to the scoped project API, including the [Package Registry](../../packages/package_registry/index.md). | +| Scope | Description | +|:-------------------|:----------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `api` | Grants complete read and write access to the scoped project API, including the [Package Registry](../../packages/package_registry/index.md). | +| `read_api` | Grants read access to the scoped project API, including the [Package Registry](../../packages/package_registry/index.md). | | `read_registry` | Grants read access (pull) to the [Container Registry](../../packages/container_registry/index.md) images if a project is private and authorization is required. | | `write_registry` | Grants write access (push) to the [Container Registry](../../packages/container_registry/index.md). | -| `read_repository` | Grants read access (pull) to the repository. | -| `write_repository` | Grants read and write access (pull and push) to the repository. | +| `read_repository` | Grants read access (pull) to the repository. | +| `write_repository` | Grants read and write access (pull and push) to the repository. | +| `create_runner` | Grants permission to create runners in the project. | ## Enable or disable project access token creation -- GitLab From 1e36da9fc8f865ae7f85564d51b2a3f8cad5c408 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 27 Jun 2023 10:12:47 +0200 Subject: [PATCH 7/7] Address MR review comment --- doc/user/group/settings/group_access_tokens.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/group/settings/group_access_tokens.md b/doc/user/group/settings/group_access_tokens.md index 4cf95a46c335ec..805c274afa6080 100644 --- a/doc/user/group/settings/group_access_tokens.md +++ b/doc/user/group/settings/group_access_tokens.md @@ -148,7 +148,7 @@ The scope determines the actions you can perform when you authenticate with a gr | `write_registry` | Grants write access (push) to the [Container Registry](../../packages/container_registry/index.md). | | `read_repository` | Grants read access (pull) to all repositories within a group. | | `write_repository` | Grants read and write access (pull and push) to all repositories within a group. | -| `create_runner` | Grants permission to create runners within a group. | +| `create_runner` | Grants permission to create runners in a group. | ## Enable or disable group access token creation -- GitLab