Merge branch '328692-refactor-find-for-git-client-jwt-controller-part' into 'master'

Refactor jwt_controller to rely on auth results in a different manner

See merge request gitlab-org/gitlab!66377

app/controllers/jwt_controller.rb

@@ -19,7 +19,7 @@ class JwtController < ApplicationController
     service = SERVICES[params[:service]]
     return head :not_found unless service
 
-    result = service.new(@authentication_result.project, @authentication_result.actor, auth_params)
+    result = service.new(@authentication_result.project, auth_user, auth_params)
       .execute(authentication_abilities: @authentication_result.authentication_abilities)
 
     render json: result, status: result[:http_status]
@@ -67,7 +67,7 @@ class JwtController < ApplicationController
   end
 
   def additional_params
-    { scopes: scopes_param }.compact
+    { scopes: scopes_param, deploy_token: @authentication_result.deploy_token }.compact
   end
 
   # We have to parse scope here, because Docker Client does not send an array of scopes,
@@ -83,8 +83,7 @@ class JwtController < ApplicationController
 
   def auth_user
     strong_memoize(:auth_user) do
-      actor = @authentication_result&.actor
-      actor.is_a?(User) ? actor : nil
+      @authentication_result.auth_user
     end
   end
 end

app/services/auth/container_registry_authentication_service.rb

@@ -21,7 +21,7 @@ module Auth
 
       return error('DENIED', status: 403, message: 'access forbidden') unless has_registry_ability?
 
-      unless scopes.any? || current_user || project
+      unless scopes.any? || current_user || deploy_token || project
         return error('DENIED', status: 403, message: 'access forbidden')
       end
 
@@ -178,8 +178,7 @@ module Auth
     end
 
     def can_user?(ability, project)
-      user = current_user.is_a?(User) ? current_user : nil
-      can?(user, ability, project)
+      can?(current_user, ability, project)
     end
 
     def build_can_pull?(requested_project)
@@ -202,16 +201,16 @@ module Auth
 
     def deploy_token_can_pull?(requested_project)
       has_authentication_ability?(:read_container_image) &&
-        current_user.is_a?(DeployToken) &&
-        current_user.has_access_to?(requested_project) &&
-        current_user.read_registry?
+        deploy_token.present? &&
+        deploy_token.has_access_to?(requested_project) &&
+        deploy_token.read_registry?
     end
 
     def deploy_token_can_push?(requested_project)
       has_authentication_ability?(:create_container_image) &&
-        current_user.is_a?(DeployToken) &&
-        current_user.has_access_to?(requested_project) &&
-        current_user.write_registry?
+        deploy_token.present? &&
+        deploy_token.has_access_to?(requested_project) &&
+        deploy_token.write_registry?
     end
 
     ##
@@ -250,6 +249,10 @@ module Auth
       {}
     end
 
+    def deploy_token
+      params[:deploy_token]
+    end
+
     def log_if_actions_denied(type, requested_project, requested_actions, authorized_actions)
       return if requested_actions == authorized_actions
 

app/services/auth/dependency_proxy_authentication_service.rb

@@ -11,7 +11,7 @@ module Auth
 
       # Because app/controllers/concerns/dependency_proxy/auth.rb consumes this
       # JWT only as `User.find`, we currently only allow User (not DeployToken, etc)
-      return error('access forbidden', 403) unless current_user.is_a?(User)
+      return error('access forbidden', 403) unless current_user
 
       { token: authorized_token.encoded }
     end

lib/gitlab/auth/result.rb

@@ -21,6 +21,14 @@ module Gitlab
       def failed?
         !success?
       end
+
+      def auth_user
+        actor.is_a?(User) ? actor : nil
+      end
+
+      def deploy_token
+        actor.is_a?(DeployToken) ? actor : nil
+      end
     end
   end
 end

spec/lib/gitlab/auth/result_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::Result do
+  subject { described_class.new(actor, nil, nil, []) }
+
+  context 'when actor is User' do
+    let(:actor) { create(:user) }
+
+    it 'returns auth_user' do
+      expect(subject.auth_user).to eq(actor)
+    end
+
+    it 'does not return deploy token' do
+      expect(subject.deploy_token).to be_nil
+    end
+  end
+
+  context 'when actor is Deploy token' do
+    let(:actor) { create(:deploy_token) }
+
+    it 'returns deploy token' do
+      expect(subject.deploy_token).to eq(actor)
+    end
+
+    it 'does not return auth_user' do
+      expect(subject.auth_user).to be_nil
+    end
+  end
+end

spec/requests/jwt_controller_spec.rb

