diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-09-06 10:35:34 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-10-24 11:33:38 +0530 |
commit | f79f3a1dd621362b0894eff0a54f220f8415a2e0 (patch) | |
tree | 0a2734407f5b0c29a8d61283629558abce79d78f /app/services/protected_branches | |
parent | a98ad03ba18da0b1534f36dafafa9a1c644d0bf1 (diff) | |
download | gitlab-ce-f79f3a1dd621362b0894eff0a54f220f8415a2e0.tar.gz |
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.
Diffstat (limited to 'app/services/protected_branches')
-rw-r--r-- | app/services/protected_branches/api_create_service.rb | 30 | ||||
-rw-r--r-- | app/services/protected_branches/api_update_service.rb | 79 |
2 files changed, 109 insertions, 0 deletions
diff --git a/app/services/protected_branches/api_create_service.rb b/app/services/protected_branches/api_create_service.rb new file mode 100644 index 00000000000..a28056035b8 --- /dev/null +++ b/app/services/protected_branches/api_create_service.rb @@ -0,0 +1,30 @@ +# 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 diff --git a/app/services/protected_branches/api_update_service.rb b/app/services/protected_branches/api_update_service.rb new file mode 100644 index 00000000000..41d3d413caa --- /dev/null +++ b/app/services/protected_branches/api_update_service.rb @@ -0,0 +1,79 @@ +# 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 |