Merge branch 'security-if-51113-hash_tokens' into 'master'

[master] Persist only SHA digest of PersonalAccessToken#token

See merge request gitlab/gitlabhq!2498

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

app/models/concerns/token_authenticatable_strategies/base.rb

+# 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

app/models/concerns/token_authenticatable_strategies/digest.rb

+# 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

app/models/concerns/token_authenticatable_strategies/insecure.rb

+# 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

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
 

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

changelogs/unreleased/security-51113-hash_personal_access_tokens.yml

+---
+title: Persist only SHA digest of PersonalAccessToken#token
+merge_request:
+author:
+type: security

db/migrate/20180910153412_add_token_digest_to_personal_access_tokens.rb

+# 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

db/migrate/20180910153413_add_index_to_token_digest_on_personal_access_tokens.rb

+# 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

db/post_migrate/20180913142237_schedule_digest_personal_access_tokens.rb

+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

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|

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)

lib/gitlab/background_migration/digest_column.rb

+# 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

lib/gitlab/crypto_helper.rb

+# 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

lib/tasks/tokens.rake

 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

spec/lib/gitlab/background_migration/digest_column_spec.rb

+# 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

spec/migrations/schedule_digest_personal_access_tokens_spec.rb

+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

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

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
 

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