scitran / core

RESTful API
https://scitran.github.io
MIT License
18 stars 18 forks source link

Revise delete permissions #1085

Closed nagem closed 6 years ago

nagem commented 6 years ago

Changes/things made clear in this PR:

Note: all permissions mentioned are those set at the project level

Project deletion:

Session/Acquisition deletion:

Individual File deletion:

A new query param is offered on container DELETE endpoints: check=true/false

List of permission failure variants:

If the container has "original data" (data not uploaded by a user or job), the API will respond with 'original_data_present' in the reason key:

DELETE api/container_name/container_id?check=true
{
  "status_code": 403,
  "message": "user not authorized to perform a DELETE operation on the container.",
  "reason": "original_data_present",
  "request_id": "fdbe14e5-1520003841"
}

If deleting this container would cause an analysis' input file to be removed without removing the analysis, the API will respond with 'analysis_conflict' in the reason key:

DELETE api/container_name/container_id?check=true

{
  "status_code": 403,
  "message": "Cannot delete acquisition 5a908946123ade00147d00cb referenced by analyses ['5a996cf3cea75600b4f7fba3']",
  "reason": "analysis_conflict",
  "request_id": "8650511c-1520004700"
}

All other permission conflicts (403) will have '{'reason': 'permission_denied'} or will be a different error (404/400, etc)

Tests will be added after above proposal is agreed upon.

nagem commented 6 years ago

@gsfr will you make sure the above proposal is what you were expecting?

nagem commented 6 years ago

Attn: @ryansanford

gsfr commented 6 years ago

LGTM.

codecov-io commented 6 years ago

Codecov Report

Merging #1085 into master will increase coverage by 0.01%. The diff coverage is 94.02%.

@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
+ Coverage    90.8%   90.82%   +0.01%     
==========================================
  Files          50       50              
  Lines        7029     7092      +63     
==========================================
+ Hits         6383     6441      +58     
- Misses        646      651       +5
nagem commented 6 years ago

I've actually been moving theDELETE responses to a 200 with no response body if the action succeeded and a proper error response if it didn't. The modified is a relic probably caused by it's storage situation - the container was "modified". I'll make the change in the response 👍.

nagem commented 6 years ago

I take that back, we have some clients that do check the modified count and I would prefer for this to not be a breaking change.