From 3cc8ae455889d4c5edf6f9bdafad68e3a0eda4eb Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Fri, 11 Mar 2022 18:05:06 +0100 Subject: [PATCH 1/2] Extract service to reset runner registration tokens --- .../admin/application_settings_controller.rb | 2 +- .../groups/settings/ci_cd_controller.rb | 2 +- .../projects/settings/ci_cd_controller.rb | 2 +- .../ci/runners_registration_token/reset.rb | 9 +- .../reset_registration_token_service.rb | 33 +++++++ lib/api/ci/runners.rb | 2 +- .../reset_registration_token_service_spec.rb | 97 +++++++++++++++++++ 7 files changed, 137 insertions(+), 10 deletions(-) create mode 100644 app/services/ci/runners/reset_registration_token_service.rb create mode 100644 spec/services/ci/runners/reset_registration_token_service_spec.rb diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 1d0930ba73cc15..f9df0307a0132a 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -71,7 +71,7 @@ def usage_data end def reset_registration_token - @application_setting.reset_runners_registration_token! + ::Ci::Runners::ResetRegistrationTokenService.new(@application_setting, current_user).execute flash[:notice] = _('New runners registration token has been generated!') redirect_to admin_runners_path diff --git a/app/controllers/groups/settings/ci_cd_controller.rb b/app/controllers/groups/settings/ci_cd_controller.rb index a290ef9b5e7b84..9b9e3f7b0bc63f 100644 --- a/app/controllers/groups/settings/ci_cd_controller.rb +++ b/app/controllers/groups/settings/ci_cd_controller.rb @@ -36,7 +36,7 @@ def update end def reset_registration_token - @group.reset_runners_token! + ::Ci::Runners::ResetRegistrationTokenService.new(@group, current_user).execute flash[:notice] = _('GroupSettings|New runners registration token has been generated!') redirect_to group_settings_ci_cd_path diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index dd2fb57f7ac362..3f4d26bb6ec8d7 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -64,7 +64,7 @@ def reset_cache end def reset_registration_token - @project.reset_runners_token! + ::Ci::Runners::ResetRegistrationTokenService.new(@project, current_user).execute flash[:toast] = _("New runners registration token has been generated!") redirect_to namespace_project_settings_ci_cd_path diff --git a/app/graphql/mutations/ci/runners_registration_token/reset.rb b/app/graphql/mutations/ci/runners_registration_token/reset.rb index fbf0276bc7d52c..29ef7aa2e81c63 100644 --- a/app/graphql/mutations/ci/runners_registration_token/reset.rb +++ b/app/graphql/mutations/ci/runners_registration_token/reset.rb @@ -45,6 +45,7 @@ def find_object(type:, **args) def reset_token(type:, **args) id = args[:id] + scope = nil case type when 'instance_type' @@ -52,15 +53,11 @@ def reset_token(type:, **args) scope = ApplicationSetting.current authorize!(scope) - - scope.reset_runners_registration_token! - ApplicationSetting.current_without_cache.runners_registration_token when 'group_type', 'project_type' scope = authorized_find!(type: type, id: id) - - scope.reset_runners_token! - scope.runners_token end + + ::Ci::Runners::ResetRegistrationTokenService.new(scope, current_user).execute if scope end end end diff --git a/app/services/ci/runners/reset_registration_token_service.rb b/app/services/ci/runners/reset_registration_token_service.rb new file mode 100644 index 00000000000000..ac28f4613669d1 --- /dev/null +++ b/app/services/ci/runners/reset_registration_token_service.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Ci + module Runners + class ResetRegistrationTokenService + # @param [ApplicationSetting, Project, Group] scope: the scope of the reset operation + # @param [User] user: the user performing the operation + def initialize(scope, user) + @scope = scope + @user = user + end + + def execute + return false unless @user.present? && @user.can?(:update_runners_registration_token, scope) + + case scope + when ::ApplicationSetting + scope.reset_runners_registration_token! + ApplicationSetting.current_without_cache.runners_registration_token + when ::Group, ::Project + scope.reset_runners_token! + scope.runners_token + end + end + + private + + attr_reader :scope, :user + end + end +end + +Ci::Runners::AssignRunnerService.prepend_mod diff --git a/lib/api/ci/runners.rb b/lib/api/ci/runners.rb index beff54c5b76770..3c9e887e751510 100644 --- a/lib/api/ci/runners.rb +++ b/lib/api/ci/runners.rb @@ -248,7 +248,7 @@ class Runners < ::API::Base post 'reset_registration_token' do authorize! :update_runners_registration_token, ApplicationSetting.current - ApplicationSetting.current.reset_runners_registration_token! + ::Ci::Runners::ResetRegistrationTokenService.new(ApplicationSetting.current, current_user).execute present ApplicationSetting.current_without_cache.runners_registration_token_with_expiration, with: Entities::Ci::ResetTokenResult end end diff --git a/spec/services/ci/runners/reset_registration_token_service_spec.rb b/spec/services/ci/runners/reset_registration_token_service_spec.rb new file mode 100644 index 00000000000000..edd2069f06b63b --- /dev/null +++ b/spec/services/ci/runners/reset_registration_token_service_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::ResetRegistrationTokenService, '#execute' do + subject { described_class.new(scope, current_user).execute } + + let_it_be(:user) { build(:user) } + let_it_be(:admin_user) { create_default(:user, :admin) } + + shared_examples 'a registration token reset operation' do + context 'without user' do + let(:current_user) { nil } + + it 'does not reset registration token and returns false' do + expect(scope).not_to receive(:reset_runners_token!) + + is_expected.to eq(false) + end + end + + context 'with unauthorized user' do + let(:current_user) { user } + + it 'does not reset registration token and returns false' do + expect(scope).not_to receive(:reset_runners_token!) + + is_expected.to eq(false) + end + end + + context 'with admin user', :enable_admin_mode do + let(:current_user) { admin_user } + + it 'resets registration token and returns value unchanged' do + expect(scope).to receive(:reset_runners_token!).once do + expect(scope).to receive(:runners_token).once.and_return('runners_token return value') + end + + is_expected.to eq('runners_token return value') + end + end + end + + context 'with instance scope' do + let_it_be(:scope) { create(:application_setting) } + + before do + allow(ApplicationSetting).to receive(:current).and_return(scope) + allow(ApplicationSetting).to receive(:current_without_cache).and_return(scope) + end + + context 'without user' do + let(:current_user) { nil } + + it 'does not reset registration token and returns false' do + expect(scope).not_to receive(:reset_runners_registration_token!) + + is_expected.to eq(false) + end + end + + context 'with unauthorized user' do + let(:current_user) { user } + + it 'calls assign_to on runner and returns value unchanged' do + expect(scope).not_to receive(:reset_runners_registration_token!) + + is_expected.to eq(false) + end + end + + context 'with admin user', :enable_admin_mode do + let(:current_user) { admin_user } + + it 'resets registration token and returns value unchanged' do + expect(scope).to receive(:reset_runners_registration_token!).once do + expect(scope).to receive(:runners_registration_token).once.and_return('runners_registration_token return value') + end + + is_expected.to eq('runners_registration_token return value') + end + end + end + + context 'with group scope' do + let_it_be(:scope) { create(:group) } + + it_behaves_like 'a registration token reset operation' + end + + context 'with project scope' do + let_it_be(:scope) { create(:project) } + + it_behaves_like 'a registration token reset operation' + end +end -- GitLab From a26714cad55f1313d7dd09934d8025b848bea8ac Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Mon, 14 Mar 2022 14:08:09 +0100 Subject: [PATCH 2/2] Address MR review comments --- .../reset_registration_token_service.rb | 4 +- .../reset_registration_token_service_spec.rb | 63 +++++++------------ 2 files changed, 22 insertions(+), 45 deletions(-) diff --git a/app/services/ci/runners/reset_registration_token_service.rb b/app/services/ci/runners/reset_registration_token_service.rb index ac28f4613669d1..bbe49c0464487a 100644 --- a/app/services/ci/runners/reset_registration_token_service.rb +++ b/app/services/ci/runners/reset_registration_token_service.rb @@ -11,7 +11,7 @@ def initialize(scope, user) end def execute - return false unless @user.present? && @user.can?(:update_runners_registration_token, scope) + return unless @user.present? && @user.can?(:update_runners_registration_token, scope) case scope when ::ApplicationSetting @@ -29,5 +29,3 @@ def execute end end end - -Ci::Runners::AssignRunnerService.prepend_mod diff --git a/spec/services/ci/runners/reset_registration_token_service_spec.rb b/spec/services/ci/runners/reset_registration_token_service_spec.rb index edd2069f06b63b..c4bfff51cc8d5e 100644 --- a/spec/services/ci/runners/reset_registration_token_service_spec.rb +++ b/spec/services/ci/runners/reset_registration_token_service_spec.rb @@ -6,26 +6,26 @@ subject { described_class.new(scope, current_user).execute } let_it_be(:user) { build(:user) } - let_it_be(:admin_user) { create_default(:user, :admin) } + let_it_be(:admin_user) { create(:user, :admin) } shared_examples 'a registration token reset operation' do context 'without user' do let(:current_user) { nil } - it 'does not reset registration token and returns false' do - expect(scope).not_to receive(:reset_runners_token!) + it 'does not reset registration token and returns nil' do + expect(scope).not_to receive(token_reset_method_name) - is_expected.to eq(false) + is_expected.to be_nil end end context 'with unauthorized user' do let(:current_user) { user } - it 'does not reset registration token and returns false' do - expect(scope).not_to receive(:reset_runners_token!) + it 'does not reset registration token and returns nil' do + expect(scope).not_to receive(token_reset_method_name) - is_expected.to eq(false) + is_expected.to be_nil end end @@ -33,11 +33,11 @@ let(:current_user) { admin_user } it 'resets registration token and returns value unchanged' do - expect(scope).to receive(:reset_runners_token!).once do - expect(scope).to receive(:runners_token).once.and_return('runners_token return value') + expect(scope).to receive(token_reset_method_name).once do + expect(scope).to receive(token_method_name).once.and_return("#{token_method_name} return value") end - is_expected.to eq('runners_token return value') + is_expected.to eq("#{token_method_name} return value") end end end @@ -50,48 +50,27 @@ allow(ApplicationSetting).to receive(:current_without_cache).and_return(scope) end - context 'without user' do - let(:current_user) { nil } - - it 'does not reset registration token and returns false' do - expect(scope).not_to receive(:reset_runners_registration_token!) - - is_expected.to eq(false) - end - end - - context 'with unauthorized user' do - let(:current_user) { user } - - it 'calls assign_to on runner and returns value unchanged' do - expect(scope).not_to receive(:reset_runners_registration_token!) - - is_expected.to eq(false) - end - end - - context 'with admin user', :enable_admin_mode do - let(:current_user) { admin_user } - - it 'resets registration token and returns value unchanged' do - expect(scope).to receive(:reset_runners_registration_token!).once do - expect(scope).to receive(:runners_registration_token).once.and_return('runners_registration_token return value') - end - - is_expected.to eq('runners_registration_token return value') - end + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_registration_token } + let(:token_reset_method_name) { :reset_runners_registration_token! } end end context 'with group scope' do let_it_be(:scope) { create(:group) } - it_behaves_like 'a registration token reset operation' + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_token } + let(:token_reset_method_name) { :reset_runners_token! } + end end context 'with project scope' do let_it_be(:scope) { create(:project) } - it_behaves_like 'a registration token reset operation' + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_token } + let(:token_reset_method_name) { :reset_runners_token! } + end end end -- GitLab