From 76f7e80455f1f5b5b052c7d8caf519713b016eea Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 23 Dec 2015 09:29:04 +0100 Subject: [PATCH 1/3] Fix method that ensures authentication token Until now, `ensure_#{token_filed_name}` method didn't persist new token in database. This closes #4235. --- app/models/concerns/token_authenticatable.rb | 8 ++------ spec/models/concerns/token_authenticatable_spec.rb | 4 ++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 488ff8c31b7..e84c5804cf4 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -17,12 +17,8 @@ module TokenAuthenticatable 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 + send("reset_#{token_field}!") if read_attribute(token_field).blank? + read_attribute(token_field) 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 a9b0b64e5de..07ec3a80e60 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -35,6 +35,10 @@ describe ApplicationSetting, 'TokenAuthenticatable' do it { is_expected.to be_a String } it { is_expected.to_not be_blank } + + it 'should persist new token' do + expect(subject).to eq described_class.current[token_field] + end end end From 37731ba1a1a926f1e4bd6dc620066822f3a4b0da Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 23 Dec 2015 10:47:18 +0100 Subject: [PATCH 2/3] Add method that persist ensured token in `TokenAuthenticatable` --- app/models/concerns/token_authenticatable.rb | 15 +++++++++++++-- .../concerns/token_authenticatable_spec.rb | 19 +++++++++++++------ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index e84c5804cf4..2e86fb31ce2 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -17,12 +17,17 @@ module TokenAuthenticatable end define_method("ensure_#{token_field}") do + current_token = read_attribute(token_field) + current_token.blank? ? write_new_token(token_field) : current_token + end + + define_method("ensure_#{token_field}!") do send("reset_#{token_field}!") if read_attribute(token_field).blank? read_attribute(token_field) end define_method("reset_#{token_field}!") do - write_attribute(token_field, generate_token_for(token_field)) + write_new_token(token_field) save! end end @@ -30,10 +35,16 @@ module TokenAuthenticatable private - def generate_token_for(token_field) + def write_new_token(token_field) + new_token = generate_token(token_field) + write_attribute(token_field, new_token) + end + + def generate_token(token_field) loop do token = Devise.friendly_token break token unless self.class.unscoped.find_by(token_field => token) end end + end diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb index 07ec3a80e60..30c0a04b840 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -2,7 +2,8 @@ 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 be_private_method_defined(:generate_token) } + it { expect(described_class).to be_private_method_defined(:write_new_token) } 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}!") } @@ -24,17 +25,21 @@ describe ApplicationSetting, 'TokenAuthenticatable' do 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 'token field accessor' do + subject { described_class.new.send(token_field) } + it { is_expected.to_not be_blank } + end 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 + + describe 'ensured! token' do + subject { described_class.new.send("ensure_#{token_field}!") } it 'should persist new token' do expect(subject).to eq described_class.current[token_field] @@ -44,7 +49,9 @@ describe ApplicationSetting, 'TokenAuthenticatable' do context 'token is generated' do before { subject.send("reset_#{token_field}!") } - it { expect(token).to be_a String } + it 'persists a new token 'do + expect(subject.send(:read_attribute, token_field)).to be_a String + end end end From 3e6950481a90a83f183397f11b8f2a5d21233cfb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 23 Dec 2015 10:48:10 +0100 Subject: [PATCH 3/3] Use method that creates runners registration token `runners_registration_token` now creates a new token if it is blank. --- app/models/application_setting.rb | 4 ++++ app/models/concerns/token_authenticatable.rb | 1 - app/views/admin/runners/index.html.haml | 2 +- lib/ci/api/helpers.rb | 2 +- spec/features/admin/admin_runners_spec.rb | 2 +- spec/requests/ci/api/runners_spec.rb | 1 - 6 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 1f4e8b3ef24..724429e7558 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -134,4 +134,8 @@ class ApplicationSetting < ActiveRecord::Base /x) self.restricted_signup_domains.reject! { |d| d.empty? } end + + def runners_registration_token + ensure_runners_registration_token! + end end diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 2e86fb31ce2..885deaf78d2 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -46,5 +46,4 @@ module TokenAuthenticatable break token unless self.class.unscoped.find_by(token_field => token) end end - end diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index c5fb3c95506..c407972cd08 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/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.ensure_runners_registration_token} + %code{ id: 'runners-token' } #{current_application_settings.runners_registration_token} .bs-callout.clearfix .pull-left diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index 443563c2e4a..1c91204e98c 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -19,7 +19,7 @@ module Ci end def runner_registration_token_valid? - params[:token] == current_application_settings.ensure_runners_registration_token + params[:token] == current_application_settings.runners_registration_token end def update_runner_last_contact diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index 66a2cc0c157..26d03944b8a 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -63,7 +63,7 @@ describe "Admin Runners" do end describe 'runners registration token' do - let!(:token) { current_application_settings.ensure_runners_registration_token } + let!(:token) { current_application_settings.runners_registration_token } before { visit admin_runners_path } it 'has a registration token' do diff --git a/spec/requests/ci/api/runners_spec.rb b/spec/requests/ci/api/runners_spec.rb index 567da013e6f..5942aa7a1b5 100644 --- a/spec/requests/ci/api/runners_spec.rb +++ b/spec/requests/ci/api/runners_spec.rb @@ -8,7 +8,6 @@ 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