Merge branch 'jej/per-user-protected-branches-api' into 'master'

ProtectedBranches API handles per user/group granularity

Closes #4131

See merge request gitlab-org/gitlab-ee!3516

app/services/protected_branches/access_level_params.rb

+module ProtectedBranches
+  class AccessLevelParams
+    prepend EE::ProtectedBranches::AccessLevelParams
+
+    attr_reader :type, :params
+
+    def initialize(type, params)
+      @type = type
+      @params = params_with_default(params)
+    end
+
+    def access_levels
+      ce_style_access_level
+    end
+
+    private
+
+    def params_with_default(params)
+      params[:"#{type}_access_level"] ||= Gitlab::Access::MASTER if use_default_access_level?(params)
+      params
+    end
+
+    def use_default_access_level?(params)
+      true
+    end
+
+    def ce_style_access_level
+      access_level = params[:"#{type}_access_level"]
+
+      return [] unless access_level
+
+      [{ access_level: access_level }]
+    end
+  end
+end

app/services/protected_branches/api_service.rb

+module ProtectedBranches
+  class ApiService < BaseService
+    prepend EE::ProtectedBranches::ApiService
+
+    def create
+      @push_params = AccessLevelParams.new(:push, params)
+      @merge_params = AccessLevelParams.new(:merge, params)
+
+      verify_params!
+
+      protected_branch_params = {
+        name: params[:name],
+        push_access_levels_attributes: @push_params.access_levels,
+        merge_access_levels_attributes: @merge_params.access_levels
+      }
+
+      ::ProtectedBranches::CreateService.new(@project, @current_user, protected_branch_params).execute
+    end
+
+    private
+
+    def verify_params!
+      # EE-only
+    end
+  end
+end

changelogs/unreleased-ee/jej-per-user-protected-branches-api.yml

+---
+title: ProtectedBranches API allows individual users and group to be specified
+merge_request: 3516
+author:
+type: changed

doc/api/protected_branches.md

@@ -114,6 +114,9 @@ curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" 'https://gitl
 | `name` | string | yes | The name of the branch or wildcard |
 | `push_access_level` | string | no | Access levels allowed to push (defaults: `40`, master access level) |
 | `merge_access_level` | string | no | Access levels allowed to merge (defaults: `40`, master access level) |
+| `allowed_to_push` | array | no | Array of access levels allowed to push, with each described by a hash |
+| `allowed_to_merge` | array | no | Array of access levels allowed to merge, with each described by a hash |
+
 
 Example response:
 
@@ -139,6 +142,39 @@ Example response:
 }
 ```
 
+### Example with user / group level access
+
+Elements in the `allowed_to_push` / `allowed_to_merge` array should take the
+form `{user_id: integer}`, `{group_id: integer}` or `{access_level: integer}`. Each user must have access to the project and each group must [have this project shared](../user/project/members/share_project_with_groups.md). These access levels allow [more granular control over protected branch access](../user/project/protected_branches.md#restricting-push-and-merge-access-to-certain-users) and were [added to the API in ][ee-3516] in GitLab 10.3 EE.
+
+```bash
+curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" 'https://gitlab.example.com/api/v4/projects/5/protected_branches?name=*-stable&allowed_to_push%5B%5D%5Buser_id%5D=1'
+```
+
+Example response:
+
+```json
+{
+  "name":"*-stable",
+  "push_access_levels": [
+    {
+      "access_level":null,
+      "user_id":1,
+      "group_id":null,
+      "access_level_description":"Administrator"
+    }
+  ],
+  "merge_access_levels": [
+    {
+      "access_level":40,
+      "user_id":null,
+      "group_id":null,
+      "access_level_description":"Masters"
+    }
+  ]
+}
+```
+
 ## Unprotect repository branches
 
 Unprotects the given protected branch or wildcard protected branch.
@@ -148,10 +184,12 @@ DELETE /projects/:id/protected_branches/:name
 ```
 
 ```bash
-curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" 'https://gitlab.example.com/api/v4/projects/5/protected_branches/*-stable'
+curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" 'https://gitlab.example.com/api/v4/projects/5/protected_branches/*-stable'
 ```
 
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `name` | string | yes | The name of the branch |
+
+[ee-3516]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3516 "ProtectedBranches API handles per user/group granularity"

ee/app/services/ee/protected_branches/access_level_params.rb

+module EE
+  module ProtectedBranches
+    module AccessLevelParams
+      def access_levels
+        raise NotImplementedError unless defined?(super)
+
+        ce_style_access_level + ee_style_access_levels
+      end
+
+      def group_ids
+        ids_for('group_id')
+      end
+
+      def user_ids
+        ids_for('user_id')
+      end
+
+      private
+
+      def use_default_access_level?(params)
+        raise NotImplementedError unless defined?(super)
+
+        params[:"allowed_to_#{type}"].blank?
+      end
+
+      def ee_style_access_levels
+        params[:"allowed_to_#{type}"] || []
+      end
+
+      def ids_for(key)
+        ee_style_access_levels.select { |level| level[key] }.flat_map(&:values)
+      end
+    end
+  end
+end

