From 7deab3172257bef7818ce834c1e0709432ddd5e0 Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Tue, 3 Apr 2018 16:34:56 -0500 Subject: [PATCH] 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 --- app/controllers/jwt_controller.rb | 7 +- app/policies/project_policy.rb | 6 +- ...ntainer_registry_authentication_service.rb | 4 +- lib/gitlab/auth.rb | 18 +- spec/lib/gitlab/auth_spec.rb | 157 ++++++++++++------ spec/policies/project_policy_spec.rb | 4 +- ...er_registry_authentication_service_spec.rb | 2 +- 7 files changed, 124 insertions(+), 74 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,73 +258,120 @@ 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 '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 '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: project, ip: 'ip')) - .to eq(auth_failure) - end - - 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 - deploy_token = create(:deploy_token, :read_registry, project: project) - auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, [:read_container_image]) + 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) 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 - end - context 'when registry disabled' do - before do - stub_container_registry_config(enabled: false) + 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 deploy token have read_registry as scope' do - deploy_token = create(:deploy_token, :read_registry, project: project) - + 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('', deploy_token.token, project: project, ip: 'ip')) + expect(gl_auth.find_for_git_client('', 'abcdef', project: project, ip: 'ip')) .to eq(auth_failure) end + + 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 '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: project, ip: 'ip')) + .to eq(auth_failure) + end + end + + 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 + + context 'when registry disabled' do + before do + stub_container_registry_config(enabled: false) + end + + 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 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