api: inconsistent error codes in REST API about invenio-files-rest HOT 11 CLOSED

Discussed IRL with @nharraud:

-Write REST API documentation which is resilient to if 404 or 403/401 is returned so that we can make changes in REST API without breaking the API in the future.

from invenio-files-rest.

This was a design decision. The reason we don't reply with 401/403 on PUT if the user has read access, is because it requires checking two or more permissions.

Currently the only permission checked so far is bucket-update permission. If you need to give a proper reply, you potentially need to check bucket-read, bucket-read-versions, object-read and object-read-versions.

Since permission checking can currently be an expensive operation, it's by design that only one permission is checked.

We can discuss if this needs changing, but I'm not sure it's worth the effort/performance. Also, I agree this is a very hard task if it needs to be solved, and not something I think we should target for a Invenio 3.0 release.

Btw, previously we replied with 401/403 errors, but that basically allowed leaking existence information about buckets and filenames inside a bucket.

from invenio-files-rest.

Btw, I'm changing this into label discussion since first we should agree on if this has to be done.

from invenio-files-rest.

Unless, we check the *-read* permissions, we really have to return 404. First step to implement full check could be making sure that the permissions are configurable so one can fall back to basic permission check in case of an DoS attack.

from invenio-files-rest.

@lnielsen @jirikuncar the delete operations returns 401/403, not 404.

This behavior also creates another performance issue for clients. They can't make the difference between a file or a bucket which has just been deleted and a file which they don't have access to, Thus they will have to make another "GET" in order to show the appropriate message to the user.
We will have this issue in B2Share as the UI uses the REST API.

from invenio-files-rest.

@nharraud That's because in order to delete an object, you need to retrieve it first. In case you want 404 errors instead of 401/403 we can change hidden parameter here: https://github.com/inveniosoftware/invenio-files-rest/blob/master/invenio_files_rest/views.py#L455

from invenio-files-rest.

They can't make the difference between a file or a bucket which has just been deleted and a file which they don't have access to

But that's kind of the point - if you don't have access, you shouldn't even know that it exists.

Is it possible to workaround it from the client side by simply:

• List objects GET /api/files/:bucket. (200, i.e. has read permissions)
• Put object PUT /api/files/:bucket/:key (404, i.e. don't have update permissions)

and:

• List objects GET /api/files/:bucket. (404, i.e. don't have read permissions)

from invenio-files-rest.

That's because in order to delete an object, you need to retrieve it first. In case you want 404 errors instead of 401/403 we can change hidden parameter here:

@lnielsen at least it would be more consistent with PUT.

Is it possible to workaround it from the client side by simply:

List objects GET /api/files/:bucket. (200, i.e. has read permissions)
Put object PUT /api/files/:bucket/:key (404, i.e. don't have update permissions)
and:

List objects GET /api/files/:bucket. (404, i.e. don't have read permissions)

This will work for me. I just hope that nobody will need separate permissions for bucket and files as these are two separate actions.

from invenio-files-rest.

This will work for me. I just hope that nobody will need separate permissions for bucket and files as these are two separate actions.

The REST API gives the possibility to define per object permissions, however the default permission factory and I think most uses cases, only allow defining permission per bucket: https://github.com/inveniosoftware/invenio-files-rest/blob/master/invenio_files_rest/permissions.py#L111-L128

from invenio-files-rest.

The REST API gives the possibility to define per object permissions, however the default permission factory and I think most uses cases, only allow defining permission per bucket:

@lnielsen thanks for the info. I had just seen while implementing file permissions that the two actions are separated.

So what do we do? move the issue of finding a way to return 403 to a later milestone and in the mean time make delete return 404 too?

We could at the same time remove the 'read' permission required for the 'delete' operation as no data is actually returned to the user. This would just require delete to not use get_object.

from invenio-files-rest.

So what do we do? move the issue of finding a way to return 403 to a later milestone and in the mean time make delete return 404 too?

I don't think fixing DELETE method is enough - you have 403 errors begin returned other places as well - e.g. I think POST and GET can return 403 as well. The tests should however show most of the places where 403 is being returned. Note, some 403s are return because the action is forbidden (e.g. the bucket is locked for modification) - these 403s should naturally stay.

However, I still think it's a bigger task to sort this out, and whoever takes needs to ensure they have enough time to actually do it properly - the worst we can do is to fix it partially, because then we have just moved the problem from one place to another.

from invenio-files-rest.