Merge branch '3191-deploy-keys-update' into 'master'

Implement ability to update deploy keys

Closes #3191

See merge request !10383

8 files changed, 292 insertions, 61 deletions

diff --git a/spec/features/admin/admin_deploy_keys_spec.rb b/spec/features/admin/admin_deploy_keys_spec.rb
index c0b6995a84a..5f5fa4e932a 100644
--- a/spec/features/admin/admin_deploy_keys_spec.rb
+++ b/spec/features/admin/admin_deploy_keys_spec.rb
@@ -11,40 +11,67 @@ RSpec.describe 'admin deploy keys', type: :feature do
   it 'show all public deploy keys' do
     visit admin_deploy_keys_path
 
-    expect(page).to have_content(deploy_key.title)
-    expect(page).to have_content(another_deploy_key.title)
+    page.within(find('.deploy-keys-list', match: :first)) do
+      expect(page).to have_content(deploy_key.title)
+      expect(page).to have_content(another_deploy_key.title)
+    end
   end
 
-  describe 'create new deploy key' do
+  describe 'create a new deploy key' do
+    let(:new_ssh_key) { attributes_for(:key)[:key] }
+
     before do
       visit admin_deploy_keys_path
       click_link 'New deploy key'
     end
 
-    it 'creates new deploy key' do
-      fill_deploy_key
+    it 'creates a new deploy key' do
+      fill_in 'deploy_key_title', with: 'laptop'
+      fill_in 'deploy_key_key', with: new_ssh_key
+      check 'deploy_key_can_push'
       click_button 'Create'
 
-      expect_renders_new_key
-    end
+      expect(current_path).to eq admin_deploy_keys_path
 
-    it 'creates new deploy key with write access' do
-      fill_deploy_key
-      check "deploy_key_can_push"
-      click_button "Create"
+      page.within(find('.deploy-keys-list', match: :first)) do
+        expect(page).to have_content('laptop')
+        expect(page).to have_content('Yes')
+      end
+    end
+  end
 
-      expect_renders_new_key
-      expect(page).to have_content('Yes')
+  describe 'update an existing deploy key' do
+    before do
+      visit admin_deploy_keys_path
+      find('tr', text: deploy_key.title).click_link('Edit')
     end
 
-    def expect_renders_new_key
+    it 'updates an existing deploy key' do
+      fill_in 'deploy_key_title', with: 'new-title'
+      check 'deploy_key_can_push'
+      click_button 'Save changes'
+
       expect(current_path).to eq admin_deploy_keys_path
-      expect(page).to have_content('laptop')
+
+      page.within(find('.deploy-keys-list', match: :first)) do
+        expect(page).to have_content('new-title')
+        expect(page).to have_content('Yes')
+      end
     end
+  end
 
-    def fill_deploy_key
-      fill_in 'deploy_key_title', with: 'laptop'
-      fill_in 'deploy_key_key', with: 'ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop'
+  describe 'remove an existing deploy key' do
+    before do
+      visit admin_deploy_keys_path
+    end
+
+    it 'removes an existing deploy key' do
+      find('tr', text: deploy_key.title).click_link('Remove')
+
+      expect(current_path).to eq admin_deploy_keys_path
+      page.within(find('.deploy-keys-list', match: :first)) do
+        expect(page).not_to have_content(deploy_key.title)
+      end
     end
   end
 end
diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb
new file mode 100644
index 00000000000..4cc38c5286e
--- /dev/null
+++ b/spec/features/projects/settings/repository_settings_spec.rb
@@ -0,0 +1,78 @@
+require 'spec_helper'
+
+feature 'Repository settings', feature: true do
+  let(:project) { create(:project_empty_repo) }
+  let(:user) { create(:user) }
+  let(:role) { :developer }
+
+  background do
+    project.team << [user, role]
+    login_as(user)
+  end
+
+  context 'for developer' do
+    given(:role) { :developer }
+
+    scenario 'is not allowed to view' do
+      visit namespace_project_settings_repository_path(project.namespace, project)
+
+      expect(page.status_code).to eq(404)
+    end
+  end
+
+  context 'for master' do
+    given(:role) { :master }
+
+    context 'Deploy Keys', js: true do
+      let(:private_deploy_key) { create(:deploy_key, title: 'private_deploy_key', public: false) }
+      let(:public_deploy_key) { create(:another_deploy_key, title: 'public_deploy_key', public: true) }
+      let(:new_ssh_key) { attributes_for(:key)[:key] }
+
+      scenario 'get list of keys' do
+        project.deploy_keys << private_deploy_key
+        project.deploy_keys << public_deploy_key
+
+        visit namespace_project_settings_repository_path(project.namespace, project)
+
+        expect(page.status_code).to eq(200)
+        expect(page).to have_content('private_deploy_key')
+        expect(page).to have_content('public_deploy_key')
+      end
+
+      scenario 'add a new deploy key' do
+        visit namespace_project_settings_repository_path(project.namespace, project)
+
+        fill_in 'deploy_key_title', with: 'new_deploy_key'
+        fill_in 'deploy_key_key', with: new_ssh_key
+        check 'deploy_key_can_push'
+        click_button 'Add key'
+
+        expect(page).to have_content('new_deploy_key')
+        expect(page).to have_content('Write access allowed')
+      end
+
+      scenario 'edit an existing deploy key' do
+        project.deploy_keys << private_deploy_key
+        visit namespace_project_settings_repository_path(project.namespace, project)
+
+        find('li', text: private_deploy_key.title).click_link('Edit')
+
+        fill_in 'deploy_key_title', with: 'updated_deploy_key'
+        check 'deploy_key_can_push'
+        click_button 'Save changes'
+
+        expect(page).to have_content('updated_deploy_key')
+        expect(page).to have_content('Write access allowed')
+      end
+
+      scenario 'remove an existing deploy key' do
+        project.deploy_keys << private_deploy_key
+        visit namespace_project_settings_repository_path(project.namespace, project)
+
+        find('li', text: private_deploy_key.title).click_button('Remove')
+
+        expect(page).not_to have_content(private_deploy_key.title)
+      end
+    end
+  end
+end
diff --git a/spec/javascripts/deploy_keys/components/key_spec.js b/spec/javascripts/deploy_keys/components/key_spec.js
index 793ab8c451d..a4b98f6140d 100644
--- a/spec/javascripts/deploy_keys/components/key_spec.js
+++ b/spec/javascripts/deploy_keys/components/key_spec.js
@@ -39,9 +39,15 @@ describe('Deploy keys key', () => {
       ).toBe(`created ${gl.utils.getTimeago().format(deployKey.created_at)}`);
     });
 
+    it('shows edit button', () => {
+      expect(
+        vm.$el.querySelectorAll('.btn')[0].textContent.trim(),
+      ).toBe('Edit');
+    });
+
     it('shows remove button', () => {
       expect(
-        vm.$el.querySelector('.btn').textContent.trim(),
+        vm.$el.querySelectorAll('.btn')[1].textContent.trim(),
       ).toBe('Remove');
     });
 
@@ -71,9 +77,15 @@ describe('Deploy keys key', () => {
       setTimeout(done);
     });
 
+    it('shows edit button', () => {
+      expect(
+        vm.$el.querySelectorAll('.btn')[0].textContent.trim(),
+      ).toBe('Edit');
+    });
+
     it('shows enable button', () => {
       expect(
-        vm.$el.querySelector('.btn').textContent.trim(),
+        vm.$el.querySelectorAll('.btn')[1].textContent.trim(),
       ).toBe('Enable');
     });
 
