From 72220a99d1cdbcf8a914f9e765c43e63eaee2548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 5 Apr 2018 15:49:18 +0200 Subject: [PATCH] Support Deploy Tokens properly without hacking abilities --- app/models/deploy_token.rb | 6 ++++- app/policies/project_policy.rb | 4 ++-- ...ntainer_registry_authentication_service.rb | 23 ++++++++++++++----- lib/gitlab/auth.rb | 20 ++++++++-------- lib/gitlab/git_access.rb | 4 ++-- spec/lib/gitlab/auth_spec.rb | 6 ++--- spec/policies/project_policy_spec.rb | 4 ++-- ...er_registry_authentication_service_spec.rb | 2 +- 8 files changed, 42 insertions(+), 27 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 + DeployToken.active.find_by(token: password) - if token && valid_scoped_token?(token, available_scopes) - Gitlab::Auth::Result.new(token, token.project, :deploy_token, abilities_for_scopes(token.scopes)) + 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