From f16377e7dc762462817dd0b34e36811c55988b10 Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Wed, 5 Apr 2017 18:59:46 +0100
Subject: [PATCH] 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 +++
.../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 ++++++--
.../protected_branches/update_service.rb | 7 ++---
app/services/protected_tags/update_service.rb | 7 ++---
app/views/projects/branches/_branch.html.haml | 2 +-
.../protected_branches/show.html.haml | 8 +++---
.../projects/protected_tags/show.html.haml | 8 +++---
...471-restrict-tag-pushes-protected-tags.yml | 4 +++
.../20170309173138_create_protected_tags.rb | 1 -
lib/gitlab/checks/change_access.rb | 10 ++-----
.../protected_branches_controller_spec.rb | 1 +
.../protected_tags_controller_spec.rb | 1 +
.../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 +++++++++++++++++++
.../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
--
GitLab