Protected Tags backend review changes

Added changelog

25 files changed, 125 insertions, 74 deletions

diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index c2a55c9500a..ba24fa9acfe 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -1,32 +1,20 @@
 class Projects::ProtectedBranchesController < Projects::ProtectedRefsController
   protected
 
-  def protected_ref
-    @protected_branch
-  end
-
-  def protected_ref=(val)
-    @protected_branch = val
-  end
-
-  def matching_refs=(val)
-    @matching_branches = val
-  end
-
   def project_refs
     @project.repository.branches
   end
 
-  def create_service
+  def create_service_class
     ::ProtectedBranches::CreateService
   end
 
-  def update_service
+  def update_service_class
     ::ProtectedBranches::UpdateService
   end
 
   def load_protected_ref
-    self.protected_ref = @project.protected_branches.find(params[:id])
+    @protected_ref = @project.protected_branches.find(params[:id])
   end
 
   def protected_ref_params
diff --git a/app/controllers/projects/protected_refs_controller.rb b/app/controllers/projects/protected_refs_controller.rb
index 63f005124a9..083a70968e5 100644
--- a/app/controllers/projects/protected_refs_controller.rb
+++ b/app/controllers/projects/protected_refs_controller.rb
@@ -1,5 +1,6 @@
 class Projects::ProtectedRefsController < Projects::ApplicationController
   include RepositorySettingsRedirect
+
   # Authorize
   before_action :require_non_empty_project
   before_action :authorize_admin_project!
@@ -12,33 +13,31 @@ class Projects::ProtectedRefsController < Projects::ApplicationController
   end
 
   def create
-    self.protected_ref = create_service.new(@project, current_user, protected_ref_params).execute
+    protected_ref = create_service_class.new(@project, current_user, protected_ref_params).execute
+
     unless protected_ref.persisted?
       flash[:alert] = protected_ref.errors.full_messages.join(', ').html_safe
     end
+
     redirect_to_repository_settings(@project)
   end
 
   def show
-    self.matching_refs = protected_ref.matching(project_refs)
+    @matching_refs = @protected_ref.matching(project_refs)
   end
 
   def update
-    self.protected_ref = update_service.new(@project, current_user, protected_ref_params).execute(protected_ref)
+    @protected_ref = update_service_class.new(@project, current_user, protected_ref_params).execute(@protected_ref)
 
-    if protected_ref.valid?
-      respond_to do |format|
-        format.json { render json: protected_ref, status: :ok }
-      end
+    if @protected_ref.valid?
+      render json: @protected_ref, status: :ok
     else
-      respond_to do |format|
-        format.json { render json: protected_ref.errors, status: :unprocessable_entity }
-      end
+      render json: @protected_ref.errors, status: :unprocessable_entity
     end
   end
 
   def destroy
-    protected_ref.destroy
+    @protected_ref.destroy
 
     respond_to do |format|
       format.html { redirect_to_repository_settings(@project) }
diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb
index 0e00baedbdf..c61ddf145e6 100644
--- a/app/controllers/projects/protected_tags_controller.rb
+++ b/app/controllers/projects/protected_tags_controller.rb
@@ -1,32 +1,20 @@
 class Projects::ProtectedTagsController < Projects::ProtectedRefsController
   protected
 
-  def protected_ref
-    @protected_tag
-  end
-
-  def protected_ref=(val)
-    @protected_tag = val
-  end
-
-  def matching_refs=(val)
-    @matching_tags = val
-  end
-
   def project_refs
     @project.repository.tags
   end
 
-  def create_service
+  def create_service_class
     ::ProtectedTags::CreateService
   end
 
-  def update_service
+  def update_service_class
     ::ProtectedTags::UpdateService
   end
 
   def load_protected_ref
-    self.protected_ref = @project.protected_tags.find(params[:id])
+    @protected_ref = @project.protected_tags.find(params[:id])
   end
 
   def protected_ref_params
diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb
index a852b90c57e..b7a28b1b4a7 100644
--- a/app/helpers/branches_helper.rb
+++ b/app/helpers/branches_helper.rb
@@ -29,4 +29,8 @@ module BranchesHelper
   def project_branches
     options_for_select(@project.repository.branch_names, @project.default_branch)
   end
+
+  def protected_branch?(project, branch)
+    ProtectedBranch.protected?(project, branch.name)
+  end
 end
diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb
index 06cae00249a..c41b807df8a 100644
--- a/app/models/concerns/protected_branch_access.rb
+++ b/app/models/concerns/protected_branch_access.rb
@@ -5,6 +5,7 @@ module ProtectedBranchAccess
     include ProtectedRefAccess
 
     belongs_to :protected_branch
+
     delegate :project, to: :protected_branch
   end
 end
diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index 7c0183952a0..62eaec2407f 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/app/models/concerns/protected_ref.rb
@@ -3,6 +3,7 @@ module ProtectedRef
 
   included do
     belongs_to :project
+
     validates :name, presence: true
     validates :project, presence: true
 
diff --git a/app/models/concerns/protected_tag_access.rb b/app/models/concerns/protected_tag_access.rb
index 9b7d31a6fd5..ee65de24dd8 100644
--- a/app/models/concerns/protected_tag_access.rb
+++ b/app/models/concerns/protected_tag_access.rb
@@ -5,6 +5,7 @@ module ProtectedTagAccess
     include ProtectedRefAccess
 
     belongs_to :protected_tag
+
     delegate :project, to: :protected_tag
   end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 2469e6f8523..2c631050042 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -134,7 +134,7 @@ class Project < ActiveRecord::Base
   has_many :snippets,           dependent: :destroy, class_name: 'ProjectSnippet'
   has_many :hooks,              dependent: :destroy, class_name: 'ProjectHook'
   has_many :protected_branches, dependent: :destroy
-  has_many :protected_tags, dependent: :destroy
+  has_many :protected_tags,     dependent: :destroy
 
   has_many :project_authorizations
   has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User'
diff --git a/app/models/protectable_dropdown.rb b/app/models/protectable_dropdown.rb
index c9b2b213cd2..122fbce257d 100644
--- a/app/models/protectable_dropdown.rb
+++ b/app/models/protectable_dropdown.rb
@@ -6,8 +6,7 @@ class ProtectableDropdown
 
   # Tags/branches which are yet to be individually protected
   def protectable_ref_names
-    non_wildcard_protections = protections.reject(&:wildcard?)
-    refs.map(&:name) - non_wildcard_protections.map(&:name)
+    @protectable_ref_names ||= ref_names - non_wildcard_protected_ref_names
   end
 
   def hash
@@ -20,7 +19,15 @@ class ProtectableDropdown
     @project.repository.public_send(@ref_type)
   end
 
+  def ref_names
+    refs.map(&:name)
+  end
+
   def protections
     @project.public_send("protected_#{@ref_type}")
   end
+
+  def non_wildcard_protected_ref_names
+    protections.reject(&:wildcard?).map(&:name)
+  end
 end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index 89d8ba60134..4b3337a5c9d 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -1,13 +1,10 @@
 module ProtectedBranches
   class UpdateService < BaseService
-    attr_reader :protected_branch
-
     def execute(protected_branch)
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
-      @protected_branch = protected_branch
-      @protected_branch.update(params)
-      @protected_branch
+      protected_branch.update(params)
+      protected_branch
     end
   end
 end
diff --git a/app/services/protected_tags/update_service.rb b/app/services/protected_tags/update_service.rb
index 8a2419efd7b..aea6a48968d 100644
--- a/app/services/protected_tags/update_service.rb
+++ b/app/services/protected_tags/update_service.rb
@@ -1,13 +1,10 @@
 module ProtectedTags
   class UpdateService < BaseService
-    attr_reader :protected_tag
-
     def execute(protected_tag)
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
-      @protected_tag = protected_tag
-      @protected_tag.update(params)
-      @protected_tag
+      protected_tag.update(params)
+      protected_tag
     end
   end
 end
diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml
index d84fa9e55c0..931a5f920d3 100644
--- a/app/views/projects/branches/_branch.html.haml
+++ b/app/views/projects/branches/_branch.html.haml
@@ -15,7 +15,7 @@
       %span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" }
         merged
 
-    - if ProtectedBranch.protected?(@project, branch.name)
+    - if protected_branch?(@project, branch)
       %span.label.label-success
         protected
     .controls.hidden-xs<
diff --git a/app/views/projects/protected_branches/show.html.haml b/app/views/projects/protected_branches/show.html.haml
index 4d8169815b3..f8cfe5e4b11 100644
--- a/app/views/projects/protected_branches/show.html.haml
+++ b/app/views/projects/protected_branches/show.html.haml
@@ -1,13 +1,13 @@
-- page_title @protected_branch.name, "Protected Branches"
+- page_title @protected_ref.name, "Protected Branches"
 
 .row.prepend-top-default.append-bottom-default
   .col-lg-3
     %h4.prepend-top-0
-      = @protected_branch.name
+      = @protected_ref.name
 
   .col-lg-9
     %h5 Matching Branches
-    - if @matching_branches.present?
+    - if @matching_refs.present?
       .table-responsive
         %table.table.protected-branches-list
           %colgroup
@@ -18,7 +18,7 @@
               %th Branch
               %th Last commit
           %tbody
-            - @matching_branches.each do |matching_branch|
+            - @matching_refs.each do |matching_branch|
               = render partial: "matching_branch", object: matching_branch
     - else
       %p.settings-message.text-center
diff --git a/app/views/projects/protected_tags/show.html.haml b/app/views/projects/protected_tags/show.html.haml
index 185807a7e8d..63743f28b3c 100644
--- a/app/views/projects/protected_tags/show.html.haml
+++ b/app/views/projects/protected_tags/show.html.haml
@@ -1,13 +1,13 @@
-- page_title @protected_tag.name, "Protected Tags"
+- page_title @protected_ref.name, "Protected Tags"
 
 .row.prepend-top-default.append-bottom-default
   .col-lg-3
     %h4.prepend-top-0
-      = @protected_tag.name
+      = @protected_ref.name
 
   .col-lg-9
     %h5 Matching Tags
-    - if @matching_tags.present?
+    - if @matching_refs.present?
       .table-responsive
         %table.table.protected-tags-list
           %colgroup
@@ -18,7 +18,7 @@
               %th Tag
               %th Last commit
           %tbody
-            - @matching_tags.each do |matching_tag|
+            - @matching_refs.each do |matching_tag|
               = render partial: "matching_tag", object: matching_tag
     - else
       %p.settings-message.text-center
diff --git a/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml b/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml
new file mode 100644
index 00000000000..c6ea5da65a5
--- /dev/null
+++ b/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml
@@ -0,0 +1,4 @@
+---
+title: Protected Tags feature
+merge_request: 10356
+author:
diff --git a/db/migrate/20170309173138_create_protected_tags.rb b/db/migrate/20170309173138_create_protected_tags.rb
index 538f28479c7..796f3c90344 100644
--- a/db/migrate/20170309173138_create_protected_tags.rb
+++ b/db/migrate/20170309173138_create_protected_tags.rb
@@ -1,7 +1,6 @@
 class CreateProtectedTags < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
-  # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
   GITLAB_ACCESS_MASTER = 40
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 0d96c4d41d7..eb2f2e144fd 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -70,14 +70,8 @@ module Gitlab
 
       def protected_tag_checks
         return unless tag_protected?
-
-        if update?
-          return "Protected tags cannot be updated."
-        end
-
-        if deletion?
-          return "Protected tags cannot be deleted."
-        end
+        return "Protected tags cannot be updated." if update?
+        return "Protected tags cannot be deleted." if deletion?
 
         unless user_access.can_create_tag?(@tag_name)
           return "You are not allowed to create this tag as it is protected."
