Merge branch 'lfs-ssh-authorization-fix' into 'master'

Do not regenerate the `lfs_token` every time `git-lfs-authenticate` is called

## What does this MR do?

 Do not regenerate the `lfs_token` every time `git-lfs-authenticate` is called, instead return the saved token if one is present.

This was causing a lot of 401s, leading to 403s, as state in #22527

As it turns out, when pushing a lot of LFS objects, the LFS client was calling `git-lfs-authenticate` in the middle of the request again. This caused the `lfs_token` to be regenerated. The problem lies in that the LFS client was not aware of this change, and was still using the old token. This caused all subsequent requests to fail with a 401 error.

Since HTTP Auth is protected by Rack Attack, this 401s where immediately flagged and resulted in the IP of the user being banned. 

With this change, GitLab returns the value stored in Redis, if one is present, thus if the LFS client calls `git-lfs-authenticate` again during the request, the auth header will remain unchanged, allowing all subsequent requests to continue without issues.

## What are the relevant issue numbers?

Fixes #22527

cc @SeanPackham @jacobvosmaer-gitlab

See merge request !6551

7 files changed, 47 insertions, 21 deletions

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 090d04544da..9a5d1ece070 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -90,7 +90,7 @@ module API
 
         {
           username: token_handler.actor_name,
-          lfs_token: token_handler.generate,
+          lfs_token: token_handler.token,
           repository_http_path: project.http_url_to_repo
         }
       end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 7c0f2115d43..aca5d0020cf 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -124,7 +124,7 @@ module Gitlab
             read_authentication_abilities
           end
 
-        Result.new(actor, nil, token_handler.type, authentication_abilities) if Devise.secure_compare(token_handler.value, password)
+        Result.new(actor, nil, token_handler.type, authentication_abilities) if Devise.secure_compare(token_handler.token, password)
       end
 
       def build_access_token_check(login, password)
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index d089a2f9b0b..7b3bbcf6a32 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -17,19 +17,18 @@ module Gitlab
         end
     end
 
-    def generate
-      token = Devise.friendly_token(TOKEN_LENGTH)
-
+    def token
       Gitlab::Redis.with do |redis|
-        redis.set(redis_key, token, ex: EXPIRY_TIME)
-      end
+        token = redis.get(redis_key)
 
-      token
-    end
+        if token
+          redis.expire(redis_key, EXPIRY_TIME)
+        else
+          token = Devise.friendly_token(TOKEN_LENGTH)
+          redis.set(redis_key, token, ex: EXPIRY_TIME)
+        end
 
-    def value
-      Gitlab::Redis.with do |redis|
-        redis.get(redis_key)
+        token
       end
     end
 
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 745fbc0df45..c9d64e99f88 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -64,7 +64,7 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes user lfs tokens' do
       user = create(:user)
       ip = 'ip'
-      token = Gitlab::LfsToken.new(user).generate
+      token = Gitlab::LfsToken.new(user).token
 
       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, nil, :lfs_token, full_authentication_abilities))
@@ -73,7 +73,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).generate
+      token = Gitlab::LfsToken.new(key).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, nil, :lfs_deploy_token, read_authentication_abilities))
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index 9f04f67e0a8..e9c1163e22a 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe Gitlab::LfsToken, lib: true do
-  describe '#generate and #value' do
+  describe '#token' do
     shared_examples 'an LFS token generator' do
       it 'returns a randomly generated token' do
-        token = handler.generate
+        token = handler.token
 
         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.generate
+        token = handler.token
 
-        expect(handler.value).to eq(token)
+        expect(handler.token).to eq(token)
       end
     end
 
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 46e8e6f1169..f0f590b0331 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -111,7 +111,7 @@ describe API::API, api: true  do
 
         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(key).value)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).token)
 
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
@@ -131,7 +131,7 @@ describe API::API, api: true  do
 
         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).value)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).token)
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
     end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 09e4e265dd1..dbdf83a0dff 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -257,6 +257,29 @@ describe 'Git LFS API and storage' do
           it_behaves_like 'responds with a file'
         end
 
+        describe 'when using a user key' do
+          let(:authorization) { authorize_user_key }
+
+          context 'when user allowed' do
+            let(:update_permissions) do
+              project.team << [user, :master]
+              project.lfs_objects << lfs_object
+            end
+
+            it_behaves_like 'responds with a file'
+          end
+
+          context 'when user not allowed' do
+            let(:update_permissions) do
+              project.lfs_objects << lfs_object
+            end
+
+            it 'responds with status 404' do
+              expect(response).to have_http_status(404)
+            end
+          end
+        end
+
         context 'when build is authorized as' do
           let(:authorization) { authorize_ci_project }
 
@@ -1110,7 +1133,11 @@ 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).generate)
+    ActionController::HttpAuthentication::Basic.encode_credentials("lfs+deploy-key-#{key.id}", Gitlab::LfsToken.new(key).token)
+  end
+
+  def authorize_user_key
+    ActionController::HttpAuthentication::Basic.encode_credentials(user.username, Gitlab::LfsToken.new(user).token)
   end
 
   def fork_project(project, user, object = nil)