Merge branch '675-protected-branch-specific-groups' into 'master'

Restrict pushes / merges to a protected branch to specific groups

- Closes #675 and https://gitlab.com/gitlab-org/gitlab-ce/issues/21153
- Related to #179 

**Default State**
![Screen_Shot_2016-09-22_at_12.53.27_PM](/uploads/3777bc09fc5b0f9ddeccbf52dbf0277e/Screen_Shot_2016-09-22_at_12.53.27_PM.png)

**Filtering**
![Screen_Shot_2016-09-22_at_12.53.39_PM](/uploads/1e649e2a3720a4b0d7ff3e4fbafe5a72/Screen_Shot_2016-09-22_at_12.53.39_PM.png)

# Tasks

- [ ]  ee#675 !645 Restrict pushes/merges to specific groups
    - [x]  Implementation
        - [x]  Basic model-level implementation
        - [x]  Test / refactor
        - [x]  Rudimentary frontend (controller actions + autocomplete for the dropdown)
            - [x]  Groups that a project hasn't been shared with can't be selected
        - [x]  Look for edge cases
    - [x]  Questions
        - [x]  What are LDAP group links?
            - A GitLab group can be synced with one or more LDAP groups
            - The sync happens in an async task, an the LDAP group users are _imported_ into the GitLab group
            - `group.users` on a GitLab group returns the LDAP group users as well (after the import task has run)
            - We check `group.users` for this feature, so we shouldn't need any additional work to support LDAP groups
    - [x]  CHANGELOG
    - [x]  Removing a group should remove the access
    - [x]  `autocomplete/groups` issue
    - [x]  Fix uniqueness validation
    - [x]  Assign to @alfredo1 for UI work
    - [x]  Wait for !581 to be merged
    - [x]  Rebase against EE master instead of !581
    - [x]  Add feature specs
    - [x]  Implement frontend
    - [ ]  Assign to endboss
    - [ ]  Get backend + frontend reviewed
    - [ ]  Wait for merge


See merge request !645

CHANGELOG-EE.md

@@ -4,6 +4,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
   - Add user activity table and service to query for active users
   - Fix 500 error updating mirror URLs for projects
+  - Restrict protected branch access to specific groups !645
   - Fix validations related to mirroring settings form. !773
   - Add multiple issue boards. !782
   - Fix Git access panel for Wikis when Kerberos authentication is enabled (Borja Aparicio)

app/assets/javascripts/protected_branch_create.js.es6 → app/assets/javascripts/protected_branches/protected_branch_create.js.es6

@@ -6,6 +6,12 @@
     PUSH: 'push_access_levels',
   };
 
