diff options
author | Imre Farkas <ifarkas@gitlab.com> | 2018-10-29 19:06:45 +0300 |
---|---|---|
committer | Jan Provaznik <jprovaznik@gitlab.com> | 2018-10-29 19:06:45 +0300 |
commit | b9652d8e4dc8544766c9371057be72cc26fe3a4b (patch) | |
tree | dd4c8407af4ef5d98a20f30069d3a348773dfbfa | |
parent | b5ca4ea15dee21b131b336d4189a75a283c8d1f1 (diff) |
[master] Persist only SHA digest of PersonalAccessToken#token
20 files changed, 700 insertions, 70 deletions
diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 522b65e4205..66db4bd92de 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -5,57 +5,50 @@ module TokenAuthenticatable private - def write_new_token(token_field) - new_token = generate_available_token(token_field) - write_attribute(token_field, new_token) - end - - def generate_available_token(token_field) - loop do - token = generate_token(token_field) - break token unless self.class.unscoped.find_by(token_field => token) - end - end - - def generate_token(token_field) - Devise.friendly_token - end - class_methods do - def authentication_token_fields - @token_fields || [] - end - private # rubocop:disable Lint/UselessAccessModifier - def add_authentication_token_field(token_field) + def add_authentication_token_field(token_field, options = {}) @token_fields = [] unless @token_fields + + if @token_fields.include?(token_field) + raise ArgumentError.new("#{token_field} already configured via add_authentication_token_field") + end + @token_fields << token_field + attr_accessor :cleartext_tokens + + strategy = if options[:digest] + TokenAuthenticatableStrategies::Digest.new(self, token_field, options) + else + TokenAuthenticatableStrategies::Insecure.new(self, token_field, options) + end + define_singleton_method("find_by_#{token_field}") do |token| - find_by(token_field => token) if token + strategy.find_token_authenticatable(token) end - define_method("ensure_#{token_field}") do - current_token = read_attribute(token_field) - current_token.blank? ? write_new_token(token_field) : current_token + define_method(token_field) do + strategy.get_token(self) end define_method("set_#{token_field}") do |token| - write_attribute(token_field, token) if token + strategy.set_token(self, token) + end + + define_method("ensure_#{token_field}") do + strategy.ensure_token(self) end # Returns a token, but only saves when the database is in read & write mode define_method("ensure_#{token_field}!") do - send("reset_#{token_field}!") if read_attribute(token_field).blank? # rubocop:disable GitlabSecurity/PublicSend - - read_attribute(token_field) + strategy.ensure_token!(self) end # Resets the token, but only saves when the database is in read & write mode define_method("reset_#{token_field}!") do - write_new_token(token_field) - save! if Gitlab::Database.read_write? + strategy.reset_token!(self) end end end diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb new file mode 100644 index 00000000000..f0f7107d627 --- /dev/null +++ b/app/models/concerns/token_authenticatable_strategies/base.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module TokenAuthenticatableStrategies + class Base + def initialize(klass, token_field, options) + @klass = klass + @token_field = token_field + @options = options + end + + def find_token_authenticatable(instance, unscoped = false) + raise NotImplementedError + end + + def get_token(instance) + raise NotImplementedError + end + + def set_token(instance) + raise NotImplementedError + end + + def ensure_token(instance) + write_new_token(instance) unless token_set?(instance) + end + + # Returns a token, but only saves when the database is in read & write mode + def ensure_token!(instance) + reset_token!(instance) unless token_set?(instance) + get_token(instance) + end + + # Resets the token, but only saves when the database is in read & write mode + def reset_token!(instance) + write_new_token(instance) + instance.save! if Gitlab::Database.read_write? + end + + protected + + def write_new_token(instance) + new_token = generate_available_token + set_token(instance, new_token) + end + + def generate_available_token + loop do + token = generate_token + break token unless find_token_authenticatable(token, true) + end + end + + def generate_token + @options[:token_generator] ? @options[:token_generator].call : Devise.friendly_token + end + + def relation(unscoped) + unscoped ? @klass.unscoped : @klass + end + + def token_set?(instance) + raise NotImplementedError + end + + def token_field_name + @token_field + end + end +end diff --git a/app/models/concerns/token_authenticatable_strategies/digest.rb b/app/models/concerns/token_authenticatable_strategies/digest.rb new file mode 100644 index 00000000000..9926662ed66 --- /dev/null +++ b/app/models/concerns/token_authenticatable_strategies/digest.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module TokenAuthenticatableStrategies + class Digest < Base + def find_token_authenticatable(token, unscoped = false) + return unless token + + token_authenticatable = relation(unscoped).find_by(token_field_name => Gitlab::CryptoHelper.sha256(token)) + + if @options[:fallback] + token_authenticatable ||= fallback_strategy.find_token_authenticatable(token) + end + + token_authenticatable + end + + def get_token(instance) + token = instance.cleartext_tokens&.[](@token_field) + token ||= fallback_strategy.get_token(instance) if @options[:fallback] + + token + end + + def set_token(instance, token) + return unless token + + instance.cleartext_tokens ||= {} + instance.cleartext_tokens[@token_field] = token + instance[token_field_name] = Gitlab::CryptoHelper.sha256(token) + instance[@token_field] = nil if @options[:fallback] + end + + protected + + def fallback_strategy + @fallback_strategy ||= TokenAuthenticatableStrategies::Insecure.new(@klass, @token_field, @options) + end + + def token_set?(instance) + token_digest = instance.read_attribute(token_field_name) + token_digest ||= instance.read_attribute(@token_field) if @options[:fallback] + + token_digest.present? + end + + def token_field_name + "#{@token_field}_digest" + end + end +end diff --git a/app/models/concerns/token_authenticatable_strategies/insecure.rb b/app/models/concerns/token_authenticatable_strategies/insecure.rb new file mode 100644 index 00000000000..5f915259521 --- /dev/null +++ b/app/models/concerns/token_authenticatable_strategies/insecure.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module TokenAuthenticatableStrategies + class Insecure < Base + def find_token_authenticatable(token, unscoped = false) + relation(unscoped).find_by(@token_field => token) if token + end + + def get_token(instance) + instance.read_attribute(@token_field) + end + + def set_token(instance, token) + instance[@token_field] = token if token + end + + protected + + def token_set?(instance) + instance.read_attribute(@token_field).present? + end + end +end diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 207146479c0..73a58f2420e 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -3,7 +3,7 @@ class PersonalAccessToken < ActiveRecord::Base include Expirable include TokenAuthenticatable - add_authentication_token_field :token + add_authentication_token_field :token, digest: true, fallback: true REDIS_EXPIRY_TIME = 3.minutes @@ -33,16 +33,22 @@ class PersonalAccessToken < ActiveRecord::Base def self.redis_getdel(user_id) Gitlab::Redis::SharedState.with do |redis| - token = redis.get(redis_shared_state_key(user_id)) + encrypted_token = redis.get(redis_shared_state_key(user_id)) redis.del(redis_shared_state_key(user_id)) - token + begin + Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token) + rescue => ex + logger.warn "Failed to decrypt PersonalAccessToken value stored in Redis for User ##{user_id}: #{ex.class}" + encrypted_token + end end end def self.redis_store!(user_id, token) + encrypted_token = Gitlab::CryptoHelper.aes256_gcm_encrypt(token) + Gitlab::Redis::SharedState.with do |redis| - redis.set(redis_shared_state_key(user_id), token, ex: REDIS_EXPIRY_TIME) - token + redis.set(redis_shared_state_key(user_id), encrypted_token, ex: REDIS_EXPIRY_TIME) end end diff --git a/app/models/user.rb b/app/models/user.rb index ca7fc3b058f..2b918ce5bd1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -28,7 +28,7 @@ class User < ActiveRecord::Base ignore_column :email_provider ignore_column :authentication_token - add_authentication_token_field :incoming_email_token + add_authentication_token_field :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) } add_authentication_token_field :feed_token default_value_for :admin, false @@ -1464,15 +1464,6 @@ class User < ActiveRecord::Base end end - def generate_token(token_field) - if token_field == :incoming_email_token - # Needs to be all lowercase and alphanumeric because it's gonna be used in an email address. - SecureRandom.hex.to_i(16).to_s(36) - else - super - end - end - def self.unique_internal(scope, username, email_pattern, &block) scope.first || create_unique_internal(scope, username, email_pattern, &block) end diff --git a/changelogs/unreleased/security-51113-hash_personal_access_tokens.yml b/changelogs/unreleased/security-51113-hash_personal_access_tokens.yml new file mode 100644 index 00000000000..4cebe814148 --- /dev/null +++ b/changelogs/unreleased/security-51113-hash_personal_access_tokens.yml @@ -0,0 +1,5 @@ +--- +title: Persist only SHA digest of PersonalAccessToken#token +merge_request: +author: +type: security diff --git a/db/migrate/20180910153412_add_token_digest_to_personal_access_tokens.rb b/db/migrate/20180910153412_add_token_digest_to_personal_access_tokens.rb new file mode 100644 index 00000000000..203fcfe8eae --- /dev/null +++ b/db/migrate/20180910153412_add_token_digest_to_personal_access_tokens.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddTokenDigestToPersonalAccessTokens < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + change_column :personal_access_tokens, :token, :string, null: true + + add_column :personal_access_tokens, :token_digest, :string + end + + def down + remove_column :personal_access_tokens, :token_digest + + change_column :personal_access_tokens, :token, :string, null: false + end +end diff --git a/db/migrate/20180910153413_add_index_to_token_digest_on_personal_access_tokens.rb b/db/migrate/20180910153413_add_index_to_token_digest_on_personal_access_tokens.rb new file mode 100644 index 00000000000..4300cd13a45 --- /dev/null +++ b/db/migrate/20180910153413_add_index_to_token_digest_on_personal_access_tokens.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToTokenDigestOnPersonalAccessTokens < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :personal_access_tokens, :token_digest, unique: true + end + + def down + remove_concurrent_index :personal_access_tokens, :token_digest if index_exists?(:personal_access_tokens, :token_digest) + end +end diff --git a/db/post_migrate/20180913142237_schedule_digest_personal_access_tokens.rb b/db/post_migrate/20180913142237_schedule_digest_personal_access_tokens.rb new file mode 100644 index 00000000000..36be819b245 --- /dev/null +++ b/db/post_migrate/20180913142237_schedule_digest_personal_access_tokens.rb @@ -0,0 +1,28 @@ +class ScheduleDigestPersonalAccessTokens < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + BATCH_SIZE = 10_000 + MIGRATION = 'DigestColumn' + DELAY_INTERVAL = 5.minutes.to_i + + disable_ddl_transaction! + + class PersonalAccessToken < ActiveRecord::Base + include EachBatch + + self.table_name = 'personal_access_tokens' + end + + def up + PersonalAccessToken.where('token is NOT NULL').each_batch(of: BATCH_SIZE) do |batch, index| + range = batch.pluck('MIN(id)', 'MAX(id)').first + BackgroundMigrationWorker.perform_in(index * DELAY_INTERVAL, MIGRATION, ['PersonalAccessToken', :token, :token_digest, *range]) + end + end + + def down + # raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index ddfccbba678..7ee2c483e54 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1513,7 +1513,7 @@ ActiveRecord::Schema.define(version: 20181013005024) do create_table "personal_access_tokens", force: :cascade do |t| t.integer "user_id", null: false - t.string "token", null: false + t.string "token" t.string "name", null: false t.boolean "revoked", default: false t.date "expires_at" @@ -1521,9 +1521,11 @@ ActiveRecord::Schema.define(version: 20181013005024) do t.datetime "updated_at", null: false t.string "scopes", default: "--- []\n", null: false t.boolean "impersonation", default: false, null: false + t.string "token_digest" end add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree + add_index "personal_access_tokens", ["token_digest"], name: "index_personal_access_tokens_on_token_digest", unique: true, using: :btree add_index "personal_access_tokens", ["user_id"], name: "index_personal_access_tokens_on_user_id", using: :btree create_table "programming_languages", force: :cascade do |t| diff --git a/lib/gitlab/auth/user_auth_finders.rb b/lib/gitlab/auth/user_auth_finders.rb index 5df6db6f366..c304adc64db 100644 --- a/lib/gitlab/auth/user_auth_finders.rb +++ b/lib/gitlab/auth/user_auth_finders.rb @@ -73,7 +73,6 @@ module Gitlab end end - # rubocop: disable CodeReuse/ActiveRecord def find_personal_access_token token = current_request.params[PRIVATE_TOKEN_PARAM].presence || @@ -82,9 +81,8 @@ module Gitlab return unless token # Expiration, revocation and scopes are verified in `validate_access_token!` - PersonalAccessToken.find_by(token: token) || raise(UnauthorizedError) + PersonalAccessToken.find_by_token(token) || raise(UnauthorizedError) end - # rubocop: enable CodeReuse/ActiveRecord def find_oauth_access_token token = Doorkeeper::OAuth::Token.from_request(current_request, *Doorkeeper.configuration.access_token_methods) diff --git a/lib/gitlab/background_migration/digest_column.rb b/lib/gitlab/background_migration/digest_column.rb new file mode 100644 index 00000000000..22a3bb8f8f3 --- /dev/null +++ b/lib/gitlab/background_migration/digest_column.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# rubocop:disable Style/Documentation +module Gitlab + module BackgroundMigration + class DigestColumn + class PersonalAccessToken < ActiveRecord::Base + self.table_name = 'personal_access_tokens' + end + + def perform(model, attribute_from, attribute_to, start_id, stop_id) + model = model.constantize if model.is_a?(String) + + model.transaction do + relation = model.where(id: start_id..stop_id).where.not(attribute_from => nil).lock + + relation.each do |instance| + instance.update_columns(attribute_to => Gitlab::CryptoHelper.sha256(instance.read_attribute(attribute_from)), + attribute_from => nil) + end + end + end + end + end +end diff --git a/lib/gitlab/crypto_helper.rb b/lib/gitlab/crypto_helper.rb new file mode 100644 index 00000000000..68d0b5d8f8a --- /dev/null +++ b/lib/gitlab/crypto_helper.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Gitlab + module CryptoHelper + extend self + + AES256_GCM_OPTIONS = { + algorithm: 'aes-256-gcm', + key: Settings.attr_encrypted_db_key_base_truncated, + iv: Settings.attr_encrypted_db_key_base_truncated[0..11] + }.freeze + + def sha256(value) + salt = Settings.attr_encrypted_db_key_base_truncated + ::Digest::SHA256.base64digest("#{value}#{salt}") + end + + def aes256_gcm_encrypt(value) + encrypted_token = Encryptor.encrypt(AES256_GCM_OPTIONS.merge(value: value)) + Base64.encode64(encrypted_token) + end + + def aes256_gcm_decrypt(value) + return unless value + + encrypted_token = Base64.decode64(value) + Encryptor.decrypt(AES256_GCM_OPTIONS.merge(value: encrypted_token)) + end + end +end diff --git a/lib/tasks/tokens.rake b/lib/tasks/tokens.rake index 81829668de8..eec024f9bbb 100644 --- a/lib/tasks/tokens.rake +++ b/lib/tasks/tokens.rake @@ -1,4 +1,7 @@ require_relative '../../app/models/concerns/token_authenticatable.rb' +require_relative '../../app/models/concerns/token_authenticatable_strategies/base.rb' +require_relative '../../app/models/concerns/token_authenticatable_strategies/insecure.rb' +require_relative '../../app/models/concerns/token_authenticatable_strategies/digest.rb' namespace :tokens do desc "Reset all GitLab incoming email tokens" @@ -26,13 +29,6 @@ class TmpUser < ActiveRecord::Base self.table_name = 'users' - def reset_incoming_email_token! - write_new_token(:incoming_email_token) - save!(validate: false) - end - - def reset_feed_token! - write_new_token(:feed_token) - save!(validate: false) - end + add_authentication_token_field :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) } + add_authentication_token_field :feed_token end 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 b3474e74aa4..4e7c8523e65 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -730,6 +730,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 |