@@ -82,7 +94,7 @@ describe('Deploy keys key', () => {
 
       Vue.nextTick(() => {
         expect(
-          vm.$el.querySelector('.btn').textContent.trim(),
+          vm.$el.querySelectorAll('.btn')[1].textContent.trim(),
         ).toBe('Disable');
 
         done();
diff --git a/spec/policies/deploy_key_policy_spec.rb b/spec/policies/deploy_key_policy_spec.rb
new file mode 100644
index 00000000000..28e10f0bfe2
--- /dev/null
+++ b/spec/policies/deploy_key_policy_spec.rb
@@ -0,0 +1,56 @@
+require 'spec_helper'
+
+describe DeployKeyPolicy, models: true do
+  subject { described_class.abilities(current_user, deploy_key).to_set }
+
+  describe 'updating a deploy_key' do
+    context 'when a regular user' do
+      let(:current_user) { create(:user) }
+
+      context 'tries to update private deploy key attached to project' do
+        let(:deploy_key) { create(:deploy_key, public: false) }
+        let(:project) { create(:project_empty_repo) }
+
+        before do
+          project.add_master(current_user)
+          project.deploy_keys << deploy_key
+        end
+
+        it { is_expected.to include(:update_deploy_key) }
+      end
+
+      context 'tries to update private deploy key attached to other project' do
+        let(:deploy_key) { create(:deploy_key, public: false) }
+        let(:other_project) { create(:project_empty_repo) }
+
+        before do
+          other_project.deploy_keys << deploy_key
+        end
+
+        it { is_expected.not_to include(:update_deploy_key) }
+      end
+
+      context 'tries to update public deploy key' do
+        let(:deploy_key) { create(:another_deploy_key, public: true) }
+
+        it { is_expected.not_to include(:update_deploy_key) }
+      end
+    end
+
+    context 'when an admin user' do
+      let(:current_user) { create(:user, :admin) }
+
+      context ' tries to update private deploy key' do
+        let(:deploy_key) { create(:deploy_key, public: false) }
+
+        it { is_expected.to include(:update_deploy_key) }
+      end
+
+      context 'when an admin user tries to update public deploy key' do
+        let(:deploy_key) { create(:another_deploy_key, public: true) }
+
+        it { is_expected.to include(:update_deploy_key) }
+      end
+    end
+  end
+end
diff --git a/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb b/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb
index 6443f86b6a1..5c39e1b5f96 100644
--- a/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb
+++ b/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb
@@ -51,10 +51,6 @@ describe Projects::Settings::DeployKeysPresenter do
       expect(presenter.available_project_keys).not_to be_empty
     end
 
-    it 'returns false if any available_project_keys are enabled' do
-      expect(presenter.any_available_project_keys_enabled?).to eq(true)
-    end
-
     it 'returns the available_project_keys size' do
       expect(presenter.available_project_keys_size).to eq(1)
     end
diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb
index 843e9862b0c..4d9cd5f3a27 100644
--- a/spec/requests/api/deploy_keys_spec.rb
+++ b/spec/requests/api/deploy_keys_spec.rb
@@ -13,7 +13,7 @@ describe API::DeployKeys do
 
   describe 'GET /deploy_keys' do
     context 'when unauthenticated' do
-      it 'should return authentication error' do
+      it 'returns authentication error' do
         get api('/deploy_keys')
 
         expect(response.status).to eq(401)
@@ -21,7 +21,7 @@ describe API::DeployKeys do
     end
 
     context 'when authenticated as non-admin user' do
-      it 'should return a 403 error' do
+      it 'returns a 403 error' do
         get api('/deploy_keys', user)
 
         expect(response.status).to eq(403)
@@ -29,7 +29,7 @@ describe API::DeployKeys do
     end
 
     context 'when authenticated as admin' do
-      it 'should return all deploy keys' do
+      it 'returns all deploy keys' do
         get api('/deploy_keys', admin)
 
         expect(response.status).to eq(200)
@@ -43,7 +43,7 @@ describe API::DeployKeys do
   describe 'GET /projects/:id/deploy_keys' do
     before { deploy_key }
 
-    it 'should return array of ssh keys' do
+    it 'returns array of ssh keys' do
       get api("/projects/#{project.id}/deploy_keys", admin)
 
       expect(response).to have_http_status(200)
@@ -54,14 +54,14 @@ describe API::DeployKeys do
   end
 
   describe 'GET /projects/:id/deploy_keys/:key_id' do
-    it 'should return a single key' do
+    it 'returns a single key' do
       get api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
 
       expect(response).to have_http_status(200)
       expect(json_response['title']).to eq(deploy_key.title)
     end
 
-    it 'should return 404 Not Found with invalid ID' do
+    it 'returns 404 Not Found with invalid ID' do
       get api("/projects/#{project.id}/deploy_keys/404", admin)
 
       expect(response).to have_http_status(404)
@@ -69,26 +69,26 @@ describe API::DeployKeys do
   end
 
   describe 'POST /projects/:id/deploy_keys' do
-    it 'should not create an invalid ssh key' do
+    it 'does not create an invalid ssh key' do
       post api("/projects/#{project.id}/deploy_keys", admin), { title: 'invalid key' }
 
       expect(response).to have_http_status(400)
       expect(json_response['error']).to eq('key is missing')
     end
 
-    it 'should not create a key without title' do
+    it 'does not create a key without title' do
       post api("/projects/#{project.id}/deploy_keys", admin), key: 'some key'
 
       expect(response).to have_http_status(400)
       expect(json_response['error']).to eq('title is missing')
     end
 
-    it 'should create new ssh key' do
+    it 'creates new ssh key' do
       key_attrs = attributes_for :another_key
 
       expect do
         post api("/projects/#{project.id}/deploy_keys", admin), key_attrs
-      end.to change{ project.deploy_keys.count }.by(1)
+      end.to change { project.deploy_keys.count }.by(1)
     end
 
     it 'returns an existing ssh key when attempting to add a duplicate' do
@@ -117,10 +117,53 @@ describe API::DeployKeys do
     end
   end
 
+  describe 'PUT /projects/:id/deploy_keys/:key_id' do
+    let(:private_deploy_key) { create(:another_deploy_key, public: false) }
+    let(:project_private_deploy_key) do
+      create(:deploy_keys_project, project: project, deploy_key: private_deploy_key)
+    end
+
+    it 'updates a public deploy key as admin' do
+      expect do
+        put api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin), { title: 'new title' }
+      end.not_to change(deploy_key, :title)
+
+      expect(response).to have_http_status(200)
+    end
+
+    it 'does not update a public deploy key as non admin' do
+      expect do
+        put api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", user), { title: 'new title' }
+      end.not_to change(deploy_key, :title)
+
+      expect(response).to have_http_status(404)
+    end
+
+    it 'does not update a private key with invalid title' do
+      project_private_deploy_key
+
+      expect do
+        put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), { title: '' }
+      end.not_to change(deploy_key, :title)
+
+      expect(response).to have_http_status(400)
+    end
+
+    it 'updates a private ssh key with correct attributes' do
+      project_private_deploy_key
+
+      put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), { title: 'new title', can_push: true }
+
+      expect(json_response['id']).to eq(private_deploy_key.id)
+      expect(json_response['title']).to eq('new title')
+      expect(json_response['can_push']).to eq(true)
+    end
+  end
+
   describe 'DELETE /projects/:id/deploy_keys/:key_id' do
     before { deploy_key }
 
-    it 'should delete existing key' do
+    it 'deletes existing key' do
       expect do
         delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
 
@@ -128,7 +171,7 @@ describe API::DeployKeys do
       end.to change{ project.deploy_keys.count }.by(-1)
     end
 
-    it 'should return 404 Not Found with invalid ID' do
+    it 'returns 404 Not Found with invalid ID' do
       delete api("/projects/#{project.id}/deploy_keys/404", admin)
 
       expect(response).to have_http_status(404)
@@ -150,7 +193,7 @@ describe API::DeployKeys do
     end
 
     context 'when authenticated as non-admin user' do
-      it 'should return a 404 error' do
+      it 'returns a 404 error' do
         post api("/projects/#{project2.id}/deploy_keys/#{deploy_key.id}/enable", user)
 
         expect(response).to have_http_status(404)
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index 54417f6b3e1..0a6778ae2ef 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -201,10 +201,12 @@ describe 'project routing' do
   #                         POST   /:project_id/deploy_keys(.:format)          deploy_keys#create
   #  new_project_deploy_key GET    /:project_id/deploy_keys/new(.:format)      deploy_keys#new
   #      project_deploy_key GET    /:project_id/deploy_keys/:id(.:format)      deploy_keys#show
+  # edit_project_deploy_key GET    /:project_id/deploy_keys/:id/edit(.:format) deploy_keys#edit
+  #      project_deploy_key PATCH  /:project_id/deploy_keys/:id(.:format)      deploy_keys#update
   #                         DELETE /:project_id/deploy_keys/:id(.:format)      deploy_keys#destroy
   describe Projects::DeployKeysController, 'routing' do
     it_behaves_like 'RESTful project resources' do
-      let(:actions)    { [:index, :new, :create] }
+      let(:actions)    { [:index, :new, :create, :edit, :update] }
       let(:controller) { 'deploy_keys' }
     end
   end
diff --git a/spec/serializers/deploy_key_entity_spec.rb b/spec/serializers/deploy_key_entity_spec.rb
index e73fbe190ca..ed89fccc3d0 100644
--- a/spec/serializers/deploy_key_entity_spec.rb
+++ b/spec/serializers/deploy_key_entity_spec.rb
@@ -12,27 +12,44 @@ describe DeployKeyEntity do
 
   let(:entity) { described_class.new(deploy_key, user: user) }
 
-  it 'returns deploy keys with projects a user can read' do
-    expected_result = {
-      id: deploy_key.id,
-      user_id: deploy_key.user_id,
-      title: deploy_key.title,
-      fingerprint: deploy_key.fingerprint,
-      can_push: deploy_key.can_push,
-      destroyed_when_orphaned: true,
-      almost_orphaned: false,
-      created_at: deploy_key.created_at,
-      updated_at: deploy_key.updated_at,
-      projects: [
-        {
-          id: project.id,
-          name: project.name,
-          full_path: namespace_project_path(project.namespace, project),
-          full_name: project.full_name
-        }
-      ]
-    }
-
-    expect(entity.as_json).to eq(expected_result)
+  describe 'returns deploy keys with projects a user can read' do
+    let(:expected_result) do
+      {
+        id: deploy_key.id,
+        user_id: deploy_key.user_id,
+        title: deploy_key.title,
+        fingerprint: deploy_key.fingerprint,
+        can_push: deploy_key.can_push,
+        destroyed_when_orphaned: true,
+        almost_orphaned: false,
+        created_at: deploy_key.created_at,
+        updated_at: deploy_key.updated_at,
+        can_edit: false,
+        projects: [
+          {
+            id: project.id,
+            name: project.name,
+            full_path: namespace_project_path(project.namespace, project),
+            full_name: project.full_name
+          }
+        ]
+      }
+    end
+
+    it { expect(entity.as_json).to eq(expected_result) }
+  end
+
+  describe 'returns can_edit true if user is a master of project' do
+    before do
+      project.add_master(user)
+    end
+
+    it { expect(entity.as_json).to include(can_edit: true) }
+  end
+
+  describe 'returns can_edit true if a user admin' do
+    let(:user) { create(:user, :admin) }
+
+    it { expect(entity.as_json).to include(can_edit: true) }
   end
 end