+  const LEVEL_TYPES = {
+    ROLE: 'role',
+    USER: 'user',
+    GROUP: 'group'
+  };
+
   gl.ProtectedBranchCreate = class {
     constructor() {
       this.$wrap = this.$form = $('#new_protected_branch');
@@ -72,14 +78,18 @@
         for (let i = 0; i < selectedItems.length; i++) {
           let current = selectedItems[i];
 
-          if (current.type === 'user') {
+          if (current.type === LEVEL_TYPES.USER) {
             levelAttributes.push({
               user_id: selectedItems[i].user_id
             });
-          } else if (current.type === 'role') {
+          } else if (current.type === LEVEL_TYPES.ROLE) {
             levelAttributes.push({
               access_level: selectedItems[i].access_level
             });
+          } else if (current.type === LEVEL_TYPES.GROUP) {
+            levelAttributes.push({
+              group_id: selectedItems[i].group_id
+            });
           }
         }
 

app/assets/javascripts/protected_branch_edit.js.es6 → app/assets/javascripts/protected_branches/protected_branch_edit.js.es6

@@ -6,6 +6,12 @@
     PUSH: 'push_access_levels',
   };
 
+  const LEVEL_TYPES = {
+    ROLE: 'role',
+    USER: 'user',
+    GROUP: 'group'
+  };
+
   gl.ProtectedBranchEdit = class {
     constructor(options) {
       this.$wraps = {};
@@ -21,6 +27,7 @@
     }
 
     buildDropdowns() {
+
       // Allowed to merge dropdown
       this['merge_access_levels_dropdown'] = new gl.ProtectedBranchAccessDropdown({
         accessLevel: ACCESS_LEVELS.MERGE,
@@ -95,25 +102,33 @@
         let currentItem = items[i];
 
         if (currentItem.user_id) {
-          // Solo haciendo esto solo para usuarios por ahora
-          // obtenemos la data más actual de los items seleccionados
+
+          // Do this only for users for now
+          // get the current data for selected items 
           let selectedItems = this[dropdownName].getSelectedItems();
           let currentSelectedItem = _.findWhere(selectedItems, { user_id: currentItem.user_id });
 
           itemToAdd = {
             id: currentItem.id,
             user_id: currentItem.user_id,
-            type: 'user',
+            type: LEVEL_TYPES.USER,
             persisted: true,
             name: currentSelectedItem.name,
             username: currentSelectedItem.username,
             avatar_url: currentSelectedItem.avatar_url
           }
+        } else if (currentItem.group_id) {
+          itemToAdd = {
+            id: currentItem.id,
+            group_id: currentItem.group_id,
+            type: LEVEL_TYPES.GROUP,
+            persisted: true
+          };
         } else {
           itemToAdd = {
             id: currentItem.id,
             access_level: currentItem.access_level,
-            type: 'role',
+            type: LEVEL_TYPES.ROLE,
             persisted: true
           }
         }

app/assets/javascripts/protected_branches/protected_branches_bundle.js

+/*= require_tree . */

app/assets/javascripts/protected_branches_access_select_ee.js.es6

-// Modified version of `UsersSelect` for use with access selection for protected branches.
-//
-// - Selections are sent via AJAX if `saveOnSelect` is `true`
-// - If `saveOnSelect` is `false`, the dropdown element must have a `field-name` data
-//   attribute. The DOM must contain two fields - "#{field-name}[access_level]" and "#{field_name}[user_id]"
-//   where the selections will be stored.
-
-class ProtectedBranchesAccessSelect {
-  constructor(container, saveOnSelect, selectDefault) {
-    this.container = container;
-    this.saveOnSelect = saveOnSelect;
-    this.selectDefault = selectDefault;
-    this.usersPath = "/autocomplete/users.json";
-    this.setupDropdown(".allowed-to-merge", gon.merge_access_levels, gon.selected_merge_access_levels);
-    this.setupDropdown(".allowed-to-push", gon.push_access_levels, gon.selected_push_access_levels);
-  }
-
-  setupDropdown(className, accessLevels, selectedAccessLevels) {
-    this.container.find(className).each((i, element) => {
-      var dropdown = $(element).glDropdown({
-        clicked: _.chain(this.onSelect).partial(element).bind(this).value(),
-        data: (term, callback) => {
-          this.getUsers(term, (users) => {
-            users = _(users).map((user) => _(user).extend({ type: "user" }));
-            accessLevels = _(accessLevels).map((accessLevel) => _(accessLevel).extend({ type: "role" }));
-            var accessLevelsWithUsers = accessLevels.concat("divider", users);
-            callback(_(accessLevelsWithUsers).reject((item) => _.contains(selectedAccessLevels, item.id)));
-          });
-        },
-        filterable: true,
-        filterRemote: true,
-        search: { fields: ['name', 'username'] },
-        selectable: true,
-        toggleLabel: (selected) => $(element).data('default-label'),
-        renderRow: (user) => {
-          if (user.before_divider != null) {
-            return "<li> <a href='#'>" + user.text + " </a> </li>";
-          }
-
-
-          var username = user.username ? "@" + user.username : null;
-          var avatar = user.avatar_url ? user.avatar_url : false;
-          var img = avatar ? "<img src='" + avatar + "' class='avatar avatar-inline' width='30' />" : '';
-
-          var listWithName = "<li> <a href='#' class='dropdown-menu-user-link'> " + img + " <strong class='dropdown-menu-user-full-name'> " + user.name + " </strong>";
-          var listWithUserName = username ? "<span class='dropdown-menu-user-username'> " + username + " </span>" : '';
-          var listClosingTags = "</a> </li>";
-
-          return listWithName + listWithUserName + listClosingTags;
-        }
-      });
-
-      if (this.selectDefault) {
-        $(dropdown).find('.dropdown-toggle-text').text(accessLevels[0].text);
-      }
-    });
-  }
-
-  onSelect(dropdown, selected, element, e) {
-    $(dropdown).find('.dropdown-toggle-text').text(selected.text || selected.name);
-
-    var access_level = selected.type == 'user' ? 40 : selected.id;
-    var user_id = selected.type == 'user' ? selected.id : null;
-
-    if (this.saveOnSelect) {
-      $.ajax({
-        type: "POST",
-        url: $(dropdown).data('url'),
-        dataType: "json",
-        data: {
-          _method: 'PATCH',
-          id: $(dropdown).data('id'),
-          protected_branch: {
-            ["" + ($(dropdown).data('type')) + "_attributes"]: [{
-              access_level: access_level,
-              user_id: user_id
-            }]
-          }
-        },
-        success: function() {
-          var row;
-          row = $(e.target);
-          row.closest('tr').effect('highlight');
-          row.closest('td').find('.access-levels-list').append("<li>" + selected.name + "</li>");
-          location.reload();
-        },
-        error: function() {
-          new Flash("Failed to update branch!", "alert");
-        }
-      });
-    } else {
-      var fieldName = $(dropdown).data('field-name');
-      $("input[name='" + fieldName + "[access_level]']").val(access_level);
-      $("input[name='" + fieldName + "[user_id]']").val(user_id);
-    }
-  }
-
-  getUsers(query, callback) {
-    var url = this.buildUrl(this.usersPath);
-    return $.ajax({
-      url: url,
-      data: {
-        search: query,
-        per_page: 20,
-        active: true,
-        project_id: gon.current_project_id,
-        push_code: true
-      },
-      dataType: "json"
-    }).done(function(users) {
-      callback(users);
-    });
-  }
-
-  buildUrl(url) {
-    if (gon.relative_url_root != null) {
-      url = gon.relative_url_root.replace(/\/$/, '') + url;
-    }
-    return url;
-  }
-}

app/controllers/autocomplete_controller.rb

 class AutocompleteController < ApplicationController
   skip_before_action :authenticate_user!, only: [:users]
-  before_action :load_project, only: [:users]
+  before_action :load_project, only: [:users, :project_groups]
   before_action :find_users, only: [:users]
 
   def users
@@ -28,6 +28,10 @@ class AutocompleteController < ApplicationController
     render json: @users, only: [:name, :username, :id], methods: [:avatar_url]
   end
 
+  def project_groups
+    render json: @project.invited_groups, only: [:id, :name], methods: [:avatar_url]
+  end
+
   def user
     @user = User.find(params[:id])
     render json: @user, only: [:name, :username, :id], methods: [:avatar_url]

app/controllers/projects/protected_branches_controller.rb

@@ -59,8 +59,8 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
 
   def protected_branch_params
     params.require(:protected_branch).permit(:name,
-                                             merge_access_levels_attributes: [:access_level, :id, :user_id, :_destroy],
-                                             push_access_levels_attributes: [:access_level, :id, :user_id, :_destroy])
+                                             merge_access_levels_attributes: [:access_level, :id, :user_id, :_destroy, :group_id],
+                                             push_access_levels_attributes: [:access_level, :id, :user_id, :_destroy, :group_id])
   end
 
   def load_protected_branches

app/helpers/branches_helper.rb

@@ -41,6 +41,8 @@ module BranchesHelper
           name: level.user.name,
           avatar_url: level.user.avatar_url
         }
+      elsif level.type == :group
+        { id: level.id, type: level.type, group_id: level.group_id }
       else
         { id: level.id, type: level.type, access_level: level.access_level }
       end

app/models/concerns/protected_branch_access.rb

@@ -2,13 +2,19 @@ module ProtectedBranchAccess
   extend ActiveSupport::Concern
 
   included do
-    validates :user_id, uniqueness: { scope: :protected_branch, allow_nil: true }
-    validates :access_level, uniqueness: { scope: :protected_branch, unless: :user_id?, conditions: -> { where(user_id: nil) } }
+    validates_uniqueness_of :group_id, scope: :protected_branch, allow_nil: true
+    validates_uniqueness_of :user_id, scope: :protected_branch, allow_nil: true
+    validates_uniqueness_of :access_level,
+                            scope: :protected_branch,
+                            unless: Proc.new { |access_level| access_level.user_id? || access_level.group_id? },
+                            conditions: -> { where(user_id: nil, group_id: nil) }
   end
 
   def type
     if self.user.present?
       :user
+    elsif self.group.present?
+      :group
     else
       :role
     end
@@ -16,6 +22,7 @@ module ProtectedBranchAccess
 
   def humanize
     return self.user.name if self.user.present?
+    return self.group.name if self.group.present?
 
     self.class.human_access_levels[self.access_level]
   end

app/models/project_group_link.rb

@@ -16,6 +16,8 @@ class ProjectGroupLink < ActiveRecord::Base
   validates :group_access, inclusion: { in: Gitlab::Access.values }, presence: true
   validate :different_group
 
+  before_destroy :delete_branch_protection
+
   def self.access_options
     Gitlab::Access.options
   end
@@ -35,4 +37,11 @@ class ProjectGroupLink < ActiveRecord::Base
       errors.add(:base, "Project cannot be shared with the project it is in.")
     end
   end
+
+  def delete_branch_protection
+    if group.present? && project.present?
+      project.protected_branches.merge_access_by_group(group).destroy_all
+      project.protected_branches.push_access_by_group(group).destroy_all
+    end
+  end
 end

app/models/protected_branch.rb

@@ -22,6 +22,14 @@ class ProtectedBranch < ActiveRecord::Base
   # access to the given user.
   scope :push_access_by_user, -> (user) { PushAccessLevel.joins(:protected_branch).where(protected_branch_id: self.ids).merge(PushAccessLevel.by_user(user)) }
 
+  # Returns all merge access levels (for protected branches in scope) that grant merge
+  # access to the given group.
+  scope :merge_access_by_group, -> (group) { MergeAccessLevel.joins(:protected_branch).where(protected_branch_id: self.ids).merge(MergeAccessLevel.by_group(group)) }
+
+  # Returns all push access levels (for protected branches in scope) that grant push
+  # access to the given group.
+  scope :push_access_by_group, -> (group) { PushAccessLevel.joins(:protected_branch).where(protected_branch_id: self.ids).merge(PushAccessLevel.by_group(group)) }
+
   def commit
     project.commit(self.name)
   end

app/models/protected_branch/merge_access_level.rb

@@ -3,6 +3,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
 
   belongs_to :protected_branch
   belongs_to :user
+  belongs_to :group
 
   delegate :project, to: :protected_branch
 
@@ -10,6 +11,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
                                                              Gitlab::Access::DEVELOPER] }
 
   scope :by_user, -> (user) { where(user: user ) }
+  scope :by_group, -> (group) { where(group: group ) }
 
   def self.human_access_levels
     {
@@ -21,6 +23,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   def check_access(user)
     return true if user.is_admin?
     return user.id == self.user_id if self.user.present?
+    return group.users.exists?(user.id) if self.group.present?
 
     project.team.max_member_access(user.id) >= access_level
   end

app/models/protected_branch/push_access_level.rb

@@ -3,6 +3,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
 
   belongs_to :protected_branch
   belongs_to :user
+  belongs_to :group
 
   delegate :project, to: :protected_branch
 
@@ -11,6 +12,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
                                                              Gitlab::Access::NO_ACCESS] }
 
   scope :by_user, -> (user) { where(user: user ) }
+  scope :by_group, -> (group) { where(group: group ) }
 
   def self.human_access_levels
     {
@@ -24,6 +26,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
     return false if access_level == Gitlab::Access::NO_ACCESS
     return true if user.is_admin?
     return user.id == self.user_id if self.user.present?
+    return group.users.exists?(user.id) if self.group.present?
 
     project.team.max_member_access(user.id) >= access_level
   end

app/views/projects/protected_branches/_access_level_dropdown.html.haml

@@ -3,7 +3,7 @@
 
 %div{ class: "#{input_basic_name}-container" }
   - if access_levels.present?
-    - dropdown_label = [pluralize(level_frequencies[:role], 'role'), pluralize(level_frequencies[:user], 'user')].to_sentence
+    - dropdown_label = [pluralize(level_frequencies[:role], 'role'), pluralize(level_frequencies[:user], 'user'), pluralize(level_frequencies[:group], 'group')].to_sentence
 
   = dropdown_tag(dropdown_label, options: { toggle_class: "#{toggle_class} js-multiselect", dropdown_class: 'dropdown-menu-user dropdown-menu-selectable', filter: true,
                                  data: { default_label: default_label, preselected_items: access_levels_data(access_levels) } })

app/views/projects/protected_branches/_create_protected_branch.html.haml

@@ -36,6 +36,10 @@
                              options: { toggle_class: 'js-allowed-to-push js-multiselect wide',
                              dropdown_class: 'dropdown-menu-user dropdown-menu-selectable', filter: true,
                              data: { input_id: 'push_access_levels_attributes', default_label: 'Select' } })
+            .help-block
+              Only groups that
+              = link_to 'have this project shared', help_page_path('workflow/share_projects_with_other_groups')
+              can be added here
 
     .panel-footer
       = f.submit 'Protect', class: 'btn-create btn', disabled: true

app/views/projects/protected_branches/index.html.haml

 - page_title "Protected branches"
+- content_for :page_specific_javascripts do
+  = page_specific_javascript_tag('protected_branches/protected_branches_bundle.js')
 
 .row.prepend-top-default.append-bottom-default
   .col-lg-3

config/application.rb

@@ -90,6 +90,7 @@ module Gitlab
     config.assets.precompile << "users/users_bundle.js"
     config.assets.precompile << "network/network_bundle.js"
     config.assets.precompile << "profile/profile_bundle.js"
+    config.assets.precompile << "protected_branches/protected_branches_bundle.js"
     config.assets.precompile << "diff_notes/diff_notes_bundle.js"
     config.assets.precompile << "boards/boards_bundle.js"
     config.assets.precompile << "merge_conflicts/merge_conflicts_bundle.js"

config/routes.rb

@@ -40,6 +40,7 @@ Rails.application.routes.draw do
   get '/autocomplete/users' => 'autocomplete#users'
   get '/autocomplete/users/:id' => 'autocomplete#user'
   get '/autocomplete/projects' => 'autocomplete#projects'
+  get '/autocomplete/project_groups' => 'autocomplete#project_groups'
 
   # Emojis
   resources :emojis, only: :index

db/migrate/20160812054342_add_group_id_columns_to_protected_branch_access_levels.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddGroupIdColumnsToProtectedBranchAccessLevels < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = true
+  DOWNTIME_REASON = "This migrations adds two foreign keys, and so requires downtime."
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :protected_branch_merge_access_levels, :group_id, :integer
+    add_foreign_key :protected_branch_merge_access_levels, :namespaces, column: :group_id
+
+    add_column :protected_branch_push_access_levels, :group_id, :integer
+    add_foreign_key :protected_branch_push_access_levels, :namespaces, column: :group_id
+  end
+end

db/schema.rb

@@ -1049,6 +1049,7 @@ ActiveRecord::Schema.define(version: 20161007133303) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.integer "user_id"
+    t.integer  "group_id"
   end
 
   add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
@@ -1060,6 +1061,7 @@ ActiveRecord::Schema.define(version: 20161007133303) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.integer "user_id"
+    t.integer  "group_id"
   end
 
   add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
@@ -1405,8 +1407,10 @@ ActiveRecord::Schema.define(version: 20161007133303) do
   add_foreign_key "path_locks", "projects"
   add_foreign_key "path_locks", "users"
   add_foreign_key "personal_access_tokens", "users"
+  add_foreign_key "protected_branch_merge_access_levels", "namespaces", column: "group_id"
   add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
   add_foreign_key "protected_branch_merge_access_levels", "users"
+  add_foreign_key "protected_branch_push_access_levels", "namespaces", column: "group_id"
   add_foreign_key "protected_branch_push_access_levels", "protected_branches"
   add_foreign_key "protected_branch_push_access_levels", "users"
   add_foreign_key "remote_mirrors", "projects"

spec/controllers/autocomplete_controller_spec.rb

@@ -338,4 +338,44 @@ describe AutocompleteController do
       end
     end
   end
+
+  context "groups" do
+    let(:matching_group) { create(:group) }
+    let(:non_matching_group) { create(:group) }
+
+    context "while fetching all groups belonging to a project" do
+      before do
+        project.team << [user, :developer]
+        project.invited_groups << matching_group
+        sign_in(user)
+        get(:project_groups, project_id: project.id)
+      end
+
+      let(:body) { JSON.parse(response.body) }
+
+      it { expect(body).to be_kind_of(Array) }
+      it { expect(body.size).to eq 1 }
+      it { expect(body.first.values_at('id', 'name')).to eq [matching_group.id, matching_group.name] }
+    end
+
+    context "while fetching all groups belonging to a project the current user cannot access" do
+      before do
+        project.invited_groups << matching_group
+        sign_in(user)
+        get(:project_groups, project_id: project.id)
+      end
+
+      it { expect(response).to be_not_found }
+    end
+
+    context "while fetching all groups belonging to an invalid project ID" do
+      before do
+        project.invited_groups << matching_group
+        sign_in(user)
+        get(:project_groups, project_id: 'invalid')
+      end
+
+      it { expect(response).to be_not_found }
+    end
+  end
 end

spec/factories/protected_branches.rb

@@ -11,6 +11,9 @@ FactoryGirl.define do
     transient do
       authorize_user_to_push nil
       authorize_user_to_merge nil
+
+      authorize_group_to_push nil
+      authorize_group_to_merge nil
     end
 
     trait :remove_default_access_levels do
@@ -47,6 +50,9 @@ FactoryGirl.define do
     after(:create) do |protected_branch, evaluator|
       protected_branch.push_access_levels.create!(user: evaluator.authorize_user_to_push) if evaluator.authorize_user_to_push
       protected_branch.merge_access_levels.create!(user: evaluator.authorize_user_to_merge) if evaluator.authorize_user_to_merge
+
+      protected_branch.push_access_levels.create!(group: evaluator.authorize_group_to_push) if evaluator.authorize_group_to_push
+      protected_branch.merge_access_levels.create!(group: evaluator.authorize_group_to_merge) if evaluator.authorize_group_to_merge
     end
   end
 end

spec/factories/protected_branches/merge_access_levels.rb

 FactoryGirl.define do
   factory :protected_branch_merge_access_level, class: ProtectedBranch::MergeAccessLevel do
     user nil
+    group nil
     protected_branch
     access_level { Gitlab::Access::DEVELOPER }
   end

spec/factories/protected_branches/push_access_levels.rb

 FactoryGirl.define do
   factory :protected_branch_push_access_level, class: ProtectedBranch::PushAccessLevel do
     user nil
+    group nil
     protected_branch
     access_level { Gitlab::Access::DEVELOPER }
   end

spec/features/protected_branches/access_control_ee_spec.rb

@@ -2,16 +2,22 @@ RSpec.shared_examples "protected branches > access control > EE" do
   [['merge', ProtectedBranch::MergeAccessLevel], ['push', ProtectedBranch::PushAccessLevel]].each do |git_operation, access_level_class|
     # Need to set a default for the `git_operation` access level that _isn't_ being tested
     other_git_operation = git_operation == 'merge' ? 'push' : 'merge'
+    roles = git_operation == 'merge' ? access_level_class.human_access_levels : access_level_class.human_access_levels.except(0)
 
-    it "allows creating protected branches that roles and users can #{git_operation} to" do
-      users = create_list(:user, 5)
+    let(:users) { create_list(:user, 5) }
+    let(:groups) { create_list(:group, 5) }
+
+    before do
       users.each { |user| project.team << [user, :developer] }
-      roles = access_level_class.human_access_levels
+      groups.each { |group| project.project_group_links.create(group: group, group_access: Gitlab::Access::DEVELOPER) }
+    end
 
+    it "allows creating protected branches that roles, users, and groups can #{git_operation} to" do
       visit namespace_project_protected_branches_path(project.namespace, project)
 
       set_protected_branch_name('master')
       set_allowed_to(git_operation, users.map(&:name))
+      set_allowed_to(git_operation, groups.map(&:name))
       set_allowed_to(git_operation, roles.values)
       set_allowed_to(other_git_operation)
 
@@ -21,13 +27,10 @@ RSpec.shared_examples "protected branches > access control > EE" do
       expect(ProtectedBranch.count).to eq(1)
       roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
       users.each { |user| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(user.id) }
+      groups.each { |group| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:group_id)).to include(group.id) }
     end
 
