Support Deploy Tokens properly without hacking abilities

8 files changed, 43 insertions, 28 deletions

diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb
index b4df44d295a..c70d1457afb 100644
--- a/app/models/deploy_token.rb
+++ b/app/models/deploy_token.rb
@@ -29,6 +29,10 @@ class DeployToken < ActiveRecord::Base
   end
 
   def username
-    User.ghost.username
+    "gitlab+deploy-token-#{id}"
+  end
+
+  def has_access_to?(project)
+    self.project == project
   end
 end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 2f9dd0384bc..21bb0934dee 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -145,7 +145,7 @@ class ProjectPolicy < BasePolicy
   # These abilities are not allowed to admins that are not members of the project,
   # that's why they are defined separately.
   rule { guest & can?(:download_code) }.enable :build_download_code
-  rule { guest & can?(:read_container_image) }.enable :project_read_container_image
+  rule { guest & can?(:read_container_image) }.enable :build_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 :project_read_container_image
+    enable :build_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 d70ac7b1b3d..2ac35f5bd64 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -109,7 +109,7 @@ module Auth
 
       case requested_action
       when 'pull'
-        build_can_pull?(requested_project) || user_can_pull?(requested_project)
+        build_can_pull?(requested_project) || user_can_pull?(requested_project) || deploy_token_can_pull?(requested_project)
       when 'push'
         build_can_push?(requested_project) || user_can_push?(requested_project)
       when '*'
@@ -123,22 +123,33 @@ module Auth
       Gitlab.config.registry
     end
 
+    def can_user?(ability, project)
+      current_user.is_a?(User) &&
+        can?(current_user, ability, project)
+    end
+
     def build_can_pull?(requested_project)
       # 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?(:project_read_container_image) &&
-        (requested_project == project || can?(current_user, :project_read_container_image, requested_project))
+      has_authentication_ability?(:build_read_container_image) &&
+        (requested_project == project || can_user?(:build_read_container_image, requested_project))
     end
 
     def user_can_admin?(requested_project)
       has_authentication_ability?(:admin_container_image) &&
-        can?(current_user, :admin_container_image, requested_project)
+        can_user?(:admin_container_image, requested_project)
     end
 
     def user_can_pull?(requested_project)
       has_authentication_ability?(:read_container_image) &&
-        can?(current_user, :read_container_image, requested_project)
+        can_user?(:read_container_image, requested_project)
+    end
+    
+    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)
     end
 
     ##
@@ -154,7 +165,7 @@ module Auth
 
     def user_can_push?(requested_project)
       has_authentication_ability?(:create_container_image) &&
-        can?(current_user, :create_container_image, requested_project)
+        can_user?(current_user, :create_container_image, requested_project)
     end
 
     def error(code, status:, message: '')
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 35458f607c6..336cdbab5f0 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -26,7 +26,7 @@ module Gitlab
           lfs_token_check(login, password, project) ||
           oauth_access_token_check(login, password) ||
           personal_access_token_check(password) ||
-          deploy_token_check(project, password) ||
+          deploy_token_check(login, password) ||
           user_with_password_for_git(login, password) ||
           Gitlab::Auth::Result.new
 
@@ -176,18 +176,18 @@ module Gitlab
       # 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)
+      def deploy_token_check(login, password)
         return unless password.present?
 
         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, token.project, :deploy_token, abilities_for_scopes(token.scopes))
+          DeployToken.active.find_by(token: password)
+
+        return unless token
+        return unless login != "gitlab+deploy-token-#{token.id}"
+        
+        scopes = abilities_for_scopes(token.scopes)
+        if valid_scoped_token?(token, scopes)
+          Gitlab::Auth::Result.new(token, token.project, :deploy_token, scopes)
         end
       end
 
@@ -242,7 +242,7 @@ module Gitlab
         [
           :read_project,
           :build_download_code,
-          :project_read_container_image,
+          :build_read_container_image,
           :build_create_container_image
         ]
       end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index e3c723ab274..0d1ee73ca1a 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -290,10 +290,10 @@ module Gitlab
     def can_read_project?
       if deploy_key?
         deploy_key.has_access_to?(project)
+      elsif deploy_token?
+        deploy_token.has_access_to?(project)
       elsif user
         user.can?(:read_project, project)
-      elsif deploy_token?
-        deploy_token.active? && deploy_token.project == project
       elsif ci?
         true # allow CI (build without a user) for backwards compatibility
       end || Guest.can?(:read_project, project)
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index f704c20f598..4ed554f06ec 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_project, :build_download_code, :project_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, :build_read_container_image]))
         end
       end
 
@@ -310,7 +310,7 @@ describe Gitlab::Auth do
           end
 
           it 'succeeds if deploy token does have read_registry as scope' do
-            abilities = %i(read_project build_download_code project_read_container_image)
+            abilities = %i(read_project build_download_code build_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: '')
@@ -477,7 +477,7 @@ describe Gitlab::Auth do
     [
       :read_project,
       :build_download_code,
-      :project_read_container_image,
+      :build_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 f5d9a58f83c..905d82b3bb1 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 project_read_container_image]
+    %i[build_download_code build_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 project_read_container_image
+      read_container_image build_download_code build_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 1cb0508cdf5..290eeae828e 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
-      [:project_read_container_image, :build_create_container_image]
+      [:build_read_container_image, :build_create_container_image]
     end
 
     before do