Commit f79f3a1d authored by Timothy Andrew's avatar Timothy Andrew

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.
parent a98ad03b
module ProtectedBranchAccess module ProtectedBranchAccess
extend ActiveSupport::Concern extend ActiveSupport::Concern
included do
scope :master, -> () { where(access_level: Gitlab::Access::MASTER) }
scope :developer, -> () { where(access_level: Gitlab::Access::DEVELOPER) }
end
def humanize def humanize
self.class.human_access_levels[self.access_level] self.class.human_access_levels[self.access_level]
end end
......
# The protected branches API still uses the `developers_can_push` and `developers_can_merge`
# flags for backward compatibility, and so performs translation between that format and the
# internal data model (separate access levels). The translation code is non-trivial, and so
# lives in this service.
module ProtectedBranches
class ApiCreateService < BaseService
def initialize(project, user, params, developers_can_push:, developers_can_merge:)
super(project, user, params)
@developers_can_merge = developers_can_merge
@developers_can_push = developers_can_push
end
def execute
if @developers_can_push
@params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
else
@params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
end
if @developers_can_merge
@params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
else
@params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
end
service = ProtectedBranches::CreateService.new(@project, @current_user, @params)
service.execute
end
end
end
# The protected branches API still uses the `developers_can_push` and `developers_can_merge`
# flags for backward compatibility, and so performs translation between that format and the
# internal data model (separate access levels). The translation code is non-trivial, and so
# lives in this service.
module ProtectedBranches
class ApiUpdateService < BaseService
def initialize(project, user, params, developers_can_push:, developers_can_merge:)
super(project, user, params)
@developers_can_merge = developers_can_merge
@developers_can_push = developers_can_push
end
def execute(protected_branch)
@protected_branch = protected_branch
protected_branch.transaction do
# If a protected branch can have only a single access level (CE), delete it to
# make room for the new access level. If a protected branch can have more than
# one access level (EE), only remove the relevant access levels. If we don't do this,
# we'll have a failed validation.
if protected_branch_restricted_to_single_access_level?
delete_redundant_ce_access_levels
else
delete_redundant_ee_access_levels
end
if @developers_can_push
params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
elsif @developers_can_push == false
params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
end
if @developers_can_merge
params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
elsif @developers_can_merge == false
params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
end
service = ProtectedBranches::UpdateService.new(@project, @current_user, @params)
service.execute(protected_branch)
end
end
private
def delete_redundant_ce_access_levels
if @developers_can_merge || @developers_can_merge == false
@protected_branch.merge_access_levels.destroy_all
end
if @developers_can_push || @developers_can_push == false
@protected_branch.push_access_levels.destroy_all
end
end
def delete_redundant_ee_access_levels
if @developers_can_merge
@protected_branch.merge_access_levels.developer.destroy_all
elsif @developers_can_merge == false
@protected_branch.merge_access_levels.developer.destroy_all
@protected_branch.merge_access_levels.master.destroy_all
end
if @developers_can_push
@protected_branch.push_access_levels.developer.destroy_all
elsif @developers_can_push == false
@protected_branch.push_access_levels.developer.destroy_all
@protected_branch.push_access_levels.master.destroy_all
end
end
def protected_branch_restricted_to_single_access_level?
length_validator = ProtectedBranch.validators_on(:push_access_levels).find do |validator|
validator.is_a? ActiveModel::Validations::LengthValidator
end
length_validator.options[:is] == 1 if length_validator
end
end
end
...@@ -57,40 +57,25 @@ module API ...@@ -57,40 +57,25 @@ module API
developers_can_merge = to_boolean(params[:developers_can_merge]) developers_can_merge = to_boolean(params[:developers_can_merge])
developers_can_push = to_boolean(params[:developers_can_push]) developers_can_push = to_boolean(params[:developers_can_push])
protected_branch_params = { params = {
name: @branch.name name: @branch.name,
} }
# If `developers_can_merge` is switched off, _all_ `DEVELOPER` service_args = [user_project, current_user, params,
# merge_access_levels need to be deleted. developers_can_push: developers_can_push,
if developers_can_merge == false developers_can_merge: developers_can_merge]
protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
end
# If `developers_can_push` is switched off, _all_ `DEVELOPER` protected_branch = if protected_branch
# push_access_levels need to be deleted. ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch)
if developers_can_push == false else
protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all ProtectedBranches::ApiCreateService.new(*service_args).execute
end end
protected_branch_params.merge!( if protected_branch.valid?
merge_access_levels_attributes: [{ present @branch, with: Entities::RepoBranch, project: user_project
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)
else else
service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params) render_api_error!(protected_branch.errors.full_messages, 422)
service.execute
end end
present @branch, with: Entities::RepoBranch, project: user_project
end end
# Unprotect a single branch # Unprotect a single branch
...@@ -123,7 +108,7 @@ module API ...@@ -123,7 +108,7 @@ module API
post ":id/repository/branches" do post ":id/repository/branches" do
authorize_push_project authorize_push_project
result = CreateBranchService.new(user_project, current_user). result = CreateBranchService.new(user_project, current_user).
execute(params[:branch_name], params[:ref]) execute(params[:branch_name], params[:ref])
if result[:status] == :success if result[:status] == :success
present result[:branch], present result[:branch],
...@@ -142,10 +127,10 @@ module API ...@@ -142,10 +127,10 @@ module API
# Example Request: # Example Request:
# DELETE /projects/:id/repository/branches/:branch # DELETE /projects/:id/repository/branches/:branch
delete ":id/repository/branches/:branch", delete ":id/repository/branches/:branch",
requirements: { branch: /.+/ } do requirements: { branch: /.+/ } do
authorize_push_project authorize_push_project
result = DeleteBranchService.new(user_project, current_user). result = DeleteBranchService.new(user_project, current_user).
execute(params[:branch]) execute(params[:branch])
if result[:status] == :success if result[:status] == :success
{ {
......
...@@ -48,92 +48,154 @@ describe API::API, api: true do ...@@ -48,92 +48,154 @@ describe API::API, api: true do
end end
describe 'PUT /projects/:id/repository/branches/:branch/protect' do describe 'PUT /projects/:id/repository/branches/:branch/protect' do
it 'protects a single branch' do context "when a protected branch doesn't already exist" do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user) it 'protects a single branch' do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(branch_name) expect(json_response['name']).to eq(branch_name)
expect(json_response['commit']['id']).to eq(branch_sha) expect(json_response['commit']['id']).to eq(branch_sha)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(false) expect(json_response['developers_can_push']).to eq(false)
expect(json_response['developers_can_merge']).to eq(false) expect(json_response['developers_can_merge']).to eq(false)
end end
it 'protects a single branch and developers can push' do it 'protects a single branch and developers can push' do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
developers_can_push: true developers_can_push: true
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(branch_name) expect(json_response['name']).to eq(branch_name)
expect(json_response['commit']['id']).to eq(branch_sha) expect(json_response['commit']['id']).to eq(branch_sha)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(true) expect(json_response['developers_can_push']).to eq(true)
expect(json_response['developers_can_merge']).to eq(false) expect(json_response['developers_can_merge']).to eq(false)
end end
it 'protects a single branch and developers can merge' do it 'protects a single branch and developers can merge' do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
developers_can_merge: true developers_can_merge: true
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(branch_name) expect(json_response['name']).to eq(branch_name)
expect(json_response['commit']['id']).to eq(branch_sha) expect(json_response['commit']['id']).to eq(branch_sha)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(false) expect(json_response['developers_can_push']).to eq(false)
expect(json_response['developers_can_merge']).to eq(true) expect(json_response['developers_can_merge']).to eq(true)
end end
it 'protects a single branch and developers can push and merge' do it 'protects a single branch and developers can push and merge' do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
developers_can_push: true, developers_can_merge: true developers_can_push: true, developers_can_merge: true
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(branch_name) expect(json_response['name']).to eq(branch_name)
expect(json_response['commit']['id']).to eq(branch_sha) expect(json_response['commit']['id']).to eq(branch_sha)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(true) expect(json_response['developers_can_push']).to eq(true)
expect(json_response['developers_can_merge']).to eq(true) expect(json_response['developers_can_merge']).to eq(true)
end end
it 'protects a single branch and developers cannot push and merge' do it 'protects a single branch and developers cannot push and merge' do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
developers_can_push: 'tru', developers_can_merge: 'tr' developers_can_push: 'tru', developers_can_merge: 'tr'
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(branch_name) expect(json_response['name']).to eq(branch_name)
expect(json_response['commit']['id']).to eq(branch_sha) expect(json_response['commit']['id']).to eq(branch_sha)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(false) expect(json_response['developers_can_push']).to eq(false)
expect(json_response['developers_can_merge']).to eq(false) expect(json_response['developers_can_merge']).to eq(false)
end
end end
context 'on a protected branch' do context 'for an existing protected branch' do
let(:protected_branch) { 'foo' }
before do before do
project.repository.add_branch(user, protected_branch, 'master') project.repository.add_branch(user, protected_branch.name, 'master')
create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch)
end end
it 'updates that a developer can push' do context "when developers can push and merge" do
put api("/projects/#{project.id}/repository/branches/#{protected_branch}/protect", user), let(:protected_branch) { create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: 'protected_branch') }
developers_can_push: false, developers_can_merge: false
it 'updates that a developer cannot push or merge' do
put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
developers_can_push: false, developers_can_merge: false
expect(response).to have_http_status(200)
expect(json_response['name']).to eq(protected_branch.name)
expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(false)
expect(json_response['developers_can_merge']).to eq(false)
end
it "doesn't result in 0 access levels when 'developers_can_push' is switched off" do
put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
developers_can_push: false
expect(response).to have_http_status(200)
expect(json_response['name']).to eq(protected_branch.name)
expect(protected_branch.reload.push_access_levels.first).to be_present
expect(protected_branch.reload.push_access_levels.first.access_level).to eq(Gitlab::Access::MASTER)
end
it "doesn't result in 0 access levels when 'developers_can_merge' is switched off" do
put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
developers_can_merge: false
expect(response).to have_http_status(200)
expect(json_response['name']).to eq(protected_branch.name)
expect(protected_branch.reload.merge_access_levels.first).to be_present
expect(protected_branch.reload.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER)
end
end
context "when developers cannot push or merge" do
let(:protected_branch) { create(:protected_branch, project: project, name: 'protected_branch') }
it 'updates that a developer can push and merge' do
put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
developers_can_push: true, developers_can_merge: true
expect(response).to have_http_status(200)
expect(json_response['name']).to eq(protected_branch.name)
expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(true)
expect(json_response['developers_can_merge']).to eq(true)
end
end
end
context "multiple API calls" do
it "returns success when `protect` is called twice" do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(protected_branch) expect(json_response['name']).to eq(branch_name)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(false) expect(json_response['developers_can_push']).to eq(false)
expect(json_response['developers_can_merge']).to eq(false) expect(json_response['developers_can_merge']).to eq(false)
end end
it 'does not update that a developer can push' do it "returns success when `protect` is called twice with `developers_can_push` turned on" do
put api("/projects/#{project.id}/repository/branches/#{protected_branch}/protect", user), put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_push: true
developers_can_push: 'foobar', developers_can_merge: 'foo' put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_push: true
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['name']).to eq(protected_branch) expect(json_response['name']).to eq(branch_name)
expect(json_response['protected']).to eq(true) expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(true) expect(json_response['developers_can_push']).to eq(true)
expect(json_response['developers_can_merge']).to eq(false)
end
it "returns success when `protect` is called twice with `developers_can_merge` turned on" do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_merge: true
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_merge: true
expect(response).to have_http_status(200)
expect(json_response['name']).to eq(branch_name)
expect(json_response['protected']).to eq(true)
expect(json_response['developers_can_push']).to eq(false)
expect(json_response['developers_can_merge']).to eq(true) expect(json_response['developers_can_merge']).to eq(true)
end end
end end
...@@ -147,12 +209,6 @@ describe API::API, api: true do ...@@ -147,12 +209,6 @@ describe API::API, api: true do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user2) put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user2)
expect(response).to have_http_status(403) expect(response).to have_http_status(403)
end end
it "returns success when protect branch again" do
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
expect(response).to have_http_status(200)
end
end end
describe "PUT /projects/:id/repository/branches/:branch/unprotect" do describe "PUT /projects/:id/repository/branches/:branch/unprotect" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment