Merge branch 'security-if-51113-hash_tokens-11-4' into 'security-11-4'

[11.4] Persist only SHA digest of PersonalAccessToken#token

See merge request gitlab/gitlabhq!2551

5 files changed, 389 insertions, 11 deletions

diff --git a/spec/lib/gitlab/background_migration/digest_column_spec.rb b/spec/lib/gitlab/background_migration/digest_column_spec.rb
new file mode 100644
index 00000000000..3e107ac3027
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/digest_column_spec.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913142237 do
+  let(:personal_access_tokens) { table(:personal_access_tokens) }
+  let(:users) { table(:users) }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    context 'token is not yet hashed' do
+      before do
+        users.create(id: 1, email: 'user@example.com', projects_limit: 10)
+        personal_access_tokens.create!(id: 1, user_id: 1, name: 'pat-01', token: 'token-01')
+      end
+
+      it 'saves token digest' do
+        expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.to(
+          change { PersonalAccessToken.find(1).token_digest }.from(nil).to(Gitlab::CryptoHelper.sha256('token-01')))
+      end
+
+      it 'erases token' do
+        expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.to(
+          change { PersonalAccessToken.find(1).token }.from('token-01').to(nil))
+      end
+    end
+
+    context 'token is already hashed' do
+      before do
+        users.create(id: 1, email: 'user@example.com', projects_limit: 10)
+        personal_access_tokens.create!(id: 1, user_id: 1, name: 'pat-01', token_digest: 'token-digest-01')
+      end
+
+      it 'does not change existing token digest' do
+        expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.not_to(
+          change { PersonalAccessToken.find(1).token_digest })
+      end
+
+      it 'leaves token empty' do
+        expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.not_to(
+          change { PersonalAccessToken.find(1).token }.from(nil))
+      end
+    end
+  end
+end
diff --git a/spec/migrations/schedule_digest_personal_access_tokens_spec.rb b/spec/migrations/schedule_digest_personal_access_tokens_spec.rb
new file mode 100644
index 00000000000..6d155f78342
--- /dev/null
+++ b/spec/migrations/schedule_digest_personal_access_tokens_spec.rb
@@ -0,0 +1,46 @@
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20180913142237_schedule_digest_personal_access_tokens.rb')
+
+describe ScheduleDigestPersonalAccessTokens, :migration, :sidekiq do
+  let(:personal_access_tokens) { table(:personal_access_tokens) }
+  let(:users) { table(:users) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 4)
+
+    users.create(id: 1, email: 'user@example.com', projects_limit: 10)
+
+    personal_access_tokens.create!(id: 1, user_id: 1, name: 'pat-01', token: 'token-01')
+    personal_access_tokens.create!(id: 2, user_id: 1, name: 'pat-02', token: 'token-02')
+    personal_access_tokens.create!(id: 3, user_id: 1, name: 'pat-03', token_digest: 'token_digest')
+    personal_access_tokens.create!(id: 4, user_id: 1, name: 'pat-04', token: 'token-04')
+    personal_access_tokens.create!(id: 5, user_id: 1, name: 'pat-05', token: 'token-05')
+    personal_access_tokens.create!(id: 6, user_id: 1, name: 'pat-06', token: 'token-06')
+  end
+
+  it 'correctly schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      migrate!
+
+      expect(described_class::MIGRATION).to(
+        be_scheduled_delayed_migration(
+          5.minutes, 'PersonalAccessToken', 'token', 'token_digest', 1, 5))
+      expect(described_class::MIGRATION).to(
+        be_scheduled_delayed_migration(
+          10.minutes, 'PersonalAccessToken', 'token', 'token_digest', 6, 6))
+      expect(BackgroundMigrationWorker.jobs.size).to eq 2
+    end
+  end
+
+  it 'schedules background migrations' do
+    perform_enqueued_jobs do
+      plain_text_token = 'token IS NOT NULL'
+
+      expect(personal_access_tokens.where(plain_text_token).count).to eq 5
+
+      migrate!
+
+      expect(personal_access_tokens.where(plain_text_token).count).to eq 0
+    end
+  end
+end
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index 9b804429138..782687516ae 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -2,8 +2,6 @@ require 'spec_helper'
 
 shared_examples 'TokenAuthenticatable' do
   describe 'dynamically defined methods' do
-    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("set_#{token_field}") }
@@ -66,13 +64,275 @@ describe ApplicationSetting, 'TokenAuthenticatable' do
   end
 
   describe 'multiple token fields' do
-    before do
+    before(:all) 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) }
+    it { is_expected.to respond_to(:ensure_runners_registration_token) }
+    it { is_expected.to respond_to(:ensure_yet_another_token) }
+  end
+
+  describe 'setting same token field multiple times' do
+    subject { described_class.send(:add_authentication_token_field, :runners_registration_token) }
+
+    it 'raises error' do
+      expect {subject}.to raise_error(ArgumentError)
+    end
+  end
+end
+
+describe PersonalAccessToken, 'TokenAuthenticatable' do
+  let(:personal_access_token_name) { 'test-pat-01' }
+  let(:token_value) { 'token' }
+  let(:user) { create(:user) }
+  let(:personal_access_token) do
+    described_class.new(name: personal_access_token_name,
+                        user_id: user.id,
+                        scopes: [:api],
+                        token: token,
+                        token_digest: token_digest)
+  end
+
+  before do
+    allow(Devise).to receive(:friendly_token).and_return(token_value)
+  end
+
+  describe '.find_by_token' do
+    subject { PersonalAccessToken.find_by_token(token_value) }
+
+    before do
+      personal_access_token.save
+    end
+
+    context 'token_digest already exists' do
+      let(:token) { nil }
+      let(:token_digest) { Gitlab::CryptoHelper.sha256(token_value) }
+
+      it 'finds the token' do
+        expect(subject).not_to be_nil
+        expect(subject.name).to eql(personal_access_token_name)
+      end
+    end
+
+    context 'token_digest does not exist' do
+      let(:token) { token_value }
+      let(:token_digest) { nil }
+
+      it 'finds the token' do
+        expect(subject).not_to be_nil
+        expect(subject.name).to eql(personal_access_token_name)
+      end
+    end
+  end
+
+  describe '#set_token'   do
+    let(:new_token_value) { 'new-token' }
+    subject { personal_access_token.set_token(new_token_value) }
+
+    context 'token_digest already exists' do
+      let(:token) { nil }
+      let(:token_digest) { Gitlab::CryptoHelper.sha256(token_value) }
+
+      it 'overwrites token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(new_token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(new_token_value))
+      end
+    end
+
+    context 'token_digest does not exist but token does' do
+      let(:token) { token_value }
+      let(:token_digest) { nil }
+
+      it 'creates new token_digest and clears token' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(new_token_value)
+        expect(personal_access_token.token_digest).to eql(Gitlab::CryptoHelper.sha256(new_token_value))
+      end
+    end
+
+    context 'token_digest does not exist, nor token' do
+      let(:token) { nil }
+      let(:token_digest) { nil }
+
+      it 'creates new token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(new_token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(new_token_value))
+      end
+    end
+  end
+
+  describe '#ensure_token' do
+    subject { personal_access_token.ensure_token }
+
+    context 'token_digest already exists' do
+      let(:token) { nil }
+      let(:token_digest) { Gitlab::CryptoHelper.sha256(token_value) }
+
+      it 'does not change token fields' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to be_nil
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+
+    context 'token_digest does not exist but token does' do
+      let(:token) { token_value }
+      let(:token_digest) { nil }
+
+      it 'does not change token fields' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to eql(token_value)
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to be_nil
+      end
+    end
+
+    context 'token_digest does not exist, nor token' do
+      let(:token) { nil }
+      let(:token_digest) { nil }
+
+      it 'creates token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+  end
+
+  describe '#ensure_token!' do
+    subject { personal_access_token.ensure_token! }
+
+    context 'token_digest already exists' do
+      let(:token) { nil }
+      let(:token_digest) { Gitlab::CryptoHelper.sha256(token_value) }
+
+      it 'does not change token fields' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to be_nil
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+
+    context 'token_digest does not exist but token does' do
+      let(:token) { token_value }
+      let(:token_digest) { nil }
+
+      it 'does not change token fields' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to eql(token_value)
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to be_nil
+      end
+    end
+
+    context 'token_digest does not exist, nor token' do
+      let(:token) { nil }
+      let(:token_digest) { nil }
+
+      it 'creates token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+  end
+
+  describe '#reset_token!' do
+    subject { personal_access_token.reset_token! }
+
+    context 'token_digest already exists' do
+      let(:token) { nil }
+      let(:token_digest) { Gitlab::CryptoHelper.sha256('old-token') }
+
+      it 'creates new token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+
+    context 'token_digest does not exist but token does' do
+      let(:token) { 'old-token' }
+      let(:token_digest) { nil }
+
+      it 'creates new token_digest and clears token' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql(Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+
+    context 'token_digest does not exist, nor token' do
+      let(:token) { nil }
+      let(:token_digest) { nil }
+
+      it 'creates new token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+
+    context 'token_digest exists and newly generated token would be the same' do
+      let(:token) { nil }
+      let(:token_digest) { Gitlab::CryptoHelper.sha256('old-token') }
+
+      before do
+        personal_access_token.save
+        allow(Devise).to receive(:friendly_token).and_return(
+          'old-token', token_value, 'boom!')
+      end
+
+      it 'regenerates a new token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
+    end
+
+    context 'token exists and newly generated token would be the same' do
+      let(:token) { 'old-token' }
+      let(:token_digest) { nil }
+
+      before do
+        personal_access_token.save
+        allow(Devise).to receive(:friendly_token).and_return(
+          'old-token', token_value, 'boom!')
+      end
+
+      it 'regenerates a new token_digest' do
+        subject
+
+        expect(personal_access_token.read_attribute('token')).to be_nil
+        expect(personal_access_token.token).to eql(token_value)
+        expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(token_value))
+      end
     end
   end
 end
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 2bb1c49b740..c82ab9c9e62 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -49,18 +49,36 @@ describe PersonalAccessToken do
 
   describe 'Redis storage' do
     let(:user_id) { 123 }
-    let(:token) { 'abc000foo' }
+    let(:token) { 'KS3wegQYXBLYhQsciwsj' }
 
-    before do
-      subject.redis_store!(user_id, token)
+    context 'reading encrypted data' do
+      before do
+        subject.redis_store!(user_id, token)
+      end
+
+      it 'returns stored data' do
+        expect(subject.redis_getdel(user_id)).to eq(token)
+      end
     end
 
-    it 'returns stored data' do
-      expect(subject.redis_getdel(user_id)).to eq(token)
+    context 'reading unencrypted data' do
+      before do
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set(described_class.redis_shared_state_key(user_id),
+                    token,
+                    ex: PersonalAccessToken::REDIS_EXPIRY_TIME)
+        end
+      end
+
+      it 'returns stored data unmodified' do
+        expect(subject.redis_getdel(user_id)).to eq(token)
+      end
     end
 
     context 'after deletion' do
       before do
+        subject.redis_store!(user_id, token)
+
         expect(subject.redis_getdel(user_id)).to eq(token)
       end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 99d17f563d9..9cc32b55375 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -721,6 +721,14 @@ describe User do
 
       expect(user.incoming_email_token).not_to be_blank
     end
+
+    it 'uses SecureRandom to generate the incoming email token' do
+      expect(SecureRandom).to receive(:hex).and_return('3b8ca303')
+
+      user = create(:user)
+
+      expect(user.incoming_email_token).to eql('gitlab')
+    end
   end
 
   describe '#ensure_user_rights_and_limits' do