Backend implementation to allow a group to be granted access to a protected branch.

1. A protected branch access level `belongs_to` a group in addition to a
   user. When access for a user is checked, see if the user is present
   in the access level's group, if any.

2. If a group is added to a project with `:reporter` access or lower,
   adding a group to a proteted branch does _not_ grant access, since
   the `push_code` ability is still checked. This is tested with a few
   hundred new tests in `git_access_spec`.

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
 
@@ -21,6 +22,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
 
@@ -24,6 +25,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

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

@@ -1048,6 +1048,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
@@ -1059,6 +1060,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
@@ -1404,8 +1406,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/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/lib/gitlab/git_access_spec.rb

@@ -201,6 +201,7 @@ describe Gitlab::GitAccess, lib: true do
                                          end
     end
 
+    # Run permission checks for a user
     def self.run_permission_checks(permissions_matrix)
       permissions_matrix.keys.each do |role|
         describe "#{role} access" do
@@ -210,6 +211,27 @@ describe Gitlab::GitAccess, lib: true do
             else
               project.team << [user, role]
             end
+
+            permissions_matrix[role].each do |action, allowed|
+              context action do
+                subject { access.push_access_check(changes[action]) }
+
+                it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
+              end
+            end
+          end
+        end
+      end
+    end
+
+    # Run permission checks for a group
+    def self.run_group_permission_checks(permissions_matrix)
+      permissions_matrix.keys.each do |role|
+        describe "#{role} access" do
+          before do
+            project.project_group_links.create(
+              group: group, group_access: Gitlab::Access.sym_options[role]
+            )
           end
 
           permissions_matrix[role].each do |action, allowed|
@@ -326,44 +348,99 @@ describe Gitlab::GitAccess, lib: true do
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
 
-      context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
-        let(:user) { create(:user) }
+      context "user-specific access control" do
+        context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
+          let(:user) { create(:user) }
 
-        before do
-          create(:protected_branch, :remove_default_access_levels, authorize_user_to_push: user, name: protected_branch_name, project: project)
+          before do
+            create(:protected_branch, :remove_default_access_levels, authorize_user_to_push: user, name: protected_branch_name, project: project)
+          end
+
+          run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
+                                                              guest: { push_protected_branch: false, merge_into_protected_branch: false },
+                                                              reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
         end
 
-        run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
-                                                            guest: { push_protected_branch: false, merge_into_protected_branch: false },
-                                                            reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
-      end
+        context "when a specific user is allowed to merge into the #{protected_branch_type} protected branch" do
+          let(:user) { create(:user) }
 
-      context "when a specific user is allowed to merge into the #{protected_branch_type} protected branch" do
-        let(:user) { create(:user) }
+          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(:protected_branch, :remove_default_access_levels, authorize_user_to_merge: user, name: protected_branch_name, project: project)
+          end
 
-        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(:protected_branch, :remove_default_access_levels, authorize_user_to_merge: user, name: protected_branch_name, project: project)
+          run_permission_checks(permissions_matrix.deep_merge(admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
+                                                              master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
+                                                              developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
+                                                              guest: { push_protected_branch: false, merge_into_protected_branch: false },
+                                                              reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
         end
 
-        run_permission_checks(permissions_matrix.deep_merge(admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
-                                                            master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
-                                                            developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
-                                                            guest: { push_protected_branch: false, merge_into_protected_branch: false },
-                                                            reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
+        context "when a specific user is allowed to push & merge into the #{protected_branch_type} protected branch" do
+          let(:user) { create(:user) }
+
+          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(:protected_branch, :remove_default_access_levels, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project)
+          end
+
+          run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
+                                                              guest: { push_protected_branch: false, merge_into_protected_branch: false },
+                                                              reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
+        end
       end
 
-      context "when a specific user is allowed to push & merge into the #{protected_branch_type} protected branch" do
-        let(:user) { create(:user) }
+      context "group-specific access control" do
+        context "when a specific group is allowed to push into the #{protected_branch_type} protected branch" do
+          let(:user) { create(:user) }
+          let(:group) { create(:group) }
 
-        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(:protected_branch, :remove_default_access_levels, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project)
+          before do
+            group.add_master(user)
+            create(:protected_branch, :remove_default_access_levels, authorize_group_to_push: group, name: protected_branch_name, project: project)
+          end
+
+          permissions = permissions_matrix.except(:admin).deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
+                                                                     guest: { push_protected_branch: false, merge_into_protected_branch: false },
+                                                                     reporter: { push_protected_branch: false, merge_into_protected_branch: false })
+
+          run_group_permission_checks(permissions)
         end
 
-        run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
-                                                            guest: { push_protected_branch: false, merge_into_protected_branch: false },
-                                                            reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
+        context "when a specific group is allowed to merge into the #{protected_branch_type} protected branch" do
+          let(:user) { create(:user) }
+          let(:group) { create(:group) }
+
+          before do
+            group.add_master(user)
+            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(:protected_branch, :remove_default_access_levels, authorize_group_to_merge: group, name: protected_branch_name, project: project)
+          end
+
+          permissions = permissions_matrix.except(:admin).deep_merge(master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
+                                                                     developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
+                                                                     guest: { push_protected_branch: false, merge_into_protected_branch: false },
+                                                                     reporter: { push_protected_branch: false, merge_into_protected_branch: false })
+
+          run_group_permission_checks(permissions)
+        end
+
+        context "when a specific group is allowed to push & merge into the #{protected_branch_type} protected branch" do
+          let(:user) { create(:user) }
+          let(:group) { create(:group) }
+
+          before do
+            group.add_master(user)
+            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(:protected_branch, :remove_default_access_levels, authorize_group_to_push: group, authorize_group_to_merge: group, name: protected_branch_name, project: project)
+          end
+
+          permissions = permissions_matrix.except(:admin).deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
+                                                                     guest: { push_protected_branch: false, merge_into_protected_branch: false },
+                                                                     reporter: { push_protected_branch: false, merge_into_protected_branch: false })
+
+          run_group_permission_checks(permissions)
+        end
       end
 
       context "when no one is allowed to push to the #{protected_branch_name} protected branch" do