-    it "allows updating protected branches that roles and users can #{git_operation} to" do
-      users = create_list(:user, 5)
-      users.each { |user| project.team << [user, :developer] }
-      roles = access_level_class.human_access_levels
-
+    it "allows updating protected branches so that roles and users can #{git_operation} to it" do
       visit namespace_project_protected_branches_path(project.namespace, project)
       set_protected_branch_name('master')
       set_allowed_to('merge')
@@ -37,6 +40,7 @@ RSpec.shared_examples "protected branches > access control > EE" do
 
       within(".js-protected-branch-edit-form") do
         set_allowed_to(git_operation, users.map(&:name))
+        set_allowed_to(git_operation, groups.map(&:name))
         set_allowed_to(git_operation, roles.values)
       end
 
@@ -45,12 +49,35 @@ RSpec.shared_examples "protected branches > access control > EE" do
       expect(ProtectedBranch.count).to eq(1)
       roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
       users.each { |user| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(user.id) }
+      groups.each { |group| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:group_id)).to include(group.id) }
+    end
+
+    it "allows updating protected branches so that roles and users cannot #{git_operation} to it" do
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('master')
+
+      users.each { |user| set_allowed_to(git_operation, user.name) }
+      roles.each { |(_, access_type_name)| set_allowed_to(git_operation, access_type_name) }
+      groups.each { |group| set_allowed_to(git_operation, group.name) }
+      set_allowed_to(other_git_operation)
+
+      click_on "Protect"
+
+      within(".js-protected-branch-edit-form") do
+        users.each { |user| set_allowed_to(git_operation, user.name) }
+        groups.each { |group| set_allowed_to(git_operation, group.name) }
+        roles.each { |(_, access_type_name)| set_allowed_to(git_operation, access_type_name) }
+      end
+
+      wait_for_ajax
+
+      expect(ProtectedBranch.count).to eq(1)
+      expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym)).to be_empty
     end
 
     it "prepends selected users that can #{git_operation} to" do
       users = create_list(:user, 21)
       users.each { |user| project.team << [user, :developer] }
