diff options
27 files changed, 298 insertions, 118 deletions
diff --git a/app/assets/stylesheets/pages/deploy_keys.scss b/app/assets/stylesheets/pages/deploy_keys.scss new file mode 100644 index 00000000000..2fafe052106 --- /dev/null +++ b/app/assets/stylesheets/pages/deploy_keys.scss @@ -0,0 +1,13 @@ +.deploy-keys-list { + width: 100%; + overflow: auto; + + table { + border: 1px solid $table-border-color; + } +} + +.deploy-keys-title { + padding-bottom: 2px; + line-height: 2; +} diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb index 285e8495342..4f6a7e9e2cb 100644 --- a/app/controllers/admin/deploy_keys_controller.rb +++ b/app/controllers/admin/deploy_keys_controller.rb @@ -10,7 +10,7 @@ class Admin::DeployKeysController < Admin::ApplicationController end def create - @deploy_key = deploy_keys.new(deploy_key_params) + @deploy_key = deploy_keys.new(deploy_key_params.merge(user: current_user)) if @deploy_key.save redirect_to admin_deploy_keys_path @@ -39,6 +39,6 @@ class Admin::DeployKeysController < Admin::ApplicationController end def deploy_key_params - params.require(:deploy_key).permit(:key, :title) + params.require(:deploy_key).permit(:key, :title, :can_push) end end diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index 529e0aa2d33..b094491e006 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/app/controllers/projects/deploy_keys_controller.rb @@ -16,7 +16,7 @@ class Projects::DeployKeysController < Projects::ApplicationController end def create - @key = DeployKey.new(deploy_key_params) + @key = DeployKey.new(deploy_key_params.merge(user: current_user)) set_index_vars if @key.valid? && @project.deploy_keys << @key @@ -53,6 +53,6 @@ class Projects::DeployKeysController < Projects::ApplicationController end def deploy_key_params - params.require(:deploy_key).permit(:key, :title) + params.require(:deploy_key).permit(:key, :title, :can_push) end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 9cda3b78761..3acbc5fea27 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -90,10 +90,12 @@ module ProjectsHelper end def project_for_deploy_key(deploy_key) - if deploy_key.projects.include?(@project) + if deploy_key.has_access_to?(@project) @project else - deploy_key.projects.find { |project| can?(current_user, :read_project, project) } + deploy_key.projects.find do |project| + can?(current_user, :read_project, project) + end end end diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 2c525d4cd7a..053f2a11aa0 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -20,4 +20,18 @@ class DeployKey < Key def destroyed_when_orphaned? self.private? end + + def has_access_to?(project) + projects.include?(project) + end + + def can_push_to?(project) + can_push? && has_access_to?(project) + end + + private + + # we don't want to notify the user for deploy keys + def notify_user + end end diff --git a/app/models/key.rb b/app/models/key.rb index a5d25409730..6f377f0e8ae 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -57,10 +57,6 @@ class Key < ActiveRecord::Base ) end - def notify_user - run_after_commit { NotificationService.new.new_key(self) } - end - def post_create_hook SystemHooksService.new.execute_hooks_for(self, :create) end @@ -86,4 +82,8 @@ class Key < ActiveRecord::Base self.fingerprint = Gitlab::KeyFingerprint.new(self.key).fingerprint end + + def notify_user + run_after_commit { NotificationService.new.new_key(self) } + end end diff --git a/app/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml index 149593e7f46..7b71bb5b287 100644 --- a/app/views/admin/deploy_keys/index.html.haml +++ b/app/views/admin/deploy_keys/index.html.haml @@ -1,27 +1,34 @@ - page_title "Deploy Keys" -.panel.panel-default.prepend-top-default - .panel-heading - Public deploy keys (#{@deploy_keys.count}) - .controls - = link_to 'New Deploy Key', new_admin_deploy_key_path, class: "btn btn-new btn-sm" - - if @deploy_keys.any? - .table-holder - %table.table - %thead.panel-heading + +%h3.page-title.deploy-keys-title + Public deploy keys (#{@deploy_keys.count}) + .pull-right + = link_to 'New Deploy Key', new_admin_deploy_key_path, class: 'btn btn-new btn-sm btn-inverted' + +- if @deploy_keys.any? + .table-holder.deploy-keys-list + %table.table + %thead + %tr + %th.col-sm-2 Title + %th.col-sm-4 Fingerprint + %th.col-sm-2 Write access allowed + %th.col-sm-2 Added at + %th.col-sm-2 + %tbody + - @deploy_keys.each do |deploy_key| %tr - %th Title - %th Fingerprint - %th Added at - %th - %tbody - - @deploy_keys.each do |deploy_key| - %tr - %td - %strong= deploy_key.title - %td - %code.key-fingerprint= deploy_key.fingerprint - %td - %span.cgray - added #{time_ago_with_tooltip(deploy_key.created_at)} - %td - = link_to 'Remove', admin_deploy_key_path(deploy_key), data: { confirm: 'Are you sure?'}, method: :delete, class: "btn btn-sm btn-remove delete-key pull-right" + %td + %strong= deploy_key.title + %td + %code.key-fingerprint= deploy_key.fingerprint + %td + - if deploy_key.can_push? + Yes + - else + No + %td + %span.cgray + added #{time_ago_with_tooltip(deploy_key.created_at)} + %td + = link_to 'Remove', admin_deploy_key_path(deploy_key), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-remove delete-key pull-right' diff --git a/app/views/admin/deploy_keys/new.html.haml b/app/views/admin/deploy_keys/new.html.haml index 5c410a695bf..a064efc231f 100644 --- a/app/views/admin/deploy_keys/new.html.haml +++ b/app/views/admin/deploy_keys/new.html.haml @@ -16,6 +16,14 @@ Paste a machine public key here. Read more about how to generate it = link_to "here", help_page_path("ssh/README") = f.text_area :key, class: "form-control thin_area", rows: 5 + .form-group + .control-label + .col-sm-10 + = f.label :can_push do + = f.check_box :can_push + %strong Write access allowed + %p.light.append-bottom-0 + Allow this key to push to repository as well? (Default only allows pull access.) .form-actions = f.submit 'Create', class: "btn-create btn" diff --git a/app/views/projects/deploy_keys/_deploy_key.html.haml b/app/views/projects/deploy_keys/_deploy_key.html.haml index 450aaeb367c..d1e3cb14022 100644 --- a/app/views/projects/deploy_keys/_deploy_key.html.haml +++ b/app/views/projects/deploy_keys/_deploy_key.html.haml @@ -6,6 +6,9 @@ = deploy_key.title .description = deploy_key.fingerprint + - if deploy_key.can_push? + .write-access-allowed + Write access allowed .deploy-key-content.prepend-left-default.deploy-key-projects - deploy_key.projects.each do |project| - if can?(current_user, :read_project, project) diff --git a/app/views/projects/deploy_keys/_form.html.haml b/app/views/projects/deploy_keys/_form.html.haml index 901605f7ca3..c91bb9c255a 100644 --- a/app/views/projects/deploy_keys/_form.html.haml +++ b/app/views/projects/deploy_keys/_form.html.haml @@ -10,4 +10,13 @@ %p.light.append-bottom-0 Paste a machine public key here. Read more about how to generate it = link_to "here", help_page_path("ssh/README") + .form-group + .checkbox + = f.label :can_push do + = f.check_box :can_push + %strong Write access allowed + .form-group + %p.light.append-bottom-0 + Allow this key to push to repository as well? (Default only allows pull access.) + = f.submit "Add key", class: "btn-create btn" diff --git a/changelogs/unreleased/feature-1376-allow-write-access-deploy-keys.yml b/changelogs/unreleased/feature-1376-allow-write-access-deploy-keys.yml new file mode 100644 index 00000000000..0fd590a877b --- /dev/null +++ b/changelogs/unreleased/feature-1376-allow-write-access-deploy-keys.yml @@ -0,0 +1,4 @@ +--- +title: Allow to add deploy keys with write-access +merge_request: 5807 +author: Ali Ibrahim diff --git a/db/migrate/20160811172945_add_can_push_to_keys.rb b/db/migrate/20160811172945_add_can_push_to_keys.rb new file mode 100644 index 00000000000..5fd303fe8fb --- /dev/null +++ b/db/migrate/20160811172945_add_can_push_to_keys.rb @@ -0,0 +1,14 @@ +class AddCanPushToKeys < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + add_column_with_default(:keys, :can_push, :boolean, default: false, allow_null: false) + end + + def down + remove_column(:keys, :can_push) + end +end diff --git a/db/schema.rb b/db/schema.rb index 9c46f573719..63e52bccce8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -525,6 +525,7 @@ ActiveRecord::Schema.define(version: 20161202152035) do t.string "type" t.string "fingerprint" t.boolean "public", default: false, null: false + t.boolean "can_push", default: false, null: false end add_index "keys", ["fingerprint"], name: "index_keys_on_fingerprint", unique: true, using: :btree diff --git a/doc/api/deploy_keys.md b/doc/api/deploy_keys.md index 5f248ab6f91..284d5f88c55 100644 --- a/doc/api/deploy_keys.md +++ b/doc/api/deploy_keys.md @@ -20,12 +20,14 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", + "can_push": false, "created_at": "2013-10-02T10:12:29Z" }, { "id": 3, "title": "Another Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", + "can_push": true, "created_at": "2013-10-02T11:12:29Z" } ] @@ -55,12 +57,14 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", + "can_push": false, "created_at": "2013-10-02T10:12:29Z" }, { "id": 3, "title": "Another Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", + "can_push": false, "created_at": "2013-10-02T11:12:29Z" } ] @@ -92,6 +96,7 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", + "can_push": false, "created_at": "2013-10-02T10:12:29Z" } ``` @@ -107,14 +112,15 @@ project only if original one was is accessible by the same user. POST /projects/:id/deploy_keys ``` -| Attribute | Type | Required | Description | -| --------- | ---- | -------- | ----------- | -| `id` | integer | yes | The ID of the project | -| `title` | string | yes | New deploy key's title | -| `key` | string | yes | New deploy key | +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of the project | +| `title` | string | yes | New deploy key's title | +| `key` | string | yes | New deploy key | +| `can_push` | boolean | no | Can deploy key push to the project's repository | ```bash -curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "Content-Type: application/json" --data '{"title": "My deploy key", "key": "ssh-rsa AAAA..."}' "https://gitlab.example.com/api/v3/projects/5/deploy_keys/" +curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "Content-Type: application/json" --data '{"title": "My deploy key", "key": "ssh-rsa AAAA...", "can_push": "true"}' "https://gitlab.example.com/api/v3/projects/5/deploy_keys/" ``` Example response: @@ -124,6 +130,7 @@ Example response: "key" : "ssh-rsa AAAA...", "id" : 12, "title" : "My deploy key", + "can_push": true, "created_at" : "2015-08-29T12:44:31.550Z" } ``` diff --git a/features/admin/deploy_keys.feature b/features/admin/deploy_keys.feature index 33439cd1e85..95ac77cddd2 100644 --- a/features/admin/deploy_keys.feature +++ b/features/admin/deploy_keys.feature @@ -13,4 +13,11 @@ Feature: Admin Deploy Keys And I click 'New Deploy Key' And I submit new deploy key Then I should be on admin deploy keys page - And I should see newly created deploy key + And I should see newly created deploy key without write access + + Scenario: Deploy Keys new with write access + When I visit admin deploy keys page + And I click 'New Deploy Key' + And I submit new deploy key with write access + Then I should be on admin deploy keys page + And I should see newly created deploy key with write access diff --git a/features/steps/admin/deploy_keys.rb b/features/steps/admin/deploy_keys.rb index 56787eeb6b3..79312a5d1c5 100644 --- a/features/steps/admin/deploy_keys.rb +++ b/features/steps/admin/deploy_keys.rb @@ -32,12 +32,25 @@ class Spinach::Features::AdminDeployKeys < Spinach::FeatureSteps click_button "Create" end + step 'I submit new deploy key with write access' do + fill_in "deploy_key_title", with: "server" + fill_in "deploy_key_key", with: "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop" + check "deploy_key_can_push" + click_button "Create" + end + step 'I should be on admin deploy keys page' do expect(current_path).to eq admin_deploy_keys_path end - step 'I should see newly created deploy key' do + step 'I should see newly created deploy key without write access' do + expect(page).to have_content(deploy_key.title) + expect(page).to have_content('No') + end + + step 'I should see newly created deploy key with write access' do expect(page).to have_content(deploy_key.title) + expect(page).to have_content('Yes') end def deploy_key diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 01c0f5072ba..bb8d740532d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -298,7 +298,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 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/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 db07b7c5fcc..13efc1ed73d 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -7,7 +7,8 @@ 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_upload: + 'This deploy key does not have write access to this project.', no_repo: 'A repository for this project does not exist yet.' } @@ -27,13 +28,14 @@ module Gitlab def check(cmd, changes) check_protocol! - check_active_user! + 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 @@ -44,29 +46,31 @@ module Gitlab end def download_access_check - if user - user_download_access_check - elsif deploy_key.nil? && !guest_can_downlod_code? + 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 user - user_push_access_check(changes) + if deploy_key + deploy_key_push_access_check + elsif user + user_push_access_check else - raise UnauthorizedError, ERROR_MESSAGES[deploy_key ? :deploy_key : :upload] + raise UnauthorizedError, ERROR_MESSAGES[:upload] end - end - def guest_can_downlod_code? - Guest.can?(:download_code, project) + return if changes.blank? # Allow access. + + check_change_access!(changes) end - def user_download_access_check - unless user_can_download_code? || build_can_download_code? - raise UnauthorizedError, ERROR_MESSAGES[:download] - end + def guest_can_download_code? + Guest.can?(:download_code, project) end def user_can_download_code? @@ -77,33 +81,16 @@ 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 - - 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 end - def change_access_check(change) - Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec + 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? @@ -136,31 +123,51 @@ 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 = 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 + end + end + end + + def check_single_change_access(change) + Checks::ChangeAccess.new( + change, + user_access: user_access, + project: project, + skip_authorization: deploy_key?).exec + end + def matching_merge_request?(newrev, branch_name) Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end def deploy_key - actor if actor.is_a?(DeployKey) + actor if deploy_key? end - def deploy_key_can_read_project? - if deploy_key - return true if project.public? - deploy_key.projects.include?(project) - else - false - end + def deploy_key? + actor.is_a?(DeployKey) end def can_read_project? - if user - user_access.can_read_project? - elsif deploy_key - deploy_key_can_read_project? - else - Guest.can?(:read_project, project) - end + if deploy_key + deploy_key.has_access_to?(project) + elsif user + user.can?(:read_project, project) + end || Guest.can?(:read_project, project) end protected diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 2c06c4ff1ef..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 @@ -8,7 +8,7 @@ module Gitlab authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code) end - def change_access_check(change) + def check_single_change_access(change) if user_access.can_do_action?(:create_wiki) build_status_object(true) else 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 diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index f1d0a190002..2c90397cc78 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -82,7 +82,7 @@ describe Gitlab::GitAccess, lib: true do end end - describe 'without acccess to project' do + describe 'without access to project' do context 'pull code' do it { expect(subject.allowed?).to be_falsey } end @@ -112,7 +112,7 @@ describe Gitlab::GitAccess, lib: true do end describe 'deploy key permissions' do - let(:key) { create(:deploy_key) } + let(:key) { create(:deploy_key, user: user) } let(:actor) { key } context 'pull code' do @@ -136,7 +136,7 @@ describe Gitlab::GitAccess, lib: true do end context 'from private project' do - let(:project) { create(:project, :internal) } + let(:project) { create(:project, :private) } it { expect(subject).not_to be_allowed } end @@ -353,13 +353,13 @@ describe Gitlab::GitAccess, lib: true do end end - shared_examples 'can not push code' do + shared_examples 'pushing code' do |can| subject { access.check('git-receive-pack', '_any') } context 'when project is authorized' do before { authorize } - it { expect(subject).not_to be_allowed } + it { expect(subject).public_send(can, be_allowed) } end context 'when unauthorized' do @@ -386,7 +386,7 @@ describe Gitlab::GitAccess, lib: true do describe 'build authentication abilities' do let(:authentication_abilities) { build_authentication_abilities } - it_behaves_like 'can not push code' do + it_behaves_like 'pushing code', :not_to do def authorize project.team << [user, :reporter] end @@ -394,12 +394,26 @@ describe Gitlab::GitAccess, lib: true do end describe 'deploy key permissions' do - let(:key) { create(:deploy_key) } + let(:key) { create(:deploy_key, user: user, can_push: can_push) } let(:actor) { key } - it_behaves_like 'can not push code' do - def authorize - key.projects << project + context 'when deploy_key can push' do + let(:can_push) { true } + + it_behaves_like 'pushing code', :to do + def authorize + key.projects << project + end + end + end + + context 'when deploy_key cannot push' do + let(:can_push) { false } + + it_behaves_like 'pushing code', :not_to do + def authorize + key.projects << project + end end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 78d6b2c5032..ac26c831fd0 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -247,6 +247,7 @@ DeployKey: - type - fingerprint - public +- can_push Service: - id - type diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index 93623e8e99b..8ef8218cf74 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -1,8 +1,22 @@ require 'spec_helper' describe DeployKey, models: true do + include EmailHelpers + describe "Associations" do it { is_expected.to have_many(:deploy_keys_projects) } it { is_expected.to have_many(:projects) } end + + describe 'notification' do + let(:user) { create(:user) } + + it 'does not send a notification' do + perform_enqueued_jobs do + create(:deploy_key, user: user) + end + + should_not_email(user) + end + end end diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 2a33d819138..7758b7ffa97 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Key, models: true do + include EmailHelpers + describe "Associations" do it { is_expected.to belong_to(:user) } end @@ -96,4 +98,16 @@ describe Key, models: true do expect(described_class.new(key: " #{valid_key} ").key).to eq(valid_key) end end + + describe 'notification' do + let(:user) { create(:user) } + + it 'sends a notification' do + perform_enqueued_jobs do + create(:key, user: user) + end + + should_email(user) + end + end end diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index f1728d61def..c701b9a1202 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -371,12 +371,26 @@ describe 'Git HTTP requests', lib: true do shared_examples 'can download code only' do it 'downloads get status 200' do - clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token + allow_any_instance_of(Repository). + to receive(:exists?).and_return(true) + + clone_get "#{project.path_with_namespace}.git", + user: 'gitlab-ci-token', password: build.token expect(response).to have_http_status(200) expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE) end + it 'downloads from non-existing repository and gets 403' do + allow_any_instance_of(Repository). + to receive(:exists?).and_return(false) + + clone_get "#{project.path_with_namespace}.git", + user: 'gitlab-ci-token', password: build.token + + expect(response).to have_http_status(403) + end + it 'uploads get status 403' do push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token |