Removes logic from Jwt and handle different scenarios on Gitlab::Auth

- When using 'read_repo' password and project are sent, so we used both
  of them to fetch for the token
- When using 'read_registry' only the password is sent, so we only use
  that for fetching the token

7 files changed, 121 insertions, 71 deletions

diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb
index 76e7473e92c..0caa5f4f439 100644
--- a/app/controllers/jwt_controller.rb
+++ b/app/controllers/jwt_controller.rb
@@ -23,8 +23,7 @@ class JwtController < ApplicationController
     @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_authentication_abilities)
 
     authenticate_with_http_basic do |login, password|
-      project = find_project_related(password)
-      @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
+      @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: nil, ip: request.ip)
 
       if @authentication_result.failed? ||
           (@authentication_result.actor.present? && !user_or_deploy_token)
@@ -59,10 +58,6 @@ class JwtController < ApplicationController
     params.permit(:service, :scope, :account, :client_id)
   end
 
-  def find_project_related(password)
-    DeployToken.active.find_by(token: password)&.project
-  end
-
   def user_or_deploy_token
     @authentication_result.actor.is_a?(User) || @authentication_result.actor.is_a?(DeployToken)
   end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index b1ed034cd00..2f9dd0384bc 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -143,9 +143,9 @@ class ProjectPolicy < BasePolicy
   end
 
   # These abilities are not allowed to admins that are not members of the project,
-  # that's why they are defined separatly.
+  # that's why they are defined separately.
   rule { guest & can?(:download_code) }.enable :build_download_code
-  rule { guest & can?(:read_container_image) }.enable :build_read_container_image
+  rule { guest & can?(:read_container_image) }.enable :project_read_container_image
 
   rule { can?(:reporter_access) }.policy do
     enable :download_code
@@ -179,7 +179,7 @@ class ProjectPolicy < BasePolicy
 
     enable :fork_project
     enable :build_download_code
-    enable :build_read_container_image
+    enable :project_read_container_image
     enable :request_access
   end
 
diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 2b77f6be72a..d70ac7b1b3d 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -127,8 +127,8 @@ module Auth
       # Build can:
       # 1. pull from its own project (for ex. a build)
       # 2. read images from dependent projects if creator of build is a team member
-      has_authentication_ability?(:build_read_container_image) &&
-        (requested_project == project || can?(current_user, :build_read_container_image, requested_project))
+      has_authentication_ability?(:project_read_container_image) &&
+        (requested_project == project || can?(current_user, :project_read_container_image, requested_project))
     end
 
     def user_can_admin?(requested_project)
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 77fef7d8cac..3ef2f7f2967 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -164,7 +164,7 @@ module Gitlab
       def abilities_for_scopes(scopes)
         abilities_by_scope = {
           api: full_authentication_abilities,
-          read_registry: [:read_container_image],
+          read_registry: build_authentication_abilities - [:build_create_container_image],
           read_repo: read_authentication_abilities - [:read_container_image]
         }
 
@@ -173,13 +173,21 @@ module Gitlab
         end.uniq
       end
 
+      # Project is always sent when using read_scope,
+      # but is not sent when using read_registry scope
+      # (since jwt is not context aware of the project)
       def deploy_token_check(project, password)
-        return unless project.present? && password.present?
+        return unless password.present?
 
-        token = DeployToken.active.find_by(project: project, token: password)
+        token =
+          if project.present?
+            DeployToken.active.find_by(project: project, token: password)
+          else
+            DeployToken.active.find_by(token: password)
+          end
 
         if token && valid_scoped_token?(token, available_scopes)
-          Gitlab::Auth::Result.new(token, project, :deploy_token, abilities_for_scopes(token.scopes))
+          Gitlab::Auth::Result.new(token, token.project, :deploy_token, abilities_for_scopes(token.scopes))
         end
       end
 
@@ -234,7 +242,7 @@ module Gitlab
         [
           :read_project,
           :build_download_code,
-          :build_read_container_image,
+          :project_read_container_image,
           :build_create_container_image
         ]
       end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 758fb17cd81..79984787e2a 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -195,7 +195,7 @@ describe Gitlab::Auth do
           personal_access_token = create(:personal_access_token, scopes: ['read_registry'])
 
           expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
-          expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_access_token, [:read_container_image]))
+          expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_access_token, [:read_project, :build_download_code, :project_read_container_image]))
         end
       end
 
@@ -258,72 +258,119 @@ describe Gitlab::Auth do
 
     context 'while using deploy tokens' do
       let(:project) { create(:project) }
-      let(:deploy_token) { create(:deploy_token, :read_repo, project: project) }
       let(:auth_failure) { Gitlab::Auth::Result.new(nil, nil) }
-      let(:abilities) { %i(read_project download_code) }
 
-      it 'succeeds when project is present, token is valid and has read_repo as scope' do
-        auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, abilities)
+      context 'when the deploy token has read_repo as scope' do
+        let(:deploy_token) { create(:deploy_token, :read_repo, project: project) }
 
-        expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
-        expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip'))
-          .to eq(auth_success)
-      end
-
-      it 'fails if token is nil' do
-        expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
-        expect(gl_auth.find_for_git_client('', nil, project: project, ip: 'ip'))
-          .to eq(auth_failure)
-      end
-
-      it 'fails if token is not related to project' do
-        expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
-        expect(gl_auth.find_for_git_client('', 'abcdef', project: project, ip: 'ip'))
-          .to eq(auth_failure)
-      end
+        it 'succeeds when project is present, token is valid and has read_repo as scope' do
+          abilities = %i(read_project download_code)
+          auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, abilities)
 