diff --git a/spec/controllers/projects/protected_branches_controller_spec.rb b/spec/controllers/projects/protected_branches_controller_spec.rb
index e378b5714fe..80be135b5d8 100644
--- a/spec/controllers/projects/protected_branches_controller_spec.rb
+++ b/spec/controllers/projects/protected_branches_controller_spec.rb
@@ -3,6 +3,7 @@ require('spec_helper')
 describe Projects::ProtectedBranchesController do
   describe "GET #index" do
     let(:project) { create(:project_empty_repo, :public) }
+
     it "redirects empty repo to projects page" do
       get(:index, namespace_id: project.namespace.to_param, project_id: project)
     end
diff --git a/spec/controllers/projects/protected_tags_controller_spec.rb b/spec/controllers/projects/protected_tags_controller_spec.rb
index ac802981294..64658988b3f 100644
--- a/spec/controllers/projects/protected_tags_controller_spec.rb
+++ b/spec/controllers/projects/protected_tags_controller_spec.rb
@@ -3,6 +3,7 @@ require('spec_helper')
 describe Projects::ProtectedTagsController do
   describe "GET #index" do
     let(:project) { create(:project_empty_repo, :public) }
+
     it "redirects empty repo to projects page" do
       get(:index, namespace_id: project.namespace.to_param, project_id: project)
     end
diff --git a/spec/features/protected_branches/access_control_ce_spec.rb b/spec/features/protected_branches/access_control_ce_spec.rb
index e4aca25a339..eb3cea775da 100644
--- a/spec/features/protected_branches/access_control_ce_spec.rb
+++ b/spec/features/protected_branches/access_control_ce_spec.rb
@@ -2,7 +2,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
   ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
     it "allows creating protected branches that #{access_type_name} can push to" do
       visit namespace_project_protected_branches_path(project.namespace, project)
+
       set_protected_branch_name('master')
+
       within('.new_protected_branch') do
         allowed_to_push_button = find(".js-allowed-to-push")
 
@@ -11,6 +13,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
           within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
       end
+
       click_on "Protect"
 
       expect(ProtectedBranch.count).to eq(1)
@@ -19,7 +22,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
 
     it "allows updating protected branches so that #{access_type_name} can push to them" do
       visit namespace_project_protected_branches_path(project.namespace, project)
+
       set_protected_branch_name('master')
+
       click_on "Protect"
 
       expect(ProtectedBranch.count).to eq(1)
@@ -34,6 +39,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
       end
 
       wait_for_ajax
+
       expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
     end
   end
@@ -41,7 +47,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
   ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
     it "allows creating protected branches that #{access_type_name} can merge to" do
       visit namespace_project_protected_branches_path(project.namespace, project)
+
       set_protected_branch_name('master')
+
       within('.new_protected_branch') do
         allowed_to_merge_button = find(".js-allowed-to-merge")
 
@@ -50,6 +58,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
           within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
       end
+
       click_on "Protect"
 
       expect(ProtectedBranch.count).to eq(1)
@@ -58,7 +67,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
 
     it "allows updating protected branches so that #{access_type_name} can merge to them" do
       visit namespace_project_protected_branches_path(project.namespace, project)
+
       set_protected_branch_name('master')
+
       click_on "Protect"
 
       expect(ProtectedBranch.count).to eq(1)
@@ -73,6 +84,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
       end
 
       wait_for_ajax
+
       expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to include(access_type_id)
     end
   end
diff --git a/spec/features/protected_tags/access_control_ce_spec.rb b/spec/features/protected_tags/access_control_ce_spec.rb
index 33a07786007..de2556041e7 100644
--- a/spec/features/protected_tags/access_control_ce_spec.rb
+++ b/spec/features/protected_tags/access_control_ce_spec.rb
@@ -2,7 +2,9 @@ RSpec.shared_examples "protected tags > access control > CE" do
   ProtectedTag::CreateAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
     it "allows creating protected tags that #{access_type_name} can create" do
       visit namespace_project_protected_tags_path(project.namespace, project)
