From f79f3a1dd621362b0894eff0a54f220f8415a2e0 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 6 Sep 2016 10:35:34 +0530 Subject: Fix branch protection API. 1. Previously, we were not removing existing access levels before creating new ones. This is not a problem for EE, but _is_ for CE, since we restrict the number of access levels in CE to 1. 2. The correct approach is: CE -> delete all access levels before updating a protected branch EE -> delete developer access levels if "developers_can_{merge,push}" is switched off 3. The dispatch is performed by checking if a "length: 1" validation is present on the access levels or not. 4. Another source of problems was that we didn't put multiple queries in a transaction. If the `destroy_all` passes, but the `update` fails, we should have a rollback. 5. Modifying the API to provide users direct access to CRUD access levels will make things a lot simpler. 6. Create `create/update` services separately for this API, which perform the necessary data translation, before calling the regular `create/update` services. The translation code was getting too large for the API endpoint itself, so this move makes sense. --- lib/api/branches.rb | 47 ++++++++++++++++------------------------------- 1 file changed, 16 insertions(+), 31 deletions(-) (limited to 'lib/api/branches.rb') diff --git a/lib/api/branches.rb b/lib/api/branches.rb index b615703df93..6941cc4aca6 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -57,40 +57,25 @@ module API developers_can_merge = to_boolean(params[:developers_can_merge]) developers_can_push = to_boolean(params[:developers_can_push]) - protected_branch_params = { - name: @branch.name + params = { + name: @branch.name, } - # If `developers_can_merge` is switched off, _all_ `DEVELOPER` - # merge_access_levels need to be deleted. - if developers_can_merge == false - protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all - end + service_args = [user_project, current_user, params, + developers_can_push: developers_can_push, + developers_can_merge: developers_can_merge] - # If `developers_can_push` is switched off, _all_ `DEVELOPER` - # push_access_levels need to be deleted. - if developers_can_push == false - protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all - end + protected_branch = if protected_branch + ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch) + else + ProtectedBranches::ApiCreateService.new(*service_args).execute + end - protected_branch_params.merge!( - merge_access_levels_attributes: [{ - access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER - }], - push_access_levels_attributes: [{ - access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER - }] - ) - - if protected_branch - service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params) - service.execute(protected_branch) + if protected_branch.valid? + present @branch, with: Entities::RepoBranch, project: user_project else - service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params) - service.execute + render_api_error!(protected_branch.errors.full_messages, 422) end - - present @branch, with: Entities::RepoBranch, project: user_project end # Unprotect a single branch @@ -123,7 +108,7 @@ module API post ":id/repository/branches" do authorize_push_project result = CreateBranchService.new(user_project, current_user). - execute(params[:branch_name], params[:ref]) + execute(params[:branch_name], params[:ref]) if result[:status] == :success present result[:branch], @@ -142,10 +127,10 @@ module API # Example Request: # DELETE /projects/:id/repository/branches/:branch delete ":id/repository/branches/:branch", - requirements: { branch: /.+/ } do + requirements: { branch: /.+/ } do authorize_push_project result = DeleteBranchService.new(user_project, current_user). - execute(params[:branch]) + execute(params[:branch]) if result[:status] == :success { -- cgit v1.2.1 From b803bc7bb8ad481790d01848902e80602e77da67 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 22 Sep 2016 20:38:05 +0530 Subject: Implement review comments from @DouweM. --- lib/api/branches.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib/api/branches.rb') diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 6941cc4aca6..7feb47784b7 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -57,11 +57,11 @@ module API developers_can_merge = to_boolean(params[:developers_can_merge]) developers_can_push = to_boolean(params[:developers_can_push]) - params = { + protected_branch_params = { name: @branch.name, } - service_args = [user_project, current_user, params, + service_args = [user_project, current_user, protected_branch_params, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge] -- cgit v1.2.1 From 1051087ac4efc3dbf45bd075e36af647d2b66d62 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 30 Sep 2016 13:14:46 +0530 Subject: Implement second round of review comments from @DouweM. - Pass `developers_and_merge` and `developers_can_push` in `params` instead of using keyword arguments. - Refactor a slightly complex boolean check to a simple `nil?` check. --- lib/api/branches.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) (limited to 'lib/api/branches.rb') diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 7feb47784b7..6d827448994 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -54,16 +54,13 @@ module API not_found!('Branch') unless @branch protected_branch = user_project.protected_branches.find_by(name: @branch.name) - developers_can_merge = to_boolean(params[:developers_can_merge]) - developers_can_push = to_boolean(params[:developers_can_push]) - protected_branch_params = { name: @branch.name, + developers_can_push: to_boolean(params[:developers_can_push]), + developers_can_merge: to_boolean(params[:developers_can_merge]) } - service_args = [user_project, current_user, protected_branch_params, - developers_can_push: developers_can_push, - developers_can_merge: developers_can_merge] + service_args = [user_project, current_user, protected_branch_params] protected_branch = if protected_branch ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch) -- cgit v1.2.1