Merge branch '18193-no-one-can-push' into 'master'

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)

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':

app/assets/javascripts/protected_branches.js

-(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);

app/assets/javascripts/protected_branches_access_select.js.es6

+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");
+        }
+      });
+    }
+  }
+}

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;
+    }
   }
 }
 

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

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

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

app/models/protected_branch/merge_access_level.rb

+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

app/models/protected_branch/push_access_level.rb

+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

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
 

app/services/protected_branches/create_service.rb

+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

app/services/protected_branches/update_service.rb

+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

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'

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"

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

db/fixtures/development/16_protected_branches.rb

+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

db/migrate/20160705054938_add_protected_branches_push_access.rb

+# 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

db/migrate/20160705054952_add_protected_branches_merge_access.rb

+# 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

db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb

+# 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

db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb

+# 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

db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb

+# 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

db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb

+# 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

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

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

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

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
 

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

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

spec/features/protected_branches_spec.rb

 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

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

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

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') }
 

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

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