diff options
author | Imre Farkas <ifarkas@gitlab.com> | 2018-11-01 10:51:26 +0100 |
---|---|---|
committer | Imre Farkas <ifarkas@gitlab.com> | 2019-02-15 13:13:02 +0100 |
commit | 566f3c575ec582c00c16a7f6e67c62ee5e5f5469 (patch) | |
tree | 121a8eb1413a73b039a3bf269fd0c6593863996a | |
parent | 0cc5e956fcb9393829e537a20be8da89a275ec1d (diff) | |
download | gitlab-ce-if-53411-remove_personal_access_tokens_token.tar.gz |
Remove undigested token column from personal_access_tokens tableif-53411-remove_personal_access_tokens_token
Token column are no longer used as token values are stored digested in
token_digest.
-rw-r--r-- | app/models/personal_access_token.rb | 5 | ||||
-rw-r--r-- | changelogs/unreleased/53411-remove_personal_access_tokens_token.yml | 5 | ||||
-rw-r--r-- | db/post_migrate/20181101091005_steal_digest_column.rb | 17 | ||||
-rw-r--r-- | db/post_migrate/20181101091124_remove_token_from_personal_access_tokens.rb | 11 | ||||
-rw-r--r-- | db/schema.rb | 2 | ||||
-rw-r--r-- | spec/factories/personal_access_tokens.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/background_migration/digest_column_spec.rb | 8 | ||||
-rw-r--r-- | spec/models/concerns/token_authenticatable_spec.rb | 235 |
8 files changed, 86 insertions, 202 deletions
diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 73a58f2420e..430467a648b 100644 --- a/app/models/personal_access_token.rb +++ b/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, fallback: false + ignore_column :token REDIS_EXPIRY_TIME = 3.minutes diff --git a/changelogs/unreleased/53411-remove_personal_access_tokens_token.yml b/changelogs/unreleased/53411-remove_personal_access_tokens_token.yml new file mode 100644 index 00000000000..32cca07f58d --- /dev/null +++ b/changelogs/unreleased/53411-remove_personal_access_tokens_token.yml @@ -0,0 +1,5 @@ +--- +title: Remove undigested token column from personal_access_tokens table from the database +merge_request: 22743 +author: +type: changed diff --git a/db/post_migrate/20181101091005_steal_digest_column.rb b/db/post_migrate/20181101091005_steal_digest_column.rb new file mode 100644 index 00000000000..58ea710c18a --- /dev/null +++ b/db/post_migrate/20181101091005_steal_digest_column.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/post_migrate/20181101091124_remove_token_from_personal_access_tokens.rb b/db/post_migrate/20181101091124_remove_token_from_personal_access_tokens.rb new file mode 100644 index 00000000000..415373068d5 --- /dev/null +++ b/db/post_migrate/20181101091124_remove_token_from_personal_access_tokens.rb @@ -0,0 +1,11 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 7c1733becb9..e380e648352 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1500,7 +1500,6 @@ ActiveRecord::Schema.define(version: 20190124200344) 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" @@ -1509,7 +1508,6 @@ ActiveRecord::Schema.define(version: 20190124200344) 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 diff --git a/spec/factories/personal_access_tokens.rb b/spec/factories/personal_access_tokens.rb index 1b12f84d7b8..e7fd22a96b2 100644 --- a/spec/factories/personal_access_tokens.rb +++ b/spec/factories/personal_access_tokens.rb @@ -1,13 +1,14 @@ 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 diff --git a/spec/lib/gitlab/background_migration/digest_column_spec.rb b/spec/lib/gitlab/background_migration/digest_column_spec.rb index 3e107ac3027..a90b9d5379b 100644 --- a/spec/lib/gitlab/background_migration/digest_column_spec.rb +++ b/spec/lib/gitlab/background_migration/digest_column_spec.rb @@ -17,12 +17,12 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913 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'))) + change { PersonalAccessToken.find(1).read_attribute(: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)) + change { PersonalAccessToken.find(1).read_attribute(:token) }.from('token-01').to(nil)) end end @@ -34,12 +34,12 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913 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 }) + change { PersonalAccessToken.find(1).read_attribute(: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)) + change { PersonalAccessToken.find(1).read_attribute(:token) }.from(nil)) end end end diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb index 55d83bc3a6b..7325ebc05de 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/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 eql(token_value) + expect(personal_access_token.token_digest).to eql(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 eql(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 eql(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 eql(new_token_value) + expect(personal_access_token.token_digest).to eql(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 |