@@ -79,7 +79,7 @@ RSpec.describe JwtController do
 
           it 'authenticates correctly' do
             expect(response).to have_gitlab_http_status(:ok)
-            expect(service_class).to have_received(:new).with(nil, deploy_token, ActionController::Parameters.new(parameters).permit!)
+            expect(service_class).to have_received(:new).with(nil, nil, ActionController::Parameters.new(parameters.merge(deploy_token: deploy_token)).permit!)
           end
 
           it 'does not log a user' do

spec/services/auth/dependency_proxy_authentication_service_spec.rb

@@ -35,12 +35,6 @@ RSpec.describe Auth::DependencyProxyAuthenticationService do
       it_behaves_like 'returning', status: 403, message: 'access forbidden'
     end
 
-    context 'with a deploy token as user' do
-      let_it_be(:user) { create(:deploy_token) }
-
-      it_behaves_like 'returning', status: 403, message: 'access forbidden'
-    end
-
     context 'with a user' do
       it 'returns a token' do
         expect(subject[:token]).not_to be_nil

spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb

@@ -830,15 +830,15 @@ RSpec.shared_examples 'a container registry auth service' do
 
   context 'for deploy tokens' do
     let(:current_params) do
-      { scopes: ["repository:#{project.full_path}:pull"] }
+      { scopes: ["repository:#{project.full_path}:pull"], deploy_token: deploy_token }
     end
 
     context 'when deploy token has read and write registry as scopes' do
-      let(:current_user) { create(:deploy_token, write_registry: true, projects: [project]) }
+      let(:deploy_token) { create(:deploy_token, write_registry: true, projects: [project]) }
 
       shared_examples 'able to login' do
         context 'registry provides read_container_image authentication_abilities' do
-          let(:current_params) { {} }
+          let(:current_params) { { deploy_token: deploy_token } }
           let(:authentication_abilities) { [:read_container_image] }
 
           it_behaves_like 'an authenticated'
@@ -854,7 +854,7 @@ RSpec.shared_examples 'a container registry auth service' do
 
         context 'when pushing' do
           let(:current_params) do
-            { scopes: ["repository:#{project.full_path}:push"] }
+            { scopes: ["repository:#{project.full_path}:push"], deploy_token: deploy_token }
           end
 
           it_behaves_like 'a pushable'
@@ -872,7 +872,7 @@ RSpec.shared_examples 'a container registry auth service' do
 
         context 'when pushing' do
           let(:current_params) do
-            { scopes: ["repository:#{project.full_path}:push"] }
+            { scopes: ["repository:#{project.full_path}:push"], deploy_token: deploy_token }
           end
 
           it_behaves_like 'a pushable'
@@ -890,7 +890,7 @@ RSpec.shared_examples 'a container registry auth service' do
 
         context 'when pushing' do
           let(:current_params) do
-            { scopes: ["repository:#{project.full_path}:push"] }
+            { scopes: ["repository:#{project.full_path}:push"], deploy_token: deploy_token }
           end
 
           it_behaves_like 'a pushable'
@@ -901,18 +901,18 @@ RSpec.shared_examples 'a container registry auth service' do
     end
 
     context 'when deploy token does not have read_registry scope' do
-      let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) }
+      let(:deploy_token) do
+        create(:deploy_token, projects: [project], read_registry: false)
+      end
 
       shared_examples 'unable to login' do
         context 'registry provides no container authentication_abilities' do
-          let(:current_params) { {} }
           let(:authentication_abilities) { [] }
 
           it_behaves_like 'a forbidden'
         end
 
         context 'registry provides inapplicable container authentication_abilities' do
-          let(:current_params) { {} }
           let(:authentication_abilities) { [:download_code] }
 
           it_behaves_like 'a forbidden'
@@ -958,7 +958,7 @@ RSpec.shared_examples 'a container registry auth service' do
     end
 
     context 'when deploy token is not related to the project' do
-      let_it_be(:current_user) { create(:deploy_token, read_registry: false) }
+      let_it_be(:deploy_token) { create(:deploy_token, read_registry: false) }
 
       context 'for public project' do
         let_it_be(:project) { create(:project, :public) }
@@ -986,7 +986,7 @@ RSpec.shared_examples 'a container registry auth service' do
     end
 
     context 'when deploy token has been revoked' do
-      let(:current_user) { create(:deploy_token, :revoked, projects: [project]) }
+      let(:deploy_token) { create(:deploy_token, :revoked, projects: [project]) }
 
       context 'for public project' do
         let_it_be(:project) { create(:project, :public) }