From 4a32b07d9e16a7926c42e11a462bf76133617f0c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 07:32:46 +0000 Subject: Add `runners_registration_token` to ApplicationSettings --- app/models/application_setting.rb | 2 +- ...0072243_add_runners_registration_token_to_application_settings.rb | 5 +++++ db/schema.rb | 3 ++- 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20151210072243_add_runners_registration_token_to_application_settings.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 1880ad9f33c..764ecd4ee20 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -27,6 +27,7 @@ # 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 @@ -128,5 +129,4 @@ class ApplicationSetting < ActiveRecord::Base /x) self.restricted_signup_domains.reject! { |d| d.empty? } end - end 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/schema.rb b/db/schema.rb index 94b87040d88..a3b649f3fd0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20151203162133) do +ActiveRecord::Schema.define(version: 20151210072243) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -49,6 +49,7 @@ ActiveRecord::Schema.define(version: 20151203162133) 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| -- cgit v1.2.1 From 9948e5bcdd9e2b9c99bba0bac119ac08daf82564 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 10:48:37 +0100 Subject: Refactor `TokenAuthenticatable` to improve reusability This adds a ability to use multiple different authentication token fields in other models. From now on it is necessary to add authentication token field manually in each class that implements this mixin. --- app/models/application_setting.rb | 3 +++ app/models/concerns/token_authenticatable.rb | 40 +++++++++++++++++----------- app/models/user.rb | 4 ++- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 764ecd4ee20..b49a5ce9054 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -31,6 +31,9 @@ # class ApplicationSetting < ActiveRecord::Base + include TokenAuthenticatable + add_authentication_token_field :runners_registration_token + CACHE_KEY = 'application_setting.last' serialize :restricted_visibility_levels diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 9b88ec1cc38..46f8ec84e25 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -1,31 +1,39 @@ 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 + write_attribute(token_field, generate_token_for(token_field)) if + read_attribute(token_field).blank? + 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 7155dd2bea7..1a8d8f1e249 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 -- cgit v1.2.1 From d90d3db32bdc5f2180651297939490821e3f7fc9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 12:43:35 +0100 Subject: Use `save!` when generating new token in `TokenAuthenticatable` --- app/models/concerns/token_authenticatable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 46f8ec84e25..fe764e8da41 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -23,7 +23,7 @@ module TokenAuthenticatable define_method("reset_#{token_field}!") do write_attribute(token_field, generate_token_for(token_field)) - save + save! end end end -- cgit v1.2.1 From 30e29bb9244e6e0395a42dd966f72dd966a59a03 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 12:44:40 +0100 Subject: Add specs for `TokenAuthenticatable` concern --- spec/models/concerns/token_authenticatable_spec.rb | 50 ++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 spec/models/concerns/token_authenticatable_spec.rb diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb new file mode 100644 index 00000000000..1b553173415 --- /dev/null +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -0,0 +1,50 @@ +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 'ensured 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 } + 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 -- cgit v1.2.1 From 5a7b94f6d1bfd7c376ecb6af45a31bb774358826 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 13:05:59 +0100 Subject: Ensure that app settings contains runners registration token --- app/models/application_setting.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index b49a5ce9054..faa0bdf840b 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -78,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 -- cgit v1.2.1 From 219afbad2adf0c1b7b3d4ccc116f44ba82818cf1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 13:06:44 +0100 Subject: Display runners registration token in CI runners config --- app/views/ci/admin/runners/index.html.haml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/views/ci/admin/runners/index.html.haml b/app/views/ci/admin/runners/index.html.haml index bacaccfbffa..c71710a831c 100644 --- a/app/views/ci/admin/runners/index.html.haml +++ b/app/views/ci/admin/runners/index.html.haml @@ -1,6 +1,9 @@ %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 #{current_application_settings.runners_registration_token} .bs-callout %p -- cgit v1.2.1 From e2e43a114b5af63aad6d9badcb727c8829abb167 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 10 Dec 2015 13:50:00 +0100 Subject: Use new runners registration token to register CI runners --- config/initializers/4_ci_app.rb | 2 -- lib/ci/api/helpers.rb | 6 +++++- lib/ci/api/runners.rb | 2 +- spec/requests/ci/api/runners_spec.rb | 9 ++++++--- 4 files changed, 12 insertions(+), 7 deletions(-) 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/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index 02502333756..12d7396189d 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! @@ -22,6 +22,10 @@ module Ci forbidden! unless token && build.valid_token?(token) end + def runner_registration_token_valid? + params[:token] == current_application_settings.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 1466fe4356e..3edf067f46d 100644 --- a/lib/ci/api/runners.rb +++ b/lib/ci/api/runners.rb @@ -40,7 +40,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/requests/ci/api/runners_spec.rb b/spec/requests/ci/api/runners_spec.rb index 11dc089e1f5..962d34a0f95 100644 --- a/spec/requests/ci/api/runners_spec.rb +++ b/spec/requests/ci/api/runners_spec.rb @@ -4,8 +4,11 @@ describe Ci::API::API do include ApiHelpers include StubGitlabCalls + let(:registration_token) { 'abcdefg123456' } + before do stub_gitlab_calls + stub_application_setting(runners_registration_token: registration_token) end describe "GET /runners" do @@ -33,20 +36,20 @@ describe Ci::API::API do 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"]) } -- cgit v1.2.1 From 2da3cf314651d22f85059d99476ec7952950b44f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Dec 2015 10:22:05 +0100 Subject: Add CI runners registration token reset button --- app/controllers/admin/application_settings_controller.rb | 6 ++++++ app/views/ci/admin/runners/index.html.haml | 13 ++++++++++++- config/routes.rb | 1 + 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index a9bcfc7456a..48040359389 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 ci_admin_runners_path + end + private def set_application_setting diff --git a/app/views/ci/admin/runners/index.html.haml b/app/views/ci/admin/runners/index.html.haml index c71710a831c..95bc8ebd40f 100644 --- a/app/views/ci/admin/runners/index.html.haml +++ b/app/views/ci/admin/runners/index.html.haml @@ -3,7 +3,18 @@ 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 #{current_application_settings.runners_registration_token} + %code{ id: 'runners-token' } #{current_application_settings.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/config/routes.rb b/config/routes.rb index 061a8fd5da4..54d6afe2dca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -276,6 +276,7 @@ Rails.application.routes.draw do resource :application_settings, only: [:show, :update] do resources :services + put :reset_runners_token end resources :labels -- cgit v1.2.1 From 28ad40d9740076677420612443358c2976fe0916 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Dec 2015 11:14:55 +0100 Subject: Add specs for feature that regenerates runners token --- spec/features/ci/admin/runners_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/spec/features/ci/admin/runners_spec.rb b/spec/features/ci/admin/runners_spec.rb index b83744f53a8..0c5aebb3a7b 100644 --- a/spec/features/ci/admin/runners_spec.rb +++ b/spec/features/ci/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.runners_registration_token } + before { visit ci_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 -- cgit v1.2.1 From 917effb7379f8e871dbc113d7c7dab89473d4bc8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Dec 2015 13:37:54 +0000 Subject: Make sure that token `ensure_*` method always returns a token --- app/models/concerns/token_authenticatable.rb | 8 ++++++-- spec/models/concerns/token_authenticatable_spec.rb | 9 ++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index fe764e8da41..56d38fe8250 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -17,8 +17,12 @@ module TokenAuthenticatable end define_method("ensure_#{token_field}") do - write_attribute(token_field, generate_token_for(token_field)) if - read_attribute(token_field).blank? + 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 diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb index 1b553173415..a9b0b64e5de 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -13,7 +13,7 @@ describe User, 'TokenAuthenticatable' do let(:token_field) { :authentication_token } it_behaves_like 'TokenAuthenticatable' - describe 'ensured authentication token' do + describe 'ensures authentication token' do subject { create(:user).send(token_field) } it { is_expected.to be_a String } end @@ -29,6 +29,13 @@ describe ApplicationSetting, 'TokenAuthenticatable' do 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 -- cgit v1.2.1 From 6b0c0d5bcc7940c7d84b8af965b2c4f9ceeb4175 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Dec 2015 14:43:44 +0100 Subject: Ensure that runners registration token is present --- app/views/ci/admin/runners/index.html.haml | 2 +- lib/ci/api/helpers.rb | 2 +- spec/features/ci/admin/runners_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/ci/admin/runners/index.html.haml b/app/views/ci/admin/runners/index.html.haml index 95bc8ebd40f..0574ed38015 100644 --- a/app/views/ci/admin/runners/index.html.haml +++ b/app/views/ci/admin/runners/index.html.haml @@ -3,7 +3,7 @@ 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.runners_registration_token} + %code{ id: 'runners-token' } #{current_application_settings.ensure_runners_registration_token} .bs-callout.clearfix .pull-left diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index 12d7396189d..abd1869efc0 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -23,7 +23,7 @@ module Ci end def runner_registration_token_valid? - params[:token] == current_application_settings.runners_registration_token + params[:token] == current_application_settings.ensure_runners_registration_token end def update_runner_last_contact diff --git a/spec/features/ci/admin/runners_spec.rb b/spec/features/ci/admin/runners_spec.rb index 0c5aebb3a7b..fd1967d0698 100644 --- a/spec/features/ci/admin/runners_spec.rb +++ b/spec/features/ci/admin/runners_spec.rb @@ -63,7 +63,7 @@ describe "Admin Runners" do end describe 'runners registration token' do - let!(:token) { current_application_settings.runners_registration_token } + let!(:token) { current_application_settings.ensure_runners_registration_token } before { visit ci_admin_runners_path } it 'has a registration token' do -- cgit v1.2.1 From bc1cfd384f4f972ad887b0501280b9f94d4c94ac Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Dec 2015 15:23:43 +0100 Subject: Stub also `ensure_*` method in CI runners API specs --- spec/requests/ci/api/runners_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/requests/ci/api/runners_spec.rb b/spec/requests/ci/api/runners_spec.rb index 962d34a0f95..ef40d757665 100644 --- a/spec/requests/ci/api/runners_spec.rb +++ b/spec/requests/ci/api/runners_spec.rb @@ -8,6 +8,7 @@ describe Ci::API::API do before do stub_gitlab_calls + stub_application_setting(ensure_runners_registration_token: registration_token) stub_application_setting(runners_registration_token: registration_token) end -- cgit v1.2.1 From 6586856a1572535e0b9ca2f9021dfd88a158ffdd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 14 Dec 2015 14:03:58 +0100 Subject: Use a new admin runners path when reseting runners token --- app/controllers/admin/application_settings_controller.rb | 2 +- spec/features/admin/admin_runners_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 48040359389..9dd16f8c735 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -16,7 +16,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController def reset_runners_token @application_setting.reset_runners_registration_token! flash[:notice] = 'New runners registration token has been generated!' - redirect_to ci_admin_runners_path + redirect_to admin_runners_path end private diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index d5dd11a01d3..66a2cc0c157 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -64,7 +64,7 @@ describe "Admin Runners" do describe 'runners registration token' do let!(:token) { current_application_settings.ensure_runners_registration_token } - before { visit ci_admin_runners_path } + before { visit admin_runners_path } it 'has a registration token' do expect(page).to have_content("Registration token is #{token}") -- cgit v1.2.1