diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-03-25 04:16:45 +0000 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-03-25 04:16:45 +0000 |
commit | 8f3f6e9efbbacf5e52f98324944f644630da2f18 (patch) | |
tree | b8bd189a7dc4919a832eb583441d398b7294cb8c | |
parent | d7aecf68e02cb9ac3b7e022aca60e15a65a62ac6 (diff) | |
parent | 4745424bd3b7f6e13e86ebf985977ad3268881e3 (diff) | |
download | gitlab-ce-8f3f6e9efbbacf5e52f98324944f644630da2f18.tar.gz |
Merge branch 'api-internal-errors' into 'master'
Respond with full GitAccess error if user has project read access.
Should help with debugging #1236.
cc @marin
See merge request !437
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 2 | ||||
-rw-r--r-- | app/helpers/branches_helper.rb | 2 | ||||
-rw-r--r-- | app/helpers/tree_helper.rb | 2 | ||||
-rw-r--r-- | app/services/files/create_service.rb | 2 | ||||
-rw-r--r-- | app/services/files/delete_service.rb | 2 | ||||
-rw-r--r-- | app/services/files/update_service.rb | 2 | ||||
-rw-r--r-- | lib/api/internal.rb | 38 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/backend/grack_auth.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/git_access.rb | 128 | ||||
-rw-r--r-- | lib/gitlab/git_access_wiki.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 38 | ||||
-rw-r--r-- | spec/lib/gitlab/git_access_wiki_spec.rb | 4 |
14 files changed, 134 insertions, 94 deletions
diff --git a/CHANGELOG b/CHANGELOG index 737831eb195..7b24fb00c6d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -36,6 +36,7 @@ v 7.10.0 (unreleased) - Don't include system notes in issue/MR comment count. - Don't mark merge request as updated when merge status relative to target branch changes. - Link note avatar to user. + - Make Git-over-SSH errors more descriptive. v 7.9.0 - Send EmailsOnPush email when branch or tag is created or deleted. diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e9b7d7e0083..47ce8467358 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -257,7 +257,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def allowed_to_push_code?(project, branch) - ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, branch) + ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch) end def merge_request_params diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 4a5edf6d101..d6eaa7d57bc 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -12,6 +12,6 @@ module BranchesHelper def can_push_branch?(project, branch_name) return false unless project.repository.branch_names.include?(branch_name) - ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, branch_name) + ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch_name) end end diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index b6fb7a8aa5a..bf6726574ec 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -56,7 +56,7 @@ module TreeHelper ref ||= @ref return false unless project.repository.branch_names.include?(ref) - ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) end def tree_breadcrumbs(tree, max_links = 2) diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index de5322e990a..eeafefc25af 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -3,7 +3,7 @@ require_relative "base_service" module Files class CreateService < BaseService def execute - allowed = Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) unless allowed return error("You are not allowed to create file in this branch") diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index 8e73c2e2727..1497a0f883b 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -3,7 +3,7 @@ require_relative "base_service" module Files class DeleteService < BaseService def execute - allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) unless allowed return error("You are not allowed to push into this branch") diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 328cf3a4b06..0724d3ae634 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -3,7 +3,7 @@ require_relative "base_service" module Files class UpdateService < BaseService def execute - allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) unless allowed return error("You are not allowed to push into this branch") diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 753d0fcbd98..f98a17773e7 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -17,42 +17,40 @@ module API post "/allowed" do status 200 - actor = if params[:key_id] - Key.find_by(id: params[:key_id]) - elsif params[:user_id] - User.find_by(id: params[:user_id]) - end + actor = + if params[:key_id] + Key.find_by(id: params[:key_id]) + elsif params[:user_id] + User.find_by(id: params[:user_id]) + end unless actor return Gitlab::GitAccessStatus.new(false, 'No such user or key') end project_path = params[:project] - + # Check for *.wiki repositories. # Strip out the .wiki from the pathname before finding the # project. This applies the correct project permissions to # the wiki repository as well. - access = - if project_path.end_with?('.wiki') - project_path.chomp!('.wiki') - Gitlab::GitAccessWiki.new - else - Gitlab::GitAccess.new - end + wiki = project_path.end_with?('.wiki') + project_path.chomp!('.wiki') if wiki project = Project.find_with_namespace(project_path) if project - status = access.check( - actor, - params[:action], - project, - params[:changes] - ) + access = + if wiki + Gitlab::GitAccessWiki.new(actor, project) + else + Gitlab::GitAccess.new(actor, project) + end + + status = access.check(params[:action], params[:changes]) end - if project && status && status.allowed? + if project && access.can_read_project? status else Gitlab::GitAccessStatus.new(false, 'No such project') diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 25b7857f4b1..f3765f5ab03 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -178,7 +178,8 @@ module API put ":id/merge_request/:merge_request_id/merge" do merge_request = user_project.merge_requests.find(params[:merge_request_id]) - allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, user_project, merge_request.target_branch) + allowed = ::Gitlab::GitAccess.new(current_user, user_project). + can_push_to_branch?(merge_request.target_branch) if allowed if merge_request.unchecked? diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index ffe4565ef1e..050b5ba29dd 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -129,7 +129,7 @@ module Grack case git_cmd when *Gitlab::GitAccess::DOWNLOAD_COMMANDS if user - Gitlab::GitAccess.new.download_access_check(user, project).allowed? + Gitlab::GitAccess.new(user, project).download_access_check.allowed? elsif project.public? # Allow clone/fetch for public projects true diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index cb69e4b13d3..d6e609e2c44 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -3,9 +3,32 @@ module Gitlab DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } PUSH_COMMANDS = %w{ git-receive-pack } - attr_reader :params, :project, :git_cmd, :user + attr_reader :actor, :project - def self.can_push_to_branch?(user, project, ref) + def initialize(actor, project) + @actor = actor + @project = project + end + + def user + return @user if defined?(@user) + + @user = + case actor + when User + actor + when DeployKey + nil + when Key + actor.user + end + end + + def deploy_key + actor if actor.is_a?(DeployKey) + end + + def can_push_to_branch?(ref) return false unless user if project.protected_branch?(ref) && @@ -16,51 +39,65 @@ module Gitlab end end - def check(actor, cmd, project, changes = nil) + def can_read_project? + if user + user.can?(:read_project, project) + elsif deploy_key + deploy_key.projects.include?(project) + else + false + end + end + + def check(cmd, changes = nil) case cmd when *DOWNLOAD_COMMANDS - download_access_check(actor, project) + download_access_check when *PUSH_COMMANDS - if actor.is_a? User - push_access_check(actor, project, changes) - elsif actor.is_a? DeployKey - return build_status_object(false, "Deploy key not allowed to push") - elsif actor.is_a? Key - push_access_check(actor.user, project, changes) - else - raise 'Wrong actor' - end + push_access_check(changes) else - return build_status_object(false, "Wrong command") + build_status_object(false, "Wrong command") end end - def download_access_check(actor, project) - if actor.is_a?(User) - user_download_access_check(actor, project) - elsif actor.is_a?(DeployKey) - if actor.projects.include?(project) - build_status_object(true) - else - build_status_object(false, "Deploy key not allowed to access this project") - end - elsif actor.is_a? Key - user_download_access_check(actor.user, project) + def download_access_check + if user + user_download_access_check + elsif deploy_key + deploy_key_download_access_check else raise 'Wrong actor' end end - def user_download_access_check(user, project) - if user && user_allowed?(user) && user.can?(:download_code, project) + def push_access_check(changes) + if user + user_push_access_check(changes) + elsif deploy_key + build_status_object(false, "Deploy key not allowed to push") + else + raise 'Wrong actor' + end + end + + def user_download_access_check + if user && user_allowed? && user.can?(:download_code, project) build_status_object(true) else build_status_object(false, "You don't have access") end end - def push_access_check(user, project, changes) - unless user && user_allowed?(user) + def deploy_key_download_access_check + if can_read_project? + build_status_object(true) + else + build_status_object(false, "Deploy key not allowed to access this project") + end + end + + def user_push_access_check(changes) + unless user && user_allowed? return build_status_object(false, "You don't have access") end @@ -76,27 +113,28 @@ module Gitlab # Iterate over all changes to find if user allowed all of them to be applied changes.map(&:strip).reject(&:blank?).each do |change| - status = change_access_check(user, project, change) + status = change_access_check(change) unless status.allowed? # If user does not have access to make at least one change - cancel all push return status end end - return build_status_object(true) + build_status_object(true) end - def change_access_check(user, project, change) + def change_access_check(change) oldrev, newrev, ref = change.split(' ') - action = if project.protected_branch?(branch_name(ref)) - protected_branch_action(project, oldrev, newrev, branch_name(ref)) - elsif protected_tag?(project, tag_name(ref)) - # Prevent any changes to existing git tag unless user has permissions - :admin_project - else - :push_code - end + action = + if project.protected_branch?(branch_name(ref)) + protected_branch_action(oldrev, newrev, branch_name(ref)) + elsif protected_tag?(tag_name(ref)) + # Prevent any changes to existing git tag unless user has permissions + :admin_project + else + :push_code + end if user.can?(action, project) build_status_object(true) @@ -105,15 +143,15 @@ module Gitlab end end - def forced_push?(project, oldrev, newrev) + def forced_push?(oldrev, newrev) Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) end private - def protected_branch_action(project, oldrev, newrev, branch_name) + def protected_branch_action(oldrev, newrev, branch_name) # we dont allow force push to protected branch - if forced_push?(project, oldrev, newrev) + if forced_push?(oldrev, newrev) :force_push_code_to_protected_branches elsif Gitlab::Git.blank_ref?(newrev) # and we dont allow remove of protected branch @@ -125,11 +163,11 @@ module Gitlab end end - def protected_tag?(project, tag_name) + def protected_tag?(tag_name) project.repository.tag_names.include?(tag_name) end - def user_allowed?(user) + def user_allowed? Gitlab::UserAccess.allowed?(user) end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index a2177c8d548..73d99b96202 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -1,6 +1,6 @@ module Gitlab class GitAccessWiki < GitAccess - def change_access_check(user, project, change) + def change_access_check(change) if user.can?(:write_wiki, project) build_status_object(true) else diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 666398eedd4..39be9d64644 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -1,25 +1,26 @@ require 'spec_helper' describe Gitlab::GitAccess do - let(:access) { Gitlab::GitAccess.new } + let(:access) { Gitlab::GitAccess.new(actor, project) } let(:project) { create(:project) } let(:user) { create(:user) } + let(:actor) { user } describe 'can_push_to_branch?' do describe 'push to none protected branch' do it "returns true if user is a master" do project.team << [user, :master] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_truthy + expect(access.can_push_to_branch?("random_branch")).to be_truthy end it "returns true if user is a developer" do project.team << [user, :developer] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_truthy + expect(access.can_push_to_branch?("random_branch")).to be_truthy end it "returns false if user is a reporter" do project.team << [user, :reporter] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_falsey + expect(access.can_push_to_branch?("random_branch")).to be_falsey end end @@ -30,17 +31,17 @@ describe Gitlab::GitAccess do it "returns true if user is a master" do project.team << [user, :master] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy + expect(access.can_push_to_branch?(@branch.name)).to be_truthy end it "returns false if user is a developer" do project.team << [user, :developer] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey + expect(access.can_push_to_branch?(@branch.name)).to be_falsey end it "returns false if user is a reporter" do project.team << [user, :reporter] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey + expect(access.can_push_to_branch?(@branch.name)).to be_falsey end end @@ -51,17 +52,17 @@ describe Gitlab::GitAccess do it "returns true if user is a master" do project.team << [user, :master] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy + expect(access.can_push_to_branch?(@branch.name)).to be_truthy end it "returns true if user is a developer" do project.team << [user, :developer] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy + expect(access.can_push_to_branch?(@branch.name)).to be_truthy end it "returns false if user is a reporter" do project.team << [user, :reporter] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey + expect(access.can_push_to_branch?(@branch.name)).to be_falsey end end @@ -72,7 +73,7 @@ describe Gitlab::GitAccess do before { project.team << [user, :master] } context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_truthy } end @@ -82,7 +83,7 @@ describe Gitlab::GitAccess do before { project.team << [user, :guest] } context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -95,7 +96,7 @@ describe Gitlab::GitAccess do end context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -103,7 +104,7 @@ describe Gitlab::GitAccess do describe 'without acccess to project' do context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -111,17 +112,18 @@ describe Gitlab::GitAccess do describe 'deploy key permissions' do let(:key) { create(:deploy_key) } + let(:actor) { key } context 'pull code' do context 'allowed' do before { key.projects << project } - subject { access.download_access_check(key, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_truthy } end context 'denied' do - subject { access.download_access_check(key, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -205,7 +207,7 @@ describe Gitlab::GitAccess do permissions_matrix[role].each do |action, allowed| context action do - subject { access.push_access_check(user, project, changes[action]) } + subject { access.push_access_check(changes[action]) } it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } end @@ -221,7 +223,7 @@ describe Gitlab::GitAccess do updated_permissions_matrix[role].each do |action, allowed| context action do - subject { access.push_access_check(user, project, changes[action]) } + subject { access.push_access_check(changes[action]) } it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } end diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index c31c6764091..4cb91094cb3 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::GitAccessWiki do - let(:access) { Gitlab::GitAccessWiki.new } + let(:access) { Gitlab::GitAccessWiki.new(user, project) } let(:project) { create(:project) } let(:user) { create(:user) } @@ -11,7 +11,7 @@ describe Gitlab::GitAccessWiki do project.team << [user, :developer] end - subject { access.push_access_check(user, project, changes) } + subject { access.push_access_check(changes) } it { expect(subject.allowed?).to be_truthy } end |