diff options
author | Kamil Trzcinski <ayufan@ayufan.eu> | 2016-09-16 13:34:05 +0200 |
---|---|---|
committer | Kamil Trzcinski <ayufan@ayufan.eu> | 2016-09-16 13:34:05 +0200 |
commit | f7ae37c1d092f89cd9b9dc24be95670abed16ffc (patch) | |
tree | 16a553a15676adaeaf4c1c9f2e9eaf46c9c8c2a8 | |
parent | 9d8afa222c678a2222f5219458759897089d7dad (diff) | |
download | gitlab-ce-f7ae37c1d092f89cd9b9dc24be95670abed16ffc.tar.gz |
Simplify checking of allowed abilities in git_http_client_controller
-rw-r--r-- | app/controllers/projects/git_http_client_controller.rb | 75 | ||||
-rw-r--r-- | lib/gitlab/auth.rb | 10 | ||||
-rw-r--r-- | spec/requests/git_http_spec.rb | 2 | ||||
-rw-r--r-- | spec/requests/lfs_http_spec.rb | 32 |
4 files changed, 60 insertions, 59 deletions
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb index 3cc915ecc2a..632dac6aac9 100644 --- a/app/controllers/projects/git_http_client_controller.rb +++ b/app/controllers/projects/git_http_client_controller.rb @@ -4,7 +4,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController include ActionController::HttpAuthentication::Basic 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 skip_before_action :verify_authenticity_token @@ -26,9 +30,12 @@ class Projects::GitHttpClientController < Projects::ApplicationController return # Allow access end 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 return # Allow access end @@ -104,56 +111,40 @@ class Projects::GitHttpClientController < Projects::ApplicationController render plain: 'Not Found', status: :not_found end - def ci? - @ci - end + def handle_basic_authentication(login, password) + @authentication_result = Gitlab::Auth.find_for_git_client( + login, password, project: project, ip: request.ip) - def user - @actor - end + return false unless @authentication_result.success? - def handle_basic_authentication(login, password) - auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip) - - case auth_result.type - when :ci - if auth_result.project == project && download_request? - @ci = true - else - return false - end - when :oauth - if download_request? - @actor = auth_result.actor - @authentication_abilities = auth_result.authentication_abilities - else - return false - 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 - when :lfs_token, :personal_token, :gitlab_or_ldap, :build - @actor = auth_result.actor - @authentication_abilities = auth_result.authentication_abilities + if download_request? + authentication_has_download_access? else - # Not allowed - return false + authentication_has_upload_access? end + end + + def authentication_has_download_access? + has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code) + end + + def authentication_has_upload_access? + has_authentication_ability?(:push_code) + end - true + def ci? + authentication_result && authentication_result.ci? && + authentication_result.project && authentication_result.project == project end 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 def has_authentication_ability?(capability) - @authentication_abilities.include?(capability) + authentication_abilities && + authentication_abilities.include?(capability) end def verify_workhorse_api! diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 3d7cc176e07..f9ae5e4543f 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -1,6 +1,14 @@ module Gitlab module Auth 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? actor.present? || type == :ci end @@ -143,6 +151,8 @@ module Gitlab end end + public + def build_authentication_abilities [ :read_project, diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 0311755dd06..f828e898740 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -346,7 +346,7 @@ describe 'Git HTTP requests', lib: true do it 'uploads get status 403' do 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 diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index cad8b914668..09e4e265dd1 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -310,8 +310,8 @@ describe 'Git LFS API and storage' do let(:build) { create(:ci_build, :running, pipeline: pipeline) } it_behaves_like 'can download LFS only from own projects' do - # We render 401, to prevent data leakage about existence of the project - let(:other_project_status) { 401 } + # We render 404, to prevent data leakage about existence of the project + let(:other_project_status) { 404 } end end end @@ -545,8 +545,8 @@ describe 'Git LFS API and storage' do let(:build) { create(:ci_build, :running, pipeline: pipeline) } it_behaves_like 'can download LFS only from own projects' do - # We render 401, to prevent data leakage about existence of the project - let(:other_project_status) { 401 } + # We render 404, to prevent data leakage about existence of the project + let(:other_project_status) { 404 } end end end @@ -706,8 +706,8 @@ describe 'Git LFS API and storage' do context 'tries to push to own project' do let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } - it 'responds with 403' do - expect(response).to have_http_status(403) + it 'responds with 401' do + expect(response).to have_http_status(401) end end @@ -716,8 +716,8 @@ describe 'Git LFS API and storage' do let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } - it 'responds with 403' do - expect(response).to have_http_status(403) + it 'responds with 401' do + expect(response).to have_http_status(401) end end end @@ -925,8 +925,8 @@ describe 'Git LFS API and storage' do put_authorize end - it 'responds with 403' do - expect(response).to have_http_status(403) + it 'responds with 401' do + expect(response).to have_http_status(401) end end @@ -939,8 +939,8 @@ describe 'Git LFS API and storage' do put_authorize end - it 'responds with 404' do - expect(response).to have_http_status(404) + it 'responds with 401' do + expect(response).to have_http_status(401) end end end @@ -1025,8 +1025,8 @@ describe 'Git LFS API and storage' do context 'tries to push to own project' do let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } - it 'responds with 403' do - expect(response).to have_http_status(403) + it 'responds with 401' do + expect(response).to have_http_status(401) end end @@ -1035,8 +1035,8 @@ describe 'Git LFS API and storage' do let(:pipeline) { create(:ci_empty_pipeline, project: other_project) } let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) } - it 'responds with 403' do - expect(response).to have_http_status(403) + it 'responds with 401' do + expect(response).to have_http_status(401) end end end |