Refactor LFS token logic to use a Redis key instead of a DB field, making it a 1 use only token.

15 files changed, 93 insertions, 79 deletions

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 1e6709dc8eb..4dff1ce6568 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -132,8 +132,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   end
 
   def lfs_deploy_key?
-    key = user
-    @lfs_deploy_key.present? && (key && key.projects.include?(project))
+    @lfs_deploy_key.present? && (user && user.projects.include?(project))
   end
 
   def verify_workhorse_api!
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 0c867fc8741..2f5709a7395 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -25,7 +25,11 @@ module LfsHelper
   def lfs_download_access?
     return false unless project.lfs_enabled?
 
-    project.public? || ci? || lfs_deploy_key? || (user && user.can?(:download_code, project))
+    return true if project.public?
+    return true if ci?
+    return true if lfs_deploy_key?
+
+    (user && user.can?(:download_code, project))
   end
 
   def lfs_upload_access?
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index de51b63c120..2c525d4cd7a 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -1,12 +1,7 @@
 class DeployKey < Key
-  include TokenAuthenticatable
-  add_authentication_token_field :lfs_token
-
   has_many :deploy_keys_projects, dependent: :destroy
   has_many :projects, through: :deploy_keys_projects
 
-  before_save :ensure_lfs_token
-
   scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
   scope :are_public,  -> { where(public: true) }
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 94ae3911859..6996740eebd 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -13,7 +13,6 @@ class User < ActiveRecord::Base
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
   add_authentication_token_field :authentication_token
-  add_authentication_token_field :lfs_token
 
   default_value_for :admin, false
   default_value_for(:external) { current_application_settings.user_default_external }
@@ -118,7 +117,7 @@ class User < ActiveRecord::Base
   before_validation :set_public_email, if: ->(user) { user.public_email_changed? }
 
   after_update :update_emails_with_primary_email, if: ->(user) { user.email_changed? }
-  before_save :ensure_authentication_token, :ensure_lfs_token
+  before_save :ensure_authentication_token
   before_save :ensure_external_user_rights
   after_save :ensure_namespace_correct
   after_initialize :set_projects_limit
diff --git a/db/migrate/20160825173042_add_lfs_token_to_users.rb b/db/migrate/20160825173042_add_lfs_token_to_users.rb
deleted file mode 100644
index f7f210bcd67..00000000000
--- a/db/migrate/20160825173042_add_lfs_token_to_users.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
-class AddLfsTokenToUsers < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # Set this constant to true if this migration requires downtime.
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def change
-    add_column :users, :lfs_token, :string
-    add_concurrent_index :users, :lfs_token
-  end
-end
diff --git a/db/migrate/20160825182924_add_lfs_token_to_keys.rb b/db/migrate/20160825182924_add_lfs_token_to_keys.rb
deleted file mode 100644
index 3ff010ef328..00000000000
--- a/db/migrate/20160825182924_add_lfs_token_to_keys.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
-class AddLfsTokenToKeys < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # Set this constant to true if this migration requires downtime.
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def change
-    add_column :keys, :lfs_token, :string
-    add_concurrent_index :keys, :lfs_token
-  end
-end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index b4fcacca896..4f736e4ec2b 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1,7 +1,7 @@
 module API
   module Entities
     class UserSafe < Grape::Entity
-      expose :name, :username, :lfs_token
+      expose :name, :username
     end
 
     class UserBasic < UserSafe
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 7c0a6eaa652..760f69663ab 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -88,12 +88,13 @@ module API
       get "/discover" do
         key = Key.find(params[:key_id])
         user = key.user
+
         if user
-          user.ensure_lfs_token!
-          present user, with: Entities::UserSafe
+          token = Gitlab::LfsToken.new(user).set_token
+          { name: user.name, username: user.username, lfs_token: token }
         else
-          key.ensure_lfs_token!
-          { username: 'lfs-deploy-key', lfs_token: key.lfs_token }
+          token = Gitlab::LfsToken.new(key).set_token
+          { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
         end
       end
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 5446093de4d..e43f8119658 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -117,12 +117,14 @@ module Gitlab
       end
 
       def lfs_token_check(login, password)
-        if login == 'lfs-deploy-key'
-          key = DeployKey.find_by_lfs_token(password)
-          Result.new(key, :lfs_deploy_token) if key
+        if login.include?('lfs-deploy-key')
+          key = DeployKey.find(login.gsub('lfs-deploy-key-', ''))
+          token = Gitlab::LfsToken.new(key).get_value
+          Result.new(key, :lfs_deploy_token) if key && token == password
         else
-          user = User.find_by_lfs_token(password)
-          Result.new(user, :lfs_token) if user && user.username == login
+          user = User.by_login(login)
+          token = Gitlab::LfsToken.new(user).get_value
+          Result.new(user, :lfs_token) if user && token == password
         end
       end
     end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
new file mode 100644
index 00000000000..0685eb775ef
--- /dev/null
+++ b/lib/gitlab/lfs_token.rb
@@ -0,0 +1,29 @@
+module Gitlab
+  class LfsToken
+    attr_accessor :actor
+
+    def initialize(actor)
+      @actor = actor
+    end
+
+    def set_token
+      token = Devise.friendly_token(50)
+      Gitlab::Redis.with do |redis|
+        redis.set(redis_key, token, ex: 3600)
+      end
+      token
+    end
+
+    def get_value
+      Gitlab::Redis.with do |redis|
+        redis.get(redis_key)
+      end
+    end
+
+    private
+
+    def redis_key
+      "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
+    end
+  end
+end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index cd00a15be3b..6ce680e3c26 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -26,17 +26,19 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes user lfs tokens' do
       user = create(:user)
       ip = 'ip'
+      token = Gitlab::LfsToken.new(user).set_token
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
-      expect(gl_auth.find_for_git_client(user.username, user.lfs_token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
+      expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
     end
 
     it 'recognizes deploy key lfs tokens' do
       key = create(:deploy_key)
       ip = 'ip'
+      token = Gitlab::LfsToken.new(key).set_token
 
-      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'lfs-deploy-key')
-      expect(gl_auth.find_for_git_client('lfs-deploy-key', key.lfs_token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs-deploy-key-#{key.id}")
+      expect(gl_auth.find_for_git_client("lfs-deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
     end
 
     it 'recognizes OAuth tokens' do
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
new file mode 100644
index 00000000000..76b348637c7
--- /dev/null
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe Gitlab::LfsToken, lib: true do
+  describe '#set_token and #get_value' do
+    shared_examples 'an LFS token generator' do
+      it 'returns a randomly generated token' do
+        token = handler.set_token
+
+        expect(token).not_to be_nil
+        expect(token).to be_a String
+        expect(token.length).to eq 50
+      end
+
+      it 'returns the correct token based on the key' do
+        token = handler.set_token
+
+        expect(handler.get_value).to eq(token)
+      end
+    end
+
+    context 'when the actor is a user' do
+      let(:actor) { create(:user) }
+      let(:handler) { described_class.new(actor) }
+
+      it_behaves_like 'an LFS token generator'
+    end
+
+    context 'when the actor is a deploy key' do
+      let(:actor) { create(:deploy_key) }
+      let(:handler) { described_class.new(actor) }
+
+      it_behaves_like 'an LFS token generator'
+    end
+  end
+end
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index 82076600f3b..eb64f3d0c83 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -18,26 +18,6 @@ describe User, 'TokenAuthenticatable' do
     subject { create(:user).send(token_field) }
     it { is_expected.to be_a String }
   end
-
-  describe 'lfs token' do
-    let(:token_field) { :lfs_token }
-    it_behaves_like 'TokenAuthenticatable'
-
-    describe 'ensure it' do
-      subject { create(:user).send(token_field) }
-      it { is_expected.to be_a String }
-    end
-  end
-end
-
-describe DeployKey, 'TokenAuthenticatable' do
-  let(:token_field) { :lfs_token }
-  it_behaves_like 'TokenAuthenticatable'
-
-  describe 'ensures authentication token' do
-    subject { create(:deploy_key).send(token_field) }
-    it { is_expected.to be_a String }
-  end
 end
 
 describe ApplicationSetting, 'TokenAuthenticatable' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 95fc5f790e8..59df5af770b 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -108,7 +108,7 @@ describe API::API, api: true  do
         expect(response).to have_http_status(200)
 
         expect(json_response['name']).to eq(user.name)
-        expect(json_response['lfs_token']).to eq(user.lfs_token)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).get_value)
       end
     end
 
@@ -120,8 +120,8 @@ describe API::API, api: true  do
 
         expect(response).to have_http_status(200)
 
-        expect(json_response['username']).to eq('lfs-deploy-key')
-        expect(json_response['lfs_token']).to eq(key.lfs_token)
+        expect(json_response['username']).to eq("lfs-deploy-key-#{key.id}")
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).get_value)
       end
     end
   end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 58f8515c0e2..d15e72b2570 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -917,7 +917,7 @@ describe 'Git LFS API and storage' do
   end
 
   def authorize_deploy_key
-    ActionController::HttpAuthentication::Basic.encode_credentials('lfs-deploy-key', key.lfs_token)
+    ActionController::HttpAuthentication::Basic.encode_credentials("lfs-deploy-key-#{key.id}", Gitlab::LfsToken.new(key).set_token)
   end
 
   def fork_project(project, user, object = nil)