diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index c2a55c9500a8de5b93143f7c1a8cd88004a24eb8..ba24fa9acfe0a66af80d7f52083513b897a693fc 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 63f005124a99f3a800c124d9a561076f6d303fa3..083a70968e55ad5fa378840d418ee057d150d8e6 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 0e00baedbdf31d020335623de490b264cc6c5a2e..c61ddf145e6fd5bb253160cb3cf048c865a31006 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 a852b90c57e087e07c87bf7a5b60e811ae19541c..b7a28b1b4a7098b82bf9ed88f8a5159de11f5c1d 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 06cae00249a1dd33dd6d1c58b14a92a1cae891b7..c41b807df8a2dbf66756f1b455569d79b4781de2 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 7c0183952a079fe9191f00282dbff5dd25d4d80c..62eaec2407f50fb020614715ecfae639ae8a8dbc 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 9b7d31a6fd5c11356dc90aede176f3edc03c3bba..ee65de24dd8aeec902134e6597422e8429bff8b5 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 2469e6f8523fe862029ed114c810af121b619b2e..2c631050042f8a56baa99f5b372313995cbc4334 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 c9b2b213cd2e08ea84e6110b09214ccac0c588e9..122fbce257db02481441f5556ea32f4988399658 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 89d8ba601345f7cea3f7921aea14da7e1428c19b..4b3337a5c9d940becec71b3ce7879c0125a09381 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 8a2419efd7bc9c293ddcaf37fdf2746d2a52089c..aea6a48968d30302b4d927ade03b868e764cbf7e 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 d84fa9e55c0110aadaaea08d796a584f64c123ca..931a5f920d36e9d9a603a6b0b13b5aab5496bc34 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 4d8169815b3916564fdd0655a97db1f50aef34b3..f8cfe5e4b11b757fefb6d0ae42c8d24cda43b880 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 185807a7e8d1a5b4fc384f5286c99e02a4d89273..63743f28b3cee7fcb33b1371c87729b7f01097a9 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 0000000000000000000000000000000000000000..c6ea5da65a54dfdfe1700094e5c1bc8448d63470
--- /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 538f28479c741ae13b1ab1e939740e26cc054adb..796f3c903443b1009c1e23b2940896d7d4c939a5 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 0d96c4d41d72a8e965d2cfe46f79531099a90975..eb2f2e144fde84767a8eb44f5b0fa75ba6f81c66 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 e378b5714fef7ec953aaa40b43f73997397e2e28..80be135b5d8b930b52252933d6ad704cb7529929 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 ac802981294c88a80043f792a2441fee438558eb..64658988b3f37d3e953c9a91c6dfb69f06112c72 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 e4aca25a3390ce489d3ae41a32db4e964e025f7f..eb3cea775da835b69ed68992e17278ea60f4aaf3 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 33a077860071f6f756de49746481ff3d1dddd058..de2556041e7dbbd4c7145defcea8522a6fba4ceb 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 7f8ef7195e58386435560f25e7ca2b6cf05c5f1d..4c9bade592b436a025b178063eaad8f5c218219b 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 05ad532935ac1c69a5f1acbd21bc9310e11fadde..51353852a932fbd2ed24efaf64dd05829dcec4fb 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 0000000000000000000000000000000000000000..62bdd49a4d7404d2d5b0c76be621e682864c075c
--- /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 0000000000000000000000000000000000000000..e78fde4c48d13cbb598b06883eb40d8b10c8e4c6
--- /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