From 3058059c0ae482d04eed433e595fde9f34ef3376 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 14 Nov 2014 18:23:55 +0200 Subject: [PATCH] Better message for failed pushes because of git hooks --- GITLAB_SHELL_VERSION | 2 +- lib/api/internal.rb | 2 +- lib/gitlab/backend/grack_auth.rb | 2 +- lib/gitlab/git_access.rb | 83 +++++++++++++++---------- lib/gitlab/git_access_status.rb | 15 +++++ lib/gitlab/git_access_wiki.rb | 8 ++- spec/lib/gitlab/git_access_spec.rb | 34 +++++----- spec/lib/gitlab/git_access_wiki_spec.rb | 4 +- spec/requests/api/internal_spec.rb | 16 ++--- 9 files changed, 101 insertions(+), 65 deletions(-) create mode 100644 lib/gitlab/git_access_status.rb diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index ccbccc3dc6..276cbf9e28 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -2.2.0 +2.3.0 diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ebf2296097..1648834f03 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -43,7 +43,7 @@ module API return false unless actor - access.allowed?( + access.check( actor, params[:action], project, diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index df1461a45c..762639414e 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -80,7 +80,7 @@ module Grack case git_cmd when *Gitlab::GitAccess::DOWNLOAD_COMMANDS if user - Gitlab::GitAccess.new.download_allowed?(user, project) + Gitlab::GitAccess.new.download_access_check(user, project).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 591c9d83c1..6b0d3d31d4 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -5,61 +5,60 @@ module Gitlab attr_reader :params, :project, :git_cmd, :user - def allowed?(actor, cmd, project, changes = nil) + def check(actor, cmd, project, changes = nil) case cmd when *DOWNLOAD_COMMANDS if actor.is_a? User - download_allowed?(actor, project) + download_access_check(actor, project) elsif actor.is_a? DeployKey actor.projects.include?(project) elsif actor.is_a? Key - download_allowed?(actor.user, project) + download_access_check(actor.user, project) else raise 'Wrong actor' end when *PUSH_COMMANDS if actor.is_a? User - push_allowed?(actor, project, changes) + push_access_check(actor, project, changes) elsif actor.is_a? DeployKey - # Deploy key not allowed to push - return false + return build_status_object(false, "Deploy key not allowed to push") elsif actor.is_a? Key - push_allowed?(actor.user, project, changes) + push_access_check(actor.user, project, changes) else raise 'Wrong actor' end else - false + return build_status_object(false, "Wrong command") end end - def download_allowed?(user, project) - if user && user_allowed?(user) - user.can?(:download_code, project) + def download_access_check(user, project) + if user && user_allowed?(user) && user.can?(:download_code, project) + build_status_object(true) else - false + build_status_object(false, "You don't have access") end end - def push_allowed?(user, project, changes) - return false unless user && user_allowed?(user) - return true if changes.blank? + def push_access_check(user, project, changes) + return build_status_object(false, "You don't have access") unless user && user_allowed?(user) + return build_status_object(true) if changes.blank? changes = changes.lines if changes.kind_of?(String) # Iterate over all changes to find if user allowed all of them to be applied changes.each do |change| - unless change_allowed?(user, project, change) + status = change_access_check(user, project, change) + unless status.allowed? # If user does not have access to make at least one change - cancel all push - return false + return status end end - # If user has access to make all changes - true + return build_status_object(true) end - def change_allowed?(user, project, change) + def change_access_check(user, project, change) oldrev, newrev, ref = change.split(' ') action = if project.protected_branch?(branch_name(ref)) @@ -79,8 +78,10 @@ module Gitlab :push_code end - user.can?(action, project) && - pass_git_hooks?(user, project, ref, oldrev, newrev) + unless user.can?(action, project) + return build_status_object(false, "You don't have permission") + end + pass_git_hooks?(user, project, ref, oldrev, newrev) end def forced_push?(project, oldrev, newrev) @@ -95,16 +96,16 @@ module Gitlab end def pass_git_hooks?(user, project, ref, oldrev, newrev) - return true unless project.git_hook + return build_status_object(true) unless project.git_hook - return true unless newrev && oldrev + return build_status_object(true) unless newrev && oldrev git_hook = project.git_hook # Prevent tag removal if git_hook.deny_delete_tag if project.repository.tag_names.include?(ref) && newrev =~ /0000000/ - return false + return build_status_object(false, "You can not delete tag") end end @@ -113,33 +114,43 @@ module Gitlab commits = project.repository.commits_between(oldrev, newrev) commits.each do |commit| if git_hook.commit_message_regex.present? - return false unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex) + unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex) + return build_status_object(false, "Commit message does not follow the pattern") + end end if git_hook.author_email_regex.present? - return false unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex) - return false unless commit.author_email =~ Regexp.new(git_hook.author_email_regex) + unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex) + return build_status_object(false, "Commiter's email does not follow the pattern") + end + unless commit.author_email =~ Regexp.new(git_hook.author_email_regex) + return build_status_object(false, "Author's email does not follow the pattern") + end end # Check whether author is a GitLab member if git_hook.member_check - return false unless User.existing_member?(commit.author_email) + unless User.existing_member?(commit.author_email) + return build_status_object(false, "Author is not a member of team") + end if commit.author_email != commit.committer_email - return false unless User.existing_member?(commit.committer_email) + unless User.existing_member?(commit.committer_email) + return build_status_object(false, "Commiter is not a member of team") + end end end if git_hook.file_name_regex.present? commit.diffs.each do |diff| - if diff.renamed_file || diff.new_file - return false if diff.new_path =~ Regexp.new(git_hook.file_name_regex) + if (diff.renamed_file || diff.new_file) && diff.new_path =~ Regexp.new(git_hook.file_name_regex) + return build_status_object(false, "File name #{diff.new_path} does not follow the pattern") end end end end end - true + build_status_object(true) end private @@ -165,6 +176,12 @@ module Gitlab nil end end + + protected + + def build_status_object(status, message = '') + GitAccessStatus.new(status, message) + end end end diff --git a/lib/gitlab/git_access_status.rb b/lib/gitlab/git_access_status.rb new file mode 100644 index 0000000000..3d451ecebe --- /dev/null +++ b/lib/gitlab/git_access_status.rb @@ -0,0 +1,15 @@ +module Gitlab + class GitAccessStatus + attr_accessor :status, :message + alias_method :allowed?, :status + + def initialize(status, message = '') + @status = status + @message = message + end + + def to_json + {status: @status, message: @message}.to_json + end + end +end \ No newline at end of file diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 9f0eb3be20..f7d1428deb 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -1,7 +1,11 @@ module Gitlab class GitAccessWiki < GitAccess - def change_allowed?(user, project, change) - user.can?(:write_wiki, project) + def change_allowed_check(user, project, change) + if user.can?(:write_wiki, project) + build_status_object(true) + else + build_status_object(false, "You don't have access") + end end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index da97208e86..4a9ee2ab0d 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -5,14 +5,14 @@ describe Gitlab::GitAccess do let(:project) { create(:project) } let(:user) { create(:user) } - describe 'download_allowed?' do + describe 'download_access_check' do describe 'master permissions' do before { project.team << [user, :master] } context 'pull code' do - subject { access.download_allowed?(user, project) } + subject { access.download_access_check(user, project) } - it { should be_true } + it { subject.allowed?.should be_true } end end @@ -20,9 +20,9 @@ describe Gitlab::GitAccess do before { project.team << [user, :guest] } context 'pull code' do - subject { access.download_allowed?(user, project) } + subject { access.download_access_check(user, project) } - it { should be_false } + it { subject.allowed?.should be_false } end end @@ -33,22 +33,22 @@ describe Gitlab::GitAccess do end context 'pull code' do - subject { access.download_allowed?(user, project) } + subject { access.download_access_check(user, project) } - it { should be_false } + it { subject.allowed?.should be_false } end end describe 'without acccess to project' do context 'pull code' do - subject { access.download_allowed?(user, project) } + subject { access.download_access_check(user, project) } - it { should be_false } + it { subject.allowed?.should be_false } end end end - describe 'push_allowed?' do + describe 'push_access_check' do def protect_feature_branch create(:protected_branch, name: 'feature', project: project) end @@ -117,9 +117,9 @@ describe Gitlab::GitAccess do permissions_matrix[role].each do |action, allowed| context action do - subject { access.push_allowed?(user, project, changes[action]) } + subject { access.push_access_check(user, project, changes[action]) } - it { should allowed ? be_true : be_false } + it { subject.allowed?.should allowed ? be_true : be_false } end end end @@ -135,7 +135,7 @@ describe Gitlab::GitAccess do it 'returns false' do project.create_git_hook project.git_hook.update(commit_message_regex: "@only.com") - access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').should be_false + access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').allowed?.should be_false end end @@ -146,12 +146,12 @@ describe Gitlab::GitAccess do end it 'returns false for non-member user' do - access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').should be_false + access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').allowed?.should be_false end it 'returns true if committer is a gitlab member' do create(:user, email: 'dmitriy.zaporozhets@gmail.com') - access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').should be_true + access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').allowed?.should be_true end end @@ -159,13 +159,13 @@ describe Gitlab::GitAccess do it 'returns false when filename is prohibited' do project.create_git_hook project.git_hook.update(file_name_regex: "jpg$") - access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').should be_false + access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').allowed?.should be_false end it 'returns true if file name is allowed' do project.create_git_hook project.git_hook.update(file_name_regex: "exe$") - access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').should be_true + access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').allowed?.should be_true end end end diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index ed5785b31e..d8d19fd50f 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -11,9 +11,9 @@ describe Gitlab::GitAccessWiki do project.team << [user, :developer] end - subject { access.push_allowed?(user, project, changes) } + subject { access.push_access_check(user, project, changes) } - it { should be_true } + it { subject.should be_true } end def changes diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 677b149404..53b7808d4c 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -37,7 +37,7 @@ describe API::API, api: true do pull(key, project) response.status.should == 200 - response.body.should == 'true' + JSON.parse(response.body)["status"].should be_true end end @@ -46,7 +46,7 @@ describe API::API, api: true do push(key, project) response.status.should == 200 - response.body.should == 'true' + JSON.parse(response.body)["status"].should be_true end end end @@ -61,7 +61,7 @@ describe API::API, api: true do pull(key, project) response.status.should == 200 - response.body.should == 'false' + JSON.parse(response.body)["status"].should be_false end end @@ -70,7 +70,7 @@ describe API::API, api: true do push(key, project) response.status.should == 200 - response.body.should == 'false' + JSON.parse(response.body)["status"].should be_false end end end @@ -87,7 +87,7 @@ describe API::API, api: true do pull(key, personal_project) response.status.should == 200 - response.body.should == 'false' + JSON.parse(response.body)["status"].should be_false end end @@ -96,7 +96,7 @@ describe API::API, api: true do push(key, personal_project) response.status.should == 200 - response.body.should == 'false' + JSON.parse(response.body)["status"].should be_false end end end @@ -114,7 +114,7 @@ describe API::API, api: true do pull(key, project) response.status.should == 200 - response.body.should == 'true' + JSON.parse(response.body)["status"].should be_true end end @@ -123,7 +123,7 @@ describe API::API, api: true do push(key, project) response.status.should == 200 - response.body.should == 'false' + JSON.parse(response.body)["status"].should be_false end end end