-      it 'fails for any other project' do
-        another_project = create(:project)
-        expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
-        expect(gl_auth.find_for_git_client('', deploy_token.token, project: another_project, ip: 'ip'))
-          .to eq(auth_failure)
-      end
+          expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
+          expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip'))
+            .to eq(auth_success)
+        end
 
-      it 'fails if token has been revoked' do
-        deploy_token.revoke!
+        it 'fails if token is nil' do
+          expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+          expect(gl_auth.find_for_git_client('', nil, project: project, ip: 'ip'))
+            .to eq(auth_failure)
+        end
 
-        expect(deploy_token.revoked?).to be_truthy
-        expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deploy-token')
-        expect(gl_auth.find_for_git_client('deploy-token', deploy_token.token, project: project, ip: 'ip'))
-          .to eq(auth_failure)
-      end
+        it 'fails if token is not related to project' do
+          expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+          expect(gl_auth.find_for_git_client('', 'abcdef', project: project, ip: 'ip'))
+            .to eq(auth_failure)
+        end
 
-      context 'when registry enabled' do
-        before do
-          stub_container_registry_config(enabled: true)
+        it 'fails for any other project' do
+          another_project = create(:project)
+          expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+          expect(gl_auth.find_for_git_client('', deploy_token.token, project: another_project, ip: 'ip'))
+            .to eq(auth_failure)
         end
 
-        it 'succeeds if deploy token does have read_registry as scope' do
-          deploy_token = create(:deploy_token, :read_registry, project: project)
-          auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, [:read_container_image])
+        it 'fails if token has been revoked' do
+          deploy_token.revoke!
 
-          expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
-          expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip'))
-            .to eq(auth_success)
+          expect(deploy_token.revoked?).to be_truthy
+          expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deploy-token')
+          expect(gl_auth.find_for_git_client('deploy-token', deploy_token.token, project: project, ip: 'ip'))
+            .to eq(auth_failure)
         end
       end
 
-      context 'when registry disabled' do
-        before do
-          stub_container_registry_config(enabled: false)
+      context 'when the deploy token has read_registry as a scope' do
+        let(:deploy_token) { create(:deploy_token, :read_registry, project: project) }
+
+        context 'when registry enabled' do
+          before do
+            stub_container_registry_config(enabled: true)
+          end
+
+          it 'succeeds if deploy token does have read_registry as scope' do
+            abilities = %i(read_project build_download_code project_read_container_image)
+            auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, abilities)
+
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
+            expect(gl_auth.find_for_git_client('', deploy_token.token, project: nil, ip: 'ip'))
+              .to eq(auth_success)
+          end
+
+          it 'fails if token is nil' do
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+            expect(gl_auth.find_for_git_client('', nil, project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
+
+          it 'fails if token is not related to project' do
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+            expect(gl_auth.find_for_git_client('', 'abcdef', project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
+
+          it 'fails if token has been revoked' do
+            deploy_token.revoke!
+
+            expect(deploy_token.revoked?).to be_truthy
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deploy-token')
+            expect(gl_auth.find_for_git_client('deploy-token', deploy_token.token, project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
         end
 
-        it 'fails if deploy token have read_registry as scope' do
-          deploy_token = create(:deploy_token, :read_registry, project: project)
+        context 'when registry disabled' do
+          before do
+            stub_container_registry_config(enabled: false)
+          end
 
-          expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
-          expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip'))
-            .to eq(auth_failure)
+          it 'fails if deploy token have read_registry as scope' do
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+            expect(gl_auth.find_for_git_client('', deploy_token.token, project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
+
+          it 'fails if token is nil' do
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+            expect(gl_auth.find_for_git_client('', nil, project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
+
+          it 'fails if token is not related to project' do
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+            expect(gl_auth.find_for_git_client('', 'abcdef', project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
+
+          it 'fails if token has been revoked' do
+            deploy_token.revoke!
+
+            expect(deploy_token.revoked?).to be_truthy
+            expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deploy-token')
+            expect(gl_auth.find_for_git_client('deploy-token', deploy_token.token, project: nil, ip: 'ip'))
+              .to eq(auth_failure)
+          end
         end
       end
     end
@@ -430,7 +477,7 @@ describe Gitlab::Auth do
     [
       :read_project,
       :build_download_code,
-      :build_read_container_image,
+      :project_read_container_image,
       :build_create_container_image
     ]
   end
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 905d82b3bb1..f5d9a58f83c 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -28,7 +28,7 @@ describe ProjectPolicy do
   end
 
   let(:team_member_reporter_permissions) do
-    %i[build_download_code build_read_container_image]
+    %i[build_download_code project_read_container_image]
   end
 
   let(:developer_permissions) do
@@ -54,7 +54,7 @@ describe ProjectPolicy do
   let(:public_permissions) do
     %i[
       download_code fork_project read_commit_status read_pipeline
-      read_container_image build_download_code build_read_container_image
+      read_container_image build_download_code project_read_container_image
       download_wiki_code
     ]
   end
diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb
index 290eeae828e..1cb0508cdf5 100644
--- a/spec/services/auth/container_registry_authentication_service_spec.rb
+++ b/spec/services/auth/container_registry_authentication_service_spec.rb
@@ -373,7 +373,7 @@ describe Auth::ContainerRegistryAuthenticationService do
     let(:current_user) { create(:user) }
 
     let(:authentication_abilities) do
-      [:build_read_container_image, :build_create_container_image]
+      [:project_read_container_image, :build_create_container_image]
     end
 
     before do