Commit 7deab317 authored by Mayra Cabrera's avatar Mayra Cabrera

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
parent 726f5bbf
...@@ -23,8 +23,7 @@ class JwtController < ApplicationController ...@@ -23,8 +23,7 @@ class JwtController < ApplicationController
@authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_authentication_abilities) @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_authentication_abilities)
authenticate_with_http_basic do |login, password| authenticate_with_http_basic do |login, password|
project = find_project_related(password) @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: nil, ip: request.ip)
@authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
if @authentication_result.failed? || if @authentication_result.failed? ||
(@authentication_result.actor.present? && !user_or_deploy_token) (@authentication_result.actor.present? && !user_or_deploy_token)
...@@ -59,10 +58,6 @@ class JwtController < ApplicationController ...@@ -59,10 +58,6 @@ class JwtController < ApplicationController
params.permit(:service, :scope, :account, :client_id) params.permit(:service, :scope, :account, :client_id)
end end
def find_project_related(password)
DeployToken.active.find_by(token: password)&.project
end
def user_or_deploy_token def user_or_deploy_token
@authentication_result.actor.is_a?(User) || @authentication_result.actor.is_a?(DeployToken) @authentication_result.actor.is_a?(User) || @authentication_result.actor.is_a?(DeployToken)
end end
......
...@@ -143,9 +143,9 @@ class ProjectPolicy < BasePolicy ...@@ -143,9 +143,9 @@ class ProjectPolicy < BasePolicy
end end
# These abilities are not allowed to admins that are not members of the project, # 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?(: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 rule { can?(:reporter_access) }.policy do
enable :download_code enable :download_code
...@@ -179,7 +179,7 @@ class ProjectPolicy < BasePolicy ...@@ -179,7 +179,7 @@ class ProjectPolicy < BasePolicy
enable :fork_project enable :fork_project
enable :build_download_code enable :build_download_code
enable :build_read_container_image enable :project_read_container_image
enable :request_access enable :request_access
end end
......
...@@ -127,8 +127,8 @@ module Auth ...@@ -127,8 +127,8 @@ module Auth
# Build can: # Build can:
# 1. pull from its own project (for ex. a build) # 1. pull from its own project (for ex. a build)
# 2. read images from dependent projects if creator of build is a team member # 2. read images from dependent projects if creator of build is a team member
has_authentication_ability?(:build_read_container_image) && has_authentication_ability?(:project_read_container_image) &&
(requested_project == project || can?(current_user, :build_read_container_image, requested_project)) (requested_project == project || can?(current_user, :project_read_container_image, requested_project))
end end
def user_can_admin?(requested_project) def user_can_admin?(requested_project)
......
...@@ -164,7 +164,7 @@ module Gitlab ...@@ -164,7 +164,7 @@ module Gitlab
def abilities_for_scopes(scopes) def abilities_for_scopes(scopes)
abilities_by_scope = { abilities_by_scope = {
api: full_authentication_abilities, 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] read_repo: read_authentication_abilities - [:read_container_image]
} }
...@@ -173,13 +173,21 @@ module Gitlab ...@@ -173,13 +173,21 @@ module Gitlab
end.uniq end.uniq
end 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) 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) 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
end end
...@@ -234,7 +242,7 @@ module Gitlab ...@@ -234,7 +242,7 @@ module Gitlab
[ [
:read_project, :read_project,
:build_download_code, :build_download_code,
:build_read_container_image, :project_read_container_image,
:build_create_container_image :build_create_container_image
] ]
end end
......
...@@ -195,7 +195,7 @@ describe Gitlab::Auth do ...@@ -195,7 +195,7 @@ describe Gitlab::Auth do
personal_access_token = create(:personal_access_token, scopes: ['read_registry']) 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).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
end end
...@@ -258,72 +258,119 @@ describe Gitlab::Auth do ...@@ -258,72 +258,119 @@ describe Gitlab::Auth do
context 'while using deploy tokens' do context 'while using deploy tokens' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:deploy_token) { create(:deploy_token, :read_repo, project: project) }
let(:auth_failure) { Gitlab::Auth::Result.new(nil, nil) } 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 context 'when the deploy token has read_repo as scope' do
auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, abilities) let(:deploy_token) { create(:deploy_token, :read_repo, project: project) }
expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '') it 'succeeds when project is present, token is valid and has read_repo as scope' do
expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip')) abilities = %i(read_project download_code)
.to eq(auth_success) auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, abilities)
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 expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
another_project = create(:project) expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip'))
expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '') .to eq(auth_success)
expect(gl_auth.find_for_git_client('', deploy_token.token, project: another_project, ip: 'ip')) end
.to eq(auth_failure)
end
it 'fails if token has been revoked' do it 'fails if token is nil' do
deploy_token.revoke! 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 it 'fails if token is not related to project' do
expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deploy-token') expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
expect(gl_auth.find_for_git_client('deploy-token', deploy_token.token, project: project, ip: 'ip')) expect(gl_auth.find_for_git_client('', 'abcdef', project: project, ip: 'ip'))
.to eq(auth_failure) .to eq(auth_failure)
end end
context 'when registry enabled' do it 'fails for any other project' do
before do another_project = create(:project)
stub_container_registry_config(enabled: true) 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 end
it 'succeeds if deploy token does have read_registry as scope' do it 'fails if token has been revoked' do
deploy_token = create(:deploy_token, :read_registry, project: project) deploy_token.revoke!
auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, [:read_container_image])
expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '') expect(deploy_token.revoked?).to be_truthy
expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip')) expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deploy-token')
.to eq(auth_success) expect(gl_auth.find_for_git_client('deploy-token', deploy_token.token, project: project, ip: 'ip'))
.to eq(auth_failure)
end end
end end
context 'when registry disabled' do context 'when the deploy token has read_registry as a scope' do
before do let(:deploy_token) { create(:deploy_token, :read_registry, project: project) }
stub_container_registry_config(enabled: false)
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 end
it 'fails if deploy token have read_registry as scope' do context 'when registry disabled' do
deploy_token = create(:deploy_token, :read_registry, project: project) before do
stub_container_registry_config(enabled: false)
end
expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '') it 'fails if deploy token have read_registry as scope' do
expect(gl_auth.find_for_git_client('', deploy_token.token, project: project, ip: 'ip')) expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
.to eq(auth_failure) 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
end end
...@@ -430,7 +477,7 @@ describe Gitlab::Auth do ...@@ -430,7 +477,7 @@ describe Gitlab::Auth do
[ [
:read_project, :read_project,
:build_download_code, :build_download_code,
:build_read_container_image, :project_read_container_image,
:build_create_container_image :build_create_container_image
] ]
end end
......
...@@ -28,7 +28,7 @@ describe ProjectPolicy do ...@@ -28,7 +28,7 @@ describe ProjectPolicy do
end end
let(:team_member_reporter_permissions) do let(:team_member_reporter_permissions) do
%i[build_download_code build_read_container_image] %i[build_download_code project_read_container_image]
end end
let(:developer_permissions) do let(:developer_permissions) do
...@@ -54,7 +54,7 @@ describe ProjectPolicy do ...@@ -54,7 +54,7 @@ describe ProjectPolicy do
let(:public_permissions) do let(:public_permissions) do
%i[ %i[
download_code fork_project read_commit_status read_pipeline 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 download_wiki_code
] ]
end end
......
...@@ -373,7 +373,7 @@ describe Auth::ContainerRegistryAuthenticationService do ...@@ -373,7 +373,7 @@ describe Auth::ContainerRegistryAuthenticationService do
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
let(:authentication_abilities) do let(:authentication_abilities) do
[:build_read_container_image, :build_create_container_image] [:project_read_container_image, :build_create_container_image]
end end
before do before do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment