From 2b73aaa15ad9f651f51f8c71de461da6664a4fbb Mon Sep 17 00:00:00 2001
From: Ali Ibrahim <aliibrahim@gmail.com>
Date: Sun, 14 Aug 2016 13:49:08 -0400
Subject: [PATCH 01/38] Allow to add deploy keys with write-access

---
 CHANGELOG                                     |  1 +
 .../projects/deploy_keys_controller.rb        |  2 +-
 .../projects/deploy_keys/_form.html.haml      |  9 +++++++
 .../20160811172945_add_can_push_to_keys.rb    | 27 +++++++++++++++++++
 db/schema.rb                                  | 21 ++++++++-------
 lib/gitlab/git_access.rb                      | 17 ++++++++----
 spec/lib/gitlab/git_access_spec.rb            | 11 +++++---
 7 files changed, 68 insertions(+), 20 deletions(-)
 create mode 100644 db/migrate/20160811172945_add_can_push_to_keys.rb

diff --git a/CHANGELOG b/CHANGELOG
index 837e9e27aba..d3d0f79039f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -126,6 +126,7 @@ v 8.10.6
   - Restore "Largest repository" sort option on Admin > Projects page. !5797
   - Fix privilege escalation via project export.
   - Require administrator privileges to perform a project import.
+  - Allow to add deploy keys with write-access !5807 (Ali Ibrahim)
 
 v 8.10.5
   - Add a data migration to fix some missing timestamps in the members table. !5670
diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb
index 529e0aa2d33..77bd883a5bd 100644
--- a/app/controllers/projects/deploy_keys_controller.rb
+++ b/app/controllers/projects/deploy_keys_controller.rb
@@ -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/views/projects/deploy_keys/_form.html.haml b/app/views/projects/deploy_keys/_form.html.haml
index 901605f7ca3..68f472e8886 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 Allow write access
+  .form-group
+    %p.light.append-bottom-0
+      Can this key be used to push to this repository? Deploy keys always have pull access
+
   = f.submit "Add key", class: "btn-create btn"
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..de7d07ccabb
--- /dev/null
+++ b/db/migrate/20160811172945_add_can_push_to_keys.rb
@@ -0,0 +1,27 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddCanPushToKeys < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  disable_ddl_transaction!
+
+  def change
+    add_column_with_default(:keys, :can_push, :boolean, default: false, allow_null: false)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 445f484a8c7..d8c6922fb1d 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160810142633) do
+ActiveRecord::Schema.define(version: 20160811172945) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -484,6 +484,7 @@ ActiveRecord::Schema.define(version: 20160810142633) 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
@@ -589,12 +590,12 @@ ActiveRecord::Schema.define(version: 20160810142633) do
     t.datetime "locked_at"
     t.integer  "updated_by_id"
     t.string   "merge_error"
+    t.text     "merge_params"
     t.boolean  "merge_when_build_succeeds",    default: false, null: false
     t.integer  "merge_user_id"
     t.string   "merge_commit_sha"
     t.datetime "deleted_at"
     t.string   "in_progress_merge_commit_sha"
-    t.text     "merge_params"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -930,8 +931,8 @@ ActiveRecord::Schema.define(version: 20160810142633) do
     t.string   "noteable_type"
     t.string   "title"
     t.text     "description"
-    t.datetime "created_at",    null: false
-    t.datetime "updated_at",    null: false
+    t.datetime "created_at",                       null: false
+    t.datetime "updated_at",                       null: false
     t.boolean  "submitted_as_ham", default: false, null: false
   end
 
@@ -1001,13 +1002,13 @@ ActiveRecord::Schema.define(version: 20160810142633) do
   add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree
 
   create_table "user_agent_details", force: :cascade do |t|
-    t.string   "user_agent",   null: false
-    t.string   "ip_address",   null: false
-    t.integer  "subject_id",   null: false
-    t.string   "subject_type", null: false
+    t.string   "user_agent",                   null: false
+    t.string   "ip_address",                   null: false
+    t.integer  "subject_id",                   null: false
+    t.string   "subject_type",                 null: false
     t.boolean  "submitted",    default: false, null: false
-    t.datetime "created_at",   null: false
-    t.datetime "updated_at",   null: false
+    t.datetime "created_at",                   null: false
+    t.datetime "updated_at",                   null: false
   end
 
   create_table "users", force: :cascade do |t|
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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index f12c9a370f7..d0e73d70e6e 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -284,19 +284,22 @@ describe Gitlab::GitAccess, lib: true do
   end
 
   describe 'deploy key permissions' do
-    let(:key) { create(:deploy_key) }
-    let(:actor) { key }
-
     context 'push code' do
       subject { access.check('git-receive-pack', '_any') }
 
       context 'when project is authorized' do
+        let(:key) { create(:deploy_key, can_push: true) }
+        let(:actor) { key }
+
         before { key.projects << project }
 
-        it { expect(subject).not_to be_allowed }
+        it { expect(subject).to be_allowed }
       end
 
       context 'when unauthorized' do
+        let(:key) { create(:deploy_key, can_push: false) }
+        let(:actor) { key }
+
         context 'to public project' do
           let(:project) { create(:project, :public) }
 

From fcc2c43ebb32c0e9a3ae636e66460b42cbae4d53 Mon Sep 17 00:00:00 2001
From: Ali Ibrahim <aliibrahim@gmail.com>
Date: Thu, 18 Aug 2016 03:27:45 -0400
Subject: [PATCH 02/38] Added can_push attribute to deploy keys and update docs
 for API

---
 doc/api/deploy_keys.md | 19 +++++++++++++------
 lib/api/entities.rb    |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/doc/api/deploy_keys.md b/doc/api/deploy_keys.md
index ca44afbf355..b3bcf0dec42 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/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

From 8bc7ffd4dceec118319aed680f41a8dbda275a06 Mon Sep 17 00:00:00 2001
From: Ali Ibrahim <aliibrahim@gmail.com>
Date: Fri, 19 Aug 2016 13:39:13 -0400
Subject: [PATCH 03/38] Rephrase wording for pull/push access

---
 app/views/projects/deploy_keys/_form.html.haml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/views/projects/deploy_keys/_form.html.haml b/app/views/projects/deploy_keys/_form.html.haml
index 68f472e8886..757260aa5b5 100644
--- a/app/views/projects/deploy_keys/_form.html.haml
+++ b/app/views/projects/deploy_keys/_form.html.haml
@@ -14,9 +14,9 @@
     .checkbox
       = f.label :can_push do
         = f.check_box :can_push
-        %strong Allow write access
+        %strong Write access allowed?
   .form-group
     %p.light.append-bottom-0
-      Can this key be used to push to this repository? Deploy keys always have pull access
+      Allow this key to push to repository as well? (Default only allows pull access.)
 
   = f.submit "Add key", class: "btn-create btn"

From 2811a213a6199978ca1f3eea002a7a77f8b87554 Mon Sep 17 00:00:00 2001
From: Ali Ibrahim <aliibrahim@gmail.com>
Date: Fri, 19 Aug 2016 14:40:45 -0400
Subject: [PATCH 04/38] added spacing

---
 lib/gitlab/git_access.rb | 1 +
 1 file changed, 1 insertion(+)

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

From c3508851bff289fdaaa114298b3ae13513646775 Mon Sep 17 00:00:00 2001
From: Ali Ibrahim <aliibrahim@gmail.com>
Date: Fri, 19 Aug 2016 19:31:40 -0400
Subject: [PATCH 05/38] Set current user as deploy key user

---
 app/controllers/projects/deploy_keys_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb
index 77bd883a5bd..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

From 633f901d67e7a0c755b9fac92518ff1476595a11 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 10 Nov 2016 23:24:56 +0800
Subject: [PATCH 06/38] Update CHANGELOG

---
 .../feature-1376-allow-write-access-deploy-keys.yml           | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased/feature-1376-allow-write-access-deploy-keys.yml

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

From 5b5722e9099d63652ec47fc5599217c348e0f9dc Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 10 Nov 2016 23:28:15 +0800
Subject: [PATCH 07/38] Cleanup migration

---
 .../20160811172945_add_can_push_to_keys.rb    | 27 +++++--------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/db/migrate/20160811172945_add_can_push_to_keys.rb b/db/migrate/20160811172945_add_can_push_to_keys.rb
index de7d07ccabb..5fd303fe8fb 100644
--- a/db/migrate/20160811172945_add_can_push_to_keys.rb
+++ b/db/migrate/20160811172945_add_can_push_to_keys.rb
@@ -1,27 +1,14 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
 class AddCanPushToKeys < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
-
-  # Set this constant to true if this migration requires downtime.
-  DOWNTIME = false
-
-  # When a migration requires downtime you **must** uncomment the following
-  # constant and define a short and easy to understand explanation as to why the
-  # migration requires downtime.
-  # DOWNTIME_REASON = ''
-
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
-  #
   disable_ddl_transaction!
 
-  def change
+  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

From 05cc87052a7755da3d352409ef3ab024921593c4 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 20:40:28 +0800
Subject: [PATCH 08/38] Improve write access check for deploy key

---
 lib/gitlab/git_access.rb | 81 ++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 45 deletions(-)

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

From d0df9b7a4c1b246f4ddf22ce7609357f9c07da23 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 20:45:10 +0800
Subject: [PATCH 09/38] Remove old entry in CHANGELOG.md

---
 CHANGELOG.md | 1 -
 1 file changed, 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e77cda75ccb..20907eef17b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -879,7 +879,6 @@ entry.
   - Restore "Largest repository" sort option on Admin > Projects page. !5797
   - Fix privilege escalation via project export.
   - Require administrator privileges to perform a project import.
-  - Allow to add deploy keys with write-access !5807 (Ali Ibrahim)
 
 ## 8.10.5
 

From 8b2426f898fdda47fc53f2c3d31c2060329430d4 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 20:47:47 +0800
Subject: [PATCH 10/38] Mark DeployKey#can_push a safe attribute

---
 spec/lib/gitlab/import_export/safe_model_attributes.yml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 07a2c316899..aac61d404a8 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -246,6 +246,7 @@ DeployKey:
 - type
 - fingerprint
 - public
+- can_push
 Service:
 - id
 - type
@@ -339,4 +340,4 @@ LabelPriority:
 - label_id
 - priority
 - created_at
-- updated_at
\ No newline at end of file
+- updated_at

From 24d9f51e7bfbfa4a063b068377b100f6cde2b971 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 21:21:43 +0800
Subject: [PATCH 11/38] Correct the test. Not sure why change it in the first
 place

---
 spec/lib/gitlab/git_access_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 502ee9ce209..f1d0a190002 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -366,7 +366,7 @@ describe Gitlab::GitAccess, lib: true do
       context 'to public project' do
         let(:project) { create(:project, :public) }
 
-        it { expect(subject).to be_allowed }
+        it { expect(subject).not_to be_allowed }
       end
 
       context 'to internal project' do

From 38fbcb99dba61cfae1b788e0ec947911c4d14dd8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 21:24:21 +0800
Subject: [PATCH 12/38] So deploy key might not have a corresponding user

---
 lib/gitlab/git_access.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

From 71ae01fefe62caf396640affb8ca618fe68db5a0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 21:44:33 +0800
Subject: [PATCH 13/38] Add more tests and fix write to project check

---
 app/models/deploy_key.rb           |  4 ++++
 lib/gitlab/git_access.rb           |  2 +-
 spec/lib/gitlab/git_access_spec.rb | 38 ++++++++++++++++++++++++------
 3 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 2c525d4cd7a..503398f5cca 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -20,4 +20,8 @@ class DeployKey < Key
   def destroyed_when_orphaned?
     self.private?
   end
+
+  def can_push_to?(project)
+    can_push? && projects.include?(project)
+  end
 end
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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index f1d0a190002..ac5352a9561 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -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
@@ -383,10 +383,20 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
+  describe 'full authentication abilities' do
+    let(:authentication_abilities) { full_authentication_abilities }
+
+    it_behaves_like 'pushing code', :to do
+      def authorize
+        project.team << [user, :developer]
+      end
+    end
+  end
+
   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 +404,26 @@ describe Gitlab::GitAccess, lib: true do
   end
 
   describe 'deploy key permissions' do
-    let(:key) { create(:deploy_key) }
+    let(:key) { create(:deploy_key, 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

From 40632455b83e78ec30af2eb93ced02afc5c4d4a0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 21:53:43 +0800
Subject: [PATCH 14/38] Fix a typo: acccess -> access

---
 spec/lib/gitlab/git_access_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index ac5352a9561..3c1df2199b2 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

From 5da9bfa453268474b3bff13c63e55b29bbcdf016 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 Nov 2016 22:26:05 +0800
Subject: [PATCH 15/38] 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(-)

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

From 721478123068c6718ec73c72a7b7d32c00c816df Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 16 Nov 2016 19:48:54 +0800
Subject: [PATCH 16/38] 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(-)

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

From a9765fb47fbbd1e1070434fc06cc76b25a42caa6 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 16 Nov 2016 20:31:08 +0800
Subject: [PATCH 17/38] Introduce has_access_to? so that we could reuse it

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_18439108
---
 app/helpers/projects_helper.rb | 4 ++--
 app/models/deploy_key.rb       | 6 +++++-
 app/models/user.rb             | 4 ++++
 lib/gitlab/auth/result.rb      | 3 +--
 lib/gitlab/git_access.rb       | 2 +-
 5 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 42c00ec3cd5..5b08fc78cfc 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -93,10 +93,10 @@ 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(&current_user.method(:has_access_to?))
     end
   end
 
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 503398f5cca..aaacbd28470 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -21,7 +21,11 @@ class DeployKey < Key
     self.private?
   end
 
+  def has_access_to?(project)
+    projects.include?(project)
+  end
+
   def can_push_to?(project)
-    can_push? && projects.include?(project)
+    can_push? && has_access_to?(project)
   end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 3813df6684e..0e96ad88638 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -501,6 +501,10 @@ class User < ActiveRecord::Base
     several_namespaces? || admin
   end
 
+  def has_access_to?(project)
+    can?(:read_project, project)
+  end
+
   def can?(action, subject)
     Ability.allowed?(self, action, subject)
   end
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

From 48090a9188e13e3ddaffb5957a7b5a264024f060 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 16 Nov 2016 22:07:04 +0800
Subject: [PATCH 18/38] Introduce no_user_or_blocked? and fix tests due to

checking user permission.
---
 lib/gitlab/user_access.rb          | 16 ++++++++++++----
 spec/lib/gitlab/git_access_spec.rb | 12 ++++++++++--
 2 files changed, 22 insertions(+), 6 deletions(-)

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 3c1df2199b2..9c19ea2d862 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -112,9 +112,13 @@ 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 }
 
+      before do
+        project.team << [user, :master]
+      end
+
       context 'pull code' do
         context 'when project is authorized' do
           before { key.projects << project }
@@ -404,9 +408,13 @@ describe Gitlab::GitAccess, lib: true do
   end
 
   describe 'deploy key permissions' do
-    let(:key) { create(:deploy_key, can_push: can_push) }
+    let(:key) { create(:deploy_key, user: user, can_push: can_push) }
     let(:actor) { key }
 
+    before do
+      project.team << [user, :master]
+    end
+
     context 'when deploy_key can push' do
       let(:can_push) { true }
 

From 2489332297b441b3ebc0c3df2e8ff14dc88a72cf Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 01:23:04 +0800
Subject: [PATCH 19/38] Don't notify user for deploy keys, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_18517263
---
 app/controllers/admin/deploy_keys_controller.rb | 2 +-
 app/models/deploy_key.rb                        | 6 ++++++
 app/models/key.rb                               | 8 ++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb
index 285e8495342..6b146712940 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
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index aaacbd28470..053f2a11aa0 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -28,4 +28,10 @@ class DeployKey < Key
   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 ff8dda2dc89..c0a64cfb5fc 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -49,10 +49,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
@@ -78,4 +74,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

From 8c1a01e05fd3c6e1621242aaf31a0ce2789ad546 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 03:48:23 +0800
Subject: [PATCH 20/38] We never check user privilege if it's a deploy key

---
 app/models/user.rb                 |  4 ----
 lib/gitlab/checks/change_access.rb | 11 +++++++++--
 lib/gitlab/git_access.rb           | 29 +++++++++++++++++++----------
 spec/lib/gitlab/git_access_spec.rb | 18 ------------------
 4 files changed, 28 insertions(+), 34 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 40130b8b25c..5a2b232c4ed 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -501,10 +501,6 @@ class User < ActiveRecord::Base
     several_namespaces? || admin
   end
 
-  def has_access_to?(project)
-    can?(:read_project, project)
-  end
-
   def can?(action, subject)
     Ability.allowed?(self, action, subject)
   end
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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 9c19ea2d862..9810d79f8b4 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -115,10 +115,6 @@ describe Gitlab::GitAccess, lib: true do
       let(:key) { create(:deploy_key, user: user) }
       let(:actor) { key }
 
-      before do
-        project.team << [user, :master]
-      end
-
       context 'pull code' do
         context 'when project is authorized' do
           before { key.projects << project }
@@ -387,16 +383,6 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe 'full authentication abilities' do
-    let(:authentication_abilities) { full_authentication_abilities }
-
-    it_behaves_like 'pushing code', :to do
-      def authorize
-        project.team << [user, :developer]
-      end
-    end
-  end
-
   describe 'build authentication abilities' do
     let(:authentication_abilities) { build_authentication_abilities }
 
@@ -411,10 +397,6 @@ describe Gitlab::GitAccess, lib: true do
     let(:key) { create(:deploy_key, user: user, can_push: can_push) }
     let(:actor) { key }
 
-    before do
-      project.team << [user, :master]
-    end
-
     context 'when deploy_key can push' do
       let(:can_push) { true }
 

From af0242b3d604cc14302f91f0dbe75af0048862d7 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 03:56:35 +0800
Subject: [PATCH 21/38] We removed User#has_access_to?

---
 app/helpers/projects_helper.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 144ac7a0d02..77075e49b17 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -93,7 +93,9 @@ module ProjectsHelper
     if deploy_key.has_access_to?(@project)
       @project
     else
-      deploy_key.projects.find(&current_user.method(:has_access_to?))
+      deploy_key.projects.find do |project|
+        can?(current_user, :read_project, project)
+      end
     end
   end
 

From 0c532dbb243bf9bb5bf77248ce87a2a0e4478421 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 04:08:24 +0800
Subject: [PATCH 22/38] 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(-)

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)

From e72e2f9ba0a160960f68035fbbdbe3f0f86b0dba Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 04:11:04 +0800
Subject: [PATCH 23/38] 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(-)

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)

From 8dbea582497bbc45735cf145a3da4c88c9e0e78d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 17:28:05 +0800
Subject: [PATCH 24/38] 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(-)

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
 

From a41ee7eb5eea96277ee18d474dc2defecb7f7596 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 18:37:32 +0800
Subject: [PATCH 25/38] Fix an old copypasta: internal -> private

---
 spec/lib/gitlab/git_access_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 9810d79f8b4..2c90397cc78 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -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

From 428061678eda85a65ce6a9ee15ac520af45f021a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 19:09:03 +0800
Subject: [PATCH 26/38] Add tests for key/deploy key notifications

---
 spec/models/deploy_key_spec.rb | 12 ++++++++++++
 spec/models/key_spec.rb        | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb
index 93623e8e99b..3a2252ea40c 100644
--- a/spec/models/deploy_key_spec.rb
+++ b/spec/models/deploy_key_spec.rb
@@ -5,4 +5,16 @@ describe DeployKey, models: true 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 1a26cee9f3d..1260f0b5f42 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -92,4 +92,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

From 28102ec28e1ef3d3203db3d05aa89ab3da234e70 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 22 Nov 2016 16:06:42 +0800
Subject: [PATCH 27/38] Allow admin to set keys with write access, and

show write access information when showing the key.

TODO: It's ugly right now, need help!
---
 app/controllers/admin/deploy_keys_controller.rb   |  2 +-
 app/views/admin/deploy_keys/index.html.haml       |  6 ++++++
 app/views/admin/deploy_keys/new.html.haml         |  8 ++++++++
 .../projects/deploy_keys/_deploy_key.html.haml    |  3 +++
 features/admin/deploy_keys.feature                |  9 ++++++++-
 features/steps/admin/deploy_keys.rb               | 15 ++++++++++++++-
 6 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb
index 6b146712940..4f6a7e9e2cb 100644
--- a/app/controllers/admin/deploy_keys_controller.rb
+++ b/app/controllers/admin/deploy_keys_controller.rb
@@ -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/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml
index 149593e7f46..dee611ae014 100644
--- a/app/views/admin/deploy_keys/index.html.haml
+++ b/app/views/admin/deploy_keys/index.html.haml
@@ -11,6 +11,7 @@
           %tr
             %th Title
             %th Fingerprint
+            %th Write access
             %th Added at
             %th
         %tbody
@@ -20,6 +21,11 @@
                 %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)}
diff --git a/app/views/admin/deploy_keys/new.html.haml b/app/views/admin/deploy_keys/new.html.haml
index 5c410a695bf..96055174ad0 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..d360f1bab28 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?
+      .can-write
+        Can write
   .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/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

From 1b150576d9de9096796f76e1635d27ddaa2b1dd5 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Dec 2016 21:09:23 +0800
Subject: [PATCH 28/38] Prefer guest_can_downlod_code?

---
 lib/gitlab/git_access.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

From 46d7c1d2b3cf87023cb6bcf40dd53aa79606d203 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Dec 2016 21:10:27 +0800
Subject: [PATCH 29/38] 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(-)

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
 

From 5430122c56928849df44520353342150d8f37d51 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Dec 2016 21:33:55 +0800
Subject: [PATCH 30/38] Now we need to include EmailHelpers for each test

---
 spec/models/deploy_key_spec.rb | 2 ++
 spec/models/key_spec.rb        | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb
index 3a2252ea40c..8ef8218cf74 100644
--- a/spec/models/deploy_key_spec.rb
+++ b/spec/models/deploy_key_spec.rb
@@ -1,6 +1,8 @@
 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) }
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 0c0444e1f2a..c5a5467ca99 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

From 57e3e942de1adef2c8621905370f07d7da7870c4 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 10 Dec 2016 01:45:13 +0800
Subject: [PATCH 31/38] 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(+)

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

From 8ac50d78eb921989d100a91aad9eb6e59cbf43dc Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 10 Dec 2016 02:04:36 +0800
Subject: [PATCH 32/38] 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(-)

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
 

From f97965395198ef1da892dde246edb1e6ef480127 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 10 Dec 2016 02:25:31 +0800
Subject: [PATCH 33/38] Use consistent words, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_19581826
---
 app/views/admin/deploy_keys/index.html.haml          | 2 +-
 app/views/admin/deploy_keys/new.html.haml            | 2 +-
 app/views/projects/deploy_keys/_deploy_key.html.haml | 4 ++--
 app/views/projects/deploy_keys/_form.html.haml       | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml
index dee611ae014..0ba8e087e08 100644
--- a/app/views/admin/deploy_keys/index.html.haml
+++ b/app/views/admin/deploy_keys/index.html.haml
@@ -11,7 +11,7 @@
           %tr
             %th Title
             %th Fingerprint
-            %th Write access
+            %th Write access allowed
             %th Added at
             %th
         %tbody
diff --git a/app/views/admin/deploy_keys/new.html.haml b/app/views/admin/deploy_keys/new.html.haml
index 96055174ad0..a064efc231f 100644
--- a/app/views/admin/deploy_keys/new.html.haml
+++ b/app/views/admin/deploy_keys/new.html.haml
@@ -21,7 +21,7 @@
       .col-sm-10
         = f.label :can_push do
           = f.check_box :can_push
-          %strong Write access allowed?
+          %strong Write access allowed
         %p.light.append-bottom-0
           Allow this key to push to repository as well? (Default only allows pull access.)
 
diff --git a/app/views/projects/deploy_keys/_deploy_key.html.haml b/app/views/projects/deploy_keys/_deploy_key.html.haml
index d360f1bab28..d1e3cb14022 100644
--- a/app/views/projects/deploy_keys/_deploy_key.html.haml
+++ b/app/views/projects/deploy_keys/_deploy_key.html.haml
@@ -7,8 +7,8 @@
     .description
       = deploy_key.fingerprint
     - if deploy_key.can_push?
-      .can-write
-        Can write
+      .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 757260aa5b5..c91bb9c255a 100644
--- a/app/views/projects/deploy_keys/_form.html.haml
+++ b/app/views/projects/deploy_keys/_form.html.haml
@@ -14,7 +14,7 @@
     .checkbox
       = f.label :can_push do
         = f.check_box :can_push
-        %strong Write access allowed?
+        %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.)

From 6269f523f0f47e98d581d62e543b84185255454a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 10 Dec 2016 03:46:50 +0800
Subject: [PATCH 34/38] Fix tests and also add tests for non-existing repo

---
 spec/requests/git_http_spec.rb | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

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
 

From f726dfba6269c68ce17a81a7f973e6b5e23964cb Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Dec 2016 18:47:12 +0800
Subject: [PATCH 35/38] Use btn-inverted for New Deploy Key

---
 app/views/admin/deploy_keys/index.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml
index 0ba8e087e08..646c9815cfe 100644
--- a/app/views/admin/deploy_keys/index.html.haml
+++ b/app/views/admin/deploy_keys/index.html.haml
@@ -3,7 +3,7 @@
   .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"
+      = link_to 'New Deploy Key', new_admin_deploy_key_path, class: "btn btn-new btn-sm btn-inverted"
   - if @deploy_keys.any?
     .table-holder
       %table.table

From ce867db6b8b1b317ebe864d36d50fde5aad787d4 Mon Sep 17 00:00:00 2001
From: Filipa Lacerda <filipa@gitlab.com>
Date: Mon, 12 Dec 2016 16:00:50 +0000
Subject: [PATCH 36/38] Adds CSS to match the mockups and makes table
 responsive

---
 app/assets/stylesheets/pages/admin.scss     | 14 +++++
 app/views/admin/deploy_keys/index.html.haml | 63 +++++++++++----------
 2 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/app/assets/stylesheets/pages/admin.scss b/app/assets/stylesheets/pages/admin.scss
index 44eac21b143..23b69e4d16f 100644
--- a/app/assets/stylesheets/pages/admin.scss
+++ b/app/assets/stylesheets/pages/admin.scss
@@ -166,3 +166,17 @@
     min-width: 120px;
   }
 }
+
+.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/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml
index 646c9815cfe..7b71bb5b287 100644
--- a/app/views/admin/deploy_keys/index.html.haml
+++ b/app/views/admin/deploy_keys/index.html.haml
@@ -1,33 +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 btn-inverted"
-  - 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 Write access allowed
-            %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
-                - 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"
+            %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'

From 884f57c9102416805427d773eb21e09fd30c2452 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 20 Dec 2016 21:19:07 +0800
Subject: [PATCH 37/38] 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 +++++++++++++------------
 spec/lib/gitlab/git_access_spec.rb      |  6 +-
 spec/lib/gitlab/git_access_wiki_spec.rb |  2 +-
 3 files changed, 47 insertions(+), 43 deletions(-)

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)
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 2c90397cc78..44b84afde47 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -50,7 +50,7 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe 'download_access_check' do
+  describe '#check_download_access!' do
     subject { access.check('git-upload-pack', '_any') }
 
     describe 'master permissions' do
@@ -183,7 +183,7 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe 'push_access_check' do
+  describe '#check_push_access!' do
     before { merge_into_protected_branch }
     let(:unprotected_branch) { FFaker::Internet.user_name }
 
@@ -231,7 +231,7 @@ describe Gitlab::GitAccess, lib: true do
 
             permissions_matrix[role].each do |action, allowed|
               context action do
-                subject { access.push_access_check(changes[action]) }
+                subject { access.send(:check_push_access!, changes[action]) }
                 it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
               end
             end
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index 578db51631e..a5d172233cc 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -27,7 +27,7 @@ describe Gitlab::GitAccessWiki, lib: true do
     ['6f6d7e7ed 570e7b2ab refs/heads/master']
   end
 
-  describe '#download_access_check' do
+  describe '#access_check_download!' do
     subject { access.check('git-upload-pack', '_any') }
 
     before do

From c1d11bf57c3091aa4695e302e21c39b9ec723f54 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 20 Dec 2016 23:30:01 +0800
Subject: [PATCH 38/38] Rubocop prefers to indent this way

---
 lib/gitlab/git_access.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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]