summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2016-07-29 10:39:45 +0000
committerRémy Coutable <remy@rymai.me>2016-07-29 10:39:45 +0000
commit242f8377264973d642b46e5d2800ef3d3bd4c0fa (patch)
tree08276afb13fe04b41b6d7a20df0cd11962fa9a6c
parent9b0e131b83cfc44d3132bddfefb6cbd4bff7d253 (diff)
parentcebcc417eda08711ad17a433d6d9b4f49830c04c (diff)
downloadgitlab-ce-242f8377264973d642b46e5d2800ef3d3bd4c0fa.tar.gz
Merge branch '18193-no-one-can-push' into 'master'
Allow creating protected branches that can't be pushed to ## What does this MR do? - Add "No one can push" as a setting to protected branches. - This applies to Masters (as well as all other users) ## What are the relevant issue numbers? Closes #18193 ## Does this need an EE merge request? Yes. gitlab-org/gitlab-ee!569 ## Screenshots ![Screen_Shot_2016-07-29_at_3.14.59_PM](/uploads/2e8774c311763bc6e570501a2e6cabf7/Screen_Shot_2016-07-29_at_3.14.59_PM.png) ## TODO - [ ] #18193 !5081 No one can push to protected branches - [x] Implementation - [x] Model changes - [x] Remove "developers_can_merge" and "developers_can_push" - [x] Replace with `ProtectedBranchPushAccess` and `ProtectedBranchMergeAccess` - [x] Reversible migration - [x] Raise error on failure - [x] MySQL - [x] Backend changes - [x] Creating a protected branch creates access rows - [x] Add `no_one` as an access level - [x] Enforce "no one can push" - [x] Allow setting levels while creating protected branches? - [x] Frontend - [x] Replace checkboxes with `select`s - [x] Add tests - [x] `GitPushService` -> new projects' default branch protection - [x] Fix existing tests - [x] Refactor - [x] Test workflows by hand - [x] from the Web UI - [x] When "Allowed to Push" is "No one" - [x] Developers can't push - [x] Masters can't push - [x] When "Allowed to Push" is "Developers + Masters" - [x] Developers can push - [x] Masters can push - [x] When "Allowed to Push" is "Masters" - [x] Developers can't push - [x] Masters can push - [x] When "Allowed to Merge" is "Masters" and "Allowed to Push" is "Masters" - [x] Developers can't push - [x] Developers can't merge - [x] Masters can merge - [x] Masters can push - [x] When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "Masters" - [x] Developers can't push - [x] Developers can merge - [x] Masters can merge - [x] Masters can push - [x] When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "No one" - [x] Developers can't push - [x] Developers can merge - [x] Masters can merge - [x] Masters can't push - [x] When "Allowed to Merge" is "Masters" and "Allowed to Push" is "No one" - [x] Developers can't push - [x] Developers can't merge - [x] Masters can merge - [x] Masters can't push - [x] from CLI - [x] When "Allowed to Push" is "No one" - [x] Developers can't push - [x] Masters can't push - [x] When "Allowed to Push" is "Developers + Masters" - [x] Developers can push - [x] Masters can push - [x] When "Allowed to Push" is "Masters" - [x] Developers can't push - [x] Masters can push - [x] When "Allowed to Merge" is "Masters" and "Allowed to Push" is "Masters" - [x] Developers can't push - [x] Developers can't merge - [x] Masters can merge - [x] Masters can push - [x] When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "Masters" - [x] Developers can't push - [x] Developers can't merge - [x] Masters can merge - [x] Masters can push - [x] When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "No one" - [x] Developers can't push - [x] Developers can't merge - [x] Masters can't merge - [x] Masters can't push - [x] When "Allowed to Merge" is "Masters" and "Allowed to Push" is "No one" - [x] Developers can't push - [x] Developers can't merge - [x] Masters can't merge - [x] Masters can't push - [x] Add tests for owners and admins - [x] CHANGELOG - [x] Screenshots - [x] Documentation - [x] Wait for ~~!4665~~ to be merged in - [x] Wait for ~~gitlab-org/gitlab-ce#19872~~ and ~~gitlab-org/gitlab-ee!564~~ to be closed - [x] Rebase against master instead of !4892 - [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/a4ca206fd1cc0332d1e385ddbc0f2e4065c3ae73/builds) is green - [x] Create EE MR - [x] Cherry pick commits - [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ee/commit/4e17190d7dc546c1f977edcafd1cbcea4bdb4043/builds) is green - [x] Address @axil's comments - [x] Assign to endboss - [x] Wait for @dbalexandre's review - [x] Address @dbalexandre's comments - [x] Address @axil's comments - [x] Align dropdowns - [x] No flash when protected branch is updated - [x] Resolve conflicts - [x] Implement protect/unprotect API - [x] Address @dbalexandre's comments - [x] Update EE MR - [x] Address @rymai's comments - [x] Create/Update service should return a `ProtectedBranch` - [x] Successfuly protected branch creation shouldn't `load_protected_branches` - [x] Rename `allowed_to_merge` as #minimum_access_level_for_merge - [x] Rename `allowed_to_push` as #minimum_access_level_for_push - [x] Use `inclusion` and `Gitlab::Access` instead of an `enum` - [x] Modify `check_access` to work with `Gitlab::Access` - [x] Pass `@protected_branch` to `#execute` in `UpdateService` - [x] simplify with a nested field `protected_branch[push_access_level][access_level]` - [x] `developers_can_{merge,push}` should be handled in the API - [x] Use `can?(current_user, ...)` instead of `current_user.can?(...)` - [x] Instantiate `ProtectedBranchesAccessSelect` in `dispatcher.js` - [x] constants regarding downtime migrations - [x] Explicit `#down` for columns with default - [x] Update EE MR - [ ] Wait for CE merge - [ ] Wait for EE merge - [ ] Create issue for UI changes proposed by @zyv See merge request !5081
-rw-r--r--CHANGELOG1
-rw-r--r--app/assets/javascripts/dispatcher.js5
-rw-r--r--app/assets/javascripts/protected_branches.js35
-rw-r--r--app/assets/javascripts/protected_branches_access_select.js.es663
-rw-r--r--app/assets/stylesheets/pages/projects.scss16
-rw-r--r--app/controllers/projects/protected_branches_controller.rb33
-rw-r--r--app/models/project.rb8
-rw-r--r--app/models/protected_branch.rb6
-rw-r--r--app/models/protected_branch/merge_access_level.rb24
-rw-r--r--app/models/protected_branch/push_access_level.rb27
-rw-r--r--app/services/git_push_service.rb15
-rw-r--r--app/services/protected_branches/create_service.rb27
-rw-r--r--app/services/protected_branches/update_service.rb13
-rw-r--r--app/views/projects/protected_branches/_branches_list.html.haml34
-rw-r--r--app/views/projects/protected_branches/_protected_branch.html.haml10
-rw-r--r--app/views/projects/protected_branches/index.html.haml24
-rw-r--r--db/fixtures/development/16_protected_branches.rb12
-rw-r--r--db/migrate/20160705054938_add_protected_branches_push_access.rb17
-rw-r--r--db/migrate/20160705054952_add_protected_branches_merge_access.rb17
-rw-r--r--db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb29
-rw-r--r--db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb29
-rw-r--r--db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb19
-rw-r--r--db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb19
-rw-r--r--db/schema.rb26
-rw-r--r--features/steps/project/commits/branches.rb2
-rw-r--r--lib/api/branches.rb37
-rw-r--r--lib/api/entities.rb6
-rw-r--r--lib/gitlab/user_access.rb10
-rw-r--r--spec/factories/protected_branches.rb23
-rw-r--r--spec/features/protected_branches_spec.rb66
-rw-r--r--spec/lib/gitlab/git_access_spec.rb92
-rw-r--r--spec/lib/gitlab/user_access_spec.rb4
-rw-r--r--spec/models/project_spec.rb40
-rw-r--r--spec/requests/api/branches_spec.rb2
-rw-r--r--spec/services/git_push_service_spec.rb20
35 files changed, 627 insertions, 184 deletions
diff --git a/CHANGELOG b/CHANGELOG
index d555d23860d..2b04c15b827 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -8,6 +8,7 @@ v 8.11.0 (unreleased)
- Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
- Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
- Optimize maximum user access level lookup in loading of notes
+ - Add "No one can push" as an option for protected branches. !5081
- Limit git rev-list output count to one in forced push check
- Clean up unused routes (Josef Strzibny)
- Add green outline to New Branch button. !5447 (winniehell)
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index d212d66da1b..9e6901962c6 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/app/assets/javascripts/dispatcher.js
@@ -171,6 +171,11 @@
break;
case 'search:show':
new Search();
+ break;
+ case 'projects:protected_branches:index':
+ new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
+ new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
+ break;
}
switch (path.first()) {
case 'admin':
diff --git a/app/assets/javascripts/protected_branches.js b/app/assets/javascripts/protected_branches.js
deleted file mode 100644
index db21a19964d..00000000000
--- a/app/assets/javascripts/protected_branches.js
+++ /dev/null
@@ -1,35 +0,0 @@
-(function() {
- $(function() {
- return $(".protected-branches-list :checkbox").change(function(e) {
- var can_push, id, name, obj, url;
- name = $(this).attr("name");
- if (name === "developers_can_push" || name === "developers_can_merge") {
- id = $(this).val();
- can_push = $(this).is(":checked");
- url = $(this).data("url");
- return $.ajax({
- type: "PATCH",
- url: url,
- dataType: "json",
- data: {
- id: id,
- protected_branch: (
- obj = {},
- obj["" + name] = can_push,
- obj
- )
- },
- success: function() {
- var row;
- row = $(e.target);
- return row.closest('tr').effect('highlight');
- },
- error: function() {
- return new Flash("Failed to update branch!", "alert");
- }
- });
- }
- });
- });
-
-}).call(this);
diff --git a/app/assets/javascripts/protected_branches_access_select.js.es6 b/app/assets/javascripts/protected_branches_access_select.js.es6
new file mode 100644
index 00000000000..e98312bbf37
--- /dev/null
+++ b/app/assets/javascripts/protected_branches_access_select.js.es6
@@ -0,0 +1,63 @@
+class ProtectedBranchesAccessSelect {
+ constructor(container, saveOnSelect, selectDefault) {
+ this.container = container;
+ this.saveOnSelect = saveOnSelect;
+
+ this.container.find(".allowed-to-merge").each((i, element) => {
+ var fieldName = $(element).data('field-name');
+ var dropdown = $(element).glDropdown({
+ data: gon.merge_access_levels,
+ selectable: true,
+ fieldName: fieldName,
+ clicked: _.chain(this.onSelect).partial(element).bind(this).value()
+ });
+
+ if (selectDefault) {
+ dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
+ }
+ });
+
+
+ this.container.find(".allowed-to-push").each((i, element) => {
+ var fieldName = $(element).data('field-name');
+ var dropdown = $(element).glDropdown({
+ data: gon.push_access_levels,
+ selectable: true,
+ fieldName: fieldName,
+ clicked: _.chain(this.onSelect).partial(element).bind(this).value()
+ });
+
+ if (selectDefault) {
+ dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
+ }
+ });
+ }
+
+ onSelect(dropdown, selected, element, e) {
+ $(dropdown).find('.dropdown-toggle-text').text(selected.text);
+ if (this.saveOnSelect) {
+ return $.ajax({
+ type: "POST",
+ url: $(dropdown).data('url'),
+ dataType: "json",
+ data: {
+ _method: 'PATCH',
+ id: $(dropdown).data('id'),
+ protected_branch: {
+ ["" + ($(dropdown).data('type')) + "_attributes"]: {
+ "access_level": selected.id
+ }
+ }
+ },
+ success: function() {
+ var row;
+ row = $(e.target);
+ return row.closest('tr').effect('highlight');
+ },
+ error: function() {
+ return new Flash("Failed to update branch!", "alert");
+ }
+ });
+ }
+ }
+}
diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss
index cc3aef5199e..4409477916f 100644
--- a/app/assets/stylesheets/pages/projects.scss
+++ b/app/assets/stylesheets/pages/projects.scss
@@ -661,14 +661,28 @@ pre.light-well {
}
}
+.new_protected_branch {
+ .dropdown {
+ display: inline;
+ margin-left: 15px;
+ }
+
+ label {
+ min-width: 120px;
+ }
+}
+
.protected-branches-list {
a {
color: $gl-gray;
- font-weight: 600;
&:hover {
color: $gl-link-color;
}
+
+ &.is-active {
+ font-weight: 600;
+ }
}
}
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index 10dca47fded..d28ec6e2eac 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -3,19 +3,24 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
before_action :require_non_empty_project
before_action :authorize_admin_project!
before_action :load_protected_branch, only: [:show, :update, :destroy]
+ before_action :load_protected_branches, only: [:index]
layout "project_settings"
def index
- @protected_branches = @project.protected_branches.order(:name).page(params[:page])
@protected_branch = @project.protected_branches.new
- gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } })
+ load_protected_branches_gon_variables
end
def create
- @project.protected_branches.create(protected_branch_params)
- redirect_to namespace_project_protected_branches_path(@project.namespace,
- @project)
+ @protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute
+ if @protected_branch.persisted?
+ redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
+ else
+ load_protected_branches
+ load_protected_branches_gon_variables
+ render :index
+ end
end
def show
@@ -23,7 +28,9 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
end
def update
- if @protected_branch && @protected_branch.update_attributes(protected_branch_params)
+ @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
@@ -50,6 +57,18 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
end
def protected_branch_params
- params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge)
+ params.require(:protected_branch).permit(:name,
+ merge_access_level_attributes: [:access_level],
+ push_access_level_attributes: [:access_level])
+ end
+
+ def load_protected_branches
+ @protected_branches = @project.protected_branches.order(:name).page(params[:page])
+ end
+
+ def load_protected_branches_gon_variables
+ gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
+ push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
+ merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
end
end
diff --git a/app/models/project.rb b/app/models/project.rb
index dc44a757b4b..7aecd7860c5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -874,14 +874,6 @@ class Project < ActiveRecord::Base
ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present?
end
- def developers_can_push_to_protected_branch?(branch_name)
- protected_branches.matching(branch_name).any?(&:developers_can_push)
- end
-
- def developers_can_merge_to_protected_branch?(branch_name)
- protected_branches.matching(branch_name).any?(&:developers_can_merge)
- end
-
def forked?
!(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
end
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index b7011d7afdf..226b3f54342 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -5,6 +5,12 @@ class ProtectedBranch < ActiveRecord::Base
validates :name, presence: true
validates :project, presence: true
+ has_one :merge_access_level, dependent: :destroy
+ has_one :push_access_level, dependent: :destroy
+
+ accepts_nested_attributes_for :push_access_level
+ accepts_nested_attributes_for :merge_access_level
+
def commit
project.commit(self.name)
end
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
new file mode 100644
index 00000000000..b1112ee737d
--- /dev/null
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -0,0 +1,24 @@
+class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
+ belongs_to :protected_branch
+ delegate :project, to: :protected_branch
+
+ validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
+ Gitlab::Access::DEVELOPER] }
+
+ def self.human_access_levels
+ {
+ Gitlab::Access::MASTER => "Masters",
+ Gitlab::Access::DEVELOPER => "Developers + Masters"
+ }.with_indifferent_access
+ end
+
+ def check_access(user)
+ return true if user.is_admin?
+
+ project.team.max_member_access(user.id) >= access_level
+ end
+
+ def humanize
+ self.class.human_access_levels[self.access_level]
+ end
+end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
new file mode 100644
index 00000000000..6a5e49cf453
--- /dev/null
+++ b/app/models/protected_branch/push_access_level.rb
@@ -0,0 +1,27 @@
+class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
+ belongs_to :protected_branch
+ delegate :project, to: :protected_branch
+
+ 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
+ return true if user.is_admin?
+
+ project.team.max_member_access(user.id) >= access_level
+ end
+
+ def humanize
+ self.class.human_access_levels[self.access_level]
+ end
+end
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index e02b50ff9a2..3f6a177bf3a 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -88,9 +88,18 @@ class GitPushService < BaseService
# Set protection on the default branch if configured
if current_application_settings.default_branch_protection != PROTECTION_NONE
- developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false
- developers_can_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? true : false
- @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge })
+
+ params = {
+ name: @project.default_branch,
+ push_access_level_attributes: {
+ access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+ },
+ merge_access_level_attributes: {
+ access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+ }
+ }
+
+ ProtectedBranches::CreateService.new(@project, current_user, params).execute
end
end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
new file mode 100644
index 00000000000..6150a2a83c9
--- /dev/null
+++ b/app/services/protected_branches/create_service.rb
@@ -0,0 +1,27 @@
+module ProtectedBranches
+ class CreateService < BaseService
+ attr_reader :protected_branch
+
+ def execute
+ raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
+
+ protected_branch = project.protected_branches.new(params)
+
+ ProtectedBranch.transaction do
+ protected_branch.save!
+
+ if protected_branch.push_access_level.blank?
+ protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+ end
+
+ if protected_branch.merge_access_level.blank?
+ protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
+ end
+ end
+
+ protected_branch
+ rescue ActiveRecord::RecordInvalid
+ protected_branch
+ end
+ end
+end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
new file mode 100644
index 00000000000..89d8ba60134
--- /dev/null
+++ b/app/services/protected_branches/update_service.rb
@@ -0,0 +1,13 @@
+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
+ end
+ end
+end
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 720d67dff7c..0603a014008 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -5,24 +5,22 @@
No branches are protected, protect a branch with the form above.
- else
- can_admin_project = can?(current_user, :admin_project, @project)
- .table-responsive
- %table.table.protected-branches-list
- %colgroup
- %col{ width: "20%" }
- %col{ width: "30%" }
- %col{ width: "25%" }
- %col{ width: "25%" }
+
+ %table.table.protected-branches-list
+ %colgroup
+ %col{ width: "20%" }
+ %col{ width: "30%" }
+ %col{ width: "25%" }
+ %col{ width: "25%" }
+ %thead
+ %tr
+ %th Branch
+ %th Last commit
+ %th Allowed to merge
+ %th Allowed to push
- if can_admin_project
- %col
- %thead
- %tr
- %th Protected Branch
- %th Commit
- %th Developers Can Push
- %th Developers Can Merge
- - if can_admin_project
- %th
- %tbody
- = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
+ %th
+ %tbody
+ = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
= paginate @protected_branches, theme: 'gitlab'
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 7fda7f96047..498e412235e 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -15,9 +15,15 @@
- else
(branch was removed from repository)
%td
- = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url })
+ = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
+ = dropdown_tag(protected_branch.merge_access_level.humanize,
+ options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
+ data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "merge_access_level" }})
%td
- = check_box_tag("developers_can_merge", protected_branch.id, protected_branch.developers_can_merge, data: { url: url })
+ = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
+ = dropdown_tag(protected_branch.push_access_level.humanize,
+ options: { title: "Allowed to push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
+ data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "push_access_level" }})
- if can_admin_project
%td
= link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index 950df740bbc..4efe44c7233 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -32,18 +32,22 @@
are supported.
.form-group
- = f.check_box :developers_can_push, class: "pull-left"
- .prepend-left-20
- = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0"
- %p.light.append-bottom-0
- Allow developers to push to this branch
+ = hidden_field_tag 'protected_branch[merge_access_level_attributes][access_level]'
+ = label_tag "Allowed to merge: ", nil, class: "label-light append-bottom-0"
+ = dropdown_tag("<Make a selection>",
+ options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
+ dropdown_class: 'dropdown-menu-selectable',
+ data: { field_name: "protected_branch[merge_access_level_attributes][access_level]" }})
.form-group
- = f.check_box :developers_can_merge, class: "pull-left"
- .prepend-left-20
- = f.label :developers_can_merge, "Developers can merge", class: "label-light append-bottom-0"
- %p.light.append-bottom-0
- Allow developers to accept merge requests to this branch
+ = hidden_field_tag 'protected_branch[push_access_level_attributes][access_level]'
+ = label_tag "Allowed to push: ", nil, class: "label-light append-bottom-0"
+ = dropdown_tag("<Make a selection>",
+ options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
+ dropdown_class: 'dropdown-menu-selectable',
+ data: { field_name: "protected_branch[push_access_level_attributes][access_level]" }})
+
+
= f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
%hr
diff --git a/db/fixtures/development/16_protected_branches.rb b/db/fixtures/development/16_protected_branches.rb
new file mode 100644
index 00000000000..103c7f9445c
--- /dev/null
+++ b/db/fixtures/development/16_protected_branches.rb
@@ -0,0 +1,12 @@
+Gitlab::Seeder.quiet do
+ admin_user = User.find(1)
+
+ Project.all.each do |project|
+ params = {
+ name: 'master'
+ }
+
+ ProtectedBranches::CreateService.new(project, admin_user, params).execute
+ print '.'
+ end
+end
diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb
new file mode 100644
index 00000000000..f27295524e1
--- /dev/null
+++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb
@@ -0,0 +1,17 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddProtectedBranchesPushAccess < ActiveRecord::Migration
+ DOWNTIME = false
+
+ def change
+ create_table :protected_branch_push_access_levels do |t|
+ t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
+
+ # Gitlab::Access::MASTER == 40
+ t.integer :access_level, default: 40, null: false
+
+ t.timestamps null: false
+ end
+ end
+end
diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
new file mode 100644
index 00000000000..32adfa266cd
--- /dev/null
+++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
@@ -0,0 +1,17 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
+ DOWNTIME = false
+
+ def change
+ create_table :protected_branch_merge_access_levels do |t|
+ t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
+
+ # Gitlab::Access::MASTER == 40
+ t.integer :access_level, default: 40, null: false
+
+ t.timestamps null: false
+ end
+ end
+end
diff --git a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
new file mode 100644
index 00000000000..fa93936ced7
--- /dev/null
+++ b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
@@ -0,0 +1,29 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration
+ DOWNTIME = true
+ DOWNTIME_REASON = <<-HEREDOC
+ We're creating a `merge_access_level` for each `protected_branch`. If a user creates a `protected_branch` while this
+ is running, we might be left with a `protected_branch` _without_ an associated `merge_access_level`. The `protected_branches`
+ table must not change while this is running, so downtime is required.
+
+ https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081#note_13247410
+ HEREDOC
+
+ def up
+ execute <<-HEREDOC
+ INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at)
+ SELECT id, (CASE WHEN developers_can_merge THEN 1 ELSE 0 END), now(), now()
+ FROM protected_branches
+ HEREDOC
+ end
+
+ def down
+ execute <<-HEREDOC
+ UPDATE protected_branches SET developers_can_merge = TRUE
+ WHERE id IN (SELECT protected_branch_id FROM protected_branch_merge_access_levels
+ WHERE access_level = 1);
+ HEREDOC
+ end
+end
diff --git a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
new file mode 100644
index 00000000000..56f6159d1d8
--- /dev/null
+++ b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
@@ -0,0 +1,29 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration
+ DOWNTIME = true
+ DOWNTIME_REASON = <<-HEREDOC
+ We're creating a `push_access_level` for each `protected_branch`. If a user creates a `protected_branch` while this
+ is running, we might be left with a `protected_branch` _without_ an associated `push_access_level`. The `protected_branches`
+ table must not change while this is running, so downtime is required.
+
+ https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081#note_13247410
+ HEREDOC
+
+ def up
+ execute <<-HEREDOC
+ INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at)
+ SELECT id, (CASE WHEN developers_can_push THEN 1 ELSE 0 END), now(), now()
+ FROM protected_branches
+ HEREDOC
+ end
+
+ def down
+ execute <<-HEREDOC
+ UPDATE protected_branches SET developers_can_push = TRUE
+ WHERE id IN (SELECT protected_branch_id FROM protected_branch_push_access_levels
+ WHERE access_level = 1);
+ HEREDOC
+ end
+end
diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
new file mode 100644
index 00000000000..f563f660ddf
--- /dev/null
+++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
@@ -0,0 +1,19 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ # This is only required for `#down`
+ disable_ddl_transaction!
+
+ DOWNTIME = false
+
+ def up
+ remove_column :protected_branches, :developers_can_push, :boolean
+ end
+
+ def down
+ add_column_with_default(:protected_branches, :developers_can_push, :boolean, default: false, null: false)
+ end
+end
diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
new file mode 100644
index 00000000000..aa71e06d36e
--- /dev/null
+++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
@@ -0,0 +1,19 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ # This is only required for `#down`
+ disable_ddl_transaction!
+
+ DOWNTIME = false
+
+ def up
+ remove_column :protected_branches, :developers_can_merge, :boolean
+ end
+
+ def down
+ add_column_with_default(:protected_branches, :developers_can_merge, :boolean, default: false, null: false)
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 15cee55a7bf..2d2ae5fd840 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -867,13 +867,29 @@ ActiveRecord::Schema.define(version: 20160722221922) do
add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree
add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
+ create_table "protected_branch_merge_access_levels", force: :cascade do |t|
+ t.integer "protected_branch_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_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
+
+ create_table "protected_branch_push_access_levels", force: :cascade do |t|
+ t.integer "protected_branch_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_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
+
create_table "protected_branches", force: :cascade do |t|
- t.integer "project_id", null: false
- t.string "name", null: false
+ t.integer "project_id", null: false
+ t.string "name", null: false
t.datetime "created_at"
t.datetime "updated_at"
- t.boolean "developers_can_push", default: false, null: false
- t.boolean "developers_can_merge", default: false, null: false
end
add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree
@@ -1136,5 +1152,7 @@ ActiveRecord::Schema.define(version: 20160722221922) do
add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
add_foreign_key "personal_access_tokens", "users"
+ add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
+ add_foreign_key "protected_branch_push_access_levels", "protected_branches"
add_foreign_key "u2f_registrations", "users"
end
diff --git a/features/steps/project/commits/branches.rb b/features/steps/project/commits/branches.rb
index 0a42931147d..4bfb7e92e99 100644
--- a/features/steps/project/commits/branches.rb
+++ b/features/steps/project/commits/branches.rb
@@ -25,7 +25,7 @@ class Spinach::Features::ProjectCommitsBranches < Spinach::FeatureSteps
step 'project "Shop" has protected branches' do
project = Project.find_by(name: "Shop")
- project.protected_branches.create(name: "stable")
+ create(:protected_branch, project: project, name: "stable")
end
step 'I click new branch link' do
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 66b853eb342..a77afe634f6 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -35,6 +35,10 @@ module API
# Protect a single branch
#
+ # Note: The internal data model moved from `developers_can_{merge,push}` to `allowed_to_{merge,push}`
+ # in `gitlab-org/gitlab-ce!5081`. The API interface has not been changed (to maintain compatibility),
+ # but it works with the changed data model to infer `developers_can_merge` and `developers_can_push`.
+ #
# Parameters:
# id (required) - The ID of a project
# branch (required) - The name of the branch
@@ -49,17 +53,36 @@ module API
@branch = user_project.repository.find_branch(params[:branch])
not_found!('Branch') unless @branch
protected_branch = user_project.protected_branches.find_by(name: @branch.name)
- developers_can_push = to_boolean(params[:developers_can_push])
+
developers_can_merge = to_boolean(params[:developers_can_merge])
+ developers_can_push = to_boolean(params[:developers_can_push])
+
+ protected_branch_params = {
+ name: @branch.name
+ }
+
+ unless developers_can_merge.nil?
+ protected_branch_params.merge!({
+ merge_access_level_attributes: {
+ access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+ }
+ })
+ end
+
+ unless developers_can_push.nil?
+ protected_branch_params.merge!({
+ push_access_level_attributes: {
+ access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+ }
+ })
+ end
if protected_branch
- protected_branch.developers_can_push = developers_can_push unless developers_can_push.nil?
- protected_branch.developers_can_merge = developers_can_merge unless developers_can_merge.nil?
- protected_branch.save
+ service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
+ service.execute(protected_branch)
else
- user_project.protected_branches.create(name: @branch.name,
- developers_can_push: developers_can_push || false,
- developers_can_merge: developers_can_merge || false)
+ service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
+ service.execute
end
present @branch, with: Entities::RepoBranch, project: user_project
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e76e7304674..4eb95d8a215 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -126,11 +126,13 @@ module API
end
expose :developers_can_push do |repo_branch, options|
- options[:project].developers_can_push_to_protected_branch? repo_branch.name
+ project = options[:project]
+ project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.access_level == Gitlab::Access::DEVELOPER }
end
expose :developers_can_merge do |repo_branch, options|
- options[:project].developers_can_merge_to_protected_branch? repo_branch.name
+ project = options[:project]
+ project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER }
end
end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index c0f85e9b3a8..3a69027368f 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -29,8 +29,9 @@ module Gitlab
def can_push_to_branch?(ref)
return false unless user
- if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref)
- user.can?(:push_code_to_protected_branches, project)
+ if project.protected_branch?(ref)
+ access_levels = project.protected_branches.matching(ref).map(&:push_access_level)
+ access_levels.any? { |access_level| access_level.check_access(user) }
else
user.can?(:push_code, project)
end
@@ -39,8 +40,9 @@ module Gitlab
def can_merge_to_branch?(ref)
return false unless user
- if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref)
- user.can?(:push_code_to_protected_branches, project)
+ if project.protected_branch?(ref)
+ access_levels = project.protected_branches.matching(ref).map(&:merge_access_level)
+ access_levels.any? { |access_level| access_level.check_access(user) }
else
user.can?(:push_code, project)
end
diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb
index 28ed8078157..5575852c2d7 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -2,5 +2,28 @@ FactoryGirl.define do
factory :protected_branch do
name
project
+
+ after(:create) do |protected_branch|
+ protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+ protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
+ end
+
+ trait :developers_can_push do
+ after(:create) do |protected_branch|
+ protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+ end
+ end
+
+ trait :developers_can_merge do
+ after(:create) do |protected_branch|
+ protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+ end
+ end
+
+ trait :no_one_can_push do
+ after(:create) do |protected_branch|
+ protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS)
+ end
+ end
end
end
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index d94dee0c797..57734b33a44 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -1,6 +1,8 @@
require 'spec_helper'
feature 'Projected Branches', feature: true, js: true do
+ include WaitForAjax
+
let(:user) { create(:user, :admin) }
let(:project) { create(:project) }
@@ -81,4 +83,68 @@ feature 'Projected Branches', feature: true, js: true do
end
end
end
+
+ describe "access control" 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
+ find(".allowed-to-push").click
+ within(".dropdown.open .dropdown-menu") { click_on access_type_name }
+ end
+ click_on "Protect"
+
+ expect(ProtectedBranch.count).to eq(1)
+ expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
+ end
+
+ 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)
+
+ within(".protected-branches-list") do
+ find(".allowed-to-push").click
+ within('.dropdown-menu.push') { click_on access_type_name }
+ end
+
+ wait_for_ajax
+ expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
+ end
+ end
+
+ 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
+ find(".allowed-to-merge").click
+ within(".dropdown.open .dropdown-menu") { click_on access_type_name }
+ end
+ click_on "Protect"
+
+ expect(ProtectedBranch.count).to eq(1)
+ expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
+ end
+
+ 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)
+
+ within(".protected-branches-list") do
+ find(".allowed-to-merge").click
+ within('.dropdown-menu.merge') { click_on access_type_name }
+ end
+
+ wait_for_ajax
+ expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
+ end
+ end
+ end
end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index ae064a878b0..8447305a316 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -151,7 +151,13 @@ describe Gitlab::GitAccess, lib: true do
def self.run_permission_checks(permissions_matrix)
permissions_matrix.keys.each do |role|
describe "#{role} access" do
- before { project.team << [user, role] }
+ before do
+ if role == :admin
+ user.update_attribute(:admin, true)
+ else
+ project.team << [user, role]
+ end
+ end
permissions_matrix[role].each do |action, allowed|
context action do
@@ -165,6 +171,17 @@ describe Gitlab::GitAccess, lib: true do
end
permissions_matrix = {
+ admin: {
+ push_new_branch: true,
+ push_master: true,
+ push_protected_branch: true,
+ push_remove_protected_branch: false,
+ push_tag: true,
+ push_new_tag: true,
+ push_all: true,
+ merge_into_protected_branch: true
+ },
+
master: {
push_new_branch: true,
push_master: true,
@@ -217,19 +234,20 @@ describe Gitlab::GitAccess, lib: true do
run_permission_checks(permissions_matrix)
end
- context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do
- before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) }
+ context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
- context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do
- before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) }
+ context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
context "when a merge request exists for the given source/target branch" do
context "when the merge request is in progress" do
before do
- create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
+ create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature',
+ state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
@@ -242,51 +260,59 @@ describe Gitlab::GitAccess, lib: true do
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
- end
- context "when a merge request does not exist for the given source/target branch" do
- run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+ context "when a merge request does not exist for the given source/target branch" do
+ run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+ end
end
end
- context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do
- before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) }
+ context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
+
+ context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
+ before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
+
+ run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+ master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+ admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
+ end
end
+ end
- describe 'deploy key permissions' do
- let(:key) { create(:deploy_key) }
- let(:actor) { key }
+ describe 'deploy key permissions' do
+ let(:key) { create(:deploy_key) }
+ let(:actor) { key }
- context 'push code' do
- subject { access.check('git-receive-pack') }
+ context 'push code' do
+ subject { access.check('git-receive-pack') }
- context 'when project is authorized' do
- before { key.projects << project }
+ context 'when project is authorized' do
+ before { key.projects << project }
- it { expect(subject).not_to be_allowed }
- end
+ it { expect(subject).not_to be_allowed }
+ end
- context 'when unauthorized' do
- context 'to public project' do
- let(:project) { create(:project, :public) }
+ context 'when unauthorized' do
+ context 'to public project' do
+ let(:project) { create(:project, :public) }
- it { expect(subject).not_to be_allowed }
- end
+ it { expect(subject).not_to be_allowed }
+ end
- context 'to internal project' do
- let(:project) { create(:project, :internal) }
+ context 'to internal project' do
+ let(:project) { create(:project, :internal) }
- it { expect(subject).not_to be_allowed }
- end
+ it { expect(subject).not_to be_allowed }
+ end
- context 'to private project' do
- let(:project) { create(:project, :internal) }
+ context 'to private project' do
+ let(:project) { create(:project, :internal) }
- it { expect(subject).not_to be_allowed }
- end
+ it { expect(subject).not_to be_allowed }
end
end
end
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
index aa9ec243498..5bb095366fa 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/spec/lib/gitlab/user_access_spec.rb
@@ -44,7 +44,7 @@ describe Gitlab::UserAccess, lib: true do
describe 'push to protected branch if allowed for developers' do
before do
- @branch = create :protected_branch, project: project, developers_can_push: true
+ @branch = create :protected_branch, :developers_can_push, project: project
end
it 'returns true if user is a master' do
@@ -65,7 +65,7 @@ describe Gitlab::UserAccess, lib: true do
describe 'merge to protected branch if allowed for developers' do
before do
- @branch = create :protected_branch, project: project, developers_can_merge: true
+ @branch = create :protected_branch, :developers_can_merge, project: project
end
it 'returns true if user is a master' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 72b8a4e25bd..e365e4e98b2 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1095,46 +1095,6 @@ describe Project, models: true do
end
end
- describe "#developers_can_push_to_protected_branch?" do
- let(:project) { create(:empty_project) }
-
- context "when the branch matches a protected branch via direct match" do
- it "returns true if 'Developers can Push' is turned on" do
- create(:protected_branch, name: "production", project: project, developers_can_push: true)
-
- expect(project.developers_can_push_to_protected_branch?('production')).to be true
- end
-
- it "returns false if 'Developers can Push' is turned off" do
- create(:protected_branch, name: "production", project: project, developers_can_push: false)
-
- expect(project.developers_can_push_to_protected_branch?('production')).to be false
- end
- end
-
- context "when the branch matches a protected branch via wilcard match" do
- it "returns true if 'Developers can Push' is turned on" do
- create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
-
- expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true
- end
-
- it "returns false if 'Developers can Push' is turned off" do
- create(:protected_branch, name: "production/*", project: project, developers_can_push: false)
-
- expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false
- end
- end
-
- context "when the branch does not match a protected branch" do
- it "returns false" do
- create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
-
- expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false
- end
- end
- end
-
describe '#container_registry_path_with_namespace' do
let(:project) { create(:empty_project, path: 'PROJECT') }
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index 719da27f919..e8fd697965f 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -112,7 +112,7 @@ describe API::API, api: true do
before do
project.repository.add_branch(user, protected_branch, 'master')
- create(:protected_branch, project: project, name: protected_branch, developers_can_push: true, developers_can_merge: true)
+ create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch)
end
it 'updates that a developer can push' do
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 47c0580e0f0..ffa998dffc3 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -7,6 +7,7 @@ describe GitPushService, services: true do
let(:project) { create :project }
before do
+ project.team << [user, :master]
@blankrev = Gitlab::Git::BLANK_SHA
@oldrev = sample_commit.parent_id
@newrev = sample_commit.id
@@ -172,7 +173,7 @@ describe GitPushService, services: true do
describe "Push Event" do
before do
service = execute_service(project, user, @oldrev, @newrev, @ref )
- @event = Event.last
+ @event = Event.find_by_action(Event::PUSHED)
@push_data = service.push_data
end
@@ -224,8 +225,10 @@ describe GitPushService, services: true do
it "when pushing a branch for the first time" do
expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master")
- expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: false })
execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+ expect(project.protected_branches).not_to be_empty
+ expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+ expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
end
it "when pushing a branch for the first time with default branch protection disabled" do
@@ -233,8 +236,8 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master")
- expect(project.protected_branches).not_to receive(:create)
execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+ expect(project.protected_branches).to be_empty
end
it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do
@@ -242,9 +245,12 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master")
- expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false })
- execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master')
+ execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+
+ expect(project.protected_branches).not_to be_empty
+ expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
+ expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
end
it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
@@ -252,8 +258,10 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master")
- expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: true })
execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+ expect(project.protected_branches).not_to be_empty
+ expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+ expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
end
it "when pushing new commits to existing branch" do