ee/app/services/ee/protected_branches/api_service.rb

+module EE
+  module ProtectedBranches
+    module ApiService
+      GroupsNotAccessibleError = Class.new(StandardError)
+      UsersNotAccessibleError = Class.new(StandardError)
+
+      def create
+        raise NotImplementedError unless defined?(super)
+
+        super
+      rescue ::EE::ProtectedBranches::ApiService::GroupsNotAccessibleError,
+             ::EE::ProtectedBranches::ApiService::UsersNotAccessibleError
+        ProtectedBranch.new.tap do |protected_branch|
+          message = 'Cannot add users or groups unless they have access to the project'
+          protected_branch.errors.add(:base, message)
+        end
+      end
+
+      private
+
+      def verify_params!
+        raise NotImplementedError unless defined?(super)
+
+        raise GroupsNotAccessibleError.new unless groups_accessible?
+        raise UsersNotAccessibleError.new unless users_accessible?
+      end
+
+      def groups_accessible?
+        group_ids = @merge_params.group_ids + @push_params.group_ids
+        allowed_groups = @project.invited_groups.where(id: group_ids)
+
+        group_ids.count == allowed_groups.count
+      end
+
+      def users_accessible?
+        user_ids = @merge_params.user_ids + @push_params.user_ids
+        allowed_users = @project.team.users.where(id: user_ids)
+
+        user_ids.count == allowed_users.count
+      end
+    end
+  end
+end

lib/api/protected_branches.rb

@@ -39,12 +39,22 @@ module API
       end
       params do
         requires :name, type: String, desc: 'The name of the protected branch'
-        optional :push_access_level, type: Integer, default: Gitlab::Access::MASTER,
+        optional :push_access_level, type: Integer,
                                      values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS,
                                      desc: 'Access levels allowed to push (defaults: `40`, master access level)'
-        optional :merge_access_level, type: Integer, default: Gitlab::Access::MASTER,
+        optional :merge_access_level, type: Integer,
                                       values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS,
                                       desc: 'Access levels allowed to merge (defaults: `40`, master access level)'
+        optional :allowed_to_push, type: Array, desc: 'An array of users/groups allowed to push' do
+          optional :access_level, type: Integer, values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS
+          optional :user_id, type: Integer
+          optional :group_id, type: Integer
+        end
+        optional :allowed_to_merge, type: Array, desc: 'An array of users/groups allowed to merge' do
+          optional :access_level, type: Integer, values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS
+          optional :user_id, type: Integer
+          optional :group_id, type: Integer
+        end
       end
       post ':id/protected_branches' do
         protected_branch = user_project.protected_branches.find_by(name: params[:name])
@@ -52,15 +62,13 @@ module API
           conflict!("Protected branch '#{params[:name]}' already exists")
         end
 
-        protected_branch_params = {
-          name: params[:name],
-          push_access_levels_attributes: [{ access_level: params[:push_access_level] }],
-          merge_access_levels_attributes: [{ access_level: params[:merge_access_level] }]
-        }
-
-        service_args = [user_project, current_user, protected_branch_params]
+        # Replace with `declared(params)` after updating to grape v1.0.2
+        # See https://github.com/ruby-grape/grape/pull/1710
+        # and https://gitlab.com/gitlab-org/gitlab-ce/issues/40843
+        declared_params = params.slice("name", "push_access_level", "merge_access_level", "allowed_to_push", "allowed_to_merge")
 
-        protected_branch = ::ProtectedBranches::CreateService.new(*service_args).execute
+        api_service = ::ProtectedBranches::ApiService.new(user_project, current_user, declared_params)
+        protected_branch = api_service.create
 
         if protected_branch.persisted?
           present protected_branch, with: Entities::ProtectedBranch, project: user_project

spec/requests/api/protected_branches_spec.rb

@@ -116,6 +116,12 @@ describe API::ProtectedBranches do
 
   describe 'POST /projects/:id/protected_branches' do
     let(:branch_name) { 'new_branch' }
+    let(:post_endpoint) { api("/projects/#{project.id}/protected_branches", user) }
+
+    def expect_protection_to_be_successful
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response['name']).to eq(branch_name)
+    end
 
     context 'when authenticated as a master' do
       before do
