Use a more consistent way to verify permission for Groups::Create/UpdateService
Concern
This is extracted from https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942#note_30781768
We have this pattern in the API:
if ::Groups::UpdateService.new(group, current_user, declared_params(include_missing: false)).execute
present group, with: Entities::GroupDetail, current_user: current_user
else
render_validation_error!(group)
end
So it looks like we're checking if it's updating or not by assuming it's returning a truthy or falsely value to represent it's successfully saved or not, but the fact is, it's not just so, it could be returning the original group whenever a permission check doesn't pass:
unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
deny_visibility_level(@group)
return @group
end
Return value summary:
- true # means that the record is saved
- false # means there are validation errors so it's not saved
- the instance! # same as above, or there are some other errors like permission check
Proposal
Since for creation, we would like to get the instance. So I propose we stop returning any booleans, and only return the instance, and we check the instance to see if it updated. That means we'll need to change the pattern to something like:
updated_group = ::Groups::UpdateService.new(group, current_user, declared_params(include_missing: false)).execute
if updated_group.saved?
present updated_group, with: Entities::GroupDetail, current_user: current_user
elsif # check if it's permission error
# render permission error
else
render_validation_error!(group)
end
After this is done for CE, we'll also need to update the check for shared_runners_minutes_limit
in EE to accommodate this.