Commit 6d92a3d4 authored by Imre Farkas's avatar Imre Farkas Committed by Nick Thomas

Remove undigested token column from personal_access_tokens table

Token column are no longer used as token values are stored digested in
token_digest.
parent 83a23297
...@@ -2,8 +2,11 @@ ...@@ -2,8 +2,11 @@
class PersonalAccessToken < ActiveRecord::Base class PersonalAccessToken < ActiveRecord::Base
include Expirable include Expirable
include IgnorableColumn
include TokenAuthenticatable 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 REDIS_EXPIRY_TIME = 3.minutes
......
---
title: Remove undigested token column from personal_access_tokens table from the database
merge_request: 22743
author:
type: changed
# 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
# 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
...@@ -1515,7 +1515,6 @@ ActiveRecord::Schema.define(version: 20190204115450) do ...@@ -1515,7 +1515,6 @@ ActiveRecord::Schema.define(version: 20190204115450) do
create_table "personal_access_tokens", force: :cascade do |t| create_table "personal_access_tokens", force: :cascade do |t|
t.integer "user_id", null: false t.integer "user_id", null: false
t.string "token"
t.string "name", null: false t.string "name", null: false
t.boolean "revoked", default: false t.boolean "revoked", default: false
t.date "expires_at" t.date "expires_at"
...@@ -1524,7 +1523,6 @@ ActiveRecord::Schema.define(version: 20190204115450) do ...@@ -1524,7 +1523,6 @@ ActiveRecord::Schema.define(version: 20190204115450) do
t.string "scopes", default: "--- []\n", null: false t.string "scopes", default: "--- []\n", null: false
t.boolean "impersonation", default: false, null: false t.boolean "impersonation", default: false, null: false
t.string "token_digest" 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 ["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 t.index ["user_id"], name: "index_personal_access_tokens_on_user_id", using: :btree
end end
......
FactoryBot.define do FactoryBot.define do
factory :personal_access_token do factory :personal_access_token do
user user
token { SecureRandom.hex(50) }
sequence(:name) { |n| "PAT #{n}" } sequence(:name) { |n| "PAT #{n}" }
revoked false revoked false
expires_at { 5.days.from_now } expires_at { 5.days.from_now }
scopes ['api'] scopes ['api']
impersonation false impersonation false
after(:build) { |personal_access_token| personal_access_token.ensure_token }
trait :impersonation do trait :impersonation do
impersonation true impersonation true
end end
...@@ -21,7 +22,7 @@ FactoryBot.define do ...@@ -21,7 +22,7 @@ FactoryBot.define do
end end
trait :invalid do trait :invalid do
token nil token_digest nil
end end
end end
end end
...@@ -22,7 +22,7 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913 ...@@ -22,7 +22,7 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913
it 'erases token' do it 'erases token' do
expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.to( 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
end end
...@@ -39,7 +39,7 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913 ...@@ -39,7 +39,7 @@ describe Gitlab::BackgroundMigration::DigestColumn, :migration, schema: 20180913
it 'leaves token empty' do it 'leaves token empty' do
expect { subject.perform(PersonalAccessToken, :token, :token_digest, 1, 2) }.not_to( 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 end
end end
......
...@@ -97,14 +97,31 @@ describe ApplicationSetting, 'TokenAuthenticatable' do ...@@ -97,14 +97,31 @@ describe ApplicationSetting, 'TokenAuthenticatable' do
end end
describe PersonalAccessToken, 'TokenAuthenticatable' do 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_value) { 'token' }
let(:token_digest) { Gitlab::CryptoHelper.sha256(token_value) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:personal_access_token) do let(:personal_access_token) do
described_class.new(name: personal_access_token_name, described_class.new(name: 'test-pat-01',
user_id: user.id, user_id: user.id,
scopes: [:api], scopes: [:api],
token: token,
token_digest: token_digest) token_digest: token_digest)
end end
...@@ -115,239 +132,71 @@ describe PersonalAccessToken, 'TokenAuthenticatable' do ...@@ -115,239 +132,71 @@ describe PersonalAccessToken, 'TokenAuthenticatable' do
describe '.find_by_token' do describe '.find_by_token' do
subject { PersonalAccessToken.find_by_token(token_value) } 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 it 'finds the token' do
expect(subject).not_to be_nil personal_access_token.save
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).to eq(personal_access_token)
expect(subject).not_to be_nil
expect(subject.name).to eql(personal_access_token_name)
end
end end
end end
describe '#set_token' do describe '#set_token' do
let(:new_token_value) { 'new-token' } 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 subject { personal_access_token.set_token(new_token_value) }
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 it 'sets new token' do
subject subject
expect(personal_access_token.read_attribute('token')).to be_nil expect(personal_access_token.token).to eq(new_token_value)
expect(personal_access_token.token).to eql(new_token_value) expect(personal_access_token.token_digest).to eq(Gitlab::CryptoHelper.sha256(new_token_value))
expect(personal_access_token.token_digest).to eql( Gitlab::CryptoHelper.sha256(new_token_value))
end
end end
end end
describe '#ensure_token' do describe '#ensure_token' do
subject { personal_access_token.ensure_token } subject { personal_access_token.ensure_token }
context 'token_digest already exists' do context 'token_digest does not exist' 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 } let(:token_digest) { nil }
it 'does not change token fields' do it_behaves_like 'changes personal access token'
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 end
context 'token_digest does not exist, nor token' do context 'token_digest already generated' do
let(:token) { nil } let(:token_digest) { 's3cr3t' }
let(:token_digest) { nil }
it 'creates token_digest' do
subject
expect(personal_access_token.read_attribute('token')).to be_nil it_behaves_like 'does not change personal access token'
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 end
describe '#ensure_token!' do describe '#ensure_token!' do
subject { personal_access_token.ensure_token! } subject { personal_access_token.ensure_token! }
context 'token_digest already exists' do context 'token_digest does not exist' 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 } let(:token_digest) { nil }
it 'does not change token fields' do it_behaves_like 'changes personal access token'
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
end
context 'token_digest does not exist, nor token' do
let(:token) { nil }
let(:token_digest) { nil }
it 'creates token_digest' do context 'token_digest already generated' do
subject let(:token_digest) { 's3cr3t' }
expect(personal_access_token.read_attribute('token')).to be_nil it_behaves_like 'does not change personal access token'
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 end
describe '#reset_token!' do describe '#reset_token!' do
subject { personal_access_token.reset_token! } subject { personal_access_token.reset_token! }
context 'token_digest already exists' do context 'token_digest does not exist' 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 } let(:token_digest) { nil }
before do it_behaves_like 'changes personal access token'
personal_access_token.save
allow(Devise).to receive(:friendly_token).and_return(
'old-token', token_value, 'boom!')
end end
it 'regenerates a new token_digest' do context 'token_digest already generated' do
subject let(:token_digest) { 's3cr3t' }
expect(personal_access_token.read_attribute('token')).to be_nil it_behaves_like 'changes personal access token'
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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment