diff options
20 files changed, 169 insertions, 34 deletions
diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index a9bcfc7456a..9dd16f8c735 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -13,6 +13,12 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController end end + def reset_runners_token + @application_setting.reset_runners_registration_token! + flash[:notice] = 'New runners registration token has been generated!' + redirect_to admin_runners_path + end + private def set_application_setting diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 1880ad9f33c..faa0bdf840b 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -27,9 +27,13 @@ # admin_notification_email :string(255) # shared_runners_enabled :boolean default(TRUE), not null # max_artifacts_size :integer default(100), not null +# runners_registration_token :string(255) # class ApplicationSetting < ActiveRecord::Base + include TokenAuthenticatable + add_authentication_token_field :runners_registration_token + CACHE_KEY = 'application_setting.last' serialize :restricted_visibility_levels @@ -74,6 +78,8 @@ class ApplicationSetting < ActiveRecord::Base end end + before_save :ensure_runners_registration_token + after_commit do Rails.cache.write(CACHE_KEY, self) end @@ -128,5 +134,4 @@ class ApplicationSetting < ActiveRecord::Base /x) self.restricted_signup_domains.reject! { |d| d.empty? } end - end diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 9b88ec1cc38..56d38fe8250 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -1,31 +1,43 @@ module TokenAuthenticatable extend ActiveSupport::Concern - module ClassMethods - def find_by_authentication_token(authentication_token = nil) - if authentication_token - where(authentication_token: authentication_token).first - end + class_methods do + def authentication_token_fields + @token_fields || [] end - end - def ensure_authentication_token - if authentication_token.blank? - self.authentication_token = generate_authentication_token - end - end + private + + def add_authentication_token_field(token_field) + @token_fields = [] unless @token_fields + @token_fields << token_field - def reset_authentication_token! - self.authentication_token = generate_authentication_token - save + define_singleton_method("find_by_#{token_field}") do |token| + where(token_field => token).first if token + end + + define_method("ensure_#{token_field}") do + current_token = read_attribute(token_field) + if current_token.blank? + write_attribute(token_field, generate_token_for(token_field)) + else + current_token + end + end + + define_method("reset_#{token_field}!") do + write_attribute(token_field, generate_token_for(token_field)) + save! + end + end end private - def generate_authentication_token + def generate_token_for(token_field) loop do token = Devise.friendly_token - break token unless self.class.unscoped.where(authentication_token: token).first + break token unless self.class.unscoped.where(token_field => token).first end end end diff --git a/app/models/user.rb b/app/models/user.rb index 87d46a49430..fdd14f4571d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -69,8 +69,10 @@ class User < ActiveRecord::Base include Gitlab::CurrentSettings include Referable include Sortable - include TokenAuthenticatable include CaseSensitivity + include TokenAuthenticatable + + add_authentication_token_field :authentication_token default_value_for :admin, false default_value_for :can_create_group, gitlab_config.default_can_create_group diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index 26b180b4052..c5fb3c95506 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/admin/runners/index.html.haml @@ -1,6 +1,20 @@ %p.lead - %span To register a new runner you should enter the following registration token. With this token the runner will request a unique runner token and use that for future communication. - %code #{GitlabCi::REGISTRATION_TOKEN} + %span + To register a new runner you should enter the following registration token. + With this token the runner will request a unique runner token and use that for future communication. + Registration token is + %code{ id: 'runners-token' } #{current_application_settings.ensure_runners_registration_token} + +.bs-callout.clearfix + .pull-left + %p + You can reset runners registration token by pressing a button below. + %p + = button_to reset_runners_token_admin_application_settings_path, + method: :put, class: 'btn btn-default', + data: { confirm: 'Are you sure you want to reset registration token?' } do + = icon('refresh') + Reset runners registration token .bs-callout %p diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index 32051852dc0..8700b4820cd 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -60,7 +60,7 @@ = project.name_with_namespace %td .pull-right - = link_to 'Disable', [:admin, project.namespace, project, runner_project], method: :delete, class: 'btn btn-danger btn-xs' + = link_to 'Disable', [:admin, project.namespace.becomes(Namespace), project, runner_project], method: :delete, class: 'btn btn-danger btn-xs' %table.table %thead diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml index 08af124274b..2168294c683 100644 --- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml +++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml @@ -12,7 +12,7 @@ - else The source branch will not be removed. - - remove_source_branch_button = @merge_request.can_remove_source_branch?(current_user) && !should_remove_source_branch + - remove_source_branch_button = @merge_request.can_remove_source_branch?(current_user) && !should_remove_source_branch && @merge_request.merge_user == current_user - user_can_cancel_automatic_merge = @merge_request.can_cancel_merge_when_build_succeeds?(current_user) - if remove_source_branch_button || user_can_cancel_automatic_merge .clearfix.prepend-top-10 diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 4d95afc28bb..47ec420189d 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -18,7 +18,7 @@ - runner_project = @project.runner_projects.find_by(runner_id: runner) = link_to 'Disable for this project', namespace_project_runner_project_path(@project.namespace, @project, runner_project), data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-sm' - elsif runner.specific? - = form_for [@project.namespace, @project, @project.runner_projects.new] do |f| + = form_for [@project.namespace.becomes(Namespace), @project, @project.runner_projects.new] do |f| = f.hidden_field :runner_id, value: runner.id = f.submit 'Enable for this project', class: 'btn btn-sm' .pull-right diff --git a/config/initializers/4_ci_app.rb b/config/initializers/4_ci_app.rb index cac8edb32bf..d252e403102 100644 --- a/config/initializers/4_ci_app.rb +++ b/config/initializers/4_ci_app.rb @@ -1,8 +1,6 @@ module GitlabCi VERSION = Gitlab::VERSION REVISION = Gitlab::REVISION - - REGISTRATION_TOKEN = SecureRandom.hex(10) def self.config Settings diff --git a/config/routes.rb b/config/routes.rb index 50836d63fa5..e2d4fcb65a8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -245,6 +245,7 @@ Rails.application.routes.draw do resource :application_settings, only: [:show, :update] do resources :services + put :reset_runners_token end resources :labels diff --git a/db/migrate/20151209145909_migrate_ci_emails.rb b/db/migrate/20151209145909_migrate_ci_emails.rb index 202fac8e3fc..7f330a2cf0a 100644 --- a/db/migrate/20151209145909_migrate_ci_emails.rb +++ b/db/migrate/20151209145909_migrate_ci_emails.rb @@ -25,7 +25,11 @@ class MigrateCiEmails < ActiveRecord::Migration # This function escapes double-quotes and slash def escape_text(name) - "REPLACE(REPLACE(#{name}, '\\', '\\\\'), '\"', '\\\"')" + if Gitlab::Database.postgresql? + "REPLACE(REPLACE(#{name}, '\\', '\\\\'), '\"', '\\\"')" + else + "REPLACE(REPLACE(#{name}, '\\\\', '\\\\\\\\'), '\\\"', '\\\\\\\"')" + end end # This function returns 0 or 1 for column diff --git a/db/migrate/20151210072243_add_runners_registration_token_to_application_settings.rb b/db/migrate/20151210072243_add_runners_registration_token_to_application_settings.rb new file mode 100644 index 00000000000..00f88180e46 --- /dev/null +++ b/db/migrate/20151210072243_add_runners_registration_token_to_application_settings.rb @@ -0,0 +1,5 @@ +class AddRunnersRegistrationTokenToApplicationSettings < ActiveRecord::Migration + def change + add_column :application_settings, :runners_registration_token, :string + end +end diff --git a/db/migrate/20151210125930_migrate_ci_to_project.rb b/db/migrate/20151210125930_migrate_ci_to_project.rb index d17b2a425f8..7dfe05174ee 100644 --- a/db/migrate/20151210125930_migrate_ci_to_project.rb +++ b/db/migrate/20151210125930_migrate_ci_to_project.rb @@ -27,11 +27,11 @@ class MigrateCiToProject < ActiveRecord::Migration def migrate_project_column(column, new_column = nil) new_column ||= column subquery = "SELECT ci_projects.#{column} FROM ci_projects WHERE projects.id = ci_projects.gitlab_id" - execute("UPDATE projects SET #{new_column}=(#{subquery}) WHERE #{new_column} IS NULL AND (#{subquery}) IS NOT NULL") + execute("UPDATE projects SET #{new_column}=(#{subquery}) WHERE (#{subquery}) IS NOT NULL") end def migrate_ci_service - subquery = "SELECT active FROM services WHERE projects.id = services.project_id AND type='GitlabCiService'" - execute("UPDATE projects SET builds_enabled=(#{subquery}) WHERE builds_enabled IS NULL AND (#{subquery}) IS NOT NULL") + subquery = "SELECT active FROM services WHERE projects.id = services.project_id AND type='GitlabCiService' LIMIT 1" + execute("UPDATE projects SET builds_enabled=(#{subquery}) WHERE (#{subquery}) IS NOT NULL") end end diff --git a/db/schema.rb b/db/schema.rb index d17930b84b0..0167e30ff8b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -49,6 +49,7 @@ ActiveRecord::Schema.define(version: 20151210125932) do t.string "admin_notification_email" t.boolean "shared_runners_enabled", default: true, null: false t.integer "max_artifacts_size", default: 100, null: false + t.string "runners_registration_token" end create_table "audit_events", force: :cascade do |t| diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index 9891b5e38ea..443563c2e4a 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -6,7 +6,7 @@ module Ci UPDATE_RUNNER_EVERY = 60 def authenticate_runners! - forbidden! unless params[:token] == GitlabCi::REGISTRATION_TOKEN + forbidden! unless runner_registration_token_valid? end def authenticate_runner! @@ -18,6 +18,10 @@ module Ci forbidden! unless token && build.valid_token?(token) end + def runner_registration_token_valid? + params[:token] == current_application_settings.ensure_runners_registration_token + end + def update_runner_last_contact # Use a random threshold to prevent beating DB updates contacted_at_max_age = UPDATE_RUNNER_EVERY + Random.rand(UPDATE_RUNNER_EVERY) diff --git a/lib/ci/api/runners.rb b/lib/ci/api/runners.rb index 8704917b9e4..bfc14fe7a6b 100644 --- a/lib/ci/api/runners.rb +++ b/lib/ci/api/runners.rb @@ -29,7 +29,7 @@ module Ci required_attributes! [:token] runner = - if params[:token] == GitlabCi::REGISTRATION_TOKEN + if runner_registration_token_valid? # Create shared runner. Requires admin access Ci::Runner.create( description: params[:description], diff --git a/spec/features/atom/builds_spec.rb b/spec/features/admin/admin_builds_spec.rb index 72764b1629d..72764b1629d 100644 --- a/spec/features/atom/builds_spec.rb +++ b/spec/features/admin/admin_builds_spec.rb diff --git a/spec/features/atom/runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index b1f2d401042..66a2cc0c157 100644 --- a/spec/features/atom/runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -61,4 +61,26 @@ describe "Admin Runners" do it { expect(page).not_to have_content(@project2.name_with_namespace) } end end + + describe 'runners registration token' do + let!(:token) { current_application_settings.ensure_runners_registration_token } + before { visit admin_runners_path } + + it 'has a registration token' do + expect(page).to have_content("Registration token is #{token}") + expect(page).to have_selector('#runners-token', text: token) + end + + describe 'reload registration token' do + let(:page_token) { find('#runners-token').text } + + before do + click_button 'Reset runners registration token' + end + + it 'changes registration token' do + expect(page_token).to_not eq token + end + end + end end diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb new file mode 100644 index 00000000000..a9b0b64e5de --- /dev/null +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +shared_examples 'TokenAuthenticatable' do + describe 'dynamically defined methods' do + it { expect(described_class).to be_private_method_defined(:generate_token_for) } + it { expect(described_class).to respond_to("find_by_#{token_field}") } + it { is_expected.to respond_to("ensure_#{token_field}") } + it { is_expected.to respond_to("reset_#{token_field}!") } + end +end + +describe User, 'TokenAuthenticatable' do + let(:token_field) { :authentication_token } + it_behaves_like 'TokenAuthenticatable' + + describe 'ensures authentication token' do + subject { create(:user).send(token_field) } + it { is_expected.to be_a String } + end +end + +describe ApplicationSetting, 'TokenAuthenticatable' do + let(:token_field) { :runners_registration_token } + it_behaves_like 'TokenAuthenticatable' + + describe 'generating new token' do + subject { described_class.new } + let(:token) { subject.send(token_field) } + + context 'token is not generated yet' do + it { expect(token).to be nil } + + describe 'ensured token' do + subject { described_class.new.send("ensure_#{token_field}") } + + it { is_expected.to be_a String } + it { is_expected.to_not be_blank } + end + end + + context 'token is generated' do + before { subject.send("reset_#{token_field}!") } + it { expect(token).to be_a String } + end + end + + describe 'multiple token fields' do + before do + described_class.send(:add_authentication_token_field, :yet_another_token) + end + + describe '.token_fields' do + subject { described_class.authentication_token_fields } + it { is_expected.to include(:runners_registration_token, :yet_another_token) } + end + end +end diff --git a/spec/requests/ci/api/runners_spec.rb b/spec/requests/ci/api/runners_spec.rb index b98c4d506c7..567da013e6f 100644 --- a/spec/requests/ci/api/runners_spec.rb +++ b/spec/requests/ci/api/runners_spec.rb @@ -4,26 +4,30 @@ describe Ci::API::API do include ApiHelpers include StubGitlabCalls + let(:registration_token) { 'abcdefg123456' } + before do stub_gitlab_calls + stub_application_setting(ensure_runners_registration_token: registration_token) + stub_application_setting(runners_registration_token: registration_token) end describe "POST /runners/register" do describe "should create a runner if token provided" do - before { post ci_api("/runners/register"), token: GitlabCi::REGISTRATION_TOKEN } + before { post ci_api("/runners/register"), token: registration_token } it { expect(response.status).to eq(201) } end describe "should create a runner with description" do - before { post ci_api("/runners/register"), token: GitlabCi::REGISTRATION_TOKEN, description: "server.hostname" } + before { post ci_api("/runners/register"), token: registration_token, description: "server.hostname" } it { expect(response.status).to eq(201) } it { expect(Ci::Runner.first.description).to eq("server.hostname") } end describe "should create a runner with tags" do - before { post ci_api("/runners/register"), token: GitlabCi::REGISTRATION_TOKEN, tag_list: "tag1, tag2" } + before { post ci_api("/runners/register"), token: registration_token, tag_list: "tag1, tag2" } it { expect(response.status).to eq(201) } it { expect(Ci::Runner.first.tag_list.sort).to eq(["tag1", "tag2"]) } |