Commit f7ae37c1 authored by Kamil Trzcinski's avatar Kamil Trzcinski

Simplify checking of allowed abilities in git_http_client_controller

parent 9d8afa22
...@@ -4,7 +4,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -4,7 +4,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController
include ActionController::HttpAuthentication::Basic include ActionController::HttpAuthentication::Basic
include KerberosSpnegoHelper include KerberosSpnegoHelper
attr_reader :actor, :authentication_abilities attr_reader :authentication_result
delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true
alias_method :user, :actor
# Git clients will not know what authenticity token to send along # Git clients will not know what authenticity token to send along
skip_before_action :verify_authenticity_token skip_before_action :verify_authenticity_token
...@@ -26,9 +30,12 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -26,9 +30,12 @@ class Projects::GitHttpClientController < Projects::ApplicationController
return # Allow access return # Allow access
end end
elsif allow_kerberos_spnego_auth? && spnego_provided? elsif allow_kerberos_spnego_auth? && spnego_provided?
@actor = find_kerberos_user user = find_kerberos_user
if user
@authentication_result = Gitlab::Auth::Result.new(
user, nil, :kerberos, Gitlab::Auth.full_authentication_abilities)
if actor
send_final_spnego_response send_final_spnego_response
return # Allow access return # Allow access
end end
...@@ -104,56 +111,40 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -104,56 +111,40 @@ class Projects::GitHttpClientController < Projects::ApplicationController
render plain: 'Not Found', status: :not_found render plain: 'Not Found', status: :not_found
end end
def ci?
@ci
end
def user
@actor
end
def handle_basic_authentication(login, password) def handle_basic_authentication(login, password)
auth_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: project, ip: request.ip)
return false unless @authentication_result.success?
case auth_result.type
when :ci
if auth_result.project == project && download_request?
@ci = true
else
return false
end
when :oauth
if download_request? if download_request?
@actor = auth_result.actor authentication_has_download_access?
@authentication_abilities = auth_result.authentication_abilities
else else
return false authentication_has_upload_access?
end end
when :lfs_deploy_token
if download_request?
@lfs_deploy_key = true
@actor = auth_result.actor
@authentication_abilities = auth_result.authentication_abilities
else
return false
end end
when :lfs_token, :personal_token, :gitlab_or_ldap, :build
@actor = auth_result.actor def authentication_has_download_access?
@authentication_abilities = auth_result.authentication_abilities has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
else end
# Not allowed
return false def authentication_has_upload_access?
has_authentication_ability?(:push_code)
end end
true def ci?
authentication_result && authentication_result.ci? &&
authentication_result.project && authentication_result.project == project
end end
def lfs_deploy_key? def lfs_deploy_key?
@lfs_deploy_key && actor && actor.projects.include?(project) authentication_result && authentication_result.lfs_deploy_token? &&
actor && actor.projects.include?(project)
end end
def has_authentication_ability?(capability) def has_authentication_ability?(capability)
@authentication_abilities.include?(capability) authentication_abilities &&
authentication_abilities.include?(capability)
end end
def verify_workhorse_api! def verify_workhorse_api!
......
module Gitlab module Gitlab
module Auth module Auth
Result = Struct.new(:actor, :project, :type, :authentication_abilities) do Result = Struct.new(:actor, :project, :type, :authentication_abilities) do
def ci?
type == :ci
end
def lfs_deploy_token?
type == :lfs_deploy_token
end
def success? def success?
actor.present? || type == :ci actor.present? || type == :ci
end end
...@@ -143,6 +151,8 @@ module Gitlab ...@@ -143,6 +151,8 @@ module Gitlab
end end
end end
public
def build_authentication_abilities def build_authentication_abilities
[ [
:read_project, :read_project,
......
...@@ -346,7 +346,7 @@ describe 'Git HTTP requests', lib: true do ...@@ -346,7 +346,7 @@ describe 'Git HTTP requests', lib: true do
it 'uploads get status 403' do it 'uploads get status 403' do
push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
expect(response).to have_http_status(403) expect(response).to have_http_status(401)
end end
end end
......
...@@ -310,8 +310,8 @@ describe 'Git LFS API and storage' do ...@@ -310,8 +310,8 @@ describe 'Git LFS API and storage' do
let(:build) { create(:ci_build, :running, pipeline: pipeline) } let(:build) { create(:ci_build, :running, pipeline: pipeline) }
it_behaves_like 'can download LFS only from own projects' do it_behaves_like 'can download LFS only from own projects' do
# We render 401, to prevent data leakage about existence of the project # We render 404, to prevent data leakage about existence of the project
let(:other_project_status) { 401 } let(:other_project_status) { 404 }
end end
end end
end end
...@@ -545,8 +545,8 @@ describe 'Git LFS API and storage' do ...@@ -545,8 +545,8 @@ describe 'Git LFS API and storage' do
let(:build) { create(:ci_build, :running, pipeline: pipeline) } let(:build) { create(:ci_build, :running, pipeline: pipeline) }
it_behaves_like 'can download LFS only from own projects' do it_behaves_like 'can download LFS only from own projects' do
# We render 401, to prevent data leakage about existence of the project # We render 404, to prevent data leakage about existence of the project
let(:other_project_status) { 401 } let(:other_project_status) { 404 }
end end
end end
end end
...@@ -706,8 +706,8 @@ describe 'Git LFS API and storage' do ...@@ -706,8 +706,8 @@ describe 'Git LFS API and storage' do
context 'tries to push to own project' do context 'tries to push to own project' do
let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
it 'responds with 403' do it 'responds with 401' do
expect(response).to have_http_status(403) expect(response).to have_http_status(401)
end end
end end
...@@ -716,8 +716,8 @@ describe 'Git LFS API and storage' do ...@@ -716,8 +716,8 @@ describe 'Git LFS API and storage' do
let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
it 'responds with 403' do it 'responds with 401' do
expect(response).to have_http_status(403) expect(response).to have_http_status(401)
end end
end end
end end
...@@ -925,8 +925,8 @@ describe 'Git LFS API and storage' do ...@@ -925,8 +925,8 @@ describe 'Git LFS API and storage' do
put_authorize put_authorize
end end
it 'responds with 403' do it 'responds with 401' do
expect(response).to have_http_status(403) expect(response).to have_http_status(401)
end end
end end
...@@ -939,8 +939,8 @@ describe 'Git LFS API and storage' do ...@@ -939,8 +939,8 @@ describe 'Git LFS API and storage' do
put_authorize put_authorize
end end
it 'responds with 404' do it 'responds with 401' do
expect(response).to have_http_status(404) expect(response).to have_http_status(401)
end end
end end
end end
...@@ -1025,8 +1025,8 @@ describe 'Git LFS API and storage' do ...@@ -1025,8 +1025,8 @@ describe 'Git LFS API and storage' do
context 'tries to push to own project' do context 'tries to push to own project' do
let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
it 'responds with 403' do it 'responds with 401' do
expect(response).to have_http_status(403) expect(response).to have_http_status(401)
end end
end end
...@@ -1035,8 +1035,8 @@ describe 'Git LFS API and storage' do ...@@ -1035,8 +1035,8 @@ describe 'Git LFS API and storage' do
let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
it 'responds with 403' do it 'responds with 401' do
expect(response).to have_http_status(403) expect(response).to have_http_status(401)
end end
end end
end end
......
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