-      roles = access_level_class.human_access_levels
 
       visit namespace_project_protected_branches_path(project.namespace, project)
 
@@ -69,7 +96,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
         click_on users.last.name
         find(".js-allowed-to-#{git_operation}").click # close
       end
-      
       wait_for_ajax
 
       # Verify the user is appended in the dropdown
@@ -81,4 +107,48 @@ RSpec.shared_examples "protected branches > access control > EE" do
       expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(users.last.id)
     end
   end
+
+  context 'When updating a protected branch' do
+    it 'discards other roles when choosing "No one"' do
+      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('fix')
+      set_allowed_to('merge')
+      set_allowed_to('push', roles.values)
+      click_on "Protect"
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(0)
+
+      within(".js-protected-branch-edit-form") do
+        set_allowed_to('push', 'No one')
+      end
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(0)
+    end
+  end
+
+  context 'When creating a protected branch' do
+    it 'discards other roles when choosing "No one"' do
+      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('master')
+      set_allowed_to('merge')
+      set_allowed_to('push', ProtectedBranch::PushAccessLevel.human_access_levels.values) # Last item (No one) should deselect the other ones
+      click_on "Protect"
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(0)
+    end
+  end
 end

spec/features/protected_branches_spec.rb

 require 'spec_helper'
 Dir["./spec/features/protected_branches/*.rb"].sort.each { |f| require f }
 
-feature 'Projected Branches', feature: true, js: true do
+feature 'Protected Branches', feature: true, js: true do
   include WaitForAjax
 
   let(:user) { create(:user, :admin) }

spec/lib/gitlab/import_export/all_models.yml

@@ -112,9 +112,11 @@ protected_branches:
 merge_access_levels:
 - user
 - protected_branch
+- group
 push_access_levels:
 - user
 - protected_branch
+- group
 project:
 - taggings
 - base_tags

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -318,6 +318,7 @@ ProtectedBranch::MergeAccessLevel:
 - created_at
 - updated_at
 - user_id
+- group_id
 ProtectedBranch::PushAccessLevel:
 - id
 - protected_branch_id
@@ -325,6 +326,7 @@ ProtectedBranch::PushAccessLevel:
 - created_at
 - updated_at
 - user_id
+- group_id
 AwardEmoji:
 - id
 - user_id

spec/models/protected_branch_spec.rb

@@ -38,6 +38,16 @@ describe ProtectedBranch, models: true do
 
           expect(protected_branch).to be_valid
         end
+
+        it "does not count a group-based #{human_association_name} with an `access_level` set" do
+          group = create(:group)
+          protected_branch = create(:protected_branch, :remove_default_access_levels)
+
+          protected_branch.send(association_name) << build(factory_name, group: group, access_level: Gitlab::Access::MASTER)
+          protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
+
+          expect(protected_branch).to be_valid
+        end
       end
 
       context "while checking uniqueness of a user-based #{human_association_name}" do
@@ -65,6 +75,34 @@ describe ProtectedBranch, models: true do
           expect(protected_branch).to be_valid
         end
       end
+
+      context "while checking uniqueness of a group-based #{human_association_name}" do
+        let(:group) { create(:group) }
+
+        it "allows a single #{human_association_name} for a group (per protected branch)" do
+          first_protected_branch = create(:protected_branch, :remove_default_access_levels)
+          second_protected_branch = create(:protected_branch, :remove_default_access_levels)
+
+          first_protected_branch.send(association_name) << build(factory_name, group: group)
+          second_protected_branch.send(association_name) << build(factory_name, group: group)
+
+          expect(first_protected_branch).to be_valid
+          expect(second_protected_branch).to be_valid
+
+          first_protected_branch.send(association_name) << build(factory_name, group: group)
+          expect(first_protected_branch).to be_invalid
+          expect(first_protected_branch.errors.full_messages.first).to match("group has already been taken")
+        end
+
+        it "ignores the `access_level` while validating a group-based #{human_association_name}" do
+          protected_branch = create(:protected_branch, :remove_default_access_levels)
+
+          protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
+          protected_branch.send(association_name) << build(factory_name, group: group, access_level: Gitlab::Access::MASTER)
+
+          expect(protected_branch).to be_valid
+        end
+      end
     end
   end