+
       set_protected_tag_name('master')
+
       within('.new_protected_tag') do
         allowed_to_create_button = find(".js-allowed-to-create")
 
@@ -11,6 +13,7 @@ RSpec.shared_examples "protected tags > access control > CE" do
           within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
       end
+
       click_on "Protect"
 
       expect(ProtectedTag.count).to eq(1)
@@ -19,7 +22,9 @@ RSpec.shared_examples "protected tags > access control > CE" do
 
     it "allows updating protected tags so that #{access_type_name} can create them" do
       visit namespace_project_protected_tags_path(project.namespace, project)
+
       set_protected_tag_name('master')
+
       click_on "Protect"
 
       expect(ProtectedTag.count).to eq(1)
@@ -34,6 +39,7 @@ RSpec.shared_examples "protected tags > access control > CE" do
       end
 
       wait_for_ajax
+
       expect(ProtectedTag.last.create_access_levels.map(&:access_level)).to include(access_type_id)
     end
   end
diff --git a/spec/models/protectable_dropdown_spec.rb b/spec/models/protectable_dropdown_spec.rb
index 7f8ef7195e5..4c9bade592b 100644
--- a/spec/models/protectable_dropdown_spec.rb
+++ b/spec/models/protectable_dropdown_spec.rb
@@ -18,6 +18,7 @@ describe ProtectableDropdown, models: true do
       create(:protected_branch, name: 'feat*', project: project)
 
       subject = described_class.new(project.reload, :branches)
+
       expect(subject.protectable_ref_names).to include('feature')
     end
   end
diff --git a/spec/models/protected_tag_spec.rb b/spec/models/protected_tag_spec.rb
index 05ad532935a..51353852a93 100644
--- a/spec/models/protected_tag_spec.rb
+++ b/spec/models/protected_tag_spec.rb
@@ -1,8 +1,6 @@
 require 'spec_helper'
 
 describe ProtectedTag, models: true do
-  subject { build_stubbed(:protected_branch) }
-
   describe 'Associations' do
     it { is_expected.to belong_to(:project) }
   end
diff --git a/spec/services/protected_branches/update_service_spec.rb b/spec/services/protected_branches/update_service_spec.rb
new file mode 100644
index 00000000000..62bdd49a4d7
--- /dev/null
+++ b/spec/services/protected_branches/update_service_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe ProtectedBranches::UpdateService, services: true do
+  let(:protected_branch) { create(:protected_branch) }
+  let(:project) { protected_branch.project }
+  let(:user) { project.owner }
+  let(:params) { { name: 'new protected branch name' } }
+
+  describe '#execute' do
+    subject(:service) { described_class.new(project, user, params) }
+
+    it 'updates a protected branch' do
+      result = service.execute(protected_branch)
+
+      expect(result.reload.name).to eq(params[:name])
+    end
+
+    context 'without admin_project permissions' do
+      let(:user) { create(:user) }
+
+      it "raises error" do
+        expect{ service.execute(protected_branch) }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
+  end
+end
diff --git a/spec/services/protected_tags/update_service_spec.rb b/spec/services/protected_tags/update_service_spec.rb
new file mode 100644
index 00000000000..e78fde4c48d
--- /dev/null
+++ b/spec/services/protected_tags/update_service_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe ProtectedTags::UpdateService, services: true do
+  let(:protected_tag) { create(:protected_tag) }
+  let(:project) { protected_tag.project }
+  let(:user) { project.owner }
+  let(:params) { { name: 'new protected tag name' } }
+
+  describe '#execute' do
+    subject(:service) { described_class.new(project, user, params) }
+
+    it 'updates a protected tag' do
+      result = service.execute(protected_tag)
+
+      expect(result.reload.name).to eq(params[:name])
+    end
+
+    context 'without admin_project permissions' do
+      let(:user) { create(:user) }
+
+      it "raises error" do
+        expect{ service.execute(protected_tag) }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
+  end
+end