From 2b73aaa15ad9f651f51f8c71de461da6664a4fbb Mon Sep 17 00:00:00 2001 From: Ali Ibrahim Date: Sun, 14 Aug 2016 13:49:08 -0400 Subject: Allow to add deploy keys with write-access --- lib/gitlab/git_access.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 1882eb8d050..f208df3cdce 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -50,10 +50,13 @@ module Gitlab end def push_access_check(changes) + unless project.repository.exists? + return build_status_object(false, "A repository for this project does not exist yet.") + end if user user_push_access_check(changes) elsif deploy_key - build_status_object(false, "Deploy keys are not allowed to push code.") + deploy_key_push_access_check(changes) else raise 'Wrong actor' end @@ -72,10 +75,6 @@ module Gitlab return build_status_object(true) end - unless project.repository.exists? - return build_status_object(false, "A repository for this project does not exist yet.") - end - changes_list = Gitlab::ChangesList.new(changes) # Iterate over all changes to find if user allowed all of them to be applied @@ -90,6 +89,14 @@ module Gitlab build_status_object(true) end + def deploy_key_push_access_check(changes) + if actor.can_push? + build_status_object(true) + else + build_status_object(false, "The deploy key does not have write access to the project.") + end + end + def change_access_check(change) Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec end -- cgit v1.2.1 From fcc2c43ebb32c0e9a3ae636e66460b42cbae4d53 Mon Sep 17 00:00:00 2001 From: Ali Ibrahim Date: Thu, 18 Aug 2016 03:27:45 -0400 Subject: Added can_push attribute to deploy keys and update docs for API --- lib/api/entities.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 055716ab1e3..5df65a2327d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -234,7 +234,7 @@ module API end class SSHKey < Grape::Entity - expose :id, :title, :key, :created_at + expose :id, :title, :key, :created_at, :can_push end class SSHKeyWithUser < SSHKey -- cgit v1.2.1 From 2811a213a6199978ca1f3eea002a7a77f8b87554 Mon Sep 17 00:00:00 2001 From: Ali Ibrahim Date: Fri, 19 Aug 2016 14:40:45 -0400 Subject: added spacing --- lib/gitlab/git_access.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index f208df3cdce..f7432df5557 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -53,6 +53,7 @@ module Gitlab unless project.repository.exists? return build_status_object(false, "A repository for this project does not exist yet.") end + if user user_push_access_check(changes) elsif deploy_key -- cgit v1.2.1 From 05cc87052a7755da3d352409ef3ab024921593c4 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 11 Nov 2016 20:40:28 +0800 Subject: Improve write access check for deploy key --- lib/gitlab/git_access.rb | 81 +++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 45 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 64b5c4b98dc..819e0657bdd 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -7,7 +7,7 @@ module Gitlab ERROR_MESSAGES = { upload: 'You are not allowed to upload code for this project.', download: 'You are not allowed to download code from this project.', - deploy_key: 'Deploy keys are not allowed to push code.', + deploy_key: 'This deploy key does not have write access to this project.', no_repo: 'A repository for this project does not exist yet.' } @@ -46,22 +46,18 @@ module Gitlab def download_access_check if user user_download_access_check - elsif deploy_key.nil? && !Guest.can?(:download_code, project) + elsif !Guest.can?(:download_code, project) raise UnauthorizedError, ERROR_MESSAGES[:download] end end def push_access_check(changes) - unless project.repository.exists? - return build_status_object(false, "A repository for this project does not exist yet.") - end - - if user - user_push_access_check(changes) - elsif deploy_key + if deploy_key deploy_key_push_access_check(changes) + elsif user + user_push_access_check(changes) else - raise UnauthorizedError, ERROR_MESSAGES[deploy_key ? :deploy_key : :upload] + raise UnauthorizedError, ERROR_MESSAGES[:upload] end end @@ -88,34 +84,19 @@ module Gitlab return # Allow access. end - unless project.repository.exists? - raise UnauthorizedError, ERROR_MESSAGES[:no_repo] - end - - changes_list = Gitlab::ChangesList.new(changes) - - # Iterate over all changes to find if user allowed all of them to be applied - changes_list.each do |change| - status = change_access_check(change) - unless status.allowed? - # If user does not have access to make at least one change - cancel all push - raise UnauthorizedError, status.message - end - end + check_repository_existence! + check_change_access!(changes) end def deploy_key_push_access_check(changes) - if actor.can_push? - build_status_object(true) + if deploy_key.can_push? + check_repository_existence! + check_change_access!(changes) else - build_status_object(false, "The deploy key does not have write access to the project.") + raise UnauthorizedError, ERROR_MESSAGES[:deploy_key] end end - def change_access_check(change) - Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec - end - def protocol_allowed? Gitlab::ProtocolAccess.allowed?(protocol) end @@ -146,6 +127,27 @@ module Gitlab end end + def check_repository_existence! + unless project.repository.exists? + raise UnauthorizedError, ERROR_MESSAGES[:no_repo] + end + end + + def check_change_access!(changes) + changes_list = Gitlab::ChangesList.new(changes) + + # Iterate over all changes to find if user allowed all of them to be applied + changes_list.each do |change| + status = Checks::ChangeAccess.new(change, + user_access: user_access, + project: project).exec + unless status.allowed? + # If user does not have access to make at least one change - cancel all push + raise UnauthorizedError, status.message + end + end + end + def matching_merge_request?(newrev, branch_name) Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end @@ -154,20 +156,11 @@ module Gitlab actor if actor.is_a?(DeployKey) end - def deploy_key_can_read_project? - if deploy_key - return true if project.public? - deploy_key.projects.include?(project) - else - false - end - end - def can_read_project? - if user + if deploy_key + project.public? || deploy_key.projects.include?(project) + elsif user user_access.can_read_project? - elsif deploy_key - deploy_key_can_read_project? else Guest.can?(:read_project, project) end @@ -182,8 +175,6 @@ module Gitlab case actor when User actor - when DeployKey - nil when Key actor.user end -- cgit v1.2.1 From 38fbcb99dba61cfae1b788e0ec947911c4d14dd8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 11 Nov 2016 21:24:21 +0800 Subject: So deploy key might not have a corresponding user --- lib/gitlab/git_access.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 819e0657bdd..78f562821ea 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -46,7 +46,7 @@ module Gitlab def download_access_check if user user_download_access_check - elsif !Guest.can?(:download_code, project) + elsif deploy_key.nil? && !Guest.can?(:download_code, project) raise UnauthorizedError, ERROR_MESSAGES[:download] end end @@ -91,7 +91,7 @@ module Gitlab def deploy_key_push_access_check(changes) if deploy_key.can_push? check_repository_existence! - check_change_access!(changes) + check_change_access!(changes) if user else raise UnauthorizedError, ERROR_MESSAGES[:deploy_key] end -- cgit v1.2.1 From 71ae01fefe62caf396640affb8ca618fe68db5a0 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 11 Nov 2016 21:44:33 +0800 Subject: Add more tests and fix write to project check --- lib/gitlab/git_access.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 78f562821ea..96979398c83 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -89,7 +89,7 @@ module Gitlab end def deploy_key_push_access_check(changes) - if deploy_key.can_push? + if deploy_key.can_push_to?(project) check_repository_existence! check_change_access!(changes) if user else -- cgit v1.2.1 From 5da9bfa453268474b3bff13c63e55b29bbcdf016 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 11 Nov 2016 22:26:05 +0800 Subject: Fix test for GitAccessWiki, it's overriding change_access_check --- lib/gitlab/git_access.rb | 9 ++++++--- lib/gitlab/git_access_wiki.rb | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 96979398c83..b462cffeaf6 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -138,9 +138,7 @@ module Gitlab # Iterate over all changes to find if user allowed all of them to be applied changes_list.each do |change| - status = Checks::ChangeAccess.new(change, - user_access: user_access, - project: project).exec + status = check_single_change_access(change) unless status.allowed? # If user does not have access to make at least one change - cancel all push raise UnauthorizedError, status.message @@ -148,6 +146,11 @@ module Gitlab end end + def check_single_change_access(change) + Checks::ChangeAccess.new( + change, user_access: user_access, project: project).exec + end + def matching_merge_request?(newrev, branch_name) Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index f71d3575909..f7976a64ef5 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(change) + def check_single_change_access(change) if user_access.can_do_action?(:create_wiki) build_status_object(true) else -- cgit v1.2.1 From 721478123068c6718ec73c72a7b7d32c00c816df Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 16 Nov 2016 19:48:54 +0800 Subject: Also need to check against push rules: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_18440615 --- lib/gitlab/git_access.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index b462cffeaf6..19bdfc878b1 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -91,7 +91,7 @@ module Gitlab def deploy_key_push_access_check(changes) if deploy_key.can_push_to?(project) check_repository_existence! - check_change_access!(changes) if user + check_change_access!(changes) else raise UnauthorizedError, ERROR_MESSAGES[:deploy_key] end -- cgit v1.2.1 From a9765fb47fbbd1e1070434fc06cc76b25a42caa6 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 16 Nov 2016 20:31:08 +0800 Subject: Introduce has_access_to? so that we could reuse it Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_18439108 --- lib/gitlab/auth/result.rb | 3 +-- lib/gitlab/git_access.rb | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/auth/result.rb b/lib/gitlab/auth/result.rb index 6be7f690676..39b86c61a18 100644 --- a/lib/gitlab/auth/result.rb +++ b/lib/gitlab/auth/result.rb @@ -9,8 +9,7 @@ module Gitlab def lfs_deploy_token?(for_project) type == :lfs_deploy_token && - actor && - actor.projects.include?(for_project) + actor.try(:has_access_to?, for_project) end def success? diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 19bdfc878b1..a7ad944e79e 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -161,7 +161,7 @@ module Gitlab def can_read_project? if deploy_key - project.public? || deploy_key.projects.include?(project) + project.public? || deploy_key.has_access_to?(project) elsif user user_access.can_read_project? else -- cgit v1.2.1 From 48090a9188e13e3ddaffb5957a7b5a264024f060 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 16 Nov 2016 22:07:04 +0800 Subject: Introduce no_user_or_blocked? and fix tests due to checking user permission. --- lib/gitlab/user_access.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 9858d2e7d83..6c7e673fb9f 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -8,6 +8,8 @@ module Gitlab end def can_do_action?(action) + return false if no_user_or_blocked? + @permission_cache ||= {} @permission_cache[action] ||= user.can?(action, project) end @@ -17,7 +19,7 @@ module Gitlab end def allowed? - return false if user.blank? || user.blocked? + return false if no_user_or_blocked? if user.requires_ldap_check? && user.try_obtain_ldap_lease return false unless Gitlab::LDAP::Access.allowed?(user) @@ -27,7 +29,7 @@ module Gitlab end def can_push_to_branch?(ref) - return false unless user + return false if no_user_or_blocked? if project.protected_branch?(ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) @@ -40,7 +42,7 @@ module Gitlab end def can_merge_to_branch?(ref) - return false unless user + return false if no_user_or_blocked? if project.protected_branch?(ref) access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten @@ -51,9 +53,15 @@ module Gitlab end def can_read_project? - return false unless user + return false if no_user_or_blocked? user.can?(:read_project, project) end + + private + + def no_user_or_blocked? + user.nil? || user.blocked? + end end end -- cgit v1.2.1 From 8c1a01e05fd3c6e1621242aaf31a0ce2789ad546 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 18 Nov 2016 03:48:23 +0800 Subject: We never check user privilege if it's a deploy key --- lib/gitlab/checks/change_access.rb | 11 +++++++++-- lib/gitlab/git_access.rb | 29 +++++++++++++++++++---------- 2 files changed, 28 insertions(+), 12 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index cb1065223d4..6b6a86ffde9 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -1,13 +1,15 @@ module Gitlab module Checks class ChangeAccess - attr_reader :user_access, :project + attr_reader :user_access, :project, :skip_authorization - def initialize(change, user_access:, project:) + def initialize( + change, user_access:, project:, skip_authorization: false) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @branch_name = Gitlab::Git.branch_name(@ref) @user_access = user_access @project = project + @skip_authorization = skip_authorization end def exec @@ -23,6 +25,7 @@ module Gitlab protected def protected_branch_checks + return if skip_authorization return unless @branch_name return unless project.protected_branch?(@branch_name) @@ -48,6 +51,8 @@ module Gitlab end def tag_checks + return if skip_authorization + tag_ref = Gitlab::Git.tag_name(@ref) if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) @@ -56,6 +61,8 @@ module Gitlab end def push_checks + return if skip_authorization + if user_access.cannot_do_action?(:push_code) "You are not allowed to push code to this project." end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index a7ad944e79e..6be0ab08a1f 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -27,7 +27,7 @@ module Gitlab def check(cmd, changes) check_protocol! - check_active_user! + check_active_user! unless deploy_key? check_project_accessibility! check_command_existence!(cmd) @@ -44,9 +44,13 @@ module Gitlab end def download_access_check - if user + if deploy_key + true + elsif user user_download_access_check - elsif deploy_key.nil? && !Guest.can?(:download_code, project) + elsif Guest.can?(:download_code, project) + true + else raise UnauthorizedError, ERROR_MESSAGES[:download] end end @@ -148,7 +152,10 @@ module Gitlab def check_single_change_access(change) Checks::ChangeAccess.new( - change, user_access: user_access, project: project).exec + change, + user_access: user_access, + project: project, + skip_authorization: deploy_key?).exec end def matching_merge_request?(newrev, branch_name) @@ -156,17 +163,19 @@ module Gitlab end def deploy_key - actor if actor.is_a?(DeployKey) + actor if deploy_key? + end + + def deploy_key? + actor.is_a?(DeployKey) end def can_read_project? if deploy_key - project.public? || deploy_key.has_access_to?(project) + deploy_key.has_access_to?(project) elsif user - user_access.can_read_project? - else - Guest.can?(:read_project, project) - end + user.can?(:read_project, project) + end || Guest.can?(:read_project, project) end protected -- cgit v1.2.1 From 0c532dbb243bf9bb5bf77248ce87a2a0e4478421 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 18 Nov 2016 04:08:24 +0800 Subject: Check if the key could really download, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_18518792 --- lib/gitlab/git_access.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 6be0ab08a1f..d5690f870e9 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -45,7 +45,7 @@ module Gitlab def download_access_check if deploy_key - true + deploy_key.has_access_to?(project) elsif user user_download_access_check elsif Guest.can?(:download_code, project) -- cgit v1.2.1 From e72e2f9ba0a160960f68035fbbdbe3f0f86b0dba Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 18 Nov 2016 04:11:04 +0800 Subject: Still grant :download_code if guest could do that Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_18518792 --- lib/gitlab/git_access.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index d5690f870e9..3f674532488 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -48,11 +48,9 @@ module Gitlab deploy_key.has_access_to?(project) elsif user user_download_access_check - elsif Guest.can?(:download_code, project) - true - else - raise UnauthorizedError, ERROR_MESSAGES[:download] - end + end || + Guest.can?(:download_code, project) || + raise(UnauthorizedError, ERROR_MESSAGES[:download]) end def push_access_check(changes) -- cgit v1.2.1 From 8dbea582497bbc45735cf145a3da4c88c9e0e78d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 18 Nov 2016 17:28:05 +0800 Subject: Check download privilege more specifically and add another error message for the new error. --- lib/gitlab/git_access.rb | 58 ++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 29 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 3f674532488..b87ca316240 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -7,7 +7,10 @@ module Gitlab ERROR_MESSAGES = { upload: 'You are not allowed to upload code for this project.', download: 'You are not allowed to download code from this project.', - deploy_key: 'This deploy key does not have write access to this project.', + deploy_key_upload: + 'This deploy key does not have write access to this project.', + deploy_key: + 'This deploy key does not have access to this project.', no_repo: 'A repository for this project does not exist yet.' } @@ -44,29 +47,36 @@ module Gitlab end def download_access_check - if deploy_key - deploy_key.has_access_to?(project) - elsif user - user_download_access_check - end || - Guest.can?(:download_code, project) || - raise(UnauthorizedError, ERROR_MESSAGES[:download]) + passed = if deploy_key + deploy_key.has_access_to?(project) + elsif user + user_can_download_code? || build_can_download_code? + end || Guest.can?(:download_code, project) + + unless passed + message = if deploy_key + ERROR_MESSAGES[:deploy_key] + else + ERROR_MESSAGES[:download] + end + + raise UnauthorizedError, message + end end def push_access_check(changes) if deploy_key - deploy_key_push_access_check(changes) + deploy_key_push_access_check elsif user - user_push_access_check(changes) + user_push_access_check else raise UnauthorizedError, ERROR_MESSAGES[:upload] end - end - def user_download_access_check - unless user_can_download_code? || build_can_download_code? - raise UnauthorizedError, ERROR_MESSAGES[:download] - end + return if changes.blank? # Allow access. + + check_repository_existence! + check_change_access!(changes) end def user_can_download_code? @@ -77,25 +87,15 @@ module Gitlab authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code) end - def user_push_access_check(changes) + def user_push_access_check unless authentication_abilities.include?(:push_code) raise UnauthorizedError, ERROR_MESSAGES[:upload] end - - if changes.blank? - return # Allow access. - end - - check_repository_existence! - check_change_access!(changes) end - def deploy_key_push_access_check(changes) - if deploy_key.can_push_to?(project) - check_repository_existence! - check_change_access!(changes) - else - raise UnauthorizedError, ERROR_MESSAGES[:deploy_key] + def deploy_key_push_access_check + unless deploy_key.can_push_to?(project) + raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload] end end -- cgit v1.2.1 From 1b150576d9de9096796f76e1635d27ddaa2b1dd5 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 6 Dec 2016 21:09:23 +0800 Subject: Prefer guest_can_downlod_code? --- lib/gitlab/git_access.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 9563fa7cafb..d42879e38cd 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -51,7 +51,7 @@ module Gitlab deploy_key.has_access_to?(project) elsif user user_can_download_code? || build_can_download_code? - end || Guest.can?(:download_code, project) + end || guest_can_downlod_code? unless passed message = if deploy_key -- cgit v1.2.1 From 46d7c1d2b3cf87023cb6bcf40dd53aa79606d203 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 6 Dec 2016 21:10:27 +0800 Subject: Prefer guest_can_download_code? and fix typo --- lib/gitlab/git_access.rb | 4 ++-- lib/gitlab/git_access_wiki.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index d42879e38cd..412f42c6320 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -51,7 +51,7 @@ module Gitlab deploy_key.has_access_to?(project) elsif user user_can_download_code? || build_can_download_code? - end || guest_can_downlod_code? + end || guest_can_download_code? unless passed message = if deploy_key @@ -79,7 +79,7 @@ module Gitlab check_change_access!(changes) end - def guest_can_downlod_code? + def guest_can_download_code? Guest.can?(:download_code, project) end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 74171f4f90e..67eaa5e088d 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 guest_can_downlod_code? + def guest_can_download_code? Guest.can?(:download_wiki_code, project) end -- cgit v1.2.1 From 57e3e942de1adef2c8621905370f07d7da7870c4 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 10 Dec 2016 01:45:13 +0800 Subject: Don't pass the actor for deploy key, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_19579483 --- lib/gitlab/git_access.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 412f42c6320..d483038e8e9 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -189,6 +189,8 @@ module Gitlab case actor when User actor + when DeployKey + nil when Key actor.user end -- cgit v1.2.1 From 8ac50d78eb921989d100a91aad9eb6e59cbf43dc Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 10 Dec 2016 02:04:36 +0800 Subject: Check project existence for push too, and we don't have to check for deploy key for downloading because deploy key could certainly download when it could already read the project. Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_19578626 --- lib/gitlab/git_access.rb | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index d483038e8e9..13efc1ed73d 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -9,8 +9,6 @@ module Gitlab download: 'You are not allowed to download code from this project.', deploy_key_upload: 'This deploy key does not have write access to this project.', - deploy_key: - 'This deploy key does not have access to this project.', no_repo: 'A repository for this project does not exist yet.' } @@ -33,10 +31,11 @@ module Gitlab check_active_user! unless deploy_key? check_project_accessibility! check_command_existence!(cmd) + check_repository_existence! case cmd when *DOWNLOAD_COMMANDS - download_access_check + download_access_check unless deploy_key? when *PUSH_COMMANDS push_access_check(changes) end @@ -47,20 +46,12 @@ module Gitlab end def download_access_check - passed = if deploy_key - deploy_key.has_access_to?(project) - elsif user - user_can_download_code? || build_can_download_code? - end || guest_can_download_code? + passed = user_can_download_code? || + build_can_download_code? || + guest_can_download_code? unless passed - message = if deploy_key - ERROR_MESSAGES[:deploy_key] - else - ERROR_MESSAGES[:download] - end - - raise UnauthorizedError, message + raise UnauthorizedError, ERROR_MESSAGES[:download] end end @@ -75,7 +66,6 @@ module Gitlab return if changes.blank? # Allow access. - check_repository_existence! check_change_access!(changes) end -- cgit v1.2.1 From 884f57c9102416805427d773eb21e09fd30c2452 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Dec 2016 21:19:07 +0800 Subject: Use consistent names and move checks to the method, and move those checks to be private. Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_20285012 https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_20285279 --- lib/gitlab/git_access.rb | 82 +++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 39 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 545506f3dfd..f0b241fb5e6 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -29,16 +29,16 @@ module Gitlab def check(cmd, changes) check_protocol! - check_active_user! unless deploy_key? + check_active_user! check_project_accessibility! check_command_existence!(cmd) check_repository_existence! case cmd when *DOWNLOAD_COMMANDS - download_access_check unless deploy_key? + check_download_access! when *PUSH_COMMANDS - push_access_check(changes) + check_push_access!(changes) end build_status_object(true) @@ -46,30 +46,6 @@ module Gitlab build_status_object(false, ex.message) end - def download_access_check - passed = user_can_download_code? || - build_can_download_code? || - guest_can_download_code? - - unless passed - raise UnauthorizedError, ERROR_MESSAGES[:download] - end - end - - def push_access_check(changes) - if deploy_key - deploy_key_push_access_check - elsif user - user_push_access_check - else - raise UnauthorizedError, ERROR_MESSAGES[:upload] - end - - return if changes.blank? # Allow access. - - check_change_access!(changes) - end - def guest_can_download_code? Guest.can?(:download_code, project) end @@ -82,18 +58,6 @@ module Gitlab authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code) end - def user_push_access_check - unless authentication_abilities.include?(:push_code) - raise UnauthorizedError, ERROR_MESSAGES[:upload] - end - end - - def deploy_key_push_access_check - unless deploy_key.can_push_to?(project) - raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload] - end - end - def protocol_allowed? Gitlab::ProtocolAccess.allowed?(protocol) end @@ -107,6 +71,8 @@ module Gitlab end def check_active_user! + return if deploy_key? + if user && !user_access.allowed? raise UnauthorizedError, "Your account has been blocked." end @@ -130,6 +96,44 @@ module Gitlab end end + def check_download_access! + return if deploy_key? + + passed = user_can_download_code? || + build_can_download_code? || + guest_can_download_code? + + unless passed + raise UnauthorizedError, ERROR_MESSAGES[:download] + end + end + + def check_push_access!(changes) + if deploy_key + check_deploy_key_push_access! + elsif user + check_user_push_access! + else + raise UnauthorizedError, ERROR_MESSAGES[:upload] + end + + return if changes.blank? # Allow access. + + check_change_access!(changes) + end + + def check_user_push_access! + unless authentication_abilities.include?(:push_code) + raise UnauthorizedError, ERROR_MESSAGES[:upload] + end + end + + def check_deploy_key_push_access! + unless deploy_key.can_push_to?(project) + raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload] + end + end + def check_change_access!(changes) changes_list = Gitlab::ChangesList.new(changes) -- cgit v1.2.1 From c1d11bf57c3091aa4695e302e21c39b9ec723f54 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Dec 2016 23:30:01 +0800 Subject: Rubocop prefers to indent this way --- lib/gitlab/git_access.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index f0b241fb5e6..7e1484613f2 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -100,8 +100,8 @@ module Gitlab return if deploy_key? passed = user_can_download_code? || - build_can_download_code? || - guest_can_download_code? + build_can_download_code? || + guest_can_download_code? unless passed raise UnauthorizedError, ERROR_MESSAGES[:download] -- cgit v1.2.1