Merge branch '21513-fix-branch-protection-api' into 'master'
Fix branch protection API. ## What does this MR do? - Fixes the branch protection API. - Closes #21513 - EE Merge Request: gitlab-org/gitlab-ee!718 ## Tasks - [ ] #21513 !6215 Protected branches API bug - [x] Investigate - [x] Test + Fix - [x] Changelog - [x] MR - [x] Wait for build to pass - [x] Review - [x] Check for EE conflicts - [x] Create EE MR - [x] Refactor + Fix - [x] Rebase EE MR against EE master - [x] Wait for builds to pass - [x] Assign to dbalexandre/douwe - [x] Implement latest review comments - [x] Wait for Douwe's review - [x] Implement changes - [x] Port changes to EE MR - [x] Assign both back to Douwe - [ ] Wait for merge - [ ] Merge gitlab-org/gitlab-ee!718 See merge request !6215
This commit is contained in:
commit
6e693771e8
|
@ -391,6 +391,7 @@ Please view this file on the master branch, on stable branches it's out of date.
|
|||
- Fix inconsistent checkbox alignment (ClemMakesApps)
|
||||
- Use the default branch for displaying the project icon instead of master !5792 (Hannes Rosenögger)
|
||||
- Adds response mime type to transaction metric action when it's not HTML
|
||||
- Fix branch protection API !6215
|
||||
- Fix hover leading space bug in pipeline graph !5980
|
||||
- Avoid conflict with admin labels when importing GitHub labels
|
||||
- User can edit closed MR with deleted fork (Katarzyna Kobierska Ula Budziszewska) !5496
|
||||
|
|
|
@ -1,6 +1,11 @@
|
|||
module ProtectedBranchAccess
|
||||
extend ActiveSupport::Concern
|
||||
|
||||
included do
|
||||
scope :master, -> { where(access_level: Gitlab::Access::MASTER) }
|
||||
scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) }
|
||||
end
|
||||
|
||||
def humanize
|
||||
self.class.human_access_levels[self.access_level]
|
||||
end
|
||||
|
|
|
@ -0,0 +1,29 @@
|
|||
# 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 execute
|
||||
push_access_level =
|
||||
if params.delete(:developers_can_push)
|
||||
Gitlab::Access::DEVELOPER
|
||||
else
|
||||
Gitlab::Access::MASTER
|
||||
end
|
||||
|
||||
merge_access_level =
|
||||
if params.delete(:developers_can_merge)
|
||||
Gitlab::Access::DEVELOPER
|
||||
else
|
||||
Gitlab::Access::MASTER
|
||||
end
|
||||
|
||||
@params.merge!(push_access_levels_attributes: [{ access_level: push_access_level }],
|
||||
merge_access_levels_attributes: [{ access_level: merge_access_level }])
|
||||
|
||||
service = ProtectedBranches::CreateService.new(@project, @current_user, @params)
|
||||
service.execute
|
||||
end
|
||||
end
|
||||
end
|
|
@ -0,0 +1,47 @@
|
|||
# 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 execute(protected_branch)
|
||||
@developers_can_push = params.delete(:developers_can_push)
|
||||
@developers_can_merge = params.delete(:developers_can_merge)
|
||||
|
||||
@protected_branch = protected_branch
|
||||
|
||||
protected_branch.transaction do
|
||||
delete_redundant_access_levels
|
||||
|
||||
case @developers_can_push
|
||||
when true
|
||||
params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
|
||||
when false
|
||||
params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
|
||||
end
|
||||
|
||||
case @developers_can_merge
|
||||
when true
|
||||
params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
|
||||
when 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_access_levels
|
||||
unless @developers_can_merge.nil?
|
||||
@protected_branch.merge_access_levels.destroy_all
|
||||
end
|
||||
|
||||
unless @developers_can_push.nil?
|
||||
@protected_branch.push_access_levels.destroy_all
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -54,43 +54,25 @@ 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
|
||||
name: @branch.name,
|
||||
developers_can_push: to_boolean(params[:developers_can_push]),
|
||||
developers_can_merge: to_boolean(params[:developers_can_merge])
|
||||
}
|
||||
|
||||
# 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, protected_branch_params]
|
||||
|
||||
# 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 +105,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 +124,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
|
||||
{
|
||||
|
|
|
@ -48,92 +48,154 @@ describe API::API, api: true do
|
|||
end
|
||||
|
||||
describe 'PUT /projects/:id/repository/branches/:branch/protect' do
|
||||
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(json_response['name']).to eq(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
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 'protects a single branch and developers can push' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
|
||||
developers_can_push: true
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response['name']).to eq(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
expect(json_response['protected']).to eq(true)
|
||||
expect(json_response['developers_can_push']).to eq(true)
|
||||
expect(json_response['developers_can_merge']).to eq(false)
|
||||
end
|
||||
|
||||
it 'protects a single branch and developers can merge' do
|
||||
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['commit']['id']).to eq(branch_sha)
|
||||
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)
|
||||
end
|
||||
|
||||
it 'protects a single branch and developers can push and merge' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{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(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
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
|
||||
|
||||
it 'protects a single branch and developers cannot push and merge' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
|
||||
developers_can_push: 'tru', developers_can_merge: 'tr'
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response['name']).to eq(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
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
|
||||
|
||||
context 'on a protected branch' do
|
||||
let(:protected_branch) { 'foo' }
|
||||
|
||||
before do
|
||||
project.repository.add_branch(user, protected_branch, 'master')
|
||||
create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch)
|
||||
end
|
||||
|
||||
it 'updates that a developer can push' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{protected_branch}/protect", user),
|
||||
developers_can_push: false, developers_can_merge: false
|
||||
context "when a protected branch doesn't already exist" do
|
||||
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(json_response['name']).to eq(protected_branch)
|
||||
expect(json_response['name']).to eq(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
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 'does not update that a developer can push' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{protected_branch}/protect", user),
|
||||
developers_can_push: 'foobar', developers_can_merge: 'foo'
|
||||
it 'protects a single branch and developers can push' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
|
||||
developers_can_push: true
|
||||
|
||||
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['commit']['id']).to eq(branch_sha)
|
||||
expect(json_response['protected']).to eq(true)
|
||||
expect(json_response['developers_can_push']).to eq(true)
|
||||
expect(json_response['developers_can_merge']).to eq(false)
|
||||
end
|
||||
|
||||
it 'protects a single branch and developers can merge' do
|
||||
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['commit']['id']).to eq(branch_sha)
|
||||
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)
|
||||
end
|
||||
|
||||
it 'protects a single branch and developers can push and merge' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{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(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
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
|
||||
|
||||
it 'protects a single branch and developers cannot push and merge' do
|
||||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user),
|
||||
developers_can_push: 'tru', developers_can_merge: 'tr'
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response['name']).to eq(branch_name)
|
||||
expect(json_response['commit']['id']).to eq(branch_sha)
|
||||
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
|
||||
end
|
||||
|
||||
context 'for an existing protected branch' do
|
||||
before do
|
||||
project.repository.add_branch(user, protected_branch.name, 'master')
|
||||
end
|
||||
|
||||
context "when developers can push and merge" do
|
||||
let(:protected_branch) { create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: 'protected_branch') }
|
||||
|
||||
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(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(false)
|
||||
end
|
||||
|
||||
it "returns success when `protect` is called twice with `developers_can_push` turned on" do
|
||||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_push: true
|
||||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_push: 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(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)
|
||||
end
|
||||
end
|
||||
|
@ -147,12 +209,6 @@ describe API::API, api: true do
|
|||
put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user2)
|
||||
expect(response).to have_http_status(403)
|
||||
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
|
||||
|
||||
describe "PUT /projects/:id/repository/branches/:branch/unprotect" do
|
||||
|
|
Loading…
Reference in New Issue