feature: contributors allowed to delete repositories/tags #1696
Conversation
|
The solution is already in place but I'm going to take the opportunity to migrate these endpoints to the API and use it in the client-side. |
vitoravelino
force-pushed
the
improve-delete-config
branch
from
5fc19e2
to
efec0b7
Compare
vitoravelino
changed the title
vitoravelino
force-pushed
the
improve-delete-config
branch
from
1b2e8f1
to
77e263b
Compare
|
@mssola This is ready to be reviewed. I'm gonna open an issue for the documentation part of this. I'll work on that whenever I finish my other tasks. |
app/models/tag.rb
Outdated
| Tag.where(digest: dig).find_each do |tag| | ||
| tags_destroyed &&= tag&.delete_by!(actor) | ||
| end | ||
| tags_destroyed |
There was a problem hiding this comment.
In tags_destroyed &&= tag&.delete_by!(actor) you may have removed tags from a previous iteration, even if this one fails. Thus, you should rename tags_destroyed to success.
|
|
||
| delete_enabled = APP_CONFIG.enabled?("delete") | ||
| is_owner = @repository.namespace.team.owners.exists?(user.id) | ||
| can_contributor_delete = APP_CONFIG["delete"]["contributors"] && |
There was a problem hiding this comment.
Change to: APP_CONFIG.enabled?("delete.contributors") as you are doing in the tag policy. Moreover, maybe it would be a good change to DRY this part and create a method like this:
def allow_destroy_for?(user, repository)
delete_enabled = APP_CONFIG.enabled?("delete")
is_owner = repository.namespace.team.owners.exists?(user.id)
can_contributor_delete = APP_CONFIG.enabled?("delete.contributors") &&
@repository.namespace.team.contributors.exists?(user.id)
delete_enabled && (user.admin? || is_owner || can_contributor_delete)
endNote that the tag policy basically does tag.repository, so the repository is the only object needed.
There was a problem hiding this comment.
Change to: APP_CONFIG.enabled?("delete.contributors") as you are doing in the tag policy.
The tag policy was actually a leftover since it doesn't work. The enabled? function works only with the enabled child attribute. In this case we have contributors being a child of delete and the value goes directly to it and not to a enabled child. I should actually fix the tag policy file.
I double checked and by using enabled? on delete.contributors, it always return true. I think that if enabled child attribute does not exist it should return false, no?
| destroy_service = ::Repositories::DestroyService.new(current_user) | ||
| destroyed = destroy_service.execute(repository) | ||
|
|
||
| error!({ "errors" => destroy_service.error }, 422, header) unless destroyed |
There was a problem hiding this comment.
On success a delete request should return a 204 status code.
There was a problem hiding this comment.
Remember that in the other PR about managing team member (still pending btw, but I need to re-review and rebase before asking another review) I was having an issue by writing just the unless? For some reason this is working now. Maybe gem version has been updated? Response seems fine as you can see below:
Request URL:http://localhost:3000/api/v1/repositories/12
Request Method:DELETE
Status Code:204 No Content
There was a problem hiding this comment.
Cool. As long as the tests works I'm fine then 👍
| service = ::Tags::DestroyService.new(current_user) | ||
| destroyed = service.execute(tag) | ||
|
|
||
| error!({ "errors" => service.error }, 422, header) unless destroyed |
There was a problem hiding this comment.
Same as in the repository.
I'm just signing the commits via GPG (-S) but not showing the signature (-s). I'll configure my |
With the variety usage of Portus, the need of giving contributors more responsibilities arose. So far only admin were able to delete repositories and tags if the capability were enabled. This patch extends the feature to also allow contributors to be able delete resources. We also took the opportunity to migrate and add two endpoints [0] [1] the API regarding this topic. Users are now able to invoke the API to perform those actions if they want to. [0] DELETE /api/v1/repositories/:id [1] DELETE /api/v1/tags/:id Signed-off-by: Vítor Avelino <[email protected]>
vitoravelino
force-pushed
the
improve-delete-config
branch
from
77e263b
to
53ce896
Compare
|
GREEN! 😄 |
|
Thanks a lot! 👍 |
Summary
With the variety usage of Portus, the need of giving contributors more responsibilities arose. So far only admin were able to delete repositories and tags if the capability were enabled. This patch extends the feature to also allow contributors to be able delete resources.
We also took the opportunity to migrate and add two endpoints [0] [1] the API regarding this topic. Users are now able to invoke the API to perform those actions if they want to.
[0]
DELETE /api/v1/repositories/:id[1]
DELETE /api/v1/tags/:idFixes #1273