Commit 4830b2be authored by Douwe Maan's avatar Douwe Maan

Refactor GitAccess to use instance variables.

parent 2953e0d1
...@@ -257,7 +257,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -257,7 +257,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
def allowed_to_push_code?(project, branch) 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 end
def merge_request_params def merge_request_params
......
...@@ -12,6 +12,6 @@ module BranchesHelper ...@@ -12,6 +12,6 @@ module BranchesHelper
def can_push_branch?(project, branch_name) def can_push_branch?(project, branch_name)
return false unless project.repository.branch_names.include?(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
end end
...@@ -56,7 +56,7 @@ module TreeHelper ...@@ -56,7 +56,7 @@ module TreeHelper
ref ||= @ref ref ||= @ref
return false unless project.repository.branch_names.include?(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 end
def tree_breadcrumbs(tree, max_links = 2) def tree_breadcrumbs(tree, max_links = 2)
......
...@@ -3,7 +3,7 @@ require_relative "base_service" ...@@ -3,7 +3,7 @@ require_relative "base_service"
module Files module Files
class CreateService < BaseService class CreateService < BaseService
def execute 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 unless allowed
return error("You are not allowed to create file in this branch") return error("You are not allowed to create file in this branch")
......
...@@ -3,7 +3,7 @@ require_relative "base_service" ...@@ -3,7 +3,7 @@ require_relative "base_service"
module Files module Files
class DeleteService < BaseService class DeleteService < BaseService
def execute 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 unless allowed
return error("You are not allowed to push into this branch") return error("You are not allowed to push into this branch")
......
...@@ -3,7 +3,7 @@ require_relative "base_service" ...@@ -3,7 +3,7 @@ require_relative "base_service"
module Files module Files
class UpdateService < BaseService class UpdateService < BaseService
def execute 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 unless allowed
return error("You are not allowed to push into this branch") return error("You are not allowed to push into this branch")
......
...@@ -17,7 +17,8 @@ module API ...@@ -17,7 +17,8 @@ module API
post "/allowed" do post "/allowed" do
status 200 status 200
actor = if params[:key_id] actor =
if params[:key_id]
Key.find_by(id: params[:key_id]) Key.find_by(id: params[:key_id])
elsif params[:user_id] elsif params[:user_id]
User.find_by(id: params[:user_id]) User.find_by(id: params[:user_id])
...@@ -33,23 +34,20 @@ module API ...@@ -33,23 +34,20 @@ module API
# Strip out the .wiki from the pathname before finding the # Strip out the .wiki from the pathname before finding the
# project. This applies the correct project permissions to # project. This applies the correct project permissions to
# the wiki repository as well. # the wiki repository as well.
access = wiki = project_path.end_with?('.wiki')
if project_path.end_with?('.wiki') project_path.chomp!('.wiki') if wiki
project_path.chomp!('.wiki')
Gitlab::GitAccessWiki.new
else
Gitlab::GitAccess.new
end
project = Project.find_with_namespace(project_path) project = Project.find_with_namespace(project_path)
if project if project
status = access.check( access =
actor, if wiki
params[:action], Gitlab::GitAccessWiki.new(actor, project)
project, else
params[:changes] Gitlab::GitAccess.new(actor, project)
) end
status = access.check(params[:action], params[:changes])
end end
if project && status && status.allowed? if project && status && status.allowed?
......
...@@ -178,7 +178,8 @@ module API ...@@ -178,7 +178,8 @@ module API
put ":id/merge_request/:merge_request_id/merge" do put ":id/merge_request/:merge_request_id/merge" do
merge_request = user_project.merge_requests.find(params[:merge_request_id]) 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 allowed
if merge_request.unchecked? if merge_request.unchecked?
......
...@@ -112,7 +112,7 @@ module Grack ...@@ -112,7 +112,7 @@ module Grack
case git_cmd case git_cmd
when *Gitlab::GitAccess::DOWNLOAD_COMMANDS when *Gitlab::GitAccess::DOWNLOAD_COMMANDS
if user if user
Gitlab::GitAccess.new.download_access_check(user, project).allowed? Gitlab::GitAccess.new(user, project).download_access_check.allowed?
elsif project.public? elsif project.public?
# Allow clone/fetch for public projects # Allow clone/fetch for public projects
true true
......
...@@ -3,9 +3,32 @@ module Gitlab ...@@ -3,9 +3,32 @@ module Gitlab
DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
PUSH_COMMANDS = %w{ git-receive-pack } 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 return false unless user
if project.protected_branch?(ref) && if project.protected_branch?(ref) &&
...@@ -16,51 +39,65 @@ module Gitlab ...@@ -16,51 +39,65 @@ module Gitlab
end end
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 case cmd
when *DOWNLOAD_COMMANDS when *DOWNLOAD_COMMANDS
download_access_check(actor, project) download_access_check
when *PUSH_COMMANDS when *PUSH_COMMANDS
if actor.is_a? User push_access_check(changes)
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 else
raise 'Wrong actor' build_status_object(false, "Wrong command")
end
else
return build_status_object(false, "Wrong command")
end end
end end
def download_access_check(actor, project) def download_access_check
if actor.is_a?(User) if user
user_download_access_check(actor, project) user_download_access_check
elsif actor.is_a?(DeployKey) elsif deploy_key
if actor.projects.include?(project) deploy_key_download_access_check
build_status_object(true)
else else
build_status_object(false, "Deploy key not allowed to access this project") raise 'Wrong actor'
end end
elsif actor.is_a? Key end
user_download_access_check(actor.user, 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 else
raise 'Wrong actor' raise 'Wrong actor'
end end
end end
def user_download_access_check(user, project) def user_download_access_check
if user && user_allowed?(user) && user.can?(:download_code, project) if user && user_allowed? && user.can?(:download_code, project)
build_status_object(true) build_status_object(true)
else else
build_status_object(false, "You don't have access") build_status_object(false, "You don't have access")
end end
end end
def push_access_check(user, project, changes) def deploy_key_download_access_check
unless user && user_allowed?(user) 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") return build_status_object(false, "You don't have access")
end end
...@@ -76,22 +113,23 @@ module Gitlab ...@@ -76,22 +113,23 @@ module Gitlab
# Iterate over all changes to find if user allowed all of them to be applied # Iterate over all changes to find if user allowed all of them to be applied
changes.map(&:strip).reject(&:blank?).each do |change| changes.map(&:strip).reject(&:blank?).each do |change|
status = change_access_check(user, project, change) status = change_access_check(change)
unless status.allowed? unless status.allowed?
# If user does not have access to make at least one change - cancel all push # If user does not have access to make at least one change - cancel all push
return status return status
end end
end end
return build_status_object(true) build_status_object(true)
end end
def change_access_check(user, project, change) def change_access_check(change)
oldrev, newrev, ref = change.split(' ') oldrev, newrev, ref = change.split(' ')
action = if project.protected_branch?(branch_name(ref)) action =
protected_branch_action(project, oldrev, newrev, branch_name(ref)) if project.protected_branch?(branch_name(ref))
elsif protected_tag?(project, tag_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 # Prevent any changes to existing git tag unless user has permissions
:admin_project :admin_project
else else
...@@ -105,15 +143,15 @@ module Gitlab ...@@ -105,15 +143,15 @@ module Gitlab
end end
end end
def forced_push?(project, oldrev, newrev) def forced_push?(oldrev, newrev)
Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
end end
private 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 # 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 :force_push_code_to_protected_branches
elsif Gitlab::Git.blank_ref?(newrev) elsif Gitlab::Git.blank_ref?(newrev)
# and we dont allow remove of protected branch # and we dont allow remove of protected branch
...@@ -125,11 +163,11 @@ module Gitlab ...@@ -125,11 +163,11 @@ module Gitlab
end end
end end
def protected_tag?(project, tag_name) def protected_tag?(tag_name)
project.repository.tag_names.include?(tag_name) project.repository.tag_names.include?(tag_name)
end end
def user_allowed?(user) def user_allowed?
Gitlab::UserAccess.allowed?(user) Gitlab::UserAccess.allowed?(user)
end end
......
module Gitlab module Gitlab
class GitAccessWiki < GitAccess class GitAccessWiki < GitAccess
def change_access_check(user, project, change) def change_access_check(change)
if user.can?(:write_wiki, project) if user.can?(:write_wiki, project)
build_status_object(true) build_status_object(true)
else else
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::GitAccess do describe Gitlab::GitAccess do
let(:access) { Gitlab::GitAccess.new } let(:access) { Gitlab::GitAccess.new(actor, project) }
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:actor) { user }
describe 'can_push_to_branch?' do describe 'can_push_to_branch?' do
describe 'push to none protected branch' do describe 'push to none protected branch' do
it "returns true if user is a master" do it "returns true if user is a master" do
project.team << [user, :master] 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 end
it "returns true if user is a developer" do it "returns true if user is a developer" do
project.team << [user, :developer] 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 end
it "returns false if user is a reporter" do it "returns false if user is a reporter" do
project.team << [user, :reporter] 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
end end
...@@ -30,17 +31,17 @@ describe Gitlab::GitAccess do ...@@ -30,17 +31,17 @@ describe Gitlab::GitAccess do
it "returns true if user is a master" do it "returns true if user is a master" do
project.team << [user, :master] 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 end
it "returns false if user is a developer" do it "returns false if user is a developer" do
project.team << [user, :developer] 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 end
it "returns false if user is a reporter" do it "returns false if user is a reporter" do
project.team << [user, :reporter] 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
end end
...@@ -51,17 +52,17 @@ describe Gitlab::GitAccess do ...@@ -51,17 +52,17 @@ describe Gitlab::GitAccess do
it "returns true if user is a master" do it "returns true if user is a master" do
project.team << [user, :master] 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 end
it "returns true if user is a developer" do it "returns true if user is a developer" do
project.team << [user, :developer] 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 end
it "returns false if user is a reporter" do it "returns false if user is a reporter" do
project.team << [user, :reporter] 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
end end
...@@ -72,7 +73,7 @@ describe Gitlab::GitAccess do ...@@ -72,7 +73,7 @@ describe Gitlab::GitAccess do
before { project.team << [user, :master] } before { project.team << [user, :master] }
context 'pull code' do context 'pull code' do
subject { access.download_access_check(user, project) } subject { access.download_access_check }
it { expect(subject.allowed?).to be_truthy } it { expect(subject.allowed?).to be_truthy }
end end
...@@ -82,7 +83,7 @@ describe Gitlab::GitAccess do ...@@ -82,7 +83,7 @@ describe Gitlab::GitAccess do
before { project.team << [user, :guest] } before { project.team << [user, :guest] }
context 'pull code' do context 'pull code' do
subject { access.download_access_check(user, project) } subject { access.download_access_check }
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
end end
...@@ -95,7 +96,7 @@ describe Gitlab::GitAccess do ...@@ -95,7 +96,7 @@ describe Gitlab::GitAccess do
end end
context 'pull code' do context 'pull code' do
subject { access.download_access_check(user, project) } subject { access.download_access_check }
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
end end
...@@ -103,7 +104,7 @@ describe Gitlab::GitAccess do ...@@ -103,7 +104,7 @@ describe Gitlab::GitAccess do
describe 'without acccess to project' do describe 'without acccess to project' do
context 'pull code' do context 'pull code' do
subject { access.download_access_check(user, project) } subject { access.download_access_check }
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
end end
...@@ -111,17 +112,18 @@ describe Gitlab::GitAccess do ...@@ -111,17 +112,18 @@ describe Gitlab::GitAccess do
describe 'deploy key permissions' do describe 'deploy key permissions' do
let(:key) { create(:deploy_key) } let(:key) { create(:deploy_key) }
let(:actor) { key }
context 'pull code' do context 'pull code' do
context 'allowed' do context 'allowed' do
before { key.projects << project } before { key.projects << project }
subject { access.download_access_check(key, project) } subject { access.download_access_check }
it { expect(subject.allowed?).to be_truthy } it { expect(subject.allowed?).to be_truthy }
end end
context 'denied' do context 'denied' do
subject { access.download_access_check(key, project) } subject { access.download_access_check }
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
end end
...@@ -205,7 +207,7 @@ describe Gitlab::GitAccess do ...@@ -205,7 +207,7 @@ describe Gitlab::GitAccess do
permissions_matrix[role].each do |action, allowed| permissions_matrix[role].each do |action, allowed|
context action do 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 } it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
end end
...@@ -221,7 +223,7 @@ describe Gitlab::GitAccess do ...@@ -221,7 +223,7 @@ describe Gitlab::GitAccess do
updated_permissions_matrix[role].each do |action, allowed| updated_permissions_matrix[role].each do |action, allowed|
context action do 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 } it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
end end
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::GitAccessWiki do describe Gitlab::GitAccessWiki do
let(:access) { Gitlab::GitAccessWiki.new } let(:access) { Gitlab::GitAccessWiki.new(user, project) }
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -11,7 +11,7 @@ describe Gitlab::GitAccessWiki do ...@@ -11,7 +11,7 @@ describe Gitlab::GitAccessWiki do
project.team << [user, :developer] project.team << [user, :developer]
end end
subject { access.push_access_check(user, project, changes) } subject { access.push_access_check(changes) }
it { expect(subject.allowed?).to be_truthy } it { expect(subject.allowed?).to be_truthy }
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