From f16377e7dc762462817dd0b34e36811c55988b10 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 5 Apr 2017 18:59:46 +0100 Subject: Protected Tags backend review changes Added changelog --- .../projects/protected_branches_controller.rb | 18 +++------------ .../projects/protected_refs_controller.rb | 21 +++++++++-------- .../projects/protected_tags_controller.rb | 18 +++------------ app/helpers/branches_helper.rb | 4 ++++ app/models/concerns/protected_branch_access.rb | 1 + app/models/concerns/protected_ref.rb | 1 + app/models/concerns/protected_tag_access.rb | 1 + app/models/project.rb | 2 +- app/models/protectable_dropdown.rb | 11 +++++++-- app/services/protected_branches/update_service.rb | 7 ++---- app/services/protected_tags/update_service.rb | 7 ++---- app/views/projects/branches/_branch.html.haml | 2 +- .../projects/protected_branches/show.html.haml | 8 +++---- app/views/projects/protected_tags/show.html.haml | 8 +++---- .../18471-restrict-tag-pushes-protected-tags.yml | 4 ++++ db/migrate/20170309173138_create_protected_tags.rb | 1 - lib/gitlab/checks/change_access.rb | 10 ++------- .../projects/protected_branches_controller_spec.rb | 1 + .../projects/protected_tags_controller_spec.rb | 1 + .../protected_branches/access_control_ce_spec.rb | 12 ++++++++++ .../protected_tags/access_control_ce_spec.rb | 6 +++++ spec/models/protectable_dropdown_spec.rb | 1 + spec/models/protected_tag_spec.rb | 2 -- .../protected_branches/update_service_spec.rb | 26 ++++++++++++++++++++++ .../services/protected_tags/update_service_spec.rb | 26 ++++++++++++++++++++++ 25 files changed, 125 insertions(+), 74 deletions(-) create mode 100644 changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml create mode 100644 spec/services/protected_branches/update_service_spec.rb create mode 100644 spec/services/protected_tags/update_service_spec.rb 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 -- cgit v1.2.3