From 3f9ab9bc91d9e6acab651cb842d35a2477a2bade Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 20 Mar 2017 18:40:28 +0530 Subject: Protect tag partial --- .../protected_tags/_create_protected_tag.html.haml | 32 ++++++++++++++++++++++ .../projects/protected_tags/_dropdown.html.haml | 15 ++++++++++ app/views/projects/protected_tags/_index.html.haml | 18 ++++++++++++ .../projects/protected_tags/_tags_list.html.haml | 6 ++++ 4 files changed, 71 insertions(+) create mode 100644 app/views/projects/protected_tags/_create_protected_tag.html.haml create mode 100644 app/views/projects/protected_tags/_dropdown.html.haml create mode 100644 app/views/projects/protected_tags/_index.html.haml create mode 100644 app/views/projects/protected_tags/_tags_list.html.haml diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml new file mode 100644 index 00000000000..6ffe7b4c24f --- /dev/null +++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml @@ -0,0 +1,32 @@ += form_for [@project.namespace.becomes(Namespace), @project, @protected_branch] do |f| + .panel.panel-default + .panel-heading + %h3.panel-title + Protect a tag + .panel-body + .form-horizontal + = form_errors(@protected_branch) + .form-group + = f.label :name, class: 'col-md-2 text-right' do + Tag: + .col-md-10 + = render partial: "projects/protected_tags/dropdown", locals: { f: f } + .help-block + = link_to 'Wildcards', help_page_path('user/project/protected_branches', anchor: 'wildcard-protected-branches') + such as + %code *-stable + or + %code production/* + are supported + .form-group + %label.col-md-2.text-right{ for: 'push_access_levels_attributes' } + Allowed to push: + .col-md-10 + .push_access_levels-container + = dropdown_tag('Select', + options: { toggle_class: 'js-allowed-to-push wide', + dropdown_class: 'dropdown-menu-selectable', + data: { field_name: 'protected_branch[push_access_levels_attributes][0][access_level]', input_id: 'push_access_levels_attributes' }}) + + .panel-footer + = f.submit 'Protect', class: 'btn-create btn', disabled: true diff --git a/app/views/projects/protected_tags/_dropdown.html.haml b/app/views/projects/protected_tags/_dropdown.html.haml new file mode 100644 index 00000000000..3da153bc521 --- /dev/null +++ b/app/views/projects/protected_tags/_dropdown.html.haml @@ -0,0 +1,15 @@ += f.hidden_field(:name) + += dropdown_tag('Select tag or create wildcard', + options: { toggle_class: 'js-protected-tag-select js-filter-submit wide', + filter: true, dropdown_class: "dropdown-menu-selectable", placeholder: "Search protected tag", + footer_content: true, + data: { show_no: true, show_any: true, show_upcoming: true, + selected: params[:protected_branch_name], + project_id: @project.try(:id) } }) do + + %ul.dropdown-footer-list + %li + = link_to '#', title: "New Protected Tag", class: "create-new-protected-tag" do + Create wildcard + %code diff --git a/app/views/projects/protected_tags/_index.html.haml b/app/views/projects/protected_tags/_index.html.haml new file mode 100644 index 00000000000..3a1da1202e9 --- /dev/null +++ b/app/views/projects/protected_tags/_index.html.haml @@ -0,0 +1,18 @@ +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('protected_branches') + +.row.prepend-top-default.append-bottom-default + .col-lg-3 + %h4.prepend-top-0 + Protected tags + %p.prepend-top-20 + By default, Protected branches are designed to: + %ul + %li Prevent tag pushes from everybody except Masters + %li Prevent anyone from force pushing to the tag + %li Prevent anyone from deleting the tag + .col-lg-9 + - if can? current_user, :admin_project, @project + = render 'projects/protected_tags/create_protected_tag' + + = render "projects/protected_tags/tags_list" diff --git a/app/views/projects/protected_tags/_tags_list.html.haml b/app/views/projects/protected_tags/_tags_list.html.haml new file mode 100644 index 00000000000..e7ce90393e9 --- /dev/null +++ b/app/views/projects/protected_tags/_tags_list.html.haml @@ -0,0 +1,6 @@ +.panel.panel-default.protected-tags-list + .panel-heading + %h3.panel-title + Protected tag (0) + %p.settings-message.text-center + There are currently no protected tags, protect a tag with the form above. -- cgit v1.2.1 From 421a386d8c76d9947bdfc007b890c853313f0c46 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 20 Mar 2017 18:40:55 +0530 Subject: Apply styles to protected tag same as protected branch --- app/assets/stylesheets/pages/projects.scss | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index efa47be9a73..f7ef9275560 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -745,7 +745,8 @@ pre.light-well { } } -.protected-branches-list { +.protected-branches-list, +.protected-tags-list { margin-bottom: 30px; a { -- cgit v1.2.1 From d20c5427af94c7043c3cc7c04288a056921b812c Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 20 Mar 2017 18:41:19 +0530 Subject: Render protected tags within Project Settings > Repository --- app/views/projects/settings/repository/show.html.haml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/projects/settings/repository/show.html.haml b/app/views/projects/settings/repository/show.html.haml index 4c02302e161..5402320cb66 100644 --- a/app/views/projects/settings/repository/show.html.haml +++ b/app/views/projects/settings/repository/show.html.haml @@ -3,3 +3,4 @@ = render @deploy_keys = render "projects/protected_branches/index" += render "projects/protected_tags/index" -- cgit v1.2.1 From 97a02fca6bf24a2e6e5feb4b164a9265511903c2 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 21 Mar 2017 21:21:56 +0530 Subject: Protected Tags Classes --- .../protected_tag_access_dropdown.js | 29 ++++++++ .../protected_tags/protected_tag_create.js | 45 ++++++++++++ .../protected_tags/protected_tag_dropdown.js | 79 ++++++++++++++++++++++ 3 files changed, 153 insertions(+) create mode 100644 app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js create mode 100644 app/assets/javascripts/protected_tags/protected_tag_create.js create mode 100644 app/assets/javascripts/protected_tags/protected_tag_dropdown.js diff --git a/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js new file mode 100644 index 00000000000..b85c2991dd9 --- /dev/null +++ b/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js @@ -0,0 +1,29 @@ +/* eslint-disable arrow-parens, no-param-reassign, object-shorthand, no-else-return, comma-dangle, max-len */ + +(global => { + global.gl = global.gl || {}; + + gl.ProtectedTagAccessDropdown = class { + constructor(options) { + const { $dropdown, data, onSelect } = options; + + $dropdown.glDropdown({ + data: data, + selectable: true, + inputId: $dropdown.data('input-id'), + fieldName: $dropdown.data('field-name'), + toggleLabel(item, el) { + if (el.is('.is-active')) { + return item.text; + } else { + return 'Select'; + } + }, + clicked(item, $el, e) { + e.preventDefault(); + onSelect(); + } + }); + } + }; +})(window); diff --git a/app/assets/javascripts/protected_tags/protected_tag_create.js b/app/assets/javascripts/protected_tags/protected_tag_create.js new file mode 100644 index 00000000000..4c652e7747f --- /dev/null +++ b/app/assets/javascripts/protected_tags/protected_tag_create.js @@ -0,0 +1,45 @@ +/* eslint-disable no-new, arrow-parens, no-param-reassign, comma-dangle, max-len */ +/* global ProtectedTagDropdown */ + +(global => { + global.gl = global.gl || {}; + + gl.ProtectedTagCreate = class { + constructor() { + this.$wrap = this.$form = $('.new_protected_tag'); + this.buildDropdowns(); + } + + buildDropdowns() { + const $allowedToPushDropdown = this.$wrap.find('.js-allowed-to-push'); + + // Cache callback + this.onSelectCallback = this.onSelect.bind(this); + + // Allowed to Push dropdown + new gl.ProtectedTagAccessDropdown({ + $dropdown: $allowedToPushDropdown, + data: gon.push_access_levels, + onSelect: this.onSelectCallback + }); + + // Select default + $allowedToPushDropdown.data('glDropdown').selectRowAtIndex(0); + + // Protected tag dropdown + new ProtectedTagDropdown({ + $dropdown: this.$wrap.find('.js-protected-tag-select'), + onSelect: this.onSelectCallback + }); + } + + // This will run after clicked callback + onSelect() { + // Enable submit button + const $tagInput = this.$wrap.find('input[name="protected_tag[name]"]'); + const $allowedToPushInput = this.$wrap.find('input[name="protected_tag[push_access_levels_attributes][0][access_level]"]'); + + this.$form.find('input[type="submit"]').attr('disabled', !($tagInput.val() && $allowedToPushInput.length)); + } + }; +})(window); diff --git a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js new file mode 100644 index 00000000000..5a0356f502c --- /dev/null +++ b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js @@ -0,0 +1,79 @@ +/* eslint-disable comma-dangle, no-unused-vars */ + +class ProtectedTagDropdown { + constructor(options) { + this.onSelect = options.onSelect; + this.$dropdown = options.$dropdown; + this.$dropdownContainer = this.$dropdown.parent(); + this.$dropdownFooter = this.$dropdownContainer.find('.dropdown-footer'); + this.$protectedTag = this.$dropdownContainer.find('.create-new-protected-tag'); + + this.buildDropdown(); + this.bindEvents(); + + // Hide footer + this.$dropdownFooter.addClass('hidden'); + } + + buildDropdown() { + this.$dropdown.glDropdown({ + data: this.getProtectedTags.bind(this), + filterable: true, + remote: false, + search: { + fields: ['title'] + }, + selectable: true, + toggleLabel(selected) { + return (selected && 'id' in selected) ? selected.title : 'Protected Tag'; + }, + fieldName: 'protected_tag[name]', + text(protectedTag) { + return _.escape(protectedTag.title); + }, + id(protectedTag) { + return _.escape(protectedTag.id); + }, + onFilter: this.toggleCreateNewButton.bind(this), + clicked: (item, $el, e) => { + e.preventDefault(); + this.onSelect(); + } + }); + } + + bindEvents() { + this.$protectedTag.on('click', this.onClickCreateWildcard.bind(this)); + } + + onClickCreateWildcard() { + this.$dropdown.data('glDropdown').remote.execute(); + this.$dropdown.data('glDropdown').selectRowAtIndex(); + } + + getProtectedTags(term, callback) { + if (this.selectedTag) { + callback(gon.open_branches.concat(this.selectedTag)); + } else { + callback(gon.open_branches); + } + } + + toggleCreateNewButton(tagName) { + this.selectedTag = { + title: tagName, + id: tagName, + text: tagName + }; + + if (tagName) { + this.$dropdownContainer + .find('.create-new-protected-tag code') + .text(tagName); + } + + this.$dropdownFooter.toggleClass('hidden', !tagName); + } +} + +window.ProtectedTagDropdown = ProtectedTagDropdown; -- cgit v1.2.1 From c5c4370a7bb71130c1fcca57a069e3e8626d9108 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 21 Mar 2017 21:22:12 +0530 Subject: Re-export protected tag classes --- app/assets/javascripts/protected_tags/protected_tags_bundle.js | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 app/assets/javascripts/protected_tags/protected_tags_bundle.js diff --git a/app/assets/javascripts/protected_tags/protected_tags_bundle.js b/app/assets/javascripts/protected_tags/protected_tags_bundle.js new file mode 100644 index 00000000000..d84d2e1ef70 --- /dev/null +++ b/app/assets/javascripts/protected_tags/protected_tags_bundle.js @@ -0,0 +1,3 @@ +require('./protected_tag_access_dropdown'); +require('./protected_tag_create'); +require('./protected_tag_dropdown'); -- cgit v1.2.1 From 167b86000838aab32b13e19f4213805f460b7d46 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 21 Mar 2017 21:22:38 +0530 Subject: Initialize Protected tags feature under Repository Settings page --- app/assets/javascripts/dispatcher.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 3557f6f617e..6234092bdc2 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -320,6 +320,7 @@ const UserCallout = require('./user_callout'); case 'projects:repository:show': new gl.ProtectedBranchCreate(); new gl.ProtectedBranchEditList(); + new gl.ProtectedTagCreate(); break; case 'projects:ci_cd:show': new gl.ProjectVariables(); -- cgit v1.2.1 From 0fd2fd652a18ccceb725ce2f8f7683ad6ad3c224 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 21 Mar 2017 21:23:17 +0530 Subject: Update property names for tags --- app/views/projects/protected_tags/_create_protected_tag.html.haml | 4 ++-- app/views/projects/protected_tags/_dropdown.html.haml | 2 +- app/views/projects/protected_tags/_index.html.haml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml index 6ffe7b4c24f..110c24ac9e4 100644 --- a/app/views/projects/protected_tags/_create_protected_tag.html.haml +++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml @@ -1,4 +1,4 @@ -= form_for [@project.namespace.becomes(Namespace), @project, @protected_branch] do |f| += form_for [@project.namespace.becomes(Namespace), @project, @protected_branch], html: { class: 'new_protected_tag' } do |f| .panel.panel-default .panel-heading %h3.panel-title @@ -26,7 +26,7 @@ = dropdown_tag('Select', options: { toggle_class: 'js-allowed-to-push wide', dropdown_class: 'dropdown-menu-selectable', - data: { field_name: 'protected_branch[push_access_levels_attributes][0][access_level]', input_id: 'push_access_levels_attributes' }}) + data: { field_name: 'protected_tag[push_access_levels_attributes][0][access_level]', input_id: 'push_access_levels_attributes' }}) .panel-footer = f.submit 'Protect', class: 'btn-create btn', disabled: true diff --git a/app/views/projects/protected_tags/_dropdown.html.haml b/app/views/projects/protected_tags/_dropdown.html.haml index 3da153bc521..74851519077 100644 --- a/app/views/projects/protected_tags/_dropdown.html.haml +++ b/app/views/projects/protected_tags/_dropdown.html.haml @@ -5,7 +5,7 @@ filter: true, dropdown_class: "dropdown-menu-selectable", placeholder: "Search protected tag", footer_content: true, data: { show_no: true, show_any: true, show_upcoming: true, - selected: params[:protected_branch_name], + selected: params[:protected_tag_name], project_id: @project.try(:id) } }) do %ul.dropdown-footer-list diff --git a/app/views/projects/protected_tags/_index.html.haml b/app/views/projects/protected_tags/_index.html.haml index 3a1da1202e9..0965bf75eae 100644 --- a/app/views/projects/protected_tags/_index.html.haml +++ b/app/views/projects/protected_tags/_index.html.haml @@ -1,5 +1,5 @@ - content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('protected_branches') + = page_specific_javascript_bundle_tag('protected_tags') .row.prepend-top-default.append-bottom-default .col-lg-3 -- cgit v1.2.1 From 99859b01f4dad72dc51c0765db89800915a94f36 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 21 Mar 2017 21:23:31 +0530 Subject: Add protected tags to bundle config --- config/webpack.config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/config/webpack.config.js b/config/webpack.config.js index c6794d6b944..d861fa0c7a4 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -39,6 +39,7 @@ var config = { network: './network/network_bundle.js', profile: './profile/profile_bundle.js', protected_branches: './protected_branches/protected_branches_bundle.js', + protected_tags: './protected_tags/protected_tags_bundle.js', snippet: './snippet/snippet_bundle.js', terminal: './terminal/terminal_bundle.js', u2f: ['vendor/u2f'], -- cgit v1.2.1 From 1a416a42f1c1b876ecd96687e41696bc915cc2c2 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 15 Mar 2017 22:26:48 +0000 Subject: Database migrations for protected tags Added schema.rb for protected tags --- db/migrate/20170309173138_create_protected_tags.rb | 39 ++++++++++++++++++++++ db/schema.rb | 30 ++++++++++++++++- 2 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20170309173138_create_protected_tags.rb diff --git a/db/migrate/20170309173138_create_protected_tags.rb b/db/migrate/20170309173138_create_protected_tags.rb new file mode 100644 index 00000000000..c69ef970410 --- /dev/null +++ b/db/migrate/20170309173138_create_protected_tags.rb @@ -0,0 +1,39 @@ +class CreateProtectedTags < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + GitlabAccessMaster = 40 + + def change + create_table :protected_tags do |t| + t.integer :project_id, null: false + t.string :name, null: false + t.string :timestamps #TODO: `null: false`? Missing from protected_branches + end + + add_index :protected_tags, :project_id + + create_table :protected_tag_merge_access_levels do |t| + t.references :protected_tag, index: { name: "index_protected_tag_merge_access" }, foreign_key: true, null: false + + t.integer :access_level, default: GitlabAccessMaster, null: true #TODO: was false, check schema + t.integer :group_id #TODO: check why group/user id missing from CE + t.integer :user_id + t.timestamps null: false + end + + create_table :protected_tag_push_access_levels do |t| + t.references :protected_tag, index: { name: "index_protected_tag_push_access" }, foreign_key: true, null: false + t.integer :access_level, default: GitlabAccessMaster, null: true #TODO: was false, check schema + t.integer :group_id + t.integer :user_id + t.timestamps null: false + end + + #TODO: These had rubocop set to disable Migration/AddConcurrentForeignKey + # add_foreign_key :protected_tag_merge_access_levels, :namespaces, column: :group_id + # add_foreign_key :protected_tag_push_access_levels, :namespaces, column: :group_id + end +end diff --git a/db/schema.rb b/db/schema.rb index f96a7d21890..05b28b6a63b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170315194013) do +ActiveRecord::Schema.define(version: 20170317203554) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -963,6 +963,32 @@ ActiveRecord::Schema.define(version: 20170315194013) do add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree + create_table "protected_tag_merge_access_levels", force: :cascade do |t| + t.integer "protected_tag_id", null: false + t.integer "access_level", default: 40, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "protected_tag_merge_access_levels", ["protected_tag_id"], name: "index_protected_tag_merge_access", using: :btree + + create_table "protected_tag_push_access_levels", force: :cascade do |t| + t.integer "protected_tag_id", null: false + t.integer "access_level", default: 40, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "protected_tag_push_access_levels", ["protected_tag_id"], name: "index_protected_tag_push_access", using: :btree + + create_table "protected_tags", force: :cascade do |t| + t.integer "project_id", null: false + t.string "name", null: false + t.string "timestamps" + end + + add_index "protected_tags", ["project_id"], name: "index_protected_tags_on_project_id", using: :btree + create_table "releases", force: :cascade do |t| t.string "tag" t.text "description" @@ -1305,6 +1331,8 @@ ActiveRecord::Schema.define(version: 20170315194013) do add_foreign_key "project_statistics", "projects", on_delete: :cascade add_foreign_key "protected_branch_merge_access_levels", "protected_branches" add_foreign_key "protected_branch_push_access_levels", "protected_branches" + add_foreign_key "protected_tag_merge_access_levels", "protected_tags" + add_foreign_key "protected_tag_push_access_levels", "protected_tags" add_foreign_key "subscriptions", "projects", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade -- cgit v1.2.1 From 91ed8ed687ee9edbda0098475e66ad41f886d7a5 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 15 Mar 2017 22:29:07 +0000 Subject: Protected tags copy/paste from protected branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should provide basic CRUD backend for frontend to work from. Doesn’t include frontend, API, or the internal API used from gitlab-shell --- .../projects/protected_tags_controller.rb | 58 +++++++++++++ app/models/concerns/protected_ref_access.rb | 21 +++++ app/models/concerns/protected_tag_access.rb | 21 +++++ app/models/project.rb | 1 + app/models/protected_branch.rb | 39 ++------- app/models/protected_ref_matcher.rb | 52 ++++++++++++ app/models/protected_tag.rb | 39 +++++++++ app/models/protected_tag/push_access_level.rb | 21 +++++ app/services/protected_tags/create_service.rb | 11 +++ app/services/protected_tags/update_service.rb | 13 +++ config/routes/project.rb | 2 + .../projects/protected_tags_controller_spec.rb | 10 +++ spec/factories/protected_tags.rb | 22 +++++ .../protected_tags/access_control_ce_spec.rb | 79 ++++++++++++++++++ spec/features/protected_tags_spec.rb | 94 ++++++++++++++++++++++ spec/models/protected_tag_spec.rb | 14 ++++ .../services/protected_tags/create_service_spec.rb | 23 ++++++ 17 files changed, 488 insertions(+), 32 deletions(-) create mode 100644 app/controllers/projects/protected_tags_controller.rb create mode 100644 app/models/concerns/protected_ref_access.rb create mode 100644 app/models/concerns/protected_tag_access.rb create mode 100644 app/models/protected_ref_matcher.rb create mode 100644 app/models/protected_tag.rb create mode 100644 app/models/protected_tag/push_access_level.rb create mode 100644 app/services/protected_tags/create_service.rb create mode 100644 app/services/protected_tags/update_service.rb create mode 100644 spec/controllers/projects/protected_tags_controller_spec.rb create mode 100644 spec/factories/protected_tags.rb create mode 100644 spec/features/protected_tags/access_control_ce_spec.rb create mode 100644 spec/features/protected_tags_spec.rb create mode 100644 spec/models/protected_tag_spec.rb create mode 100644 spec/services/protected_tags/create_service_spec.rb diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb new file mode 100644 index 00000000000..5ab5d1d997b --- /dev/null +++ b/app/controllers/projects/protected_tags_controller.rb @@ -0,0 +1,58 @@ +class Projects::ProtectedTagsController < Projects::ApplicationController + include RepositorySettingsRedirect + # Authorize + before_action :require_non_empty_project + before_action :authorize_admin_project! + before_action :load_protected_tag, only: [:show, :update, :destroy] + + layout "project_settings" + + def index + redirect_to_repository_settings(@project) + end + + def create + @protected_tag = ::ProtectedTags::CreateService.new(@project, current_user, protected_tag_params).execute + unless @protected_tag.persisted? + flash[:alert] = @protected_tags.errors.full_messages.join(', ').html_safe + end + redirect_to_repository_settings(@project) + end + + def show + @matching_tags = @protected_tag.matching(@project.repository.tags) + end + + def update + @protected_tag = ::ProtectedTags::UpdateService.new(@project, current_user, protected_tag_params).execute(@protected_tag) + + if @protected_tag.valid? + respond_to do |format| + format.json { render json: @protected_tag, status: :ok } + end + else + respond_to do |format| + format.json { render json: @protected_tag.errors, status: :unprocessable_entity } + end + end + end + + def destroy + @protected_tag.destroy + + respond_to do |format| + format.html { redirect_to_repository_settings(@project) } + format.js { head :ok } + end + end + + private + + def load_protected_tag + @protected_tag = @project.protected_tags.find(params[:id]) + end + + def protected_tag_params + params.require(:protected_tag).permit(:name, push_access_levels_attributes: [:access_level, :id]) + end +end diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb new file mode 100644 index 00000000000..9dd4d9c6f24 --- /dev/null +++ b/app/models/concerns/protected_ref_access.rb @@ -0,0 +1,21 @@ +module ProtectedBranchAccess + extend ActiveSupport::Concern + + included do + belongs_to :protected_branch + delegate :project, to: :protected_branch + + scope :master, -> { where(access_level: Gitlab::Access::MASTER) } + scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } + end + + def humanize + self.class.human_access_levels[self.access_level] + end + + def check_access(user) + return true if user.is_admin? + + project.team.max_member_access(user.id) >= access_level + end +end diff --git a/app/models/concerns/protected_tag_access.rb b/app/models/concerns/protected_tag_access.rb new file mode 100644 index 00000000000..cf66a6434b5 --- /dev/null +++ b/app/models/concerns/protected_tag_access.rb @@ -0,0 +1,21 @@ +module ProtectedTagAccess + extend ActiveSupport::Concern + + included do + belongs_to :protected_tag + delegate :project, to: :protected_tag + + scope :master, -> { where(access_level: Gitlab::Access::MASTER) } + scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } + end + + def humanize + self.class.human_access_levels[self.access_level] + end + + def check_access(user) + return true if user.is_admin? + + project.team.max_member_access(user.id) >= access_level + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 4a3faff7d5b..3f1a8a1a1e1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -132,6 +132,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 :project_authorizations has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 39e979ef15b..7681d5b5112 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -18,50 +18,25 @@ class ProtectedBranch < ActiveRecord::Base project.commit(self.name) end - # Returns all protected branches that match the given branch name. - # This realizes all records from the scope built up so far, and does - # _not_ return a relation. - # - # This method optionally takes in a list of `protected_branches` to search - # through, to avoid calling out to the database. def self.matching(branch_name, protected_branches: nil) - (protected_branches || all).select { |protected_branch| protected_branch.matches?(branch_name) } + ProtectedRefMatcher.matching(ProtectedBranch, branch_name, protected_refs: protected_branches) end - # Returns all branches (among the given list of branches [`Gitlab::Git::Branch`]) - # that match the current protected branch. def matching(branches) - branches.select { |branch| self.matches?(branch.name) } + ref_matcher.matching(branches) end - # Checks if the protected branch matches the given branch name. def matches?(branch_name) - return false if self.name.blank? - - exact_match?(branch_name) || wildcard_match?(branch_name) + ref_matcher.matches?(branch_name) end - # Checks if this protected branch contains a wildcard def wildcard? - self.name && self.name.include?('*') + ref_matcher.wildcard? end - protected - - def exact_match?(branch_name) - self.name == branch_name - end - - def wildcard_match?(branch_name) - wildcard_regex === branch_name - end + private - def wildcard_regex - @wildcard_regex ||= begin - name = self.name.gsub('*', 'STAR_DONT_ESCAPE') - quoted_name = Regexp.quote(name) - regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?') - /\A#{regex_string}\z/ - end + def ref_matcher + @ref_matcher ||= ProtectedRefMatcher.new(self) end end diff --git a/app/models/protected_ref_matcher.rb b/app/models/protected_ref_matcher.rb new file mode 100644 index 00000000000..83f44240259 --- /dev/null +++ b/app/models/protected_ref_matcher.rb @@ -0,0 +1,52 @@ +class ProtectedRefMatcher + def initialize(protected_ref) + @protected_ref = protected_ref + end + + # Returns all protected refs that match the given ref name. + # This realizes all records from the scope built up so far, and does + # _not_ return a relation. + # + # This method optionally takes in a list of `protected_refs` to search + # through, to avoid calling out to the database. + def self.matching(type, ref_name, protected_refs: nil) + (protected_refs || type.all).select { |protected_ref| protected_ref.matches?(ref_name) } + end + + # Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`]) + # that match the current protected ref. + def matching(refs) + refs.select { |ref| @protected_ref.matches?(ref.name) } + end + + # Checks if the protected ref matches the given ref name. + def matches?(ref_name) + return false if @protected_ref.name.blank? + + exact_match?(ref_name) || wildcard_match?(ref_name) + end + + # Checks if this protected ref contains a wildcard + def wildcard? + @protected_ref.name && @protected_ref.name.include?('*') + end + + protected + + def exact_match?(ref_name) + @protected_ref.name == ref_name + end + + def wildcard_match?(ref_name) + wildcard_regex === ref_name + end + + def wildcard_regex + @wildcard_regex ||= begin + name = @protected_ref.name.gsub('*', 'STAR_DONT_ESCAPE') + quoted_name = Regexp.quote(name) + regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?') + /\A#{regex_string}\z/ + end + end +end diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb new file mode 100644 index 00000000000..d307549aa49 --- /dev/null +++ b/app/models/protected_tag.rb @@ -0,0 +1,39 @@ +class ProtectedTag < ActiveRecord::Base + include Gitlab::ShellAdapter + + belongs_to :project + validates :name, presence: true + validates :project, presence: true + + has_many :push_access_levels, dependent: :destroy + + validates :push_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." } + + accepts_nested_attributes_for :push_access_levels + + def commit + project.commit(self.name) + end + + def self.matching(tag_name, protected_tags: nil) + ProtectedRefMatcher.matching(ProtectedTag, tag_name, protected_refs: protected_tags) + end + + def matching(branches) + ref_matcher.matching(branches) + end + + def matches?(tag_name) + ref_matcher.matches?(tag_name) + end + + def wildcard? + ref_matcher.wildcard? + end + + private + + def ref_matcher + @ref_matcher ||= ProtectedRefMatcher.new(self) + end +end diff --git a/app/models/protected_tag/push_access_level.rb b/app/models/protected_tag/push_access_level.rb new file mode 100644 index 00000000000..9282af841ce --- /dev/null +++ b/app/models/protected_tag/push_access_level.rb @@ -0,0 +1,21 @@ +class ProtectedTag::PushAccessLevel < ActiveRecord::Base + include ProtectedTagAccess + + validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, + Gitlab::Access::DEVELOPER, + Gitlab::Access::NO_ACCESS] } + + def self.human_access_levels + { + Gitlab::Access::MASTER => "Masters", + Gitlab::Access::DEVELOPER => "Developers + Masters", + Gitlab::Access::NO_ACCESS => "No one" + }.with_indifferent_access + end + + def check_access(user) + return false if access_level == Gitlab::Access::NO_ACCESS + + super + end +end diff --git a/app/services/protected_tags/create_service.rb b/app/services/protected_tags/create_service.rb new file mode 100644 index 00000000000..faba7865a17 --- /dev/null +++ b/app/services/protected_tags/create_service.rb @@ -0,0 +1,11 @@ +module ProtectedTags + class CreateService < BaseService + attr_reader :protected_tag + + def execute + raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) + + project.protected_tags.create(params) + end + end +end diff --git a/app/services/protected_tags/update_service.rb b/app/services/protected_tags/update_service.rb new file mode 100644 index 00000000000..8a2419efd7b --- /dev/null +++ b/app/services/protected_tags/update_service.rb @@ -0,0 +1,13 @@ +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 + end + end +end diff --git a/config/routes/project.rb b/config/routes/project.rb index 44b8ae7aedd..f0735bf73e8 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -134,6 +134,8 @@ constraints(ProjectUrlConstrainer.new) do end resources :protected_branches, only: [:index, :show, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } + resources :protected_tags, only: [:index, :show, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } + resources :variables, only: [:index, :show, :update, :create, :destroy] resources :triggers, only: [:index, :create, :edit, :update, :destroy] do member do diff --git a/spec/controllers/projects/protected_tags_controller_spec.rb b/spec/controllers/projects/protected_tags_controller_spec.rb new file mode 100644 index 00000000000..729017c1483 --- /dev/null +++ b/spec/controllers/projects/protected_tags_controller_spec.rb @@ -0,0 +1,10 @@ +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 + # end +end diff --git a/spec/factories/protected_tags.rb b/spec/factories/protected_tags.rb new file mode 100644 index 00000000000..f0016b37d66 --- /dev/null +++ b/spec/factories/protected_tags.rb @@ -0,0 +1,22 @@ +FactoryGirl.define do + factory :protected_tag do + name + project + + after(:build) do |protected_tag| + protected_tag.push_access_levels.new(access_level: Gitlab::Access::MASTER) + end + + trait :developers_can_push do + after(:create) do |protected_tag| + protected_tag.push_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER) + end + end + + trait :no_one_can_push do + after(:create) do |protected_tag| + protected_tag.push_access_levels.first.update!(access_level: Gitlab::Access::NO_ACCESS) + end + end + end +end diff --git a/spec/features/protected_tags/access_control_ce_spec.rb b/spec/features/protected_tags/access_control_ce_spec.rb new file mode 100644 index 00000000000..545d3bca74d --- /dev/null +++ b/spec/features/protected_tags/access_control_ce_spec.rb @@ -0,0 +1,79 @@ +# RSpec.shared_examples "protected tags > access control > CE" do +# ProtectedTag::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| +# it "allows creating protected tags that #{access_type_name} can push to" do +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('master') +# within('.new_protected_tag') do +# allowed_to_push_button = find(".js-allowed-to-push") + +# unless allowed_to_push_button.text == access_type_name +# allowed_to_push_button.click +# within(".dropdown.open .dropdown-menu") { click_on access_type_name } +# end +# end +# click_on "Protect" + +# expect(ProtectedTag.count).to eq(1) +# expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to eq([access_type_id]) +# end + +# it "allows updating protected tags so that #{access_type_name} can push to 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) + +# within(".protected-tags-list") do +# find(".js-allowed-to-push").click + +# within('.js-allowed-to-push-container') do +# expect(first("li")).to have_content("Roles") +# click_on access_type_name +# end +# end + +# wait_for_ajax +# expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to include(access_type_id) +# end +# end + +# ProtectedTag::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| +# it "allows creating protected tags that #{access_type_name} can merge to" do +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('master') +# within('.new_protected_tag') do +# allowed_to_merge_button = find(".js-allowed-to-merge") + +# unless allowed_to_merge_button.text == access_type_name +# allowed_to_merge_button.click +# within(".dropdown.open .dropdown-menu") { click_on access_type_name } +# end +# end +# click_on "Protect" + +# expect(ProtectedTag.count).to eq(1) +# expect(ProtectedTag.last.merge_access_levels.map(&:access_level)).to eq([access_type_id]) +# end + +# it "allows updating protected tags so that #{access_type_name} can merge to 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) + +# within(".protected-tags-list") do +# find(".js-allowed-to-merge").click + +# within('.js-allowed-to-merge-container') do +# expect(first("li")).to have_content("Roles") +# click_on access_type_name +# end +# end + +# wait_for_ajax +# expect(ProtectedTag.last.merge_access_levels.map(&:access_level)).to include(access_type_id) +# end +# end +# end diff --git a/spec/features/protected_tags_spec.rb b/spec/features/protected_tags_spec.rb new file mode 100644 index 00000000000..546d6037ef7 --- /dev/null +++ b/spec/features/protected_tags_spec.rb @@ -0,0 +1,94 @@ +# require 'spec_helper' +# Dir["./spec/features/protected_tags/*.rb"].sort.each { |f| require f } + +# feature 'Projected Tags', feature: true, js: true do +# include WaitForAjax + +# let(:user) { create(:user, :admin) } +# let(:project) { create(:project) } + +# before { login_as(user) } + +# def set_protected_tag_name(tag_name) +# find(".js-protected-tag-select").click +# find(".dropdown-input-field").set(tag_name) +# click_on("Create wildcard #{tag_name}") +# end + +# describe "explicit protected tags" do +# it "allows creating explicit protected tags" do +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('some-tag') +# click_on "Protect" + +# within(".protected-tags-list") { expect(page).to have_content('some-tag') } +# expect(ProtectedTag.count).to eq(1) +# expect(ProtectedTag.last.name).to eq('some-tag') +# end + +# it "displays the last commit on the matching tag if it exists" do +# commit = create(:commit, project: project) +# project.repository.add_tag(user, 'some-tag', commit.id) + +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('some-tag') +# click_on "Protect" + +# within(".protected-tags-list") { expect(page).to have_content(commit.id[0..7]) } +# end + +# it "displays an error message if the named tag does not exist" do +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('some-tag') +# click_on "Protect" + +# within(".protected-tags-list") { expect(page).to have_content('tag was removed') } +# end +# end + +# describe "wildcard protected tags" do +# it "allows creating protected tags with a wildcard" do +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('*-stable') +# click_on "Protect" + +# within(".protected-tags-list") { expect(page).to have_content('*-stable') } +# expect(ProtectedTag.count).to eq(1) +# expect(ProtectedTag.last.name).to eq('*-stable') +# end + +# it "displays the number of matching tags" do +# project.repository.add_tag(user, 'production-stable', 'master') +# project.repository.add_tag(user, 'staging-stable', 'master') + +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('*-stable') +# click_on "Protect" + +# within(".protected-tags-list") { expect(page).to have_content("2 matching tags") } +# end + +# it "displays all the tags matching the wildcard" do +# project.repository.add_tag(user, 'production-stable', 'master') +# project.repository.add_tag(user, 'staging-stable', 'master') +# project.repository.add_tag(user, 'development', 'master') + +# visit namespace_project_protected_tags_path(project.namespace, project) +# set_protected_tag_name('*-stable') +# click_on "Protect" + +# visit namespace_project_protected_tags_path(project.namespace, project) +# click_on "2 matching tags" + +# within(".protected-tags-list") do +# expect(page).to have_content("production-stable") +# expect(page).to have_content("staging-stable") +# expect(page).not_to have_content("development") +# end +# end +# end + +# describe "access control" do +# include_examples "protected tags > access control > CE" +# end +# end diff --git a/spec/models/protected_tag_spec.rb b/spec/models/protected_tag_spec.rb new file mode 100644 index 00000000000..05ad532935a --- /dev/null +++ b/spec/models/protected_tag_spec.rb @@ -0,0 +1,14 @@ +require 'spec_helper' + +describe ProtectedTag, models: true do + subject { build_stubbed(:protected_branch) } + + describe 'Associations' do + it { is_expected.to belong_to(:project) } + end + + describe 'Validation' do + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:name) } + end +end diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb new file mode 100644 index 00000000000..e1fd73dbd07 --- /dev/null +++ b/spec/services/protected_tags/create_service_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe ProtectedTags::CreateService, services: true do + let(:project) { create(:empty_project) } + let(:user) { project.owner } + let(:params) do + { + name: 'master', + merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }], + push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }] + } + end + + describe '#execute' do + subject(:service) { described_class.new(project, user, params) } + + it 'creates a new protected tag' do + expect { service.execute }.to change(ProtectedTag, :count).by(1) + expect(project.protected_tags.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) + expect(project.protected_tags.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) + end + end +end -- cgit v1.2.1 From f51eac1df967856299467f65ac6fb81e2d610ff5 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 17 Mar 2017 19:55:15 +0000 Subject: Settings::RepositoryController includes protected tags in JS --- .../projects/settings/repository_controller.rb | 30 +++++++++++++++----- app/models/concerns/protected_ref_access.rb | 32 +++++++++++----------- app/models/project.rb | 1 + 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index b6ce4abca45..5160ee5e1e4 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -7,22 +7,23 @@ module Projects @deploy_keys = DeployKeysPresenter .new(@project, current_user: current_user) - define_protected_branches + define_protected_refs end private - def define_protected_branches - load_protected_branches + def define_protected_refs + @protected_branches = @project.protected_branches.order(:name).page(params[:page]) + @protected_tags = @project.protected_tags.order(:name).page(params[:page]) @protected_branch = @project.protected_branches.new + @protected_tag = @project.protected_tags.new load_gon_index end - def load_protected_branches - @protected_branches = @project.protected_branches.order(:name).page(params[:page]) - end def access_levels_options + #TODO: consider protected tags + #TODO: Refactor ProtectedBranch::PushAccessLevel so it doesn't mention branches { push_access_levels: { roles: ProtectedBranch::PushAccessLevel.human_access_levels.map do |id, text| @@ -37,13 +38,28 @@ module Projects } end + #TODO: Move to Protections::TagMatcher.new(project).unprotected + def unprotected_tags + exact_protected_tag_names = @project.protected_tags.reject(&:wildcard?).map(&:name) + tag_names = @project.repository.tags.map(&:name) + non_open_tag_names = Set.new(exact_protected_tag_names).intersection(Set.new(tag_names)) + @project.repository.tags.reject { |tag| non_open_tag_names.include? tag.name } + end + + def unprotected_tags_hash + tags = unprotected_tags.map { |tag| { text: tag.name, id: tag.name, title: tag.name } } + { open_tags: tags } + end + def open_branches branches = @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } { open_branches: branches } end def load_gon_index - gon.push(open_branches.merge(access_levels_options)) + gon.push(open_branches) + gon.push(unprotected_tags_hash) + gon.push(access_levels_options) end end end diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index 9dd4d9c6f24..2870cd9385b 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -1,21 +1,21 @@ -module ProtectedBranchAccess - extend ActiveSupport::Concern +# module ProtectedRefAccess +# extend ActiveSupport::Concern - included do - belongs_to :protected_branch - delegate :project, to: :protected_branch +# included do +# # belongs_to :protected_branch +# # delegate :project, to: :protected_branch - scope :master, -> { where(access_level: Gitlab::Access::MASTER) } - scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } - end +# scope :master, -> { where(access_level: Gitlab::Access::MASTER) } +# scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } +# end - def humanize - self.class.human_access_levels[self.access_level] - end +# def humanize +# self.class.human_access_levels[self.access_level] +# end - def check_access(user) - return true if user.is_admin? +# def check_access(user) +# return true if user.is_admin? - project.team.max_member_access(user.id) >= access_level - end -end +# project.team.max_member_access(user.id) >= access_level +# end +# end diff --git a/app/models/project.rb b/app/models/project.rb index 3f1a8a1a1e1..c04effc53bd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -866,6 +866,7 @@ class Project < ActiveRecord::Base end # Branches that are not _exactly_ matched by a protected branch. + #TODO: Move to Protections::BranchMatcher.new(project).unprotecte def open_branches exact_protected_branch_names = protected_branches.reject(&:wildcard?).map(&:name) branch_names = repository.branches.map(&:name) -- cgit v1.2.1 From 18b445ade4280c03e73ccbf2ca4d175e97a887c8 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Thu, 30 Mar 2017 23:35:19 +0100 Subject: Protected tags can be added/listed via UI --- .../protected_tags/protected_tag_dropdown.js | 4 +-- .../protected_tags/_create_protected_tag.html.haml | 4 +-- app/views/projects/protected_tags/_index.html.haml | 2 +- .../protected_tags/_protected_tag.html.haml | 21 +++++++++++++++ .../projects/protected_tags/_tags_list.html.haml | 30 ++++++++++++++++++---- .../protected_tags/_update_protected_tag.haml | 5 ++++ 6 files changed, 56 insertions(+), 10 deletions(-) create mode 100644 app/views/projects/protected_tags/_protected_tag.html.haml create mode 100644 app/views/projects/protected_tags/_update_protected_tag.haml diff --git a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js index 5a0356f502c..ccc4c81fa18 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js +++ b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js @@ -53,9 +53,9 @@ class ProtectedTagDropdown { getProtectedTags(term, callback) { if (this.selectedTag) { - callback(gon.open_branches.concat(this.selectedTag)); + callback(gon.open_tags.concat(this.selectedTag)); } else { - callback(gon.open_branches); + callback(gon.open_tags); } } diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml index 110c24ac9e4..9fdebf2c982 100644 --- a/app/views/projects/protected_tags/_create_protected_tag.html.haml +++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml @@ -1,11 +1,11 @@ -= form_for [@project.namespace.becomes(Namespace), @project, @protected_branch], html: { class: 'new_protected_tag' } do |f| += form_for [@project.namespace.becomes(Namespace), @project, @protected_tag], html: { class: 'new_protected_tag' } do |f| .panel.panel-default .panel-heading %h3.panel-title Protect a tag .panel-body .form-horizontal - = form_errors(@protected_branch) + = form_errors(@protected_tag) .form-group = f.label :name, class: 'col-md-2 text-right' do Tag: diff --git a/app/views/projects/protected_tags/_index.html.haml b/app/views/projects/protected_tags/_index.html.haml index 0965bf75eae..591d64ae7de 100644 --- a/app/views/projects/protected_tags/_index.html.haml +++ b/app/views/projects/protected_tags/_index.html.haml @@ -6,7 +6,7 @@ %h4.prepend-top-0 Protected tags %p.prepend-top-20 - By default, Protected branches are designed to: + By default, Protected tags are designed to: %ul %li Prevent tag pushes from everybody except Masters %li Prevent anyone from force pushing to the tag diff --git a/app/views/projects/protected_tags/_protected_tag.html.haml b/app/views/projects/protected_tags/_protected_tag.html.haml new file mode 100644 index 00000000000..26bd3a1f5ed --- /dev/null +++ b/app/views/projects/protected_tags/_protected_tag.html.haml @@ -0,0 +1,21 @@ +%tr.js-protected-tag-edit-form{ data: { url: namespace_project_protected_tag_path(@project.namespace, @project, protected_tag) } } + %td + = protected_tag.name + - if @project.root_ref?(protected_tag.name) + %span.label.label-info.prepend-left-5 default + %td + - if protected_tag.wildcard? + - matching_tags = protected_tag.matching(repository.tags) + = link_to pluralize(matching_tags.count, "matching tag"), namespace_project_protected_tag_path(@project.namespace, @project, protected_tag) + - else + - if commit = protected_tag.commit + = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id') + = time_ago_with_tooltip(commit.committed_date) + - else + (tag was removed from repository) + + = render partial: 'projects/protected_tags/update_protected_tag', locals: { protected_tag: protected_tag } + + - if can_admin_project + %td + = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_tag], data: { confirm: 'tag will be writable for developers. Are you sure?' }, method: :delete, class: 'btn btn-warning' diff --git a/app/views/projects/protected_tags/_tags_list.html.haml b/app/views/projects/protected_tags/_tags_list.html.haml index e7ce90393e9..6dcd356e6f1 100644 --- a/app/views/projects/protected_tags/_tags_list.html.haml +++ b/app/views/projects/protected_tags/_tags_list.html.haml @@ -1,6 +1,26 @@ .panel.panel-default.protected-tags-list - .panel-heading - %h3.panel-title - Protected tag (0) - %p.settings-message.text-center - There are currently no protected tags, protect a tag with the form above. + - if @protected_tags.empty? + .panel-heading + %h3.panel-title + Protected tag (#{@protected_tags.size}) + %p.settings-message.text-center + There are currently no protected tags, protect a tag with the form above. + - else + - can_admin_project = can?(current_user, :admin_project, @project) + + %table.table.table-bordered + %colgroup + %col{ width: "25%" } + %col{ width: "55%" } + %col{ width: "20%" } + %thead + %tr + %th Protected tag (#{@protected_tags.size}) + %th Last commit + %th Allowed to push + - if can_admin_project + %th + %tbody + = render partial: 'projects/protected_tags/protected_tag', collection: @protected_tags, locals: { can_admin_project: can_admin_project} + + = paginate @protected_tags, theme: 'gitlab' diff --git a/app/views/projects/protected_tags/_update_protected_tag.haml b/app/views/projects/protected_tags/_update_protected_tag.haml new file mode 100644 index 00000000000..729a784a559 --- /dev/null +++ b/app/views/projects/protected_tags/_update_protected_tag.haml @@ -0,0 +1,5 @@ +%td + = hidden_field_tag "allowed_to_push_#{protected_tag.id}", protected_tag.push_access_levels.first.access_level + / = dropdown_tag( (protected_tag.push_access_levels.first.humanize || 'Select') , + / options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', + / data: { field_name: "allowed_to_push_#{protected_tag.id}", access_level_id: protected_tag.push_access_levels.first.id }}) -- cgit v1.2.1 From b5fce1d5ac87546e8f31fb0ef6f6c4d514670198 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 31 Mar 2017 17:56:26 +0100 Subject: =?UTF-8?q?Removed=20unnecessary=20table=20=E2=80=98protected=5Fta?= =?UTF-8?q?g=5Fmerge=5Faccess=5Flevels=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed timestamps on protected_tags --- db/migrate/20170309173138_create_protected_tags.rb | 23 ++++++---------------- db/schema.rb | 22 +++++++++------------ 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/db/migrate/20170309173138_create_protected_tags.rb b/db/migrate/20170309173138_create_protected_tags.rb index c69ef970410..00794529143 100644 --- a/db/migrate/20170309173138_create_protected_tags.rb +++ b/db/migrate/20170309173138_create_protected_tags.rb @@ -4,36 +4,25 @@ class CreateProtectedTags < ActiveRecord::Migration # Set this constant to true if this migration requires downtime. DOWNTIME = false - GitlabAccessMaster = 40 + GITLAB_ACCESS_MASTER = 40 def change create_table :protected_tags do |t| t.integer :project_id, null: false t.string :name, null: false - t.string :timestamps #TODO: `null: false`? Missing from protected_branches + t.timestamps null: false end add_index :protected_tags, :project_id - create_table :protected_tag_merge_access_levels do |t| - t.references :protected_tag, index: { name: "index_protected_tag_merge_access" }, foreign_key: true, null: false - - t.integer :access_level, default: GitlabAccessMaster, null: true #TODO: was false, check schema - t.integer :group_id #TODO: check why group/user id missing from CE - t.integer :user_id - t.timestamps null: false - end - create_table :protected_tag_push_access_levels do |t| t.references :protected_tag, index: { name: "index_protected_tag_push_access" }, foreign_key: true, null: false - t.integer :access_level, default: GitlabAccessMaster, null: true #TODO: was false, check schema - t.integer :group_id - t.integer :user_id + t.integer :access_level, default: GITLAB_ACCESS_MASTER, null: true + t.references :user, foreign_key: true, index: true + t.integer :group_id#TODO: Should this have an index? Doesn't appear in brances #, index: true t.timestamps null: false end - #TODO: These had rubocop set to disable Migration/AddConcurrentForeignKey - # add_foreign_key :protected_tag_merge_access_levels, :namespaces, column: :group_id - # add_foreign_key :protected_tag_push_access_levels, :namespaces, column: :group_id + add_foreign_key :protected_tag_push_access_levels, :namespaces, column: :group_id # rubocop: disable Migration/AddConcurrentForeignKey end end diff --git a/db/schema.rb b/db/schema.rb index 05b28b6a63b..650b18bb013 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170317203554) do +ActiveRecord::Schema.define(version: 20170315194013) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -963,28 +963,23 @@ ActiveRecord::Schema.define(version: 20170317203554) do add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree - create_table "protected_tag_merge_access_levels", force: :cascade do |t| - t.integer "protected_tag_id", null: false - t.integer "access_level", default: 40, null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - - add_index "protected_tag_merge_access_levels", ["protected_tag_id"], name: "index_protected_tag_merge_access", using: :btree - create_table "protected_tag_push_access_levels", force: :cascade do |t| t.integer "protected_tag_id", null: false - t.integer "access_level", default: 40, null: false + t.integer "access_level", default: 40 + t.integer "user_id" + t.integer "group_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false end add_index "protected_tag_push_access_levels", ["protected_tag_id"], name: "index_protected_tag_push_access", using: :btree + add_index "protected_tag_push_access_levels", ["user_id"], name: "index_protected_tag_push_access_levels_on_user_id", using: :btree create_table "protected_tags", force: :cascade do |t| t.integer "project_id", null: false t.string "name", null: false - t.string "timestamps" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end add_index "protected_tags", ["project_id"], name: "index_protected_tags_on_project_id", using: :btree @@ -1331,8 +1326,9 @@ ActiveRecord::Schema.define(version: 20170317203554) do add_foreign_key "project_statistics", "projects", on_delete: :cascade add_foreign_key "protected_branch_merge_access_levels", "protected_branches" add_foreign_key "protected_branch_push_access_levels", "protected_branches" - add_foreign_key "protected_tag_merge_access_levels", "protected_tags" + add_foreign_key "protected_tag_push_access_levels", "namespaces", column: "group_id" add_foreign_key "protected_tag_push_access_levels", "protected_tags" + add_foreign_key "protected_tag_push_access_levels", "users" add_foreign_key "subscriptions", "projects", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade -- cgit v1.2.1 From e3fbcd0093b07bbc084061992bb8ae6bd4343d52 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 31 Mar 2017 17:57:29 +0100 Subject: Protected Tags enforced over git --- app/models/project.rb | 8 ++++++ lib/gitlab/checks/change_access.rb | 37 ++++++++++++++++++++---- lib/gitlab/user_access.rb | 16 +++++++++++ spec/lib/gitlab/checks/change_access_spec.rb | 42 +++++++++++++++++++++++++++- 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index c04effc53bd..7681da36335 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -893,6 +893,7 @@ class Project < ActiveRecord::Base end # Check if current branch name is marked as protected in the system + #TODO: Move elsewhere def protected_branch?(branch_name) return true if empty_repo? && default_branch_protected? @@ -900,6 +901,13 @@ class Project < ActiveRecord::Base ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present? end + #TODO: Move elsewhere + def protected_tag?(tag_name) + #TODO: Check if memoization necessary, find way to have it work elsewhere + @protected_tags ||= self.protected_tags.to_a + ProtectedTag.matching(tag_name, protected_tags: @protected_tags).present? + end + def user_can_push_to_empty_repo?(user) !default_branch_protected? || team.max_member_access(user.id) > Gitlab::Access::DEVELOPER end diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index c85f79127bc..0d8f114cc59 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -10,6 +10,7 @@ module Gitlab ) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @branch_name = Gitlab::Git.branch_name(@ref) + @tag_name = Gitlab::Git.tag_name(@ref) @user_access = user_access @project = project @env = env @@ -36,7 +37,7 @@ module Gitlab if forced_push? return "You are not allowed to force push code to a protected branch on this project." - elsif Gitlab::Git.blank_ref?(@newrev) + elsif blank_ref? return "You are not allowed to delete protected branches from this project." end @@ -58,11 +59,33 @@ module Gitlab def tag_checks return if skip_authorization - tag_ref = Gitlab::Git.tag_name(@ref) + return unless @tag_name - if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) + if tag_exists? && user_access.cannot_do_action?(:admin_project) "You are not allowed to change existing tags on this project." end + + protected_tag_checks + end + + def protected_tag_checks + return unless tag_protected? + + if forced_push? + return "You are not allowed to force push protected tags." #TODO: Wording, 'not allowed to update proteted tags'? + end + + if Gitlab::Git.blank_ref?(@newrev) + return "You are not allowed to delete protected tags." #TODO: Wording, do these need to mention 'you' if the rule applies to everyone + end + + if !user_access.can_push_tag?(@tag_name) + return "You are not allowed to create protected tags on this project." #TODO: Wording, it is a specific tag which you don't have access too, not all protected tags which might have different levels + end + end + + def tag_protected? + project.protected_tag?(@tag_name) end def push_checks @@ -75,14 +98,18 @@ module Gitlab private - def protected_tag?(tag_name) - project.repository.tag_exists?(tag_name) + def tag_exists? + project.repository.tag_exists?(@tag_name) end def forced_push? Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env) end + def blank_ref? + Gitlab::Git.blank_ref?(@newrev) + end + def matching_merge_request? Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match? end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index f260c0c535f..921159d91ef 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -28,6 +28,22 @@ module Gitlab true end + #TODO: Test this + #TODO move most to ProtectedTag::AccessChecker. Or maybe UserAccess::Protections::Tag + #TODO: then consider removing method, if it turns out can_access_git? and can?(:push_code are checked in change_access + def can_push_tag?(ref) + return false unless can_access_git? + + if project.protected_tag?(ref) + access_levels = project.protected_tags.matching(ref).map(&:push_access_levels).flatten + has_access = access_levels.any? { |access_level| access_level.check_access(user) } + + has_access + else + user.can?(:push_code, project) + end + end + def can_push_to_branch?(ref) return false unless can_access_git? diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index e22f88b7a32..cb2989c32df 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -23,7 +23,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do ).exec end - before { allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) } + before { project.add_developer(user) } context 'without failed checks' do it "doesn't return any error" do @@ -50,11 +50,51 @@ describe Gitlab::Checks::ChangeAccess, lib: true do end it 'returns an error if the user is not allowed to update tags' do + allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false) expect(subject.status).to be(false) expect(subject.message).to eq('You are not allowed to change existing tags on this project.') end + + context 'with protected tag' do + let!(:protected_tag) { create(:protected_tag, project: project, name: 'v*') } + + context 'deletion' do + let(:changes) do + { + oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', + newrev: '0000000000000000000000000000000000000000', + ref: 'refs/tags/v1.0.0' + } + end + + it 'is prevented' do + expect(subject.status).to be(false) + expect(subject.message).to include('delete protected tags') + end + end + + it 'prevents force push' do + expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true) + + expect(subject.status).to be(false) + expect(subject.message).to include('force push protected tags') + end + + it 'prevents creation below access level' do + expect(subject.status).to be(false) + expect(subject.message).to include('allowed to') + end + + context 'when user has access' do + let!(:protected_tag) { create(:protected_tag, :developers_can_push, project: project, name: 'v*') } + + it 'allows tag creation' do + expect(subject.status).to be(true) + end + end + end end context 'protected branches check' do -- cgit v1.2.1 From ab46353fd9c0c76c137bf828788ecbc34d0fe99a Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 31 Mar 2017 19:29:25 +0100 Subject: Added ProtectedTags#show page Uncommented protected tags feature specs copied from protected branches --- .../protected_tags/_matching_tag.html.haml | 9 + app/views/projects/protected_tags/show.html.haml | 25 +++ .../protected_tags/access_control_ce_spec.rb | 117 +++++-------- spec/features/protected_tags_spec.rb | 188 ++++++++++----------- 4 files changed, 167 insertions(+), 172 deletions(-) create mode 100644 app/views/projects/protected_tags/_matching_tag.html.haml create mode 100644 app/views/projects/protected_tags/show.html.haml diff --git a/app/views/projects/protected_tags/_matching_tag.html.haml b/app/views/projects/protected_tags/_matching_tag.html.haml new file mode 100644 index 00000000000..97e5cd6f9d2 --- /dev/null +++ b/app/views/projects/protected_tags/_matching_tag.html.haml @@ -0,0 +1,9 @@ +%tr + %td + = link_to matching_tag.name, namespace_project_tree_path(@project.namespace, @project, matching_tag.name) + - if @project.root_ref?(matching_tag.name) + %span.label.label-info.prepend-left-5 default + %td + - commit = @project.commit(matching_tag.name) + = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id') + = time_ago_with_tooltip(commit.committed_date) diff --git a/app/views/projects/protected_tags/show.html.haml b/app/views/projects/protected_tags/show.html.haml new file mode 100644 index 00000000000..185807a7e8d --- /dev/null +++ b/app/views/projects/protected_tags/show.html.haml @@ -0,0 +1,25 @@ +- page_title @protected_tag.name, "Protected Tags" + +.row.prepend-top-default.append-bottom-default + .col-lg-3 + %h4.prepend-top-0 + = @protected_tag.name + + .col-lg-9 + %h5 Matching Tags + - if @matching_tags.present? + .table-responsive + %table.table.protected-tags-list + %colgroup + %col{ width: "30%" } + %col{ width: "30%" } + %thead + %tr + %th Tag + %th Last commit + %tbody + - @matching_tags.each do |matching_tag| + = render partial: "matching_tag", object: matching_tag + - else + %p.settings-message.text-center + Couldn't find any matching tags. diff --git a/spec/features/protected_tags/access_control_ce_spec.rb b/spec/features/protected_tags/access_control_ce_spec.rb index 545d3bca74d..d7152926629 100644 --- a/spec/features/protected_tags/access_control_ce_spec.rb +++ b/spec/features/protected_tags/access_control_ce_spec.rb @@ -1,79 +1,40 @@ -# RSpec.shared_examples "protected tags > access control > CE" do -# ProtectedTag::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| -# it "allows creating protected tags that #{access_type_name} can push to" do -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('master') -# within('.new_protected_tag') do -# allowed_to_push_button = find(".js-allowed-to-push") - -# unless allowed_to_push_button.text == access_type_name -# allowed_to_push_button.click -# within(".dropdown.open .dropdown-menu") { click_on access_type_name } -# end -# end -# click_on "Protect" - -# expect(ProtectedTag.count).to eq(1) -# expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to eq([access_type_id]) -# end - -# it "allows updating protected tags so that #{access_type_name} can push to 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) - -# within(".protected-tags-list") do -# find(".js-allowed-to-push").click +RSpec.shared_examples "protected tags > access control > CE" do + ProtectedTag::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| + it "allows creating protected tags that #{access_type_name} can push to" do + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('master') + within('.new_protected_tag') do + allowed_to_push_button = find(".js-allowed-to-push") + + unless allowed_to_push_button.text == access_type_name + allowed_to_push_button.click + within(".dropdown.open .dropdown-menu") { click_on access_type_name } + end + end + click_on "Protect" + + expect(ProtectedTag.count).to eq(1) + expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to eq([access_type_id]) + end + + it "allows updating protected tags so that #{access_type_name} can push to 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) + + within(".protected-tags-list") do + find(".js-allowed-to-push").click -# within('.js-allowed-to-push-container') do -# expect(first("li")).to have_content("Roles") -# click_on access_type_name -# end -# end - -# wait_for_ajax -# expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to include(access_type_id) -# end -# end - -# ProtectedTag::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| -# it "allows creating protected tags that #{access_type_name} can merge to" do -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('master') -# within('.new_protected_tag') do -# allowed_to_merge_button = find(".js-allowed-to-merge") - -# unless allowed_to_merge_button.text == access_type_name -# allowed_to_merge_button.click -# within(".dropdown.open .dropdown-menu") { click_on access_type_name } -# end -# end -# click_on "Protect" - -# expect(ProtectedTag.count).to eq(1) -# expect(ProtectedTag.last.merge_access_levels.map(&:access_level)).to eq([access_type_id]) -# end - -# it "allows updating protected tags so that #{access_type_name} can merge to 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) - -# within(".protected-tags-list") do -# find(".js-allowed-to-merge").click - -# within('.js-allowed-to-merge-container') do -# expect(first("li")).to have_content("Roles") -# click_on access_type_name -# end -# end - -# wait_for_ajax -# expect(ProtectedTag.last.merge_access_levels.map(&:access_level)).to include(access_type_id) -# end -# end -# end + within('.js-allowed-to-push-container') do + expect(first("li")).to have_content("Roles") + click_on access_type_name + end + end + + wait_for_ajax + expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to include(access_type_id) + end + end +end diff --git a/spec/features/protected_tags_spec.rb b/spec/features/protected_tags_spec.rb index 546d6037ef7..09e8c850de3 100644 --- a/spec/features/protected_tags_spec.rb +++ b/spec/features/protected_tags_spec.rb @@ -1,94 +1,94 @@ -# require 'spec_helper' -# Dir["./spec/features/protected_tags/*.rb"].sort.each { |f| require f } - -# feature 'Projected Tags', feature: true, js: true do -# include WaitForAjax - -# let(:user) { create(:user, :admin) } -# let(:project) { create(:project) } - -# before { login_as(user) } - -# def set_protected_tag_name(tag_name) -# find(".js-protected-tag-select").click -# find(".dropdown-input-field").set(tag_name) -# click_on("Create wildcard #{tag_name}") -# end - -# describe "explicit protected tags" do -# it "allows creating explicit protected tags" do -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('some-tag') -# click_on "Protect" - -# within(".protected-tags-list") { expect(page).to have_content('some-tag') } -# expect(ProtectedTag.count).to eq(1) -# expect(ProtectedTag.last.name).to eq('some-tag') -# end - -# it "displays the last commit on the matching tag if it exists" do -# commit = create(:commit, project: project) -# project.repository.add_tag(user, 'some-tag', commit.id) - -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('some-tag') -# click_on "Protect" - -# within(".protected-tags-list") { expect(page).to have_content(commit.id[0..7]) } -# end - -# it "displays an error message if the named tag does not exist" do -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('some-tag') -# click_on "Protect" - -# within(".protected-tags-list") { expect(page).to have_content('tag was removed') } -# end -# end - -# describe "wildcard protected tags" do -# it "allows creating protected tags with a wildcard" do -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('*-stable') -# click_on "Protect" - -# within(".protected-tags-list") { expect(page).to have_content('*-stable') } -# expect(ProtectedTag.count).to eq(1) -# expect(ProtectedTag.last.name).to eq('*-stable') -# end - -# it "displays the number of matching tags" do -# project.repository.add_tag(user, 'production-stable', 'master') -# project.repository.add_tag(user, 'staging-stable', 'master') - -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('*-stable') -# click_on "Protect" - -# within(".protected-tags-list") { expect(page).to have_content("2 matching tags") } -# end - -# it "displays all the tags matching the wildcard" do -# project.repository.add_tag(user, 'production-stable', 'master') -# project.repository.add_tag(user, 'staging-stable', 'master') -# project.repository.add_tag(user, 'development', 'master') - -# visit namespace_project_protected_tags_path(project.namespace, project) -# set_protected_tag_name('*-stable') -# click_on "Protect" - -# visit namespace_project_protected_tags_path(project.namespace, project) -# click_on "2 matching tags" - -# within(".protected-tags-list") do -# expect(page).to have_content("production-stable") -# expect(page).to have_content("staging-stable") -# expect(page).not_to have_content("development") -# end -# end -# end - -# describe "access control" do -# include_examples "protected tags > access control > CE" -# end -# end +require 'spec_helper' +Dir["./spec/features/protected_tags/*.rb"].sort.each { |f| require f } + +feature 'Projected Tags', feature: true, js: true do + include WaitForAjax + + let(:user) { create(:user, :admin) } + let(:project) { create(:project) } + + before { login_as(user) } + + def set_protected_tag_name(tag_name) + find(".js-protected-tag-select").click + find(".dropdown-input-field").set(tag_name) + click_on("Create wildcard #{tag_name}") + end + + describe "explicit protected tags" do + it "allows creating explicit protected tags" do + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('some-tag') + click_on "Protect" + + within(".protected-tags-list") { expect(page).to have_content('some-tag') } + expect(ProtectedTag.count).to eq(1) + expect(ProtectedTag.last.name).to eq('some-tag') + end + + it "displays the last commit on the matching tag if it exists" do + commit = create(:commit, project: project) + project.repository.add_tag(user, 'some-tag', commit.id) + + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('some-tag') + click_on "Protect" + + within(".protected-tags-list") { expect(page).to have_content(commit.id[0..7]) } + end + + it "displays an error message if the named tag does not exist" do + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('some-tag') + click_on "Protect" + + within(".protected-tags-list") { expect(page).to have_content('tag was removed') } + end + end + + describe "wildcard protected tags" do + it "allows creating protected tags with a wildcard" do + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('*-stable') + click_on "Protect" + + within(".protected-tags-list") { expect(page).to have_content('*-stable') } + expect(ProtectedTag.count).to eq(1) + expect(ProtectedTag.last.name).to eq('*-stable') + end + + it "displays the number of matching tags" do + project.repository.add_tag(user, 'production-stable', 'master') + project.repository.add_tag(user, 'staging-stable', 'master') + + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('*-stable') + click_on "Protect" + + within(".protected-tags-list") { expect(page).to have_content("2 matching tags") } + end + + it "displays all the tags matching the wildcard" do + project.repository.add_tag(user, 'production-stable', 'master') + project.repository.add_tag(user, 'staging-stable', 'master') + project.repository.add_tag(user, 'development', 'master') + + visit namespace_project_protected_tags_path(project.namespace, project) + set_protected_tag_name('*-stable') + click_on "Protect" + + visit namespace_project_protected_tags_path(project.namespace, project) + click_on "2 matching tags" + + within(".protected-tags-list") do + expect(page).to have_content("production-stable") + expect(page).to have_content("staging-stable") + expect(page).not_to have_content("development") + end + end + end + + describe "access control" do + include_examples "protected tags > access control > CE" + end +end -- cgit v1.2.1 From 553cf9ea54ccb0736a8b44e2f3d047d0860aa71e Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 31 Mar 2017 19:30:33 +0100 Subject: =?UTF-8?q?Added=20=E2=80=98protected=E2=80=99=20label=20and=20dis?= =?UTF-8?q?abled=20delete=20button=20for=20tags=20index/show?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/helpers/tags_helper.rb | 4 ++++ app/views/projects/tags/_tag.html.haml | 7 ++++++- app/views/projects/tags/show.html.haml | 5 ++++- lib/gitlab/checks/change_access.rb | 2 +- 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app/helpers/tags_helper.rb b/app/helpers/tags_helper.rb index c0ec1634cdb..6672e3da348 100644 --- a/app/helpers/tags_helper.rb +++ b/app/helpers/tags_helper.rb @@ -21,4 +21,8 @@ module TagsHelper html.html_safe end + + def protected_tag?(project, tag) + project.protected_tag?(tag.name) + end end diff --git a/app/views/projects/tags/_tag.html.haml b/app/views/projects/tags/_tag.html.haml index dffe908e85a..451e011a4b8 100644 --- a/app/views/projects/tags/_tag.html.haml +++ b/app/views/projects/tags/_tag.html.haml @@ -6,6 +6,11 @@ %span.item-title = icon('tag') = tag.name + + - if protected_tag?(@project, tag) + %span.label.label-success + protected + - if tag.message.present?   = strip_gpg_signature(tag.message) @@ -30,5 +35,5 @@ = icon("pencil") - if can?(current_user, :admin_project, @project) - = link_to namespace_project_tag_path(@project.namespace, @project, tag.name), class: 'btn btn-remove remove-row has-tooltip', title: "Delete tag", method: :delete, data: { confirm: "Deleting the '#{tag.name}' tag cannot be undone. Are you sure?", container: 'body' }, remote: true do + = link_to namespace_project_tag_path(@project.namespace, @project, tag.name), class: "btn btn-remove remove-row has-tooltip #{protected_tag?(@project, tag) ? 'disabled' : ''}", title: "Delete tag", method: :delete, data: { confirm: "Deleting the '#{tag.name}' tag cannot be undone. Are you sure?", container: 'body' }, remote: true do = icon("trash-o") diff --git a/app/views/projects/tags/show.html.haml b/app/views/projects/tags/show.html.haml index fad3c5c2173..1c4135c8a54 100644 --- a/app/views/projects/tags/show.html.haml +++ b/app/views/projects/tags/show.html.haml @@ -7,6 +7,9 @@ .nav-text .title %span.item-title= @tag.name + - if protected_tag?(@project, @tag) + %span.label.label-success + protected - if @commit = render 'projects/branches/commit', commit: @commit, project: @project - else @@ -24,7 +27,7 @@ = render 'projects/buttons/download', project: @project, ref: @tag.name - if can?(current_user, :admin_project, @project) .btn-container.controls-item-full - = link_to namespace_project_tag_path(@project.namespace, @project, @tag.name), class: 'btn btn-remove remove-row has-tooltip', title: "Delete tag", method: :delete, data: { confirm: "Deleting the '#{@tag.name}' tag cannot be undone. Are you sure?" } do + = link_to namespace_project_tag_path(@project.namespace, @project, @tag.name), class: "btn btn-remove remove-row has-tooltip #{protected_tag?(@project, @tag) ? 'disabled' : ''}", title: "Delete tag", method: :delete, data: { confirm: "Deleting the '#{@tag.name}' tag cannot be undone. Are you sure?" } do %i.fa.fa-trash-o - if @tag.message.present? diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 0d8f114cc59..540d95f2d1f 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -80,7 +80,7 @@ module Gitlab end if !user_access.can_push_tag?(@tag_name) - return "You are not allowed to create protected tags on this project." #TODO: Wording, it is a specific tag which you don't have access too, not all protected tags which might have different levels + return "You are not allowed to create this tag as it is protected." end end -- cgit v1.2.1 From 85e1fa8e21c73746bb6f6270a658453a230bb9a2 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:40:21 +0530 Subject: ProtectedTagEdit class for edit protected tags --- .../protected_tags/protected_tag_edit.js | 54 ++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 app/assets/javascripts/protected_tags/protected_tag_edit.js diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit.js b/app/assets/javascripts/protected_tags/protected_tag_edit.js new file mode 100644 index 00000000000..b93e903621e --- /dev/null +++ b/app/assets/javascripts/protected_tags/protected_tag_edit.js @@ -0,0 +1,54 @@ +/* eslint-disable no-new, arrow-parens, no-param-reassign, comma-dangle, max-len */ +/* global Flash */ + +(global => { + global.gl = global.gl || {}; + + gl.ProtectedTagEdit = class { + constructor(options) { + this.$wrap = options.$wrap; + this.$allowedToPushDropdown = this.$wrap.find('.js-allowed-to-push'); + + this.buildDropdowns(); + } + + buildDropdowns() { + // Allowed to push dropdown + new gl.ProtectedTagAccessDropdown({ + $dropdown: this.$allowedToPushDropdown, + data: gon.push_access_levels, + onSelect: this.onSelect.bind(this) + }); + } + + onSelect() { + const $allowedToPushInput = this.$wrap.find(`input[name="${this.$allowedToPushDropdown.data('fieldName')}"]`); + + // Do not update if one dropdown has not selected any option + if (!$allowedToPushInput.length) return; + + this.$allowedToPushDropdown.disable(); + + $.ajax({ + type: 'POST', + url: this.$wrap.data('url'), + dataType: 'json', + data: { + _method: 'PATCH', + protected_tag: { + push_access_levels_attributes: [{ + id: this.$allowedToPushDropdown.data('access-level-id'), + access_level: $allowedToPushInput.val() + }] + } + }, + error() { + $.scrollTo(0); + new Flash('Failed to update tag!'); + } + }).always(() => { + this.$allowedToPushDropdown.enable(); + }); + } + }; +})(window); -- cgit v1.2.1 From 551dea7efe11c55799e4a0cacd9c75988ef9583b Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:40:43 +0530 Subject: Protected Tags List initializer --- .../protected_tags/protected_tag_edit_list.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 app/assets/javascripts/protected_tags/protected_tag_edit_list.js diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit_list.js b/app/assets/javascripts/protected_tags/protected_tag_edit_list.js new file mode 100644 index 00000000000..ba40c227ef4 --- /dev/null +++ b/app/assets/javascripts/protected_tags/protected_tag_edit_list.js @@ -0,0 +1,18 @@ +/* eslint-disable arrow-parens, no-param-reassign, no-new, comma-dangle */ + +(global => { + global.gl = global.gl || {}; + + gl.ProtectedTagEditList = class { + constructor() { + this.$wrap = $('.protected-tags-list'); + + // Build edit forms + this.$wrap.find('.js-protected-tag-edit-form').each((i, el) => { + new gl.ProtectedTagEdit({ + $wrap: $(el) + }); + }); + } + }; +})(window); -- cgit v1.2.1 From bbb09feaa1b535e10b20790d1857385bd34ae5f6 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:41:05 +0530 Subject: Export Protected Tags Editing classes --- app/assets/javascripts/protected_tags/protected_tags_bundle.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/protected_tags/protected_tags_bundle.js b/app/assets/javascripts/protected_tags/protected_tags_bundle.js index d84d2e1ef70..889a8053e6f 100644 --- a/app/assets/javascripts/protected_tags/protected_tags_bundle.js +++ b/app/assets/javascripts/protected_tags/protected_tags_bundle.js @@ -1,3 +1,5 @@ require('./protected_tag_access_dropdown'); require('./protected_tag_create'); require('./protected_tag_dropdown'); +require('./protected_tag_edit'); +require('./protected_tag_edit_list'); -- cgit v1.2.1 From 8d232b2f2b1b4d33741925a0baf2eaf74f52008e Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:41:28 +0530 Subject: Initialize Protected Tags Edit functionality --- app/assets/javascripts/dispatcher.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 6234092bdc2..d384927cc5b 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -318,9 +318,12 @@ const UserCallout = require('./user_callout'); new Search(); break; case 'projects:repository:show': + // Initialize Protected Branch Settings new gl.ProtectedBranchCreate(); new gl.ProtectedBranchEditList(); + // Initialize Protected Tag Settings new gl.ProtectedTagCreate(); + new gl.ProtectedTagEditList(); break; case 'projects:ci_cd:show': new gl.ProjectVariables(); -- cgit v1.2.1 From 48150f273d8b7cc5b042eb0a9f88b0a7a96431f4 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:42:05 +0530 Subject: Render Push levels dropdown --- app/views/projects/protected_tags/_update_protected_tag.haml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/projects/protected_tags/_update_protected_tag.haml b/app/views/projects/protected_tags/_update_protected_tag.haml index 729a784a559..802a5ef3b98 100644 --- a/app/views/projects/protected_tags/_update_protected_tag.haml +++ b/app/views/projects/protected_tags/_update_protected_tag.haml @@ -1,5 +1,5 @@ %td = hidden_field_tag "allowed_to_push_#{protected_tag.id}", protected_tag.push_access_levels.first.access_level - / = dropdown_tag( (protected_tag.push_access_levels.first.humanize || 'Select') , - / options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', - / data: { field_name: "allowed_to_push_#{protected_tag.id}", access_level_id: protected_tag.push_access_levels.first.id }}) + = dropdown_tag( (protected_tag.push_access_levels.first.humanize || 'Select') , + options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', + data: { field_name: "allowed_to_push_#{protected_tag.id}", access_level_id: protected_tag.push_access_levels.first.id }}) -- cgit v1.2.1 From df54560f5f5d94fd9117c21d03d946d07d28f6fa Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:42:37 +0530 Subject: Increase dropdown width within Tags list --- app/assets/stylesheets/pages/projects.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index f7ef9275560..7c93ee8af18 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -778,6 +778,12 @@ pre.light-well { } } +.protected-tags-list { + .dropdown-menu-toggle { + width: 300px; + } +} + .custom-notifications-form { .is-loading { .custom-notification-event-loading { -- cgit v1.2.1 From a7c71c7f292c9cdf892f7d33dfb52d7e16af28e6 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 3 Apr 2017 11:42:55 +0530 Subject: Update column widths --- app/views/projects/protected_tags/_tags_list.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/protected_tags/_tags_list.html.haml b/app/views/projects/protected_tags/_tags_list.html.haml index 6dcd356e6f1..6f63971923d 100644 --- a/app/views/projects/protected_tags/_tags_list.html.haml +++ b/app/views/projects/protected_tags/_tags_list.html.haml @@ -11,8 +11,8 @@ %table.table.table-bordered %colgroup %col{ width: "25%" } - %col{ width: "55%" } - %col{ width: "20%" } + %col{ width: "25%" } + %col{ width: "50%" } %thead %tr %th Protected tag (#{@protected_tags.size}) -- cgit v1.2.1 From 65f3d5062f081d8f8ebf727a3408650d90ec9711 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 15:17:24 +0100 Subject: Extract ProtectedRef Concern --- app/models/concerns/protected_ref.rb | 47 ++++++++++++++++++++++ app/models/project.rb | 4 +- app/models/protected_branch.rb | 31 +------------- app/models/protected_tag.rb | 31 +------------- lib/api/entities.rb | 4 +- lib/gitlab/user_access.rb | 11 ++--- spec/models/protected_branch_spec.rb | 8 ++-- .../services/protected_tags/create_service_spec.rb | 2 - 8 files changed, 60 insertions(+), 78 deletions(-) create mode 100644 app/models/concerns/protected_ref.rb diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb new file mode 100644 index 00000000000..77d7b597534 --- /dev/null +++ b/app/models/concerns/protected_ref.rb @@ -0,0 +1,47 @@ +module ProtectedRef + extend ActiveSupport::Concern + + included do + belongs_to :project + validates :name, presence: true + validates :project, presence: true + + def self.matching_refs_accesible_to(ref, user, action: :push) + access_levels_for_ref(ref, action).any? do |access_level| + access_level.check_access(user) + end + end + + def self.access_levels_for_ref(ref, action: :push) + self.matching(ref).map(&:"@#{action}_access_levels").flatten + end + + private + + def self.matching(ref_name, protected_refs: nil) + ProtectedRefMatcher.matching(self, ref_name, protected_refs: protected_refs) + end + end + + def commit + project.commit(self.name) + end + + def matching(refs) + ref_matcher.matching(refs) + end + + def matches?(refs) + ref_matcher.matches?(refs) + end + + def wildcard? + ref_matcher.wildcard? + end + + private + + def ref_matcher + @ref_matcher ||= ProtectedRefMatcher.new(self) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 7681da36335..970de324a5b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -898,14 +898,14 @@ class Project < ActiveRecord::Base return true if empty_repo? && default_branch_protected? @protected_branches ||= self.protected_branches.to_a - ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present? + ProtectedBranch.matching(branch_name, protected_refs: @protected_branches).present? end #TODO: Move elsewhere def protected_tag?(tag_name) #TODO: Check if memoization necessary, find way to have it work elsewhere @protected_tags ||= self.protected_tags.to_a - ProtectedTag.matching(tag_name, protected_tags: @protected_tags).present? + ProtectedTag.matching(tag_name, protected_refs: @protected_tags).present? end def user_can_push_to_empty_repo?(user) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 7681d5b5112..a0dbcf80c3d 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -1,9 +1,6 @@ class ProtectedBranch < ActiveRecord::Base include Gitlab::ShellAdapter - - belongs_to :project - validates :name, presence: true - validates :project, presence: true + include ProtectedRef has_many :merge_access_levels, dependent: :destroy has_many :push_access_levels, dependent: :destroy @@ -13,30 +10,4 @@ class ProtectedBranch < ActiveRecord::Base accepts_nested_attributes_for :push_access_levels accepts_nested_attributes_for :merge_access_levels - - def commit - project.commit(self.name) - end - - def self.matching(branch_name, protected_branches: nil) - ProtectedRefMatcher.matching(ProtectedBranch, branch_name, protected_refs: protected_branches) - end - - def matching(branches) - ref_matcher.matching(branches) - end - - def matches?(branch_name) - ref_matcher.matches?(branch_name) - end - - def wildcard? - ref_matcher.wildcard? - end - - private - - def ref_matcher - @ref_matcher ||= ProtectedRefMatcher.new(self) - end end diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index d307549aa49..301fe2092e9 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -1,39 +1,10 @@ class ProtectedTag < ActiveRecord::Base include Gitlab::ShellAdapter - - belongs_to :project - validates :name, presence: true - validates :project, presence: true + include ProtectedRef has_many :push_access_levels, dependent: :destroy validates :push_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." } accepts_nested_attributes_for :push_access_levels - - def commit - project.commit(self.name) - end - - def self.matching(tag_name, protected_tags: nil) - ProtectedRefMatcher.matching(ProtectedTag, tag_name, protected_refs: protected_tags) - end - - def matching(branches) - ref_matcher.matching(branches) - end - - def matches?(tag_name) - ref_matcher.matches?(tag_name) - end - - def wildcard? - ref_matcher.wildcard? - end - - private - - def ref_matcher - @ref_matcher ||= ProtectedRefMatcher.new(self) - end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5954aea8041..e000e3e33d0 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -189,13 +189,13 @@ module API expose :developers_can_push do |repo_branch, options| project = options[:project] - access_levels = project.protected_branches.matching(repo_branch.name).map(&:push_access_levels).flatten + access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :push) access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } end expose :developers_can_merge do |repo_branch, options| project = options[:project] - access_levels = project.protected_branches.matching(repo_branch.name).map(&:merge_access_levels).flatten + access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :merge) access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 921159d91ef..5a5a4ebd08b 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -35,10 +35,7 @@ module Gitlab return false unless can_access_git? if project.protected_tag?(ref) - access_levels = project.protected_tags.matching(ref).map(&:push_access_levels).flatten - has_access = access_levels.any? { |access_level| access_level.check_access(user) } - - has_access + project.protected_tags.matching_refs_accesible_to(ref, user) else user.can?(:push_code, project) end @@ -50,8 +47,7 @@ module Gitlab if project.protected_branch?(ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) - access_levels = project.protected_branches.matching(ref).map(&:push_access_levels).flatten - has_access = access_levels.any? { |access_level| access_level.check_access(user) } + has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push) has_access || !project.repository.branch_exists?(ref) && can_merge_to_branch?(ref) else @@ -63,8 +59,7 @@ module Gitlab return false unless can_access_git? if project.protected_branch?(ref) - access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten - access_levels.any? { |access_level| access_level.check_access(user) } + project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge) else user.can?(:push_code, project) end diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 8bf0d24a128..1c02f8bfc3f 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -113,8 +113,8 @@ describe ProtectedBranch, models: true do staging = build(:protected_branch, name: "staging") expect(ProtectedBranch.matching("production")).to be_empty - expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).to include(production) - expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).not_to include(staging) + expect(ProtectedBranch.matching("production", protected_refs: [production, staging])).to include(production) + expect(ProtectedBranch.matching("production", protected_refs: [production, staging])).not_to include(staging) end end @@ -132,8 +132,8 @@ describe ProtectedBranch, models: true do staging = build(:protected_branch, name: "staging/*") expect(ProtectedBranch.matching("production/some-branch")).to be_empty - expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).to include(production) - expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).not_to include(staging) + expect(ProtectedBranch.matching("production/some-branch", protected_refs: [production, staging])).to include(production) + expect(ProtectedBranch.matching("production/some-branch", protected_refs: [production, staging])).not_to include(staging) end end end diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb index e1fd73dbd07..70ea96a954f 100644 --- a/spec/services/protected_tags/create_service_spec.rb +++ b/spec/services/protected_tags/create_service_spec.rb @@ -6,7 +6,6 @@ describe ProtectedTags::CreateService, services: true do let(:params) do { name: 'master', - merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }], push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }] } end @@ -17,7 +16,6 @@ describe ProtectedTags::CreateService, services: true do it 'creates a new protected tag' do expect { service.execute }.to change(ProtectedTag, :count).by(1) expect(project.protected_tags.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) - expect(project.protected_tags.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) end end end -- cgit v1.2.1 From b8c7bef5c092152ea85d1840e587cfc04293e1d7 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 17:10:58 +0100 Subject: Extracted ProtectableDropdown to clean up Project#open_branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makes it clear this is only used in dropdowns, instead of cluttering up Project class. Since we only care about branch names, it is also possible to refactor out a lot of the set/reject logic. A benchmark on Array/Set subtraction favoured using Arrays. This was with 5000 ‘branches’ and 2000 ‘protections’ to ensure a similar comparison to the commit which introduced using Set for intersection. Comparison: array subtraction: 485.8 i/s set subtraction: 128.7 i/s - 3.78x slower --- .../projects/settings/repository_controller.rb | 25 ++++++--------------- app/models/project.rb | 9 -------- app/models/protectable_dropdown.rb | 26 ++++++++++++++++++++++ spec/models/project_spec.rb | 19 ---------------- spec/models/protectable_dropdown_spec.rb | 24 ++++++++++++++++++++ 5 files changed, 57 insertions(+), 46 deletions(-) create mode 100644 app/models/protectable_dropdown.rb create mode 100644 spec/models/protectable_dropdown_spec.rb diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 5160ee5e1e4..a87927fe1ec 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -4,8 +4,7 @@ module Projects before_action :authorize_admin_project! def show - @deploy_keys = DeployKeysPresenter - .new(@project, current_user: current_user) + @deploy_keys = DeployKeysPresenter.new(@project, current_user: current_user) define_protected_refs end @@ -38,27 +37,17 @@ module Projects } end - #TODO: Move to Protections::TagMatcher.new(project).unprotected - def unprotected_tags - exact_protected_tag_names = @project.protected_tags.reject(&:wildcard?).map(&:name) - tag_names = @project.repository.tags.map(&:name) - non_open_tag_names = Set.new(exact_protected_tag_names).intersection(Set.new(tag_names)) - @project.repository.tags.reject { |tag| non_open_tag_names.include? tag.name } + def protectable_tags_for_dropdown + { open_tags: ProtectableDropdown.new(@project, :tags).hash } end - def unprotected_tags_hash - tags = unprotected_tags.map { |tag| { text: tag.name, id: tag.name, title: tag.name } } - { open_tags: tags } - end - - def open_branches - branches = @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } - { open_branches: branches } + def protectable_branches_for_dropdown + { open_branches: ProtectableDropdown.new(@project, :branches).hash } end def load_gon_index - gon.push(open_branches) - gon.push(unprotected_tags_hash) + gon.push(protectable_tags_for_dropdown) + gon.push(protectable_branches_for_dropdown) gon.push(access_levels_options) end end diff --git a/app/models/project.rb b/app/models/project.rb index 970de324a5b..4175bfab0a9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -865,15 +865,6 @@ class Project < ActiveRecord::Base @repo_exists = false end - # Branches that are not _exactly_ matched by a protected branch. - #TODO: Move to Protections::BranchMatcher.new(project).unprotecte - def open_branches - exact_protected_branch_names = protected_branches.reject(&:wildcard?).map(&:name) - branch_names = repository.branches.map(&:name) - non_open_branch_names = Set.new(exact_protected_branch_names).intersection(Set.new(branch_names)) - repository.branches.reject { |branch| non_open_branch_names.include? branch.name } - end - def root_ref?(branch) repository.root_ref == branch end diff --git a/app/models/protectable_dropdown.rb b/app/models/protectable_dropdown.rb new file mode 100644 index 00000000000..c9b2b213cd2 --- /dev/null +++ b/app/models/protectable_dropdown.rb @@ -0,0 +1,26 @@ +class ProtectableDropdown + def initialize(project, ref_type) + @project = project + @ref_type = ref_type + end + + # 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) + end + + def hash + protectable_ref_names.map { |ref_name| { text: ref_name, id: ref_name, title: ref_name } } + end + + private + + def refs + @project.repository.public_send(@ref_type) + end + + def protections + @project.public_send("protected_#{@ref_type}") + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f68631ebe06..cc06949974e 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -702,25 +702,6 @@ describe Project, models: true do end end - describe '#open_branches' do - let(:project) { create(:project, :repository) } - - before do - project.protected_branches.create(name: 'master') - end - - it { expect(project.open_branches.map(&:name)).to include('feature') } - it { expect(project.open_branches.map(&:name)).not_to include('master') } - - it "includes branches matching a protected branch wildcard" do - expect(project.open_branches.map(&:name)).to include('feature') - - create(:protected_branch, name: 'feat*', project: project) - - expect(Project.find(project.id).open_branches.map(&:name)).to include('feature') - end - end - describe '#star_count' do it 'counts stars from multiple users' do user1 = create :user diff --git a/spec/models/protectable_dropdown_spec.rb b/spec/models/protectable_dropdown_spec.rb new file mode 100644 index 00000000000..7f8ef7195e5 --- /dev/null +++ b/spec/models/protectable_dropdown_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe ProtectableDropdown, models: true do + let(:project) { create(:project, :repository) } + let(:subject) { described_class.new(project, :branches) } + + describe '#protectable_ref_names' do + before do + project.protected_branches.create(name: 'master') + end + + it { expect(subject.protectable_ref_names).to include('feature') } + it { expect(subject.protectable_ref_names).not_to include('master') } + + it "includes branches matching a protected branch wildcard" do + expect(subject.protectable_ref_names).to include('feature') + + create(:protected_branch, name: 'feat*', project: project) + + subject = described_class.new(project.reload, :branches) + expect(subject.protectable_ref_names).to include('feature') + end + end +end -- cgit v1.2.1 From bf3cc824e4ce6cf49a82210eaaf1cca06f7fd281 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 18:59:58 +0100 Subject: Moved Project#protected_branch? to ProtectedBranch, similar for tags --- app/helpers/branches_helper.rb | 2 +- app/helpers/tags_helper.rb | 2 +- app/models/merge_request.rb | 2 +- app/models/project.rb | 22 +++++------ app/models/protected_branch.rb | 8 ++++ app/models/protected_tag.rb | 5 +++ app/services/delete_branch_service.rb | 2 +- app/services/git_push_service.rb | 2 +- app/views/projects/branches/_branch.html.haml | 2 +- lib/api/entities.rb | 2 +- lib/gitlab/checks/change_access.rb | 4 +- lib/gitlab/user_access.rb | 6 +-- spec/lib/gitlab/checks/change_access_spec.rb | 2 +- spec/models/merge_request_spec.rb | 2 +- spec/models/project_spec.rb | 56 --------------------------- spec/models/protected_branch_spec.rb | 56 +++++++++++++++++++++++++++ 16 files changed, 94 insertions(+), 81 deletions(-) diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 3fc85dc6b2b..a852b90c57e 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -1,6 +1,6 @@ module BranchesHelper def can_remove_branch?(project, branch_name) - if project.protected_branch? branch_name + if ProtectedBranch.protected?(project, branch_name) false elsif branch_name == project.repository.root_ref false diff --git a/app/helpers/tags_helper.rb b/app/helpers/tags_helper.rb index 6672e3da348..31aaf9e5607 100644 --- a/app/helpers/tags_helper.rb +++ b/app/helpers/tags_helper.rb @@ -23,6 +23,6 @@ module TagsHelper end def protected_tag?(project, tag) - project.protected_tag?(tag.name) + ProtectedTag.protected?(project, tag.name) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index cef8ad76b07..38eefad96b6 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -442,7 +442,7 @@ class MergeRequest < ActiveRecord::Base end def can_remove_source_branch?(current_user) - !source_project.protected_branch?(source_branch) && + !ProtectedBranch.protected?(source_project, source_branch) && !source_project.root_ref?(source_branch) && Ability.allowed?(current_user, :push_code, source_project) && diff_head_commit == source_branch_head diff --git a/app/models/project.rb b/app/models/project.rb index 4175bfab0a9..5cc224b4440 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -883,20 +883,19 @@ class Project < ActiveRecord::Base "#{url}.git" end - # Check if current branch name is marked as protected in the system - #TODO: Move elsewhere - def protected_branch?(branch_name) - return true if empty_repo? && default_branch_protected? - @protected_branches ||= self.protected_branches.to_a - ProtectedBranch.matching(branch_name, protected_refs: @protected_branches).present? + def empty_and_default_branch_protected? + empty_repo? && default_branch_protected? end - #TODO: Move elsewhere - def protected_tag?(tag_name) - #TODO: Check if memoization necessary, find way to have it work elsewhere - @protected_tags ||= self.protected_tags.to_a - ProtectedTag.matching(tag_name, protected_refs: @protected_tags).present? + #TODO: Check with if this is still needed, maybe because of `.select {` in ProtectedRefsMatcher + #Either with tests or by asking Tim + def protected_tags_array + @protected_tags_array ||= self.protected_tags.to_a + end + + def protected_branches_array + @protected_branches_array ||= self.protected_branches.to_a end def user_can_push_to_empty_repo?(user) @@ -1367,6 +1366,7 @@ class Project < ActiveRecord::Base "projects/#{id}/pushes_since_gc" end + #TODO: Move this and methods which depend upon it def default_branch_protected? current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL || current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index a0dbcf80c3d..eca8d5e0f7d 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -10,4 +10,12 @@ class ProtectedBranch < ActiveRecord::Base accepts_nested_attributes_for :push_access_levels accepts_nested_attributes_for :merge_access_levels + + # Check if branch name is marked as protected in the system + def self.protected?(project, ref_name) + return true if project.empty_and_default_branch_protected? + + protected_refs = project.protected_branches_array + self.matching(ref_name, protected_refs: protected_refs).present? + end end diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index 301fe2092e9..bca5522759d 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -7,4 +7,9 @@ class ProtectedTag < ActiveRecord::Base validates :push_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." } accepts_nested_attributes_for :push_access_levels + + def self.protected?(project, ref_name) + protected_refs = project.protected_tags_array + self.matching(ref_name, protected_refs: protected_refs).present? + end end diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index 11a045f4c31..38a113caec7 100644 --- a/app/services/delete_branch_service.rb +++ b/app/services/delete_branch_service.rb @@ -11,7 +11,7 @@ class DeleteBranchService < BaseService return error('Cannot remove HEAD branch', 405) end - if project.protected_branch?(branch_name) + if ProtectedBranch.protected?(project, branch_name) return error('Protected branch cant be removed', 405) end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index bc7431c89a8..45411c779cc 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -127,7 +127,7 @@ class GitPushService < BaseService project.change_head(branch_name) # Set protection on the default branch if configured - if current_application_settings.default_branch_protection != PROTECTION_NONE && !@project.protected_branch?(@project.default_branch) + if current_application_settings.default_branch_protection != PROTECTION_NONE && !ProtectedBranch.protected?(@project, @project.default_branch) params = { name: @project.default_branch, diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index 9eb610ba9c0..d84fa9e55c0 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 @project.protected_branch? branch.name + - if ProtectedBranch.protected?(@project, branch.name) %span.label.label-success protected .controls.hidden-xs< diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e000e3e33d0..0fe7eb864e4 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -184,7 +184,7 @@ module API end expose :protected do |repo_branch, options| - options[:project].protected_branch?(repo_branch.name) + ProtectedBranch.protected?(options[:project], repo_branch.name) end expose :developers_can_push do |repo_branch, options| diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 540d95f2d1f..6f574a41727 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -33,7 +33,7 @@ module Gitlab def protected_branch_checks return if skip_authorization return unless @branch_name - return unless project.protected_branch?(@branch_name) + return unless ProtectedBranch.protected?(project, @branch_name) if forced_push? return "You are not allowed to force push code to a protected branch on this project." @@ -85,7 +85,7 @@ module Gitlab end def tag_protected? - project.protected_tag?(@tag_name) + ProtectedTag.protected?(project, @tag_name) end def push_checks diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 5a5a4ebd08b..5541a45e948 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -34,7 +34,7 @@ module Gitlab def can_push_tag?(ref) return false unless can_access_git? - if project.protected_tag?(ref) + if ProtectedTag.protected?(project, ref) project.protected_tags.matching_refs_accesible_to(ref, user) else user.can?(:push_code, project) @@ -44,7 +44,7 @@ module Gitlab def can_push_to_branch?(ref) return false unless can_access_git? - if project.protected_branch?(ref) + if ProtectedBranch.protected?(project, ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push) @@ -58,7 +58,7 @@ module Gitlab def can_merge_to_branch?(ref) return false unless can_access_git? - if project.protected_branch?(ref) + if ProtectedBranch.protected?(project, ref) project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge) else user.can?(:push_code, project) diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index cb2989c32df..8525422908b 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -99,7 +99,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do context 'protected branches check' do before do - allow(project).to receive(:protected_branch?).with('master').and_return(true) + allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true) end it 'returns an error if the user is not allowed to do forced pushes to protected branches' do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 24e7c1b17d9..2f6614c133e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -441,7 +441,7 @@ describe MergeRequest, models: true do end it "can't be removed when its a protected branch" do - allow(subject.source_project).to receive(:protected_branch?).and_return(true) + allow(ProtectedBranch).to receive(:protected?).and_return(true) expect(subject.can_remove_source_branch?(user)).to be_falsey end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cc06949974e..e6b23a1cc05 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1272,62 +1272,6 @@ describe Project, models: true do end end - describe '#protected_branch?' do - context 'existing project' do - let(:project) { create(:project, :repository) } - - it 'returns true when the branch matches a protected branch via direct match' do - create(:protected_branch, project: project, name: "foo") - - expect(project.protected_branch?('foo')).to eq(true) - end - - it 'returns true when the branch matches a protected branch via wildcard match' do - create(:protected_branch, project: project, name: "production/*") - - expect(project.protected_branch?('production/some-branch')).to eq(true) - end - - it 'returns false when the branch does not match a protected branch via direct match' do - expect(project.protected_branch?('foo')).to eq(false) - end - - it 'returns false when the branch does not match a protected branch via wildcard match' do - create(:protected_branch, project: project, name: "production/*") - - expect(project.protected_branch?('staging/some-branch')).to eq(false) - end - end - - context "new project" do - let(:project) { create(:empty_project) } - - it 'returns false when default_protected_branch is unprotected' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) - - expect(project.protected_branch?('master')).to be false - end - - it 'returns false when default_protected_branch lets developers push' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) - - expect(project.protected_branch?('master')).to be false - end - - it 'returns true when default_branch_protection does not let developers push but let developer merge branches' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) - - expect(project.protected_branch?('master')).to be true - end - - it 'returns true when default_branch_protection is in full protection' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL) - - expect(project.protected_branch?('master')).to be true - end - end - end - describe '#user_can_push_to_empty_repo?' do let(:project) { create(:empty_project) } let(:user) { create(:user) } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 1c02f8bfc3f..179a443c43d 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -137,4 +137,60 @@ describe ProtectedBranch, models: true do end end end + + describe '#protected?' do + context 'existing project' do + let(:project) { create(:project, :repository) } + + it 'returns true when the branch matches a protected branch via direct match' do + create(:protected_branch, project: project, name: "foo") + + expect(ProtectedBranch.protected?(project, 'foo')).to eq(true) + end + + it 'returns true when the branch matches a protected branch via wildcard match' do + create(:protected_branch, project: project, name: "production/*") + + expect(ProtectedBranch.protected?(project, 'production/some-branch')).to eq(true) + end + + it 'returns false when the branch does not match a protected branch via direct match' do + expect(ProtectedBranch.protected?(project, 'foo')).to eq(false) + end + + it 'returns false when the branch does not match a protected branch via wildcard match' do + create(:protected_branch, project: project, name: "production/*") + + expect(ProtectedBranch.protected?(project, 'staging/some-branch')).to eq(false) + end + end + + context "new project" do + let(:project) { create(:empty_project) } + + it 'returns false when default_protected_branch is unprotected' do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) + + expect(ProtectedBranch.protected?(project, 'master')).to be false + end + + it 'returns false when default_protected_branch lets developers push' do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) + + expect(ProtectedBranch.protected?(project, 'master')).to be false + end + + it 'returns true when default_branch_protection does not let developers push but let developer merge branches' do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + + expect(ProtectedBranch.protected?(project, 'master')).to be true + end + + it 'returns true when default_branch_protection is in full protection' do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL) + + expect(ProtectedBranch.protected?(project, 'master')).to be true + end + end + end end -- cgit v1.2.1 From 4f71c29c8bb35642ed5d26015619fc3f838f5e56 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 19:28:54 +0100 Subject: Moved default_branch_protected? out of Project --- app/models/project.rb | 13 +------------ app/models/protected_branch.rb | 7 ++++++- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 5cc224b4440..fdb0a679e28 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -883,11 +883,6 @@ class Project < ActiveRecord::Base "#{url}.git" end - - def empty_and_default_branch_protected? - empty_repo? && default_branch_protected? - end - #TODO: Check with if this is still needed, maybe because of `.select {` in ProtectedRefsMatcher #Either with tests or by asking Tim def protected_tags_array @@ -899,7 +894,7 @@ class Project < ActiveRecord::Base end def user_can_push_to_empty_repo?(user) - !default_branch_protected? || team.max_member_access(user.id) > Gitlab::Access::DEVELOPER + !ProtectedBranch.default_branch_protected? || team.max_member_access(user.id) > Gitlab::Access::DEVELOPER end def forked? @@ -1366,12 +1361,6 @@ class Project < ActiveRecord::Base "projects/#{id}/pushes_since_gc" end - #TODO: Move this and methods which depend upon it - def default_branch_protected? - current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL || - current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE - end - # Similar to the normal callbacks that hook into the life cycle of an # Active Record object, you can also define callbacks that get triggered # when you add an object to an association collection. If any of these diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index eca8d5e0f7d..0f0ac18b1a3 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -13,9 +13,14 @@ class ProtectedBranch < ActiveRecord::Base # Check if branch name is marked as protected in the system def self.protected?(project, ref_name) - return true if project.empty_and_default_branch_protected? + return true if project.empty_repo? && default_branch_protected? protected_refs = project.protected_branches_array self.matching(ref_name, protected_refs: protected_refs).present? end + + def self.default_branch_protected? + current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL || + current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE + end end -- cgit v1.2.1 From 04a50bd9515e09d047d6f678c5d9485f89b31df7 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 19:47:08 +0100 Subject: Removed protected_tags_array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This memorized array appears to originally come from https://gitlab.com/gitlab-org/gitlab-ee/commit/19c2c90ccac86a21eb4266b9a5972162f917f692 which has a commit message of ‘fix warnings’. Without any comments on the original pull request I think we can safely get rid of it unless warnings re-appear. --- app/models/project.rb | 10 ---------- app/models/protected_branch.rb | 3 +-- app/models/protected_tag.rb | 3 +-- 3 files changed, 2 insertions(+), 14 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index fdb0a679e28..13c5c181cc5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -883,16 +883,6 @@ class Project < ActiveRecord::Base "#{url}.git" end - #TODO: Check with if this is still needed, maybe because of `.select {` in ProtectedRefsMatcher - #Either with tests or by asking Tim - def protected_tags_array - @protected_tags_array ||= self.protected_tags.to_a - end - - def protected_branches_array - @protected_branches_array ||= self.protected_branches.to_a - end - def user_can_push_to_empty_repo?(user) !ProtectedBranch.default_branch_protected? || team.max_member_access(user.id) > Gitlab::Access::DEVELOPER end diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 0f0ac18b1a3..28b7d5ad072 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -15,8 +15,7 @@ class ProtectedBranch < ActiveRecord::Base def self.protected?(project, ref_name) return true if project.empty_repo? && default_branch_protected? - protected_refs = project.protected_branches_array - self.matching(ref_name, protected_refs: protected_refs).present? + self.matching(ref_name, protected_refs: project.protected_branches).present? end def self.default_branch_protected? diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index bca5522759d..a52fe90bb2b 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -9,7 +9,6 @@ class ProtectedTag < ActiveRecord::Base accepts_nested_attributes_for :push_access_levels def self.protected?(project, ref_name) - protected_refs = project.protected_tags_array - self.matching(ref_name, protected_refs: protected_refs).present? + self.matching(ref_name, protected_refs: project.protected_tags).present? end end -- cgit v1.2.1 From 35b719f60b21fbd09a1a2b4dc0d3f1e3e74e89e1 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 19:48:00 +0100 Subject: Use delegation in ProtectedRef concern --- app/models/concerns/protected_ref.rb | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 77d7b597534..4f3500d998a 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -6,6 +6,8 @@ module ProtectedRef validates :name, presence: true validates :project, presence: true + delegate :matching, :matches?, :wildcard?, to: :ref_matcher + def self.matching_refs_accesible_to(ref, user, action: :push) access_levels_for_ref(ref, action).any? do |access_level| access_level.check_access(user) @@ -27,18 +29,6 @@ module ProtectedRef project.commit(self.name) end - def matching(refs) - ref_matcher.matching(refs) - end - - def matches?(refs) - ref_matcher.matches?(refs) - end - - def wildcard? - ref_matcher.wildcard? - end - private def ref_matcher -- cgit v1.2.1 From 9f4b8dba805915bd21d315f159035449f9f4bef0 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 20:06:06 +0100 Subject: Clean up non TODO rubocop errors --- app/controllers/projects/settings/repository_controller.rb | 2 +- app/models/concerns/protected_ref.rb | 2 -- app/models/concerns/protected_ref_access.rb | 1 + lib/gitlab/checks/change_access.rb | 8 ++++---- lib/gitlab/import_export/relation_factory.rb | 2 +- spec/controllers/projects/protected_tags_controller_spec.rb | 12 ++++++------ 6 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index a87927fe1ec..fb175b4a636 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -13,7 +13,7 @@ module Projects def define_protected_refs @protected_branches = @project.protected_branches.order(:name).page(params[:page]) - @protected_tags = @project.protected_tags.order(:name).page(params[:page]) + @protected_tags = @project.protected_tags.order(:name).page(params[:page]) #TODO duplicated pagination param? @protected_branch = @project.protected_branches.new @protected_tag = @project.protected_tags.new load_gon_index diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 4f3500d998a..3681ae63e3a 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -18,8 +18,6 @@ module ProtectedRef self.matching(ref).map(&:"@#{action}_access_levels").flatten end - private - def self.matching(ref_name, protected_refs: nil) ProtectedRefMatcher.matching(self, ref_name, protected_refs: protected_refs) end diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index 2870cd9385b..08377127f69 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -1,3 +1,4 @@ +#TODO: Refactor, checking EE # module ProtectedRefAccess # extend ActiveSupport::Concern diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 6f574a41727..07fd4024346 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -71,15 +71,15 @@ module Gitlab def protected_tag_checks return unless tag_protected? - if forced_push? - return "You are not allowed to force push protected tags." #TODO: Wording, 'not allowed to update proteted tags'? + if forced_push? #TODO: Verify if this should prevent all updates, and mention in UI and documentation + return "Protected tags cannot be updated." end if Gitlab::Git.blank_ref?(@newrev) - return "You are not allowed to delete protected tags." #TODO: Wording, do these need to mention 'you' if the rule applies to everyone + return "Protected tags cannot be deleted." end - if !user_access.can_push_tag?(@tag_name) + unless user_access.can_push_tag?(@tag_name) return "You are not allowed to create this tag as it is protected." end end diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index d44563333a5..9d269c5d384 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -7,7 +7,7 @@ module Gitlab triggers: 'Ci::Trigger', builds: 'Ci::Build', hooks: 'ProjectHook', - merge_access_levels: 'ProtectedBranch::MergeAccessLevel', + merge_access_levels: 'ProtectedBranch::MergeAccessLevel', #TODO: Tags push_access_levels: 'ProtectedBranch::PushAccessLevel', labels: :project_labels, priorities: :label_priorities, diff --git a/spec/controllers/projects/protected_tags_controller_spec.rb b/spec/controllers/projects/protected_tags_controller_spec.rb index 729017c1483..ac802981294 100644 --- a/spec/controllers/projects/protected_tags_controller_spec.rb +++ b/spec/controllers/projects/protected_tags_controller_spec.rb @@ -1,10 +1,10 @@ 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 - # end + 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 + end end -- cgit v1.2.1 From 3c91841d032f02b0b0d4c532998bbc923247e804 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 21:00:51 +0100 Subject: Created ProtectedRefsController to reduce Tags/Branches duplication Fixes ProtectedBranches#create flash errors bug due to typo in 'flash[:alert] = @protected_branches.errors' --- .../projects/protected_branches_controller.rb | 58 +++++++--------------- .../projects/protected_refs_controller.rb | 48 ++++++++++++++++++ .../projects/protected_tags_controller.rb | 58 +++++++--------------- 3 files changed, 84 insertions(+), 80 deletions(-) create mode 100644 app/controllers/projects/protected_refs_controller.rb diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index a8cb07eb67a..a245a60910e 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -1,58 +1,36 @@ -class Projects::ProtectedBranchesController < Projects::ApplicationController - include RepositorySettingsRedirect - # Authorize - before_action :require_non_empty_project - before_action :authorize_admin_project! - before_action :load_protected_branch, only: [:show, :update, :destroy] +class Projects::ProtectedBranchesController < Projects::ProtectedRefsController - layout "project_settings" + protected - def index - redirect_to_repository_settings(@project) + def protected_ref + @protected_branch end - def create - @protected_branch = ::ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute - unless @protected_branch.persisted? - flash[:alert] = @protected_branches.errors.full_messages.join(', ').html_safe - end - redirect_to_repository_settings(@project) + def protected_ref=(val) + @protected_branch = val end - def show - @matching_branches = @protected_branch.matching(@project.repository.branches) + def matching_refs=(val) + @matching_branches = val end - def update - @protected_branch = ::ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch) - - if @protected_branch.valid? - respond_to do |format| - format.json { render json: @protected_branch, status: :ok } - end - else - respond_to do |format| - format.json { render json: @protected_branch.errors, status: :unprocessable_entity } - end - end + def project_refs + @project.repository.branches end - def destroy - @protected_branch.destroy - - respond_to do |format| - format.html { redirect_to_repository_settings(@project) } - format.js { head :ok } - end + def create_service + ::ProtectedBranches::CreateService end - private + def update_service + ::ProtectedBranches::UpdateService + end - def load_protected_branch - @protected_branch = @project.protected_branches.find(params[:id]) + def load_protected_ref + self.protected_ref = @project.protected_branches.find(params[:id]) end - def protected_branch_params + def protected_ref_params params.require(:protected_branch).permit(:name, merge_access_levels_attributes: [:access_level, :id], push_access_levels_attributes: [:access_level, :id]) diff --git a/app/controllers/projects/protected_refs_controller.rb b/app/controllers/projects/protected_refs_controller.rb new file mode 100644 index 00000000000..63f005124a9 --- /dev/null +++ b/app/controllers/projects/protected_refs_controller.rb @@ -0,0 +1,48 @@ +class Projects::ProtectedRefsController < Projects::ApplicationController + include RepositorySettingsRedirect + # Authorize + before_action :require_non_empty_project + before_action :authorize_admin_project! + before_action :load_protected_ref, only: [:show, :update, :destroy] + + layout "project_settings" + + def index + redirect_to_repository_settings(@project) + end + + def create + self.protected_ref = create_service.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) + end + + def update + self.protected_ref = update_service.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 + else + respond_to do |format| + format.json { render json: protected_ref.errors, status: :unprocessable_entity } + end + end + end + + def destroy + protected_ref.destroy + + respond_to do |format| + format.html { redirect_to_repository_settings(@project) } + format.js { head :ok } + end + end +end diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb index 5ab5d1d997b..8f407b42ac8 100644 --- a/app/controllers/projects/protected_tags_controller.rb +++ b/app/controllers/projects/protected_tags_controller.rb @@ -1,58 +1,36 @@ -class Projects::ProtectedTagsController < Projects::ApplicationController - include RepositorySettingsRedirect - # Authorize - before_action :require_non_empty_project - before_action :authorize_admin_project! - before_action :load_protected_tag, only: [:show, :update, :destroy] +class Projects::ProtectedTagsController < Projects::ProtectedRefsController - layout "project_settings" + protected - def index - redirect_to_repository_settings(@project) + def protected_ref + @protected_tag end - def create - @protected_tag = ::ProtectedTags::CreateService.new(@project, current_user, protected_tag_params).execute - unless @protected_tag.persisted? - flash[:alert] = @protected_tags.errors.full_messages.join(', ').html_safe - end - redirect_to_repository_settings(@project) + def protected_ref=(val) + @protected_tag = val end - def show - @matching_tags = @protected_tag.matching(@project.repository.tags) + def matching_refs=(val) + @matching_tags = val end - def update - @protected_tag = ::ProtectedTags::UpdateService.new(@project, current_user, protected_tag_params).execute(@protected_tag) - - if @protected_tag.valid? - respond_to do |format| - format.json { render json: @protected_tag, status: :ok } - end - else - respond_to do |format| - format.json { render json: @protected_tag.errors, status: :unprocessable_entity } - end - end + def project_refs + @project.repository.tags end - def destroy - @protected_tag.destroy - - respond_to do |format| - format.html { redirect_to_repository_settings(@project) } - format.js { head :ok } - end + def create_service + ::ProtectedTags::CreateService end - private + def update_service + ::ProtectedTags::UpdateService + end - def load_protected_tag - @protected_tag = @project.protected_tags.find(params[:id]) + def load_protected_ref + self.protected_ref = @project.protected_tags.find(params[:id]) end - def protected_tag_params + def protected_ref_params params.require(:protected_tag).permit(:name, push_access_levels_attributes: [:access_level, :id]) end end -- cgit v1.2.1 From ff2713a57046bd08764ad391d7f34bd27f787610 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 3 Apr 2017 22:04:37 +0100 Subject: Fix typos in ProtectedRef concern and whitespace detected by rubocop --- app/controllers/projects/protected_branches_controller.rb | 1 - app/controllers/projects/protected_tags_controller.rb | 1 - app/controllers/projects/settings/repository_controller.rb | 1 - app/models/concerns/protected_ref.rb | 4 ++-- 4 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index a245a60910e..c2a55c9500a 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -1,5 +1,4 @@ class Projects::ProtectedBranchesController < Projects::ProtectedRefsController - protected def protected_ref diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb index 8f407b42ac8..ff132056aa4 100644 --- a/app/controllers/projects/protected_tags_controller.rb +++ b/app/controllers/projects/protected_tags_controller.rb @@ -1,5 +1,4 @@ class Projects::ProtectedTagsController < Projects::ProtectedRefsController - protected def protected_ref diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index fb175b4a636..ff818d9e51a 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -19,7 +19,6 @@ module Projects load_gon_index end - def access_levels_options #TODO: consider protected tags #TODO: Refactor ProtectedBranch::PushAccessLevel so it doesn't mention branches diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 3681ae63e3a..f6841669ab0 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -9,13 +9,13 @@ module ProtectedRef delegate :matching, :matches?, :wildcard?, to: :ref_matcher def self.matching_refs_accesible_to(ref, user, action: :push) - access_levels_for_ref(ref, action).any? do |access_level| + access_levels_for_ref(ref, action: action).any? do |access_level| access_level.check_access(user) end end def self.access_levels_for_ref(ref, action: :push) - self.matching(ref).map(&:"@#{action}_access_levels").flatten + self.matching(ref).map(&:"#{action}_access_levels").flatten end def self.matching(ref_name, protected_refs: nil) -- cgit v1.2.1 From d5acb69e116cbbed105e29552d7cca2e864f0c8f Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 00:05:51 +0100 Subject: Protected Tags prevents all updates instead of just force pushes. This only changes behaviour for masters, as developers are already prevented from updating/deleting tags without the Protected Tags feature --- app/views/projects/protected_tags/_index.html.haml | 4 +- lib/gitlab/checks/change_access.rb | 14 ++-- spec/lib/gitlab/checks/change_access_spec.rb | 83 ++++++++++------------ 3 files changed, 49 insertions(+), 52 deletions(-) diff --git a/app/views/projects/protected_tags/_index.html.haml b/app/views/projects/protected_tags/_index.html.haml index 591d64ae7de..0bfb1ad191d 100644 --- a/app/views/projects/protected_tags/_index.html.haml +++ b/app/views/projects/protected_tags/_index.html.haml @@ -8,8 +8,8 @@ %p.prepend-top-20 By default, Protected tags are designed to: %ul - %li Prevent tag pushes from everybody except Masters - %li Prevent anyone from force pushing to the tag + %li Prevent tag creation by everybody except Masters + %li Prevent anyone from updating the tag %li Prevent anyone from deleting the tag .col-lg-9 - if can? current_user, :admin_project, @project diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 07fd4024346..d0bbd713710 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -37,7 +37,7 @@ module Gitlab if forced_push? return "You are not allowed to force push code to a protected branch on this project." - elsif blank_ref? + elsif deletion? return "You are not allowed to delete protected branches from this project." end @@ -62,7 +62,7 @@ module Gitlab return unless @tag_name if tag_exists? && user_access.cannot_do_action?(:admin_project) - "You are not allowed to change existing tags on this project." + return "You are not allowed to change existing tags on this project." end protected_tag_checks @@ -71,11 +71,11 @@ module Gitlab def protected_tag_checks return unless tag_protected? - if forced_push? #TODO: Verify if this should prevent all updates, and mention in UI and documentation + if update? return "Protected tags cannot be updated." end - if Gitlab::Git.blank_ref?(@newrev) + if deletion? return "Protected tags cannot be deleted." end @@ -106,7 +106,11 @@ module Gitlab Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env) end - def blank_ref? + def update? + !Gitlab::Git.blank_ref?(@oldrev) && !deletion? + end + + def deletion? Gitlab::Git.blank_ref?(@newrev) end diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index 8525422908b..afc29baa7e6 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -5,13 +5,10 @@ describe Gitlab::Checks::ChangeAccess, lib: true do let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:user_access) { Gitlab::UserAccess.new(user, project: project) } - let(:changes) do - { - oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', - newrev: '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51', - ref: 'refs/heads/master' - } - end + let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' } + let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } + let(:ref) { 'refs/heads/master' } + let(:changes) { { oldrev: oldrev, newrev: newrev, ref: ref } } let(:protocol) { 'ssh' } subject do @@ -41,13 +38,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do end context 'tags check' do - let(:changes) do - { - oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', - newrev: '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51', - ref: 'refs/tags/v1.0.0' - } - end + let(:ref) { 'refs/tags/v1.0.0' } it 'returns an error if the user is not allowed to update tags' do allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) @@ -60,38 +51,46 @@ describe Gitlab::Checks::ChangeAccess, lib: true do context 'with protected tag' do let!(:protected_tag) { create(:protected_tag, project: project, name: 'v*') } - context 'deletion' do - let(:changes) do - { - oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', - newrev: '0000000000000000000000000000000000000000', - ref: 'refs/tags/v1.0.0' - } - end + context 'as master' do + before { project.add_master(user) } - it 'is prevented' do - expect(subject.status).to be(false) - expect(subject.message).to include('delete protected tags') + context 'deletion' do + let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' } + let(:newrev) { '0000000000000000000000000000000000000000' } + + it 'is prevented' do + expect(subject.status).to be(false) + expect(subject.message).to include('cannot be deleted') + end end - end - it 'prevents force push' do - expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true) + context 'update' do + let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' } + let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } - expect(subject.status).to be(false) - expect(subject.message).to include('force push protected tags') + it 'is prevented' do + expect(subject.status).to be(false) + expect(subject.message).to include('cannot be updated') + end + end end - it 'prevents creation below access level' do - expect(subject.status).to be(false) - expect(subject.message).to include('allowed to') - end + context 'creation' do + let(:oldrev) { '0000000000000000000000000000000000000000' } + let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } + let(:ref) { 'refs/tags/v9.1.0' } - context 'when user has access' do - let!(:protected_tag) { create(:protected_tag, :developers_can_push, project: project, name: 'v*') } + it 'prevents creation below access level' do + expect(subject.status).to be(false) + expect(subject.message).to include('allowed to create this tag as it is protected') + end + + context 'when user has access' do + let!(:protected_tag) { create(:protected_tag, :developers_can_push, project: project, name: 'v*') } - it 'allows tag creation' do - expect(subject.status).to be(true) + it 'allows tag creation' do + expect(subject.status).to be(true) + end end end end @@ -126,13 +125,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do end context 'branch deletion' do - let(:changes) do - { - oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', - newrev: '0000000000000000000000000000000000000000', - ref: 'refs/heads/master' - } - end + let(:newrev) { '0000000000000000000000000000000000000000' } it 'returns an error if the user is not allowed to delete protected branches' do expect(subject.status).to be(false) -- cgit v1.2.1 From 90c8bb8301b4bc3268a5fa4ea8bddafbc29d6871 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 01:39:34 +0100 Subject: Fixed developers_can_push in RepoBranch API entity --- app/models/concerns/protected_ref.rb | 6 ++++++ lib/api/entities.rb | 8 ++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index f6841669ab0..a04dea0bc55 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -14,6 +14,12 @@ module ProtectedRef end end + def self.developers_can?(action, ref) + access_levels_for_ref(ref, action: action).any? do |access_level| + access_level.access_level == Gitlab::Access::DEVELOPER + end + end + def self.access_levels_for_ref(ref, action: :push) self.matching(ref).map(&:"#{action}_access_levels").flatten end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 0fe7eb864e4..0cc6188938d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -188,15 +188,11 @@ module API end expose :developers_can_push do |repo_branch, options| - project = options[:project] - access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :push) - access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } + options[:project].protected_branches.developers_can?(:push, repo_branch.name) end expose :developers_can_merge do |repo_branch, options| - project = options[:project] - access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :merge) - access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } + options[:project].protected_branches.developers_can?(:merge, repo_branch.name) end end -- cgit v1.2.1 From 1e15444ae6dda02744db42d08c817252953c7b1f Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 02:05:42 +0100 Subject: Cleanup & tests for UserAccess#can_create_tag? --- app/models/concerns/protected_ref.rb | 2 +- lib/gitlab/checks/change_access.rb | 2 +- lib/gitlab/user_access.rb | 11 +++--- spec/lib/gitlab/user_access_spec.rb | 70 ++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 9 deletions(-) diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index a04dea0bc55..ab28eb19b64 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -8,7 +8,7 @@ module ProtectedRef delegate :matching, :matches?, :wildcard?, to: :ref_matcher - def self.matching_refs_accesible_to(ref, user, action: :push) + def self.protected_ref_accessible_to?(ref, user, action: :push) access_levels_for_ref(ref, action: action).any? do |access_level| access_level.check_access(user) end diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index d0bbd713710..0d96c4d41d7 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -79,7 +79,7 @@ module Gitlab return "Protected tags cannot be deleted." end - unless user_access.can_push_tag?(@tag_name) + unless user_access.can_create_tag?(@tag_name) return "You are not allowed to create this tag as it is protected." end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 5541a45e948..6af5de4dc08 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -28,14 +28,11 @@ module Gitlab true end - #TODO: Test this - #TODO move most to ProtectedTag::AccessChecker. Or maybe UserAccess::Protections::Tag - #TODO: then consider removing method, if it turns out can_access_git? and can?(:push_code are checked in change_access - def can_push_tag?(ref) + def can_create_tag?(ref) return false unless can_access_git? if ProtectedTag.protected?(project, ref) - project.protected_tags.matching_refs_accesible_to(ref, user) + project.protected_tags.protected_ref_accessible_to?(ref, user) else user.can?(:push_code, project) end @@ -47,7 +44,7 @@ module Gitlab if ProtectedBranch.protected?(project, ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) - has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push) + has_access = project.protected_branches.protected_ref_accessible_to?(ref, user, action: :push) has_access || !project.repository.branch_exists?(ref) && can_merge_to_branch?(ref) else @@ -59,7 +56,7 @@ module Gitlab return false unless can_access_git? if ProtectedBranch.protected?(project, ref) - project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge) + project.protected_branches.protected_ref_accessible_to?(ref, user, action: :merge) else user.can?(:push_code, project) end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 369e55f61f1..c425aef359a 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -142,4 +142,74 @@ describe Gitlab::UserAccess, lib: true do end end end + + describe 'can_create_tag?' do + describe 'push to none protected tag' do + it 'returns true if user is a master' do + project.add_user(user, :master) + + expect(access.can_create_tag?('random_tag')).to be_truthy + end + + it 'returns true if user is a developer' do + project.add_user(user, :developer) + + expect(access.can_create_tag?('random_tag')).to be_truthy + end + + it 'returns false if user is a reporter' do + project.add_user(user, :reporter) + + expect(access.can_create_tag?('random_tag')).to be_falsey + end + end + + + describe 'push to protected tag' do + let(:tag) { create(:protected_tag, project: project, name: "test") } + let(:not_existing_tag) { create :protected_tag, project: project } + + it 'returns true if user is a master' do + project.add_user(user, :master) + + expect(access.can_create_tag?(tag.name)).to be_truthy + end + + it 'returns false if user is a developer' do + project.add_user(user, :developer) + + expect(access.can_create_tag?(tag.name)).to be_falsey + end + + it 'returns false if user is a reporter' do + project.add_user(user, :reporter) + + expect(access.can_create_tag?(tag.name)).to be_falsey + end + end + + describe 'push to protected tag if allowed for developers' do + before do + @tag = create(:protected_tag, :developers_can_push, project: project) + end + + it 'returns true if user is a master' do + project.add_user(user, :master) + + expect(access.can_create_tag?(@tag.name)).to be_truthy + end + + it 'returns true if user is a developer' do + project.add_user(user, :developer) + + expect(access.can_create_tag?(@tag.name)).to be_truthy + end + + it 'returns false if user is a reporter' do + project.add_user(user, :reporter) + + expect(access.can_create_tag?(@tag.name)).to be_falsey + end + end + end end -- cgit v1.2.1 From 3bb3a6886f3b206a2ec089d6b1e8854615daa0b8 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 02:19:04 +0100 Subject: Attempt to fix import/export of push_access_levels for protected tags --- lib/gitlab/import_export/import_export.yml | 2 ++ lib/gitlab/import_export/relation_factory.rb | 3 ++- spec/lib/gitlab/import_export/all_models.yml | 5 +++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index ab74c8782f6..7bf5568a8ee 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -46,6 +46,8 @@ project_tree: - protected_branches: - :merge_access_levels - :push_access_levels + - protected_tags: + - :push_access_levels - :project_feature # Only include the following attributes for the models specified. diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index 9d269c5d384..b1d8e385f2e 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -7,8 +7,9 @@ module Gitlab triggers: 'Ci::Trigger', builds: 'Ci::Build', hooks: 'ProjectHook', - merge_access_levels: 'ProtectedBranch::MergeAccessLevel', #TODO: Tags + merge_access_levels: 'ProtectedBranch::MergeAccessLevel', push_access_levels: 'ProtectedBranch::PushAccessLevel', + #TODO: How to add?- push_access_levels: 'ProtectedTag::PushAccessLevel', labels: :project_labels, priorities: :label_priorities, label: :project_label }.freeze diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index ddeb71730e7..83503c73e75 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -111,10 +111,14 @@ protected_branches: - project - merge_access_levels - push_access_levels +protected_tags: +- project +- push_access_levels merge_access_levels: - protected_branch push_access_levels: - protected_branch +- protected_tag project: - taggings - base_tags @@ -169,6 +173,7 @@ project: - snippets - hooks - protected_branches +- protected_tags - project_members - users - requesters -- cgit v1.2.1 From f9e849c076efb3162a3d951d8aae2e7be3e574f4 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 02:59:37 +0100 Subject: Cleaned up duplication with ProtectedRefAccess concern --- .../projects/settings/repository_controller.rb | 19 ++++++------- app/models/concerns/protected_branch_access.rb | 15 ++-------- app/models/concerns/protected_ref_access.rb | 32 ++++++++++------------ app/models/concerns/protected_tag_access.rb | 15 ++-------- spec/lib/gitlab/user_access_spec.rb | 1 - 5 files changed, 27 insertions(+), 55 deletions(-) diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index ff818d9e51a..9022cf8f0d8 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -23,19 +23,18 @@ module Projects #TODO: consider protected tags #TODO: Refactor ProtectedBranch::PushAccessLevel so it doesn't mention branches { - push_access_levels: { - roles: ProtectedBranch::PushAccessLevel.human_access_levels.map do |id, text| - { id: id, text: text, before_divider: true } - end - }, - merge_access_levels: { - roles: ProtectedBranch::MergeAccessLevel.human_access_levels.map do |id, text| - { id: id, text: text, before_divider: true } - end - } + push_access_levels: levels_for_dropdown(ProtectedBranch::PushAccessLevel), + merge_access_levels: levels_for_dropdown(ProtectedBranch::MergeAccessLevel) } end + def levels_for_dropdown(access_level_type) + roles = access_level_type.human_access_levels.map do |id, text| + { id: id, text: text, before_divider: true } + end + { roles: roles } + end + def protectable_tags_for_dropdown { open_tags: ProtectableDropdown.new(@project, :tags).hash } end diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb index 9dd4d9c6f24..06cae00249a 100644 --- a/app/models/concerns/protected_branch_access.rb +++ b/app/models/concerns/protected_branch_access.rb @@ -2,20 +2,9 @@ module ProtectedBranchAccess extend ActiveSupport::Concern included do + include ProtectedRefAccess + belongs_to :protected_branch delegate :project, to: :protected_branch - - scope :master, -> { where(access_level: Gitlab::Access::MASTER) } - scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } - end - - def humanize - self.class.human_access_levels[self.access_level] - end - - def check_access(user) - return true if user.is_admin? - - project.team.max_member_access(user.id) >= access_level end end diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index 08377127f69..0c7e5157cdf 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -1,22 +1,18 @@ -#TODO: Refactor, checking EE -# module ProtectedRefAccess -# extend ActiveSupport::Concern +module ProtectedRefAccess + extend ActiveSupport::Concern -# included do -# # belongs_to :protected_branch -# # delegate :project, to: :protected_branch + included do + scope :master, -> { where(access_level: Gitlab::Access::MASTER) } + scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } + end -# scope :master, -> { where(access_level: Gitlab::Access::MASTER) } -# scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } -# end + def humanize + self.class.human_access_levels[self.access_level] + end -# def humanize -# self.class.human_access_levels[self.access_level] -# end + def check_access(user) + return true if user.is_admin? -# def check_access(user) -# return true if user.is_admin? - -# project.team.max_member_access(user.id) >= access_level -# end -# end + project.team.max_member_access(user.id) >= access_level + end +end diff --git a/app/models/concerns/protected_tag_access.rb b/app/models/concerns/protected_tag_access.rb index cf66a6434b5..9b7d31a6fd5 100644 --- a/app/models/concerns/protected_tag_access.rb +++ b/app/models/concerns/protected_tag_access.rb @@ -2,20 +2,9 @@ module ProtectedTagAccess extend ActiveSupport::Concern included do + include ProtectedRefAccess + belongs_to :protected_tag delegate :project, to: :protected_tag - - scope :master, -> { where(access_level: Gitlab::Access::MASTER) } - scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } - end - - def humanize - self.class.human_access_levels[self.access_level] - end - - def check_access(user) - return true if user.is_admin? - - project.team.max_member_access(user.id) >= access_level end end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index c425aef359a..c69ff3446ea 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -164,7 +164,6 @@ describe Gitlab::UserAccess, lib: true do end end - describe 'push to protected tag' do let(:tag) { create(:protected_tag, project: project, name: "test") } let(:not_existing_tag) { create :protected_tag, project: project } -- cgit v1.2.1 From 07d7d8e65905a39164b63f55eccdcea8f10f5d14 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 03:37:22 +0100 Subject: Renamed ProtectedTag push_access_levels to create_access_levels --- .../protected_tags/protected_tag_create.js | 14 +++++++------- .../protected_tags/protected_tag_edit.js | 22 +++++++++++----------- .../projects/protected_tags_controller.rb | 2 +- .../projects/settings/repository_controller.rb | 5 ++--- app/models/protected_tag.rb | 6 +++--- app/models/protected_tag/create_access_level.rb | 21 +++++++++++++++++++++ app/models/protected_tag/push_access_level.rb | 21 --------------------- .../protected_tags/_create_protected_tag.html.haml | 10 +++++----- .../projects/protected_tags/_tags_list.html.haml | 2 +- .../protected_tags/_update_protected_tag.haml | 8 ++++---- db/migrate/20170309173138_create_protected_tags.rb | 8 ++++---- db/schema.rb | 12 ++++++------ lib/gitlab/import_export/import_export.yml | 2 +- lib/gitlab/import_export/relation_factory.rb | 2 +- spec/factories/protected_tags.rb | 10 +++++----- .../protected_tags/access_control_ce_spec.rb | 20 ++++++++++---------- spec/lib/gitlab/checks/change_access_spec.rb | 2 +- spec/lib/gitlab/import_export/all_models.yml | 3 ++- spec/lib/gitlab/user_access_spec.rb | 2 +- .../services/protected_tags/create_service_spec.rb | 4 ++-- 20 files changed, 88 insertions(+), 88 deletions(-) create mode 100644 app/models/protected_tag/create_access_level.rb delete mode 100644 app/models/protected_tag/push_access_level.rb diff --git a/app/assets/javascripts/protected_tags/protected_tag_create.js b/app/assets/javascripts/protected_tags/protected_tag_create.js index 4c652e7747f..84b1b232649 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_create.js +++ b/app/assets/javascripts/protected_tags/protected_tag_create.js @@ -11,20 +11,20 @@ } buildDropdowns() { - const $allowedToPushDropdown = this.$wrap.find('.js-allowed-to-push'); + const $allowedToCreateDropdown = this.$wrap.find('.js-allowed-to-create'); // Cache callback this.onSelectCallback = this.onSelect.bind(this); - // Allowed to Push dropdown + // Allowed to Create dropdown new gl.ProtectedTagAccessDropdown({ - $dropdown: $allowedToPushDropdown, - data: gon.push_access_levels, + $dropdown: $allowedToCreateDropdown, + data: gon.create_access_levels, onSelect: this.onSelectCallback }); // Select default - $allowedToPushDropdown.data('glDropdown').selectRowAtIndex(0); + $allowedToCreateDropdown.data('glDropdown').selectRowAtIndex(0); // Protected tag dropdown new ProtectedTagDropdown({ @@ -37,9 +37,9 @@ onSelect() { // Enable submit button const $tagInput = this.$wrap.find('input[name="protected_tag[name]"]'); - const $allowedToPushInput = this.$wrap.find('input[name="protected_tag[push_access_levels_attributes][0][access_level]"]'); + const $allowedToCreateInput = this.$wrap.find('input[name="protected_tag[create_access_levels_attributes][0][access_level]"]'); - this.$form.find('input[type="submit"]').attr('disabled', !($tagInput.val() && $allowedToPushInput.length)); + this.$form.find('input[type="submit"]').attr('disabled', !($tagInput.val() && $allowedToCreateInput.length)); } }; })(window); diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit.js b/app/assets/javascripts/protected_tags/protected_tag_edit.js index b93e903621e..0227be35c8f 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_edit.js +++ b/app/assets/javascripts/protected_tags/protected_tag_edit.js @@ -7,27 +7,27 @@ gl.ProtectedTagEdit = class { constructor(options) { this.$wrap = options.$wrap; - this.$allowedToPushDropdown = this.$wrap.find('.js-allowed-to-push'); + this.$allowedToCreateDropdown = this.$wrap.find('.js-allowed-to-create'); this.buildDropdowns(); } buildDropdowns() { - // Allowed to push dropdown + // Allowed to create dropdown new gl.ProtectedTagAccessDropdown({ - $dropdown: this.$allowedToPushDropdown, - data: gon.push_access_levels, + $dropdown: this.$allowedToCreateDropdown, + data: gon.create_access_levels, onSelect: this.onSelect.bind(this) }); } onSelect() { - const $allowedToPushInput = this.$wrap.find(`input[name="${this.$allowedToPushDropdown.data('fieldName')}"]`); + const $allowedToCreateInput = this.$wrap.find(`input[name="${this.$allowedToCreateDropdown.data('fieldName')}"]`); // Do not update if one dropdown has not selected any option - if (!$allowedToPushInput.length) return; + if (!$allowedToCreateInput.length) return; - this.$allowedToPushDropdown.disable(); + this.$allowedToCreateDropdown.disable(); $.ajax({ type: 'POST', @@ -36,9 +36,9 @@ data: { _method: 'PATCH', protected_tag: { - push_access_levels_attributes: [{ - id: this.$allowedToPushDropdown.data('access-level-id'), - access_level: $allowedToPushInput.val() + create_access_levels_attributes: [{ + id: this.$allowedToCreateDropdown.data('access-level-id'), + access_level: $allowedToCreateInput.val() }] } }, @@ -47,7 +47,7 @@ new Flash('Failed to update tag!'); } }).always(() => { - this.$allowedToPushDropdown.enable(); + this.$allowedToCreateDropdown.enable(); }); } }; diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb index ff132056aa4..0e00baedbdf 100644 --- a/app/controllers/projects/protected_tags_controller.rb +++ b/app/controllers/projects/protected_tags_controller.rb @@ -30,6 +30,6 @@ class Projects::ProtectedTagsController < Projects::ProtectedRefsController end def protected_ref_params - params.require(:protected_tag).permit(:name, push_access_levels_attributes: [:access_level, :id]) + params.require(:protected_tag).permit(:name, create_access_levels_attributes: [:access_level, :id]) end end diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 9022cf8f0d8..44de8a49593 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -13,16 +13,15 @@ module Projects def define_protected_refs @protected_branches = @project.protected_branches.order(:name).page(params[:page]) - @protected_tags = @project.protected_tags.order(:name).page(params[:page]) #TODO duplicated pagination param? + @protected_tags = @project.protected_tags.order(:name).page(params[:page]) @protected_branch = @project.protected_branches.new @protected_tag = @project.protected_tags.new load_gon_index end def access_levels_options - #TODO: consider protected tags - #TODO: Refactor ProtectedBranch::PushAccessLevel so it doesn't mention branches { + create_access_levels: levels_for_dropdown(ProtectedTag::CreateAccessLevel), push_access_levels: levels_for_dropdown(ProtectedBranch::PushAccessLevel), merge_access_levels: levels_for_dropdown(ProtectedBranch::MergeAccessLevel) } diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index a52fe90bb2b..83964095516 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -2,11 +2,11 @@ class ProtectedTag < ActiveRecord::Base include Gitlab::ShellAdapter include ProtectedRef - has_many :push_access_levels, dependent: :destroy + has_many :create_access_levels, dependent: :destroy - validates :push_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." } + validates :create_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." } - accepts_nested_attributes_for :push_access_levels + accepts_nested_attributes_for :create_access_levels def self.protected?(project, ref_name) self.matching(ref_name, protected_refs: project.protected_tags).present? diff --git a/app/models/protected_tag/create_access_level.rb b/app/models/protected_tag/create_access_level.rb new file mode 100644 index 00000000000..c7e1319719d --- /dev/null +++ b/app/models/protected_tag/create_access_level.rb @@ -0,0 +1,21 @@ +class ProtectedTag::CreateAccessLevel < ActiveRecord::Base + include ProtectedTagAccess + + validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, + Gitlab::Access::DEVELOPER, + Gitlab::Access::NO_ACCESS] } + + def self.human_access_levels + { + Gitlab::Access::MASTER => "Masters", + Gitlab::Access::DEVELOPER => "Developers + Masters", + Gitlab::Access::NO_ACCESS => "No one" + }.with_indifferent_access + end + + def check_access(user) + return false if access_level == Gitlab::Access::NO_ACCESS + + super + end +end diff --git a/app/models/protected_tag/push_access_level.rb b/app/models/protected_tag/push_access_level.rb deleted file mode 100644 index 9282af841ce..00000000000 --- a/app/models/protected_tag/push_access_level.rb +++ /dev/null @@ -1,21 +0,0 @@ -class ProtectedTag::PushAccessLevel < ActiveRecord::Base - include ProtectedTagAccess - - validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, - Gitlab::Access::DEVELOPER, - Gitlab::Access::NO_ACCESS] } - - def self.human_access_levels - { - Gitlab::Access::MASTER => "Masters", - Gitlab::Access::DEVELOPER => "Developers + Masters", - Gitlab::Access::NO_ACCESS => "No one" - }.with_indifferent_access - end - - def check_access(user) - return false if access_level == Gitlab::Access::NO_ACCESS - - super - end -end diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml index 9fdebf2c982..af332f942d6 100644 --- a/app/views/projects/protected_tags/_create_protected_tag.html.haml +++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml @@ -19,14 +19,14 @@ %code production/* are supported .form-group - %label.col-md-2.text-right{ for: 'push_access_levels_attributes' } - Allowed to push: + %label.col-md-2.text-right{ for: 'create_access_levels_attributes' } + Allowed to create: .col-md-10 - .push_access_levels-container + .create_access_levels-container = dropdown_tag('Select', - options: { toggle_class: 'js-allowed-to-push wide', + options: { toggle_class: 'js-allowed-to-create wide', dropdown_class: 'dropdown-menu-selectable', - data: { field_name: 'protected_tag[push_access_levels_attributes][0][access_level]', input_id: 'push_access_levels_attributes' }}) + data: { field_name: 'protected_tag[create_access_levels_attributes][0][access_level]', input_id: 'create_access_levels_attributes' }}) .panel-footer = f.submit 'Protect', class: 'btn-create btn', disabled: true diff --git a/app/views/projects/protected_tags/_tags_list.html.haml b/app/views/projects/protected_tags/_tags_list.html.haml index 6f63971923d..cc006ed8a0b 100644 --- a/app/views/projects/protected_tags/_tags_list.html.haml +++ b/app/views/projects/protected_tags/_tags_list.html.haml @@ -17,7 +17,7 @@ %tr %th Protected tag (#{@protected_tags.size}) %th Last commit - %th Allowed to push + %th Allowed to create - if can_admin_project %th %tbody diff --git a/app/views/projects/protected_tags/_update_protected_tag.haml b/app/views/projects/protected_tags/_update_protected_tag.haml index 802a5ef3b98..62823bee46e 100644 --- a/app/views/projects/protected_tags/_update_protected_tag.haml +++ b/app/views/projects/protected_tags/_update_protected_tag.haml @@ -1,5 +1,5 @@ %td - = hidden_field_tag "allowed_to_push_#{protected_tag.id}", protected_tag.push_access_levels.first.access_level - = dropdown_tag( (protected_tag.push_access_levels.first.humanize || 'Select') , - options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', - data: { field_name: "allowed_to_push_#{protected_tag.id}", access_level_id: protected_tag.push_access_levels.first.id }}) + = hidden_field_tag "allowed_to_create_#{protected_tag.id}", protected_tag.create_access_levels.first.access_level + = dropdown_tag( (protected_tag.create_access_levels.first.humanize || 'Select') , + options: { toggle_class: 'js-allowed-to-create', dropdown_class: 'dropdown-menu-selectable js-allowed-to-create-container', + data: { field_name: "allowed_to_create_#{protected_tag.id}", access_level_id: protected_tag.create_access_levels.first.id }}) diff --git a/db/migrate/20170309173138_create_protected_tags.rb b/db/migrate/20170309173138_create_protected_tags.rb index 00794529143..538f28479c7 100644 --- a/db/migrate/20170309173138_create_protected_tags.rb +++ b/db/migrate/20170309173138_create_protected_tags.rb @@ -15,14 +15,14 @@ class CreateProtectedTags < ActiveRecord::Migration add_index :protected_tags, :project_id - create_table :protected_tag_push_access_levels do |t| - t.references :protected_tag, index: { name: "index_protected_tag_push_access" }, foreign_key: true, null: false + create_table :protected_tag_create_access_levels do |t| + t.references :protected_tag, index: { name: "index_protected_tag_create_access" }, foreign_key: true, null: false t.integer :access_level, default: GITLAB_ACCESS_MASTER, null: true t.references :user, foreign_key: true, index: true - t.integer :group_id#TODO: Should this have an index? Doesn't appear in brances #, index: true + t.integer :group_id t.timestamps null: false end - add_foreign_key :protected_tag_push_access_levels, :namespaces, column: :group_id # rubocop: disable Migration/AddConcurrentForeignKey + add_foreign_key :protected_tag_create_access_levels, :namespaces, column: :group_id # rubocop: disable Migration/AddConcurrentForeignKey end end diff --git a/db/schema.rb b/db/schema.rb index 650b18bb013..2a070583834 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -963,7 +963,7 @@ ActiveRecord::Schema.define(version: 20170315194013) do add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree - create_table "protected_tag_push_access_levels", force: :cascade do |t| + create_table "protected_tag_create_access_levels", force: :cascade do |t| t.integer "protected_tag_id", null: false t.integer "access_level", default: 40 t.integer "user_id" @@ -972,8 +972,8 @@ ActiveRecord::Schema.define(version: 20170315194013) do t.datetime "updated_at", null: false end - add_index "protected_tag_push_access_levels", ["protected_tag_id"], name: "index_protected_tag_push_access", using: :btree - add_index "protected_tag_push_access_levels", ["user_id"], name: "index_protected_tag_push_access_levels_on_user_id", using: :btree + add_index "protected_tag_create_access_levels", ["protected_tag_id"], name: "index_protected_tag_create_access", using: :btree + add_index "protected_tag_create_access_levels", ["user_id"], name: "index_protected_tag_create_access_levels_on_user_id", using: :btree create_table "protected_tags", force: :cascade do |t| t.integer "project_id", null: false @@ -1326,9 +1326,9 @@ ActiveRecord::Schema.define(version: 20170315194013) do add_foreign_key "project_statistics", "projects", on_delete: :cascade add_foreign_key "protected_branch_merge_access_levels", "protected_branches" add_foreign_key "protected_branch_push_access_levels", "protected_branches" - add_foreign_key "protected_tag_push_access_levels", "namespaces", column: "group_id" - add_foreign_key "protected_tag_push_access_levels", "protected_tags" - add_foreign_key "protected_tag_push_access_levels", "users" + add_foreign_key "protected_tag_create_access_levels", "namespaces", column: "group_id" + add_foreign_key "protected_tag_create_access_levels", "protected_tags" + add_foreign_key "protected_tag_create_access_levels", "users" add_foreign_key "subscriptions", "projects", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 7bf5568a8ee..25e03ec64d3 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -47,7 +47,7 @@ project_tree: - :merge_access_levels - :push_access_levels - protected_tags: - - :push_access_levels + - :create_access_levels - :project_feature # Only include the following attributes for the models specified. diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index b1d8e385f2e..430de9a1bf8 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -9,7 +9,7 @@ module Gitlab hooks: 'ProjectHook', merge_access_levels: 'ProtectedBranch::MergeAccessLevel', push_access_levels: 'ProtectedBranch::PushAccessLevel', - #TODO: How to add?- push_access_levels: 'ProtectedTag::PushAccessLevel', + create_access_levels: 'ProtectedTag::CreateAccessLevel', labels: :project_labels, priorities: :label_priorities, label: :project_label }.freeze diff --git a/spec/factories/protected_tags.rb b/spec/factories/protected_tags.rb index f0016b37d66..d8e90ae1ee1 100644 --- a/spec/factories/protected_tags.rb +++ b/spec/factories/protected_tags.rb @@ -4,18 +4,18 @@ FactoryGirl.define do project after(:build) do |protected_tag| - protected_tag.push_access_levels.new(access_level: Gitlab::Access::MASTER) + protected_tag.create_access_levels.new(access_level: Gitlab::Access::MASTER) end - trait :developers_can_push do + trait :developers_can_create do after(:create) do |protected_tag| - protected_tag.push_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER) + protected_tag.create_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER) end end - trait :no_one_can_push do + trait :no_one_can_create do after(:create) do |protected_tag| - protected_tag.push_access_levels.first.update!(access_level: Gitlab::Access::NO_ACCESS) + protected_tag.create_access_levels.first.update!(access_level: Gitlab::Access::NO_ACCESS) end 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 d7152926629..33a07786007 100644 --- a/spec/features/protected_tags/access_control_ce_spec.rb +++ b/spec/features/protected_tags/access_control_ce_spec.rb @@ -1,23 +1,23 @@ RSpec.shared_examples "protected tags > access control > CE" do - ProtectedTag::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| - it "allows creating protected tags that #{access_type_name} can push to" 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_push_button = find(".js-allowed-to-push") + allowed_to_create_button = find(".js-allowed-to-create") - unless allowed_to_push_button.text == access_type_name - allowed_to_push_button.click + unless allowed_to_create_button.text == access_type_name + allowed_to_create_button.click within(".dropdown.open .dropdown-menu") { click_on access_type_name } end end click_on "Protect" expect(ProtectedTag.count).to eq(1) - expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to eq([access_type_id]) + expect(ProtectedTag.last.create_access_levels.map(&:access_level)).to eq([access_type_id]) end - it "allows updating protected tags so that #{access_type_name} can push to them" 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" @@ -25,16 +25,16 @@ RSpec.shared_examples "protected tags > access control > CE" do expect(ProtectedTag.count).to eq(1) within(".protected-tags-list") do - find(".js-allowed-to-push").click + find(".js-allowed-to-create").click - within('.js-allowed-to-push-container') do + within('.js-allowed-to-create-container') do expect(first("li")).to have_content("Roles") click_on access_type_name end end wait_for_ajax - expect(ProtectedTag.last.push_access_levels.map(&:access_level)).to include(access_type_id) + expect(ProtectedTag.last.create_access_levels.map(&:access_level)).to include(access_type_id) end end end diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index afc29baa7e6..959ae02c222 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -86,7 +86,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do end context 'when user has access' do - let!(:protected_tag) { create(:protected_tag, :developers_can_push, project: project, name: 'v*') } + let!(:protected_tag) { create(:protected_tag, :developers_can_create, project: project, name: 'v*') } it 'allows tag creation' do expect(subject.status).to be(true) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 83503c73e75..53cfd674b02 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -113,11 +113,12 @@ protected_branches: - push_access_levels protected_tags: - project -- push_access_levels +- create_access_levels merge_access_levels: - protected_branch push_access_levels: - protected_branch +create_access_levels: - protected_tag project: - taggings diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index c69ff3446ea..611cdbbc865 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -189,7 +189,7 @@ describe Gitlab::UserAccess, lib: true do describe 'push to protected tag if allowed for developers' do before do - @tag = create(:protected_tag, :developers_can_push, project: project) + @tag = create(:protected_tag, :developers_can_create, project: project) end it 'returns true if user is a master' do diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb index 70ea96a954f..d91a58e8de5 100644 --- a/spec/services/protected_tags/create_service_spec.rb +++ b/spec/services/protected_tags/create_service_spec.rb @@ -6,7 +6,7 @@ describe ProtectedTags::CreateService, services: true do let(:params) do { name: 'master', - push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }] + create_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }] } end @@ -15,7 +15,7 @@ describe ProtectedTags::CreateService, services: true do it 'creates a new protected tag' do expect { service.execute }.to change(ProtectedTag, :count).by(1) - expect(project.protected_tags.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) + expect(project.protected_tags.last.create_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) end end end -- cgit v1.2.1 From d85471ac1a4574053b057dd7cc02858d591a8ffd Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 03:50:15 +0100 Subject: Fixed UserAccess#can_create_tag? after create_access_levels rename --- app/models/concerns/protected_ref.rb | 4 ++-- lib/gitlab/user_access.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index ab28eb19b64..7c0183952a0 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -8,7 +8,7 @@ module ProtectedRef delegate :matching, :matches?, :wildcard?, to: :ref_matcher - def self.protected_ref_accessible_to?(ref, user, action: :push) + def self.protected_ref_accessible_to?(ref, user, action:) access_levels_for_ref(ref, action: action).any? do |access_level| access_level.check_access(user) end @@ -20,7 +20,7 @@ module ProtectedRef end end - def self.access_levels_for_ref(ref, action: :push) + def self.access_levels_for_ref(ref, action:) self.matching(ref).map(&:"#{action}_access_levels").flatten end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 6af5de4dc08..54728e5ff0e 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -32,7 +32,7 @@ module Gitlab return false unless can_access_git? if ProtectedTag.protected?(project, ref) - project.protected_tags.protected_ref_accessible_to?(ref, user) + project.protected_tags.protected_ref_accessible_to?(ref, user, action: :create) else user.can?(:push_code, project) end -- cgit v1.2.1 From 9f9a7a2d90f6775b42bef308ba72d9f59345f50f Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 4 Apr 2017 15:20:32 +0100 Subject: Added ProtectedTag to import/export son and safe_model_attributes --- spec/lib/gitlab/import_export/project.json | 18 ++++++++++++++++++ .../gitlab/import_export/project_tree_restorer_spec.rb | 4 ++++ .../lib/gitlab/import_export/safe_model_attributes.yml | 14 ++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index d9b67426818..7a0b0b06d4b 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -7455,6 +7455,24 @@ ] } ], + "protected_tags": [ + { + "id": 1, + "project_id": 9, + "name": "v*", + "created_at": "2017-04-04T13:48:13.426Z", + "updated_at": "2017-04-04T13:48:13.426Z", + "create_access_levels": [ + { + "id": 1, + "protected_tag_id": 1, + "access_level": 40, + "created_at": "2017-04-04T13:48:13.458Z", + "updated_at": "2017-04-04T13:48:13.458Z" + } + ] + } + ], "project_feature": { "builds_access_level": 0, "created_at": "2014-12-26T09:26:45.000Z", diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index c36f12dbd82..96d5ff414dc 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -64,6 +64,10 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do expect(ProtectedBranch.first.push_access_levels).not_to be_empty end + it 'contains the create access levels on a protected tag' do + expect(ProtectedTag.first.create_access_levels).not_to be_empty + end + context 'event at forth level of the tree' do let(:event) { Event.where(title: 'test levels').first } diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 1ad16a9b57d..0c315ac672e 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -300,6 +300,12 @@ ProtectedBranch: - name - created_at - updated_at +ProtectedTag: +- id +- project_id +- name +- created_at +- updated_at Project: - description - issues_enabled @@ -333,6 +339,14 @@ ProtectedBranch::PushAccessLevel: - access_level - created_at - updated_at +ProtectedTag::CreateAccessLevel: +- id +- protected_tag_id +- access_level +- created_at +- updated_at +- user_id +- group_id AwardEmoji: - id - user_id -- cgit v1.2.1 From 678672b0664d94d33b606d8ad13e1333cad2a5be Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 14:16:52 +0530 Subject: Bundle renamed to `index.js` --- app/assets/javascripts/protected_tags/index.js | 5 +++++ app/assets/javascripts/protected_tags/protected_tags_bundle.js | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) create mode 100644 app/assets/javascripts/protected_tags/index.js delete mode 100644 app/assets/javascripts/protected_tags/protected_tags_bundle.js diff --git a/app/assets/javascripts/protected_tags/index.js b/app/assets/javascripts/protected_tags/index.js new file mode 100644 index 00000000000..889a8053e6f --- /dev/null +++ b/app/assets/javascripts/protected_tags/index.js @@ -0,0 +1,5 @@ +require('./protected_tag_access_dropdown'); +require('./protected_tag_create'); +require('./protected_tag_dropdown'); +require('./protected_tag_edit'); +require('./protected_tag_edit_list'); diff --git a/app/assets/javascripts/protected_tags/protected_tags_bundle.js b/app/assets/javascripts/protected_tags/protected_tags_bundle.js deleted file mode 100644 index 889a8053e6f..00000000000 --- a/app/assets/javascripts/protected_tags/protected_tags_bundle.js +++ /dev/null @@ -1,5 +0,0 @@ -require('./protected_tag_access_dropdown'); -require('./protected_tag_create'); -require('./protected_tag_dropdown'); -require('./protected_tag_edit'); -require('./protected_tag_edit_list'); -- cgit v1.2.1 From 115d4a41061a94c294fa7cbd218a983ef06e0965 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 14:17:19 +0530 Subject: Convert to pure ES6 class --- .../protected_tag_access_dropdown.js | 51 +++++------ .../protected_tags/protected_tag_create.js | 86 +++++++++--------- .../protected_tags/protected_tag_dropdown.js | 12 +-- .../protected_tags/protected_tag_edit.js | 100 ++++++++++----------- .../protected_tags/protected_tag_edit_list.js | 29 +++--- 5 files changed, 132 insertions(+), 146 deletions(-) diff --git a/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js index b85c2991dd9..681b060f859 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js +++ b/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js @@ -1,29 +1,26 @@ -/* eslint-disable arrow-parens, no-param-reassign, object-shorthand, no-else-return, comma-dangle, max-len */ +export default class ProtectedTagAccessDropdown { + constructor(options) { + this.options = options; + this.initDropdown(); + } -(global => { - global.gl = global.gl || {}; - - gl.ProtectedTagAccessDropdown = class { - constructor(options) { - const { $dropdown, data, onSelect } = options; - - $dropdown.glDropdown({ - data: data, - selectable: true, - inputId: $dropdown.data('input-id'), - fieldName: $dropdown.data('field-name'), - toggleLabel(item, el) { - if (el.is('.is-active')) { - return item.text; - } else { - return 'Select'; - } - }, - clicked(item, $el, e) { - e.preventDefault(); - onSelect(); + initDropdown() { + const { onSelect } = this.options; + this.options.$dropdown.glDropdown({ + data: this.options.data, + selectable: true, + inputId: this.options.$dropdown.data('input-id'), + fieldName: this.options.$dropdown.data('field-name'), + toggleLabel(item, el) { + if (el.is('.is-active')) { + return item.text; } - }); - } - }; -})(window); + return 'Select'; + }, + clicked(item, $el, e) { + e.preventDefault(); + onSelect(); + }, + }); + } +} diff --git a/app/assets/javascripts/protected_tags/protected_tag_create.js b/app/assets/javascripts/protected_tags/protected_tag_create.js index 84b1b232649..964e67c9de0 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_create.js +++ b/app/assets/javascripts/protected_tags/protected_tag_create.js @@ -1,45 +1,41 @@ -/* eslint-disable no-new, arrow-parens, no-param-reassign, comma-dangle, max-len */ -/* global ProtectedTagDropdown */ - -(global => { - global.gl = global.gl || {}; - - gl.ProtectedTagCreate = class { - constructor() { - this.$wrap = this.$form = $('.new_protected_tag'); - this.buildDropdowns(); - } - - buildDropdowns() { - const $allowedToCreateDropdown = this.$wrap.find('.js-allowed-to-create'); - - // Cache callback - this.onSelectCallback = this.onSelect.bind(this); - - // Allowed to Create dropdown - new gl.ProtectedTagAccessDropdown({ - $dropdown: $allowedToCreateDropdown, - data: gon.create_access_levels, - onSelect: this.onSelectCallback - }); - - // Select default - $allowedToCreateDropdown.data('glDropdown').selectRowAtIndex(0); - - // Protected tag dropdown - new ProtectedTagDropdown({ - $dropdown: this.$wrap.find('.js-protected-tag-select'), - onSelect: this.onSelectCallback - }); - } - - // This will run after clicked callback - onSelect() { - // Enable submit button - const $tagInput = this.$wrap.find('input[name="protected_tag[name]"]'); - const $allowedToCreateInput = this.$wrap.find('input[name="protected_tag[create_access_levels_attributes][0][access_level]"]'); - - this.$form.find('input[type="submit"]').attr('disabled', !($tagInput.val() && $allowedToCreateInput.length)); - } - }; -})(window); +import ProtectedTagAccessDropdown from './protected_tag_access_dropdown'; +import ProtectedTagDropdown from './protected_tag_dropdown'; + +export default class ProtectedTagCreate { + constructor() { + this.$form = $('.new_protected_tag'); + this.buildDropdowns(); + } + + buildDropdowns() { + const $allowedToCreateDropdown = this.$form.find('.js-allowed-to-create'); + + // Cache callback + this.onSelectCallback = this.onSelect.bind(this); + + // Allowed to Create dropdown + this.protectedTagAccessDropdown = new ProtectedTagAccessDropdown({ + $dropdown: $allowedToCreateDropdown, + data: gon.create_access_levels, + onSelect: this.onSelectCallback, + }); + + // Select default + $allowedToCreateDropdown.data('glDropdown').selectRowAtIndex(0); + + // Protected tag dropdown + this.protectedTagDropdown = new ProtectedTagDropdown({ + $dropdown: this.$form.find('.js-protected-tag-select'), + onSelect: this.onSelectCallback, + }); + } + + // This will run after clicked callback + onSelect() { + // Enable submit button + const $tagInput = this.$form.find('input[name="protected_tag[name]"]'); + const $allowedToCreateInput = this.$form.find('input[name="protected_tag[create_access_levels_attributes][0][access_level]"]'); + + this.$form.find('input[type="submit"]').attr('disabled', !($tagInput.val() && $allowedToCreateInput.length)); + } +} diff --git a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js index ccc4c81fa18..9be9e2bea6f 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js +++ b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js @@ -1,6 +1,4 @@ -/* eslint-disable comma-dangle, no-unused-vars */ - -class ProtectedTagDropdown { +export default class ProtectedTagDropdown { constructor(options) { this.onSelect = options.onSelect; this.$dropdown = options.$dropdown; @@ -21,7 +19,7 @@ class ProtectedTagDropdown { filterable: true, remote: false, search: { - fields: ['title'] + fields: ['title'], }, selectable: true, toggleLabel(selected) { @@ -38,7 +36,7 @@ class ProtectedTagDropdown { clicked: (item, $el, e) => { e.preventDefault(); this.onSelect(); - } + }, }); } @@ -63,7 +61,7 @@ class ProtectedTagDropdown { this.selectedTag = { title: tagName, id: tagName, - text: tagName + text: tagName, }; if (tagName) { @@ -75,5 +73,3 @@ class ProtectedTagDropdown { this.$dropdownFooter.toggleClass('hidden', !tagName); } } - -window.ProtectedTagDropdown = ProtectedTagDropdown; diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit.js b/app/assets/javascripts/protected_tags/protected_tag_edit.js index 0227be35c8f..b5092877138 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_edit.js +++ b/app/assets/javascripts/protected_tags/protected_tag_edit.js @@ -1,54 +1,52 @@ -/* eslint-disable no-new, arrow-parens, no-param-reassign, comma-dangle, max-len */ +/* eslint-disable no-new */ /* global Flash */ -(global => { - global.gl = global.gl || {}; - - gl.ProtectedTagEdit = class { - constructor(options) { - this.$wrap = options.$wrap; - this.$allowedToCreateDropdown = this.$wrap.find('.js-allowed-to-create'); - - this.buildDropdowns(); - } - - buildDropdowns() { - // Allowed to create dropdown - new gl.ProtectedTagAccessDropdown({ - $dropdown: this.$allowedToCreateDropdown, - data: gon.create_access_levels, - onSelect: this.onSelect.bind(this) - }); - } - - onSelect() { - const $allowedToCreateInput = this.$wrap.find(`input[name="${this.$allowedToCreateDropdown.data('fieldName')}"]`); - - // Do not update if one dropdown has not selected any option - if (!$allowedToCreateInput.length) return; - - this.$allowedToCreateDropdown.disable(); - - $.ajax({ - type: 'POST', - url: this.$wrap.data('url'), - dataType: 'json', - data: { - _method: 'PATCH', - protected_tag: { - create_access_levels_attributes: [{ - id: this.$allowedToCreateDropdown.data('access-level-id'), - access_level: $allowedToCreateInput.val() - }] - } +import ProtectedTagAccessDropdown from './protected_tag_access_dropdown'; + +export default class ProtectedTagEdit { + constructor(options) { + this.$wrap = options.$wrap; + this.$allowedToCreateDropdown = this.$wrap.find('.js-allowed-to-create'); + + this.buildDropdowns(); + } + + buildDropdowns() { + // Allowed to create dropdown + this.protectedTagAccessDropdown = new ProtectedTagAccessDropdown({ + $dropdown: this.$allowedToCreateDropdown, + data: gon.create_access_levels, + onSelect: this.onSelect.bind(this), + }); + } + + onSelect() { + const $allowedToCreateInput = this.$wrap.find(`input[name="${this.$allowedToCreateDropdown.data('fieldName')}"]`); + + // Do not update if one dropdown has not selected any option + if (!$allowedToCreateInput.length) return; + + this.$allowedToCreateDropdown.disable(); + + $.ajax({ + type: 'POST', + url: this.$wrap.data('url'), + dataType: 'json', + data: { + _method: 'PATCH', + protected_tag: { + create_access_levels_attributes: [{ + id: this.$allowedToCreateDropdown.data('access-level-id'), + access_level: $allowedToCreateInput.val(), + }], }, - error() { - $.scrollTo(0); - new Flash('Failed to update tag!'); - } - }).always(() => { - this.$allowedToCreateDropdown.enable(); - }); - } - }; -})(window); + }, + error() { + $.scrollTo(0); + new Flash('Failed to update tag!'); + }, + }).always(() => { + this.$allowedToCreateDropdown.enable(); + }); + } +} diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit_list.js b/app/assets/javascripts/protected_tags/protected_tag_edit_list.js index ba40c227ef4..88c7accdec6 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_edit_list.js +++ b/app/assets/javascripts/protected_tags/protected_tag_edit_list.js @@ -1,18 +1,17 @@ -/* eslint-disable arrow-parens, no-param-reassign, no-new, comma-dangle */ +import ProtectedTagEdit from './protected_tag_edit'; -(global => { - global.gl = global.gl || {}; +export default class ProtectedTagEditList { + constructor() { + this.$wrap = $('.protected-tags-list'); + this.protectedTagList = []; + this.initEditForm(); + } - gl.ProtectedTagEditList = class { - constructor() { - this.$wrap = $('.protected-tags-list'); - - // Build edit forms - this.$wrap.find('.js-protected-tag-edit-form').each((i, el) => { - new gl.ProtectedTagEdit({ - $wrap: $(el) - }); + initEditForm() { + this.$wrap.find('.js-protected-tag-edit-form').each((i, el) => { + this.protectedTagList[i] = new ProtectedTagEdit({ + $wrap: $(el), }); - } - }; -})(window); + }); + } +} -- cgit v1.2.1 From 92b75704c2b6e827d333b85ab3d1d99aed84cb6f Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 14:17:41 +0530 Subject: Re-export classes using ES6 `export` --- app/assets/javascripts/protected_tags/index.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/protected_tags/index.js b/app/assets/javascripts/protected_tags/index.js index 889a8053e6f..da036f5437e 100644 --- a/app/assets/javascripts/protected_tags/index.js +++ b/app/assets/javascripts/protected_tags/index.js @@ -1,5 +1,5 @@ -require('./protected_tag_access_dropdown'); -require('./protected_tag_create'); -require('./protected_tag_dropdown'); -require('./protected_tag_edit'); -require('./protected_tag_edit_list'); +export { default as ProtectedTagAccessDropdown } from './protected_tag_access_dropdown'; +export { default as ProtectedTagCreate } from './protected_tag_create'; +export { default as ProtectedTagDropdown } from './protected_tag_dropdown'; +export { default as ProtectedTagEdit } from './protected_tag_edit'; +export { default as ProtectedTagEditList } from './protected_tag_edit_list'; -- cgit v1.2.1 From 59be20f06fa6d4a98dac5b45c3dee2aeebe813e2 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 14:18:03 +0530 Subject: Import Protected Tags classes --- app/assets/javascripts/dispatcher.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index d384927cc5b..4dc82bee6c3 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -41,6 +41,7 @@ import GroupsList from './groups_list'; import ProjectsList from './projects_list'; import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; import BlobLinePermalinkUpdater from './blob/blob_line_permalink_updater'; +import { ProtectedTagCreate, ProtectedTagEditList } from './protected_tags'; const ShortcutsBlob = require('./shortcuts_blob'); const UserCallout = require('./user_callout'); @@ -322,8 +323,8 @@ const UserCallout = require('./user_callout'); new gl.ProtectedBranchCreate(); new gl.ProtectedBranchEditList(); // Initialize Protected Tag Settings - new gl.ProtectedTagCreate(); - new gl.ProtectedTagEditList(); + new ProtectedTagCreate(); + new ProtectedTagEditList(); break; case 'projects:ci_cd:show': new gl.ProjectVariables(); -- cgit v1.2.1 From 3c414a9fa92e48ca021c1671fca9ad096a34d1c4 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 14:18:17 +0530 Subject: Update bundle path for Protected Tags bundle --- config/webpack.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/webpack.config.js b/config/webpack.config.js index d861fa0c7a4..d5580650545 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -39,7 +39,7 @@ var config = { network: './network/network_bundle.js', profile: './profile/profile_bundle.js', protected_branches: './protected_branches/protected_branches_bundle.js', - protected_tags: './protected_tags/protected_tags_bundle.js', + protected_tags: './protected_tags', snippet: './snippet/snippet_bundle.js', terminal: './terminal/terminal_bundle.js', u2f: ['vendor/u2f'], -- cgit v1.2.1 From cd5b36d04e79ed8fcd649127e0d47e09ec325242 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 14:47:50 +0530 Subject: Add constructor doc, toggle helper --- .../protected_tags/protected_tag_dropdown.js | 26 +++++++++++++++------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js index 9be9e2bea6f..9c78f2816a4 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js +++ b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js @@ -1,4 +1,10 @@ export default class ProtectedTagDropdown { + /** + * @param {Object} options containing + * `$dropdown` target element + * `onSelect` event callback + * $dropdown must be an element created using `dropdown_tag()` rails helper + */ constructor(options) { this.onSelect = options.onSelect; this.$dropdown = options.$dropdown; @@ -10,7 +16,7 @@ export default class ProtectedTagDropdown { this.bindEvents(); // Hide footer - this.$dropdownFooter.addClass('hidden'); + this.toggleFooter(true); } buildDropdown() { @@ -58,18 +64,22 @@ export default class ProtectedTagDropdown { } toggleCreateNewButton(tagName) { - this.selectedTag = { - title: tagName, - id: tagName, - text: tagName, - }; - if (tagName) { + this.selectedTag = { + title: tagName, + id: tagName, + text: tagName, + }; + this.$dropdownContainer .find('.create-new-protected-tag code') .text(tagName); } - this.$dropdownFooter.toggleClass('hidden', !tagName); + this.toggleFooter(!tagName); + } + + toggleFooter(toggleState) { + this.$dropdownFooter.toggleClass('hidden', toggleState); } } -- cgit v1.2.1 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.1 From ca9cded45d78e821a1c778e22fde523873ffaf93 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 6 Apr 2017 19:09:48 +0530 Subject: Fixes as per feedback --- app/assets/javascripts/protected_tags/index.js | 3 --- .../protected_tags/protected_tag_access_dropdown.js | 4 ++-- .../javascripts/protected_tags/protected_tag_create.js | 4 ++-- .../javascripts/protected_tags/protected_tag_dropdown.js | 3 ++- .../javascripts/protected_tags/protected_tag_edit.js | 16 ++++++++-------- .../protected_tags/protected_tag_edit_list.js | 5 +++-- app/assets/stylesheets/pages/projects.scss | 3 ++- .../protected_tags/_create_protected_tag.html.haml | 2 +- 8 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/protected_tags/index.js b/app/assets/javascripts/protected_tags/index.js index da036f5437e..61e7ba53862 100644 --- a/app/assets/javascripts/protected_tags/index.js +++ b/app/assets/javascripts/protected_tags/index.js @@ -1,5 +1,2 @@ -export { default as ProtectedTagAccessDropdown } from './protected_tag_access_dropdown'; export { default as ProtectedTagCreate } from './protected_tag_create'; -export { default as ProtectedTagDropdown } from './protected_tag_dropdown'; -export { default as ProtectedTagEdit } from './protected_tag_edit'; export { default as ProtectedTagEditList } from './protected_tag_edit_list'; diff --git a/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js index 681b060f859..fff83f3af3b 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js +++ b/app/assets/javascripts/protected_tags/protected_tag_access_dropdown.js @@ -11,8 +11,8 @@ export default class ProtectedTagAccessDropdown { selectable: true, inputId: this.options.$dropdown.data('input-id'), fieldName: this.options.$dropdown.data('field-name'), - toggleLabel(item, el) { - if (el.is('.is-active')) { + toggleLabel(item, $el) { + if ($el.is('.is-active')) { return item.text; } return 'Select'; diff --git a/app/assets/javascripts/protected_tags/protected_tag_create.js b/app/assets/javascripts/protected_tags/protected_tag_create.js index 964e67c9de0..91bd140bd12 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_create.js +++ b/app/assets/javascripts/protected_tags/protected_tag_create.js @@ -3,7 +3,7 @@ import ProtectedTagDropdown from './protected_tag_dropdown'; export default class ProtectedTagCreate { constructor() { - this.$form = $('.new_protected_tag'); + this.$form = $('.js-new-protected-tag'); this.buildDropdowns(); } @@ -34,7 +34,7 @@ export default class ProtectedTagCreate { onSelect() { // Enable submit button const $tagInput = this.$form.find('input[name="protected_tag[name]"]'); - const $allowedToCreateInput = this.$form.find('input[name="protected_tag[create_access_levels_attributes][0][access_level]"]'); + const $allowedToCreateInput = this.$form.find('#create_access_levels_attributes'); this.$form.find('input[type="submit"]').attr('disabled', !($tagInput.val() && $allowedToCreateInput.length)); } diff --git a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js index 9c78f2816a4..5ff4e443262 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js +++ b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js @@ -50,9 +50,10 @@ export default class ProtectedTagDropdown { this.$protectedTag.on('click', this.onClickCreateWildcard.bind(this)); } - onClickCreateWildcard() { + onClickCreateWildcard(e) { this.$dropdown.data('glDropdown').remote.execute(); this.$dropdown.data('glDropdown').selectRowAtIndex(); + e.preventDefault(); } getProtectedTags(term, callback) { diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit.js b/app/assets/javascripts/protected_tags/protected_tag_edit.js index b5092877138..624067a5a09 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_edit.js +++ b/app/assets/javascripts/protected_tags/protected_tag_edit.js @@ -6,7 +6,8 @@ import ProtectedTagAccessDropdown from './protected_tag_access_dropdown'; export default class ProtectedTagEdit { constructor(options) { this.$wrap = options.$wrap; - this.$allowedToCreateDropdown = this.$wrap.find('.js-allowed-to-create'); + this.$allowedToCreateDropdownButton = this.$wrap.find('.js-allowed-to-create'); + this.onSelectCallback = this.onSelect.bind(this); this.buildDropdowns(); } @@ -14,19 +15,19 @@ export default class ProtectedTagEdit { buildDropdowns() { // Allowed to create dropdown this.protectedTagAccessDropdown = new ProtectedTagAccessDropdown({ - $dropdown: this.$allowedToCreateDropdown, + $dropdown: this.$allowedToCreateDropdownButton, data: gon.create_access_levels, - onSelect: this.onSelect.bind(this), + onSelect: this.onSelectCallback, }); } onSelect() { - const $allowedToCreateInput = this.$wrap.find(`input[name="${this.$allowedToCreateDropdown.data('fieldName')}"]`); + const $allowedToCreateInput = this.$wrap.find(`input[name="${this.$allowedToCreateDropdownButton.data('fieldName')}"]`); // Do not update if one dropdown has not selected any option if (!$allowedToCreateInput.length) return; - this.$allowedToCreateDropdown.disable(); + this.$allowedToCreateDropdownButton.disable(); $.ajax({ type: 'POST', @@ -36,17 +37,16 @@ export default class ProtectedTagEdit { _method: 'PATCH', protected_tag: { create_access_levels_attributes: [{ - id: this.$allowedToCreateDropdown.data('access-level-id'), + id: this.$allowedToCreateDropdownButton.data('access-level-id'), access_level: $allowedToCreateInput.val(), }], }, }, error() { - $.scrollTo(0); new Flash('Failed to update tag!'); }, }).always(() => { - this.$allowedToCreateDropdown.enable(); + this.$allowedToCreateDropdownButton.enable(); }); } } diff --git a/app/assets/javascripts/protected_tags/protected_tag_edit_list.js b/app/assets/javascripts/protected_tags/protected_tag_edit_list.js index 88c7accdec6..bd9fc872266 100644 --- a/app/assets/javascripts/protected_tags/protected_tag_edit_list.js +++ b/app/assets/javascripts/protected_tags/protected_tag_edit_list.js @@ -1,15 +1,16 @@ +/* eslint-disable no-new */ + import ProtectedTagEdit from './protected_tag_edit'; export default class ProtectedTagEditList { constructor() { this.$wrap = $('.protected-tags-list'); - this.protectedTagList = []; this.initEditForm(); } initEditForm() { this.$wrap.find('.js-protected-tag-edit-form').each((i, el) => { - this.protectedTagList[i] = new ProtectedTagEdit({ + new ProtectedTagEdit({ $wrap: $(el), }); }); diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 960875674cc..61b973e0aa9 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -779,7 +779,8 @@ pre.light-well { .protected-tags-list { .dropdown-menu-toggle { - width: 300px; + width: 100%; + max-width: 300px; } } diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml index af332f942d6..148efc16e64 100644 --- a/app/views/projects/protected_tags/_create_protected_tag.html.haml +++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml @@ -1,4 +1,4 @@ -= form_for [@project.namespace.becomes(Namespace), @project, @protected_tag], html: { class: 'new_protected_tag' } do |f| += form_for [@project.namespace.becomes(Namespace), @project, @protected_tag], html: { class: 'js-new-protected-tag' } do |f| .panel.panel-default .panel-heading %h3.panel-title -- cgit v1.2.1 From 7d17fcea84760d4c8d94b03adc4a23ef733ad4e5 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Thu, 6 Apr 2017 15:47:13 +0100 Subject: Fix projected import failing on missing relations --- lib/gitlab/import_export/project_tree_restorer.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index df21ff22216..2e349b5f9a9 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -52,7 +52,11 @@ module Gitlab create_sub_relations(relation, @tree_hash) if relation.is_a?(Hash) relation_key = relation.is_a?(Hash) ? relation.keys.first : relation - relation_hash = create_relation(relation_key, @tree_hash[relation_key.to_s]) + relation_hash_list = @tree_hash[relation_key.to_s] + + next unless relation_hash_list + + relation_hash = create_relation(relation_key, relation_hash_list) saved << restored_project.append_or_update_attribute(relation_key, relation_hash) end saved.all? -- cgit v1.2.1 From 902054db59e02cb14c28ecffd9dff95994dbb01f Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Thu, 6 Apr 2017 21:21:13 +0100 Subject: Fix within('#new_protected_tag') in protected tags spec --- spec/features/protected_tags/access_control_ce_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/protected_tags/access_control_ce_spec.rb b/spec/features/protected_tags/access_control_ce_spec.rb index de2556041e7..6e2bff82785 100644 --- a/spec/features/protected_tags/access_control_ce_spec.rb +++ b/spec/features/protected_tags/access_control_ce_spec.rb @@ -5,7 +5,7 @@ RSpec.shared_examples "protected tags > access control > CE" do set_protected_tag_name('master') - within('.new_protected_tag') do + within('#new_protected_tag') do allowed_to_create_button = find(".js-allowed-to-create") unless allowed_to_create_button.text == access_type_name -- cgit v1.2.1 From 3dc8c3127bf96a820efc807c86e62fbffc9a5b0d Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Fri, 7 Apr 2017 02:24:45 +0530 Subject: Fix selector for form wrapper --- spec/features/protected_tags/access_control_ce_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/protected_tags/access_control_ce_spec.rb b/spec/features/protected_tags/access_control_ce_spec.rb index de2556041e7..5b2baf8616c 100644 --- a/spec/features/protected_tags/access_control_ce_spec.rb +++ b/spec/features/protected_tags/access_control_ce_spec.rb @@ -5,7 +5,7 @@ RSpec.shared_examples "protected tags > access control > CE" do set_protected_tag_name('master') - within('.new_protected_tag') do + within('.js-new-protected-tag') do allowed_to_create_button = find(".js-allowed-to-create") unless allowed_to_create_button.text == access_type_name @@ -31,7 +31,7 @@ RSpec.shared_examples "protected tags > access control > CE" do within(".protected-tags-list") do find(".js-allowed-to-create").click - + within('.js-allowed-to-create-container') do expect(first("li")).to have_content("Roles") click_on access_type_name -- cgit v1.2.1 From 9db87fce130bdcb4831631c46fbb9d5717472af2 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 7 Apr 2017 01:14:10 +0100 Subject: Protected tags changes from backend maintainer review --- app/models/concerns/protected_ref_access.rb | 2 +- app/models/protected_ref_matcher.rb | 4 +++- changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index 0c7e5157cdf..c4f158e569a 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -11,7 +11,7 @@ module ProtectedRefAccess end def check_access(user) - return true if user.is_admin? + return true if user.admin? project.team.max_member_access(user.id) >= access_level end diff --git a/app/models/protected_ref_matcher.rb b/app/models/protected_ref_matcher.rb index 83f44240259..d970f2b01fc 100644 --- a/app/models/protected_ref_matcher.rb +++ b/app/models/protected_ref_matcher.rb @@ -4,7 +4,7 @@ class ProtectedRefMatcher end # Returns all protected refs that match the given ref name. - # This realizes all records from the scope built up so far, and does + # This checks all records from the scope built up so far, and does # _not_ return a relation. # # This method optionally takes in a list of `protected_refs` to search @@ -38,6 +38,8 @@ class ProtectedRefMatcher end def wildcard_match?(ref_name) + return false unless wildcard? + wildcard_regex === ref_name end diff --git a/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml b/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml index c6ea5da65a5..fabe24e485a 100644 --- a/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml +++ b/changelogs/unreleased/18471-restrict-tag-pushes-protected-tags.yml @@ -1,4 +1,4 @@ --- -title: Protected Tags feature +title: Tags can be protected, restricting creation of matching tags by user role merge_request: 10356 author: -- cgit v1.2.1 From 72419f3a8b94874de059f3567980844c7aaa6336 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 7 Apr 2017 02:35:59 +0100 Subject: Documentation for protected tags --- .../protected_tags/_create_protected_tag.html.haml | 6 +-- doc/university/glossary/README.md | 4 ++ doc/user/permissions.md | 1 + .../project/img/project_repository_settings.png | Bin 0 -> 35236 bytes doc/user/project/img/protected_tag_matches.png | Bin 0 -> 85305 bytes doc/user/project/img/protected_tags_list.png | Bin 0 -> 24490 bytes doc/user/project/img/protected_tags_page.png | Bin 0 -> 56112 bytes .../img/protected_tags_permissions_dropdown.png | Bin 0 -> 26514 bytes doc/user/project/protected_tags.md | 60 +++++++++++++++++++++ doc/workflow/README.md | 1 + 10 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 doc/user/project/img/project_repository_settings.png create mode 100644 doc/user/project/img/protected_tag_matches.png create mode 100644 doc/user/project/img/protected_tags_list.png create mode 100644 doc/user/project/img/protected_tags_page.png create mode 100644 doc/user/project/img/protected_tags_permissions_dropdown.png create mode 100644 doc/user/project/protected_tags.md diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml index 148efc16e64..6e187b54a59 100644 --- a/app/views/projects/protected_tags/_create_protected_tag.html.haml +++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml @@ -12,11 +12,11 @@ .col-md-10 = render partial: "projects/protected_tags/dropdown", locals: { f: f } .help-block - = link_to 'Wildcards', help_page_path('user/project/protected_branches', anchor: 'wildcard-protected-branches') + = link_to 'Wildcards', help_page_path('user/project/protected_tags', anchor: 'wildcard-protected-tags') such as - %code *-stable + %code v* or - %code production/* + %code *-release are supported .form-group %label.col-md-2.text-right{ for: 'create_access_levels_attributes' } diff --git a/doc/university/glossary/README.md b/doc/university/glossary/README.md index ec565c3e7bf..0b17e4ff7c1 100644 --- a/doc/university/glossary/README.md +++ b/doc/university/glossary/README.md @@ -411,6 +411,10 @@ An [object-relational](https://en.wikipedia.org/wiki/PostgreSQL) database. Toute A [feature](https://docs.gitlab.com/ce/user/project/protected_branches.html) that protects branches from unauthorized pushes, force pushing or deletion. +### Protected Tags + +A [feature](https://docs.gitlab.com/ce/user/project/protected_tags.html) that protects tags from unauthorized creation, update or deletion + ### Pull Git command to [synchronize](https://git-scm.com/docs/git-pull) the local repository with the remote repository, by fetching all remote changes and merging them into the local repository. diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 0ea6d01411f..3122e95fc0e 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -55,6 +55,7 @@ The following table depicts the various user permission levels in a project. | Push to protected branches | | | | ✓ | ✓ | | Enable/disable branch protection | | | | ✓ | ✓ | | Turn on/off protected branch push for devs| | | | ✓ | ✓ | +| Enable/disable tag protections | | | | ✓ | ✓ | | Rewrite/remove Git tags | | | | ✓ | ✓ | | Edit project | | | | ✓ | ✓ | | Add deploy keys to project | | | | ✓ | ✓ | diff --git a/doc/user/project/img/project_repository_settings.png b/doc/user/project/img/project_repository_settings.png new file mode 100644 index 00000000000..1aa7efc36f1 Binary files /dev/null and b/doc/user/project/img/project_repository_settings.png differ diff --git a/doc/user/project/img/protected_tag_matches.png b/doc/user/project/img/protected_tag_matches.png new file mode 100644 index 00000000000..a36a11a1271 Binary files /dev/null and b/doc/user/project/img/protected_tag_matches.png differ diff --git a/doc/user/project/img/protected_tags_list.png b/doc/user/project/img/protected_tags_list.png new file mode 100644 index 00000000000..c5e42dc0705 Binary files /dev/null and b/doc/user/project/img/protected_tags_list.png differ diff --git a/doc/user/project/img/protected_tags_page.png b/doc/user/project/img/protected_tags_page.png new file mode 100644 index 00000000000..3848d91ebd6 Binary files /dev/null and b/doc/user/project/img/protected_tags_page.png differ diff --git a/doc/user/project/img/protected_tags_permissions_dropdown.png b/doc/user/project/img/protected_tags_permissions_dropdown.png new file mode 100644 index 00000000000..9e0fc4e2a43 Binary files /dev/null and b/doc/user/project/img/protected_tags_permissions_dropdown.png differ diff --git a/doc/user/project/protected_tags.md b/doc/user/project/protected_tags.md new file mode 100644 index 00000000000..0cb7aefdb2f --- /dev/null +++ b/doc/user/project/protected_tags.md @@ -0,0 +1,60 @@ +# Protected Tags + +> [Introduced][ce-10356] in GitLab 9.1. + +Protected Tags allow control over who has permission to create tags as well as preventing accidental update or deletion once created. Each rule allows you to match either an individual tag name, or use wildcards to control multiple tags at once. + +This feature evolved out of [Protected Branches](protected_branches.md) + +## Overview + +Protected tags will prevent anyone from updating or deleting the tag, as and will prevent creation of matching tags based on the permissions you have selected. By default, anyone without Master permission will be prevented from creating tags. + + +## Configuring protected tags + +To protect a tag, you need to have at least Master permission level. + +1. Navigate to the project's Settings -> Repository page + + ![Repository Settings](img/project_repository_settings.png) + +1. From the **Tag** dropdown menu, select the tag you want to protect or type and click `Create wildcard`. In the screenshot below, we chose to protect all tags matching `v*`. + + ![Protected tags page](img/protected_tags_page.png) + +1. From the `Allowed to create` dropdown, select who will have permission to create matching tags and then click `Protect`. + + ![Allowed to create tags dropdown](img/protected_tags_permissions_dropdown.png) + +1. Once done, the protected tag will appear in the "Protected tags" list. + + ![Protected tags list](img/protected_tags_list.png) + +## Wildcard protected tags + +You can specify a wildcard protected tag, which will protect all tags +matching the wildcard. For example: + +| Wildcard Protected Tag | Matching Tags | +|------------------------+-------------------------------| +| `v*` | `v1.0.0`, `version-9.1` | +| `*-deploy` | `march-deploy`, `1.0-deploy` | +| `*gitlab*` | `gitlab`, `gitlab/v1` | +| `*` | `v1.0.1rc2`, `accidental-tag` | + + +Two different wildcards can potentially match the same tag. For example, +`*-stable` and `production-*` would both match a `production-stable` tag. +In that case, if _any_ of these protected tags have a setting like +"Allowed to create", then `production-stable` will also inherit this setting. + +If you click on a protected tag's name, you will be presented with a list of +all matching tags: + +![Protected tag matches](img/protected_tag_matches.png) + + +--- + +[ce-10356]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10356 "Protected Tags" diff --git a/doc/workflow/README.md b/doc/workflow/README.md index 6a8de51a199..a1852650cfb 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -20,6 +20,7 @@ - [Project forking workflow](forking_workflow.md) - [Project users](add-user/add-user.md) - [Protected branches](../user/project/protected_branches.md) +- [Protected tags](../user/project/protected_tags.md) - [Slash commands](../user/project/slash_commands.md) - [Sharing a project with a group](share_with_group.md) - [Share projects with other groups](share_projects_with_other_groups.md) -- cgit v1.2.1 From 4bf6ffa51a8e1ed67af2c31bde6b472c67b4a963 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 7 Apr 2017 00:07:39 -0500 Subject: Remove `undefined` class from `.emoji-menu-list` Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/30496 --- app/assets/javascripts/awards_handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/awards_handler.js b/app/assets/javascripts/awards_handler.js index 67106e85a37..ce426741637 100644 --- a/app/assets/javascripts/awards_handler.js +++ b/app/assets/javascripts/awards_handler.js @@ -51,7 +51,7 @@ function renderCategory(name, emojiList, opts = {}) {
${name}
-
    +
      ${emojiList.map(emojiName => `