@@ -123,7 +129,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch' do
-        post api("/projects/#{project.id}/protected_branches", user), name: branch_name
+        post post_endpoint, name: branch_name
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -132,8 +138,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch and developers can push' do
-        post api("/projects/#{project.id}/protected_branches", user),
-            name: branch_name, push_access_level: 30
+        post post_endpoint, name: branch_name, push_access_level: 30
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -142,8 +147,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch and developers can merge' do
-        post api("/projects/#{project.id}/protected_branches", user),
-            name: branch_name, merge_access_level: 30
+        post post_endpoint, name: branch_name, merge_access_level: 30
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -152,8 +156,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch and developers can push and merge' do
-        post api("/projects/#{project.id}/protected_branches", user),
-            name: branch_name, push_access_level: 30, merge_access_level: 30
+        post post_endpoint, name: branch_name, push_access_level: 30, merge_access_level: 30
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -162,8 +165,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch and no one can push' do
-        post api("/projects/#{project.id}/protected_branches", user),
-            name: branch_name, push_access_level: 0
+        post post_endpoint, name: branch_name, push_access_level: 0
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -172,8 +174,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch and no one can merge' do
-        post api("/projects/#{project.id}/protected_branches", user),
-            name: branch_name, merge_access_level: 0
+        post post_endpoint, name: branch_name, merge_access_level: 0
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -182,8 +183,7 @@ describe API::ProtectedBranches do
       end
 
       it 'protects a single branch and no one can push or merge' do
-        post api("/projects/#{project.id}/protected_branches", user),
-            name: branch_name, push_access_level: 0, merge_access_level: 0
+        post post_endpoint, name: branch_name, push_access_level: 0, merge_access_level: 0
 
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['name']).to eq(branch_name)
@@ -191,8 +191,85 @@ describe API::ProtectedBranches do
         expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::NO_ACCESS)
       end
 
+      context 'with granular access' do
+        let(:invited_group) do
+          create(:project_group_link, project: project).group
+        end
+
+        let(:project_member) do
+          create(:project_member, project: project).user
+        end
+
+        it 'can protect a branch while allowing an individual user to push' do
+          push_user = project_member
+
+          post post_endpoint, name: branch_name, allowed_to_push: [{ user_id: push_user.id }]
+
+          expect_protection_to_be_successful
+          expect(json_response['push_access_levels'][0]['user_id']).to eq(push_user.id)
+        end
+
+        it 'can protect a branch while allowing an individual user to merge' do
+          merge_user = project_member
+
+          post post_endpoint, name: branch_name, allowed_to_merge: [{ user_id: merge_user.id }]
+
+          expect_protection_to_be_successful
+          expect(json_response['merge_access_levels'][0]['user_id']).to eq(merge_user.id)
+        end
+
+        it 'can protect a branch while allowing a group to push' do
+          push_group = invited_group
+
+          post post_endpoint, name: branch_name, allowed_to_push: [{ group_id: push_group.id }]
+
+          expect_protection_to_be_successful
+          expect(json_response['push_access_levels'][0]['group_id']).to eq(push_group.id)
+        end
+
+        it 'can protect a branch while allowing a group to merge' do
+          merge_group = invited_group
+
+          post post_endpoint, name: branch_name, allowed_to_merge: [{ group_id: merge_group.id }]
+
+          expect_protection_to_be_successful
+          expect(json_response['merge_access_levels'][0]['group_id']).to eq(merge_group.id)
+        end
+
+        it "fails if users don't all have access to the project" do
+          push_user = create(:user)
+
+          post post_endpoint, name: branch_name, allowed_to_merge: [{ user_id: push_user.id }]
+
+          expect(response).to have_gitlab_http_status(422)
+          expect(json_response['message'][0]).to match(/Cannot add users or groups/)
+        end
+
+        it "fails if groups aren't all invited to the project" do
+          merge_group = create(:group)
+
+          post post_endpoint, name: branch_name, allowed_to_merge: [{ group_id: merge_group.id }]
+
+          expect(response).to have_gitlab_http_status(422)
+          expect(json_response['message'][0]).to match(/Cannot add users or groups/)
+        end
+
+        it 'avoids creating default access levels unless necessary' do
+          push_user = project_member
+
+          post post_endpoint, name: branch_name, allowed_to_push: [{ user_id: push_user.id }]
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['push_access_levels'].count).to eq(1)
+          expect(json_response['merge_access_levels'].count).to eq(1)
+          expect(json_response['push_access_levels'][0]['user_id']).to eq(push_user.id)
+          expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
+        end
+      end
+
       it 'returns a 409 error if the same branch is protected twice' do
-        post api("/projects/#{project.id}/protected_branches", user), name: protected_name
+        post post_endpoint, name: protected_name
+
         expect(response).to have_gitlab_http_status(409)
       end
 
@@ -200,10 +277,9 @@ describe API::ProtectedBranches do
         let(:branch_name) { 'feature/*' }
 
         it "protects multiple branches with a wildcard in the name" do
-          post api("/projects/#{project.id}/protected_branches", user), name: branch_name
+          post post_endpoint, name: branch_name
 
-          expect(response).to have_gitlab_http_status(201)
-          expect(json_response['name']).to eq(branch_name)
+          expect_protection_to_be_successful
           expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
           expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
         end
@@ -216,7 +292,7 @@ describe API::ProtectedBranches do
       end
 
       it "returns a 403 error if guest" do
-        post api("/projects/#{project.id}/protected_branches/", user), name: branch_name
+        post post_endpoint, name: branch_name
 
         expect(response).to have_gitlab_http_status(403)
       end