Refactored LFS auth logic when using SSH to use its own API endpoint `/lfs_authenticate` and added tests.

7 files changed, 62 insertions, 32 deletions

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 760f69663ab..1b3388347a8 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -69,12 +69,26 @@ module API
             else
               project.repository.path_to_repo
             end
+        end
+
+        response
+      end
+
+      post "/lfs_authenticate" do
+        status 200
+
+        key = Key.find(params[:key_id])
+        user = key.user
 
-          # Return HTTP full path, so that gitlab-shell has this information
-          # ready for git-lfs-authenticate
-          response[:repository_http_path] = project.http_url_to_repo
+        if user
+          token = Gitlab::LfsToken.new(user).generate
+          response = { username: user.username, lfs_token: token }
+        else
+          token = Gitlab::LfsToken.new(key).generate
+          response = { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
         end
 
+        response[:repository_http_path] = project.http_url_to_repo
         response
       end
 
@@ -87,15 +101,7 @@ module API
       #
       get "/discover" do
         key = Key.find(params[:key_id])
-        user = key.user
-
-        if user
-          token = Gitlab::LfsToken.new(user).set_token
-          { name: user.name, username: user.username, lfs_token: token }
-        else
-          token = Gitlab::LfsToken.new(key).set_token
-          { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
-        end
+        present key.user, with: Entities::UserSafe
       end
 
       get "/check" do
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index e43f8119658..1b0398d18ee 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -119,11 +119,11 @@ module Gitlab
       def lfs_token_check(login, password)
         if login.include?('lfs-deploy-key')
           key = DeployKey.find(login.gsub('lfs-deploy-key-', ''))
-          token = Gitlab::LfsToken.new(key).get_value
+          token = Gitlab::LfsToken.new(key).value
           Result.new(key, :lfs_deploy_token) if key && token == password
         else
           user = User.by_login(login)
-          token = Gitlab::LfsToken.new(user).get_value
+          token = Gitlab::LfsToken.new(user).value
           Result.new(user, :lfs_token) if user && token == password
         end
       end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 0685eb775ef..63656f0b4f1 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -6,15 +6,17 @@ module Gitlab
       @actor = actor
     end
 
-    def set_token
+    def generate
       token = Devise.friendly_token(50)
+
       Gitlab::Redis.with do |redis|
-        redis.set(redis_key, token, ex: 3600)
+        redis.set(redis_key, token, ex: 600)
       end
+
       token
     end
 
-    def get_value
+    def value
       Gitlab::Redis.with do |redis|
         redis.get(redis_key)
       end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 6ce680e3c26..4c8e09cd904 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -26,7 +26,7 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes user lfs tokens' do
       user = create(:user)
       ip = 'ip'
-      token = Gitlab::LfsToken.new(user).set_token
+      token = Gitlab::LfsToken.new(user).generate
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
       expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
@@ -35,7 +35,7 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes deploy key lfs tokens' do
       key = create(:deploy_key)
       ip = 'ip'
-      token = Gitlab::LfsToken.new(key).set_token
+      token = Gitlab::LfsToken.new(key).generate
 
       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))
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index 76b348637c7..1d2e4fd9566 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -4,7 +4,7 @@ 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
+        token = handler.generate
 
         expect(token).not_to be_nil
         expect(token).to be_a String
@@ -12,9 +12,9 @@ describe Gitlab::LfsToken, lib: true do
       end
 
       it 'returns the correct token based on the key' do
-        token = handler.set_token
+        token = handler.generate
 
-        expect(handler.get_value).to eq(token)
+        expect(handler.value).to eq(token)
       end
     end
 
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 59df5af770b..ff697286927 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -100,15 +100,20 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /internal/discover" do
+  describe "POST /internal/lfs_authenticate" do
+    before do
+      project.team << [user, :developer]
+    end
+
     context 'user key' do
       it 'returns the correct information about the key' do
-        get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+        lfs_auth(key, project)
 
         expect(response).to have_http_status(200)
+        expect(json_response['username']).to eq(user.username)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).value)
 
-        expect(json_response['name']).to eq(user.name)
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).get_value)
+        expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
     end
 
@@ -116,16 +121,26 @@ describe API::API, api: true  do
       let(:key) { create(:deploy_key) }
 
       it 'returns the correct information about the key' do
-        get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+        lfs_auth(key, project)
 
         expect(response).to have_http_status(200)
-
         expect(json_response['username']).to eq("lfs-deploy-key-#{key.id}")
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).get_value)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value)
+        expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
     end
   end
 
+  describe "GET /internal/discover" do
+    it do
+      get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+
+      expect(response).to have_http_status(200)
+
+      expect(json_response['name']).to eq(user.name)
+    end
+  end
+
   describe "POST /internal/allowed" do
     context "access granted" do
       before do
@@ -159,7 +174,6 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
-          expect(json_response["repository_http_path"]).to eq(project.http_url_to_repo)
         end
       end
 
@@ -170,7 +184,6 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
-          expect(json_response["repository_http_path"]).to eq(project.http_url_to_repo)
         end
       end
     end
@@ -407,4 +420,13 @@ describe API::API, api: true  do
       protocol: 'ssh'
     )
   end
+
+  def lfs_auth(key, project)
+    post(
+      api("/internal/lfs_authenticate"),
+      key_id: key.id,
+      secret_token: secret_token,
+      project: project.path_with_namespace
+    )
+  end
 end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index d15e72b2570..e61502400ff 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.id}", Gitlab::LfsToken.new(key).set_token)
+    ActionController::HttpAuthentication::Basic.encode_credentials("lfs-deploy-key-#{key.id}", Gitlab::LfsToken.new(key).generate)
   end
 
   def fork_project(project, user, object = nil)