Commit 8f3f6e9e authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

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
parents d7aecf68 4745424b
......@@ -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.
......
......@@ -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
......
......@@ -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
......@@ -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)
......
......@@ -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")
......
......@@ -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")
......
......@@ -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")
......
......@@ -17,7 +17,8 @@ module API
post "/allowed" do
status 200
actor = if params[:key_id]
actor =
if params[:key_id]
Key.find_by(id: params[:key_id])
elsif params[:user_id]
User.find_by(id: params[:user_id])
......@@ -33,26 +34,23 @@ module API
# 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')
......
......@@ -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?
......
......@@ -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
......
......@@ -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)
push_access_check(changes)
else
raise 'Wrong actor'
end
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)
def download_access_check
if user
user_download_access_check
elsif deploy_key
deploy_key_download_access_check
else
build_status_object(false, "Deploy key not allowed to access this project")
raise 'Wrong actor'
end
elsif actor.is_a? Key
user_download_access_check(actor.user, project)
end
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(user, project)
if user && user_allowed?(user) && user.can?(:download_code, project)
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,22 +113,23 @@ 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))
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
......@@ -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
......
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
......
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
......
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
......
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