Merge branch 'if-53411-remove_personal_access_tokens_token' into 'master'

Remove undigested token column from personal_access_tokens table

See merge request gitlab-org/gitlab-ce!22743

app/models/personal_access_token.rb

@@ -2,8 +2,11 @@
 
 class PersonalAccessToken < ActiveRecord::Base
   include Expirable
+  include IgnorableColumn
   include TokenAuthenticatable
-  add_authentication_token_field :token, digest: true, fallback: true
+
+  add_authentication_token_field :token, digest: true
+  ignore_column :token
 
   REDIS_EXPIRY_TIME = 3.minutes
 

changelogs/unreleased/53411-remove_personal_access_tokens_token.yml

+---
+title: Remove undigested token column from personal_access_tokens table from the database
+merge_request: 22743
+author:
+type: changed

db/post_migrate/20181101091005_steal_digest_column.rb

+# frozen_string_literal: true
+
+class StealDigestColumn < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    Gitlab::BackgroundMigration.steal('DigestColumn')
+  end
+
+  def down
+    # raise ActiveRecord::IrreversibleMigration
+  end
+end

db/post_migrate/20181101091124_remove_token_from_personal_access_tokens.rb

+# frozen_string_literal: true
+
+class RemoveTokenFromPersonalAccessTokens < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    remove_column :personal_access_tokens, :token, :string
+  end
+end

db/schema.rb

@@ -1515,7 +1515,6 @@ ActiveRecord::Schema.define(version: 20190204115450) do
 
   create_table "personal_access_tokens", force: :cascade do |t|
     t.integer "user_id", null: false
-    t.string "token"
     t.string "name", null: false
     t.boolean "revoked", default: false
     t.date "expires_at"
@@ -1524,7 +1523,6 @@ ActiveRecord::Schema.define(version: 20190204115450) do
     t.string "scopes", default: "--- []\n", null: false
     t.boolean "impersonation", default: false, null: false
     t.string "token_digest"
-    t.index ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree
     t.index ["token_digest"], name: "index_personal_access_tokens_on_token_digest", unique: true, using: :btree
     t.index ["user_id"], name: "index_personal_access_tokens_on_user_id", using: :btree
   end

spec/factories/personal_access_tokens.rb

 FactoryBot.define do
   factory :personal_access_token do
     user
-    token { SecureRandom.hex(50) }
     sequence(:name) { |n| "PAT #{n}" }
     revoked false
     expires_at { 5.days.from_now }
     scopes ['api']
     impersonation false
 
+    after(:build) { |personal_access_token| personal_access_token.ensure_token }
+
     trait :impersonation do
       impersonation true
     end
@@ -21,7 +22,7 @@ FactoryBot.define do
     end
 
     trait :invalid do
-      token nil
+      token_digest nil
     end
   end
 end

spec/lib/gitlab/background_migration/digest_column_spec.rb

@@ -22,7 +22,7 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913
 
       it 'erases token' do
         expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.to(
-          change { PersonalAccessToken.find(1).token }.from('token-01').to(nil))
+          change { PersonalAccessToken.find(1).read_attribute(:token) }.from('token-01').to(nil))
       end
     end
 
@@ -39,7 +39,7 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913
 
       it 'leaves token empty' do
         expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.not_to(
-          change { PersonalAccessToken.find(1).token }.from(nil))
+          change { PersonalAccessToken.find(1).read_attribute(:token) }.from(nil))
       end
     end
   end

spec/models/concerns/token_authenticatable_spec.rb

@@ -97,14 +97,31 @@ describe ApplicationSetting, 'TokenAuthenticatable' do
 end
 
 describe PersonalAccessToken, 'TokenAuthenticatable' do
-  let(:personal_access_token_name) { 'test-pat-01' }
+  shared_examples 'changes personal access token' do
+    it 'sets new token' do
+      subject
+
+      expect(personal_access_token.token).to eq(token_value)
+      expect(personal_access_token.token_digest).to eq(Gitlab::CryptoHelper.sha256(token_value))
+    end
+  end
+
+  shared_examples 'does not change personal access token' do
+    it 'sets new token' do
+      subject
+
+      expect(personal_access_token.token).to be(nil)
+      expect(personal_access_token.token_digest).to eq(token_digest)
+    end
+  end
+
   let(:token_value) { 'token' }
+  let(:token_digest) { Gitlab::CryptoHelper.sha256(token_value) }
   let(:user) { create(:user) }
   let(:personal_access_token) do
-    described_class.new(name: personal_access_token_name,
+    described_class.new(name: 'test-pat-01',
                         user_id: user.id,
                         scopes: [:api],
-                        token: token,
                         token_digest: token_digest)
   end
 
@@ -115,239 +132,71 @@ describe PersonalAccessToken, 'TokenAuthenticatable' do
   describe '.find_by_token' do
     subject { PersonalAccessToken.find_by_token(token_value) }
 
-    before do
+    it 'finds the token' 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
+      expect(subject).to eq(personal_access_token)
     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 }
+    subject { personal_access_token.set_token(new_token_value) }
 
-      it 'creates new token_digest' do
-        subject
+    it 'sets new 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
+      expect(personal_access_token.token).to eq(new_token_value)
+      expect(personal_access_token.token_digest).to eq(Gitlab::CryptoHelper.sha256(new_token_value))
     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 }
+    context 'token_digest does not exist' do
       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
+      it_behaves_like 'changes personal access token'
     end
 
-    context 'token_digest does not exist, nor token' do
-      let(:token) { nil }
-      let(:token_digest) { nil }
-
-      it 'creates token_digest' do
-        subject
+    context 'token_digest already generated' do
+      let(:token_digest) { 's3cr3t' }
 
-        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
+      it_behaves_like 'does not change personal access token'
     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 }
+    context 'token_digest does not exist' do
       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
+      it_behaves_like 'changes personal access token'
     end
 
-    context 'token_digest does not exist, nor token' do
-      let(:token) { nil }
-      let(:token_digest) { nil }
+    context 'token_digest already generated' do
+      let(:token_digest) { 's3cr3t' }
 
-      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
+      it_behaves_like 'does not change personal access token'
     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 }
+    context 'token_digest does not exist' do
       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
+      it_behaves_like 'changes personal access token'
     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
+    context 'token_digest already generated' do
+      let(:token_digest) { 's3cr3t' }
 
-      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
+      it_behaves_like 'changes personal access token'
     end
   end
 end