Merge branch 'fix/ci-runners-token-persistence' into 'master'

Fix method that ensures authentication token

Until now, `ensure_#{token_filed_name}!` method didn't persist new token in database.

This closes #4235.

See merge request !2185

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

app/models/concerns/token_authenticatable.rb

@@ -18,15 +18,16 @@ module TokenAuthenticatable
       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
+        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
@@ -34,7 +35,12 @@ 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)

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

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

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

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,11 +25,11 @@ 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}") }
@@ -36,11 +37,21 @@ describe ApplicationSetting, 'TokenAuthenticatable' do
         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]
+        end
+      end
     end
     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

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