Need a much safer default: DjangoAuthorization should not allow read access to all model object

[thomasyip]

Looking at tastypie/authorization.py (around like 133 -- master branch on 1/4/2016), the default, both read_list and read_details bypass user.has_perm() check, which is quite unsafe and is a bad default.

Django's Admin default is unconventional. So, I could see how it was misinterpreted.

https://docs.djangoproject.com/es/1.9/topics/auth/default/#permissions-and-authorization

The Django admin site uses permissions as follows:
*   ... 
* Access to view the change list, view the “change” form and change an object is limited to users with the “change” permission for that type of object.

Essentially, "change_xyz" is the permission code for both "read" and "update". I think the better default would be following Django's Admin:

diff --git a/tastypie/authorization.py b/tastypie/authorization.py
index 1d6f5aa..44b2d56 100644
--- a/tastypie/authorization.py
+++ b/tastypie/authorization.py
@@ -151,22 +151,14 @@ class DjangoAuthorization(Authorization):
         return model_klass

     def read_list(self, object_list, bundle):
-        klass = self.base_checks(bundle.request, object_list.model)
-
-        if klass is False:
-            return []
+        # By default, follows `ModelAdmin` "convention" to use `app.change_model`
+        # `django.contrib.auth.models.Permission` for both viewing and updating.
+        # https://docs.djangoproject.com/es/1.9/topics/auth/default/#permissions-and-authorization

-        # GET-style methods are always allowed.
-        return object_list
+        return self.update_list(object_list, bundle)

     def read_detail(self, object_list, bundle):
-        klass = self.base_checks(bundle.request, bundle.obj.__class__)
-
-        if klass is False:
-            raise Unauthorized("You are not allowed to access that resource.")
-
-        # GET-style methods are always allowed.
-        return True
+        return self.update_detail(object_list, bundle)

[miraculixx]

I appreciate this is merged already but I still take issue with it. Before opening a new request, here's my concern:

This change unfortunately breaks existing code and thus is backwards incompatible (previously all GET requests would pass DjangoAuthorization). To fix one's code, the only two options are to either give all users 'change' permission (bad), or to subclass DjangoAuthorization for a custom implementation of the read actions (potentially lots of work). Was this intended?

I appreciate the reasoning behind this in the django admin context, but I doubt setting 'read' as the equivalent to 'change' in an API context is a sensible default assumption as it is not the expected behavior. After all, if GET is an allowed method along with DjangoAuthorization, why would it refuse access based on the fact that the user does not have the change permission?

I propose he following alternative implementation:

for read_detail

• if the model has a view permission, check that (= improvement for those who need it)
• if there is no view permission, always allow (= backwards compatible)

for read_list

• if the model has a list permission, check that (= improvement for those who need it)
• if there is no list permission, always allow (= backwards compatible)

This way tastypie adds a default that's compatible with Django's permission defaults while giving an easy path to improve by adding view and list permissions for those who need it, short of writing a custom implementation of DjangoAuthorization.

At least there should be a way to specify the default permission:

DjangoAuthorization(read_permission='view', # applies to both list, detail if not spec'd
                    read_list_permission='view', # list only
                    read_detail_permission='view') # detail only
# while we're at it, why not add the other permissions too
DjangoAuthorization(change_permission='change',
                    delete_permission='delete',
                    add_permission='add', 
                    # ... add options as per above for each
                    # <action>_<level> permission where 
                    # action is `change,delete,add,read`,  level is `list,detail`)

[thomasyip]

@SeanHayes will be the one who decide.

Just my opinion here, I think it was a security issue to allow read by default. I totally didn't expect it, until I was very close to production. I think breaking backward-compat is necessary in this context.

Look like your suggestion require changing params pass to DjangoAuthorization to your existing code, in that case I will think it is just clearer to call the class something else.

Consider, read_permission='view'case, here might be exactly what you need. I don't see why it less optimal than

class ModifiedDjangoAuthorization(DjangoAuthorization):
    READ_PERM_CODE = 'view'

If you like to change other, just override the methods. The change actually designed to make it very easy to do it.

class ModifiedDjangoAuthorization(DjangoAuthorization):
    def delete_list(self, object_list, bundle):
        return self.perm_list_checks(bundle.request, 'del', object_list)

    def delete_detail(self, object_list, bundle):
        return self.perm_obj_checks(bundle.request, 'del', bundle.obj)

The change did take into modification in mind and make it easy. I don't necessary think it should be done as init params.

[miraculixx]

I think it was a security issue to allow read by default.

I'm not questioning the intent of the original issue. Just pointing out that the implementation as merged is breaking people's existing code & assumptions without saying so and with no efficient option to revert.

Look like your suggestion require changing params pass to DjangoAuthorization to your existing code,

If you look at my proposed solution again, what I'm advocating is to give people options with a safe and backwards compatible default. No changes would be necessary in user's code in this case.

I think breaking backward-compat is necessary in this context.

I disagree. The change as currently merged not only breaks backwards-compatibility but also introduces a much larger potential security issue in that the obvious way (implied by the current implementation) is to assign the change permission to all users that should be allowed GET.

Frankly, I fail to see how the change permission for read actions increases security as at the same time this permission also allows PUT. Mixing permissions for different actions doesn't seem a good choice.

Unfortunately, your proposed ModifiedDjangoAuthorization won't do the trick unless you actually add the view permission to the model, so it is again breaking backwards compatibility. At least it requires changing code -- so we break backwards compatibility and force users to rework their code base.

Of course overriding is always an option to achieve one's specific requirements, however I think the general idea in tastypie is to provide sensible & safe defaults that don't require adding custom code...

In short I think this change should be reverted for a better implementation.

[thomasyip]

The change permission is coming from Django itself. It is Django's default, it is how Django Admin app is setup. The option view isn't. I personally named mine as read.

Not sure what you mean by checking the model. If you mean checking the _meta, then it might be incomplete. If you mean hitting the db, I found it un-neccesary expensive.

To my preference, what you proposed seem to be on the "too much magic" side for a default authorization. Just having a safe default, easily overridable, seem to be enough. But, that's just my opinion.

[SeanHayes]

The change was documented here: https://github.com/django-tastypie/django-tastypie/blob/master/docs/release_notes/v0.13.2.rst

Frankly, I fail to see how the change permission for read actions increases security as at the same time this permission also allows PUT. Mixing permissions for different actions doesn't seem a good choice.

We know that if a user can change something, then they can read it, that's how the Django admin does things.

This version is more secure, because it forces the developer to think about what they're doing. If instead of inventing their own "read" permission, the developer chooses to deliberately give everyone "change" permissions when they should only have "read" permissions, that's their problem; I can't stop other developers from deliberately doing stupid things, I'm here to stop Tastypie from doing stupid things. The point of this change was to prevent global read permissions on any resource that used DjangoAuthorization, which developers might not expect; the new behavior is in line with what developers experience in the Django admin.

If you want the old behavior:

1. Don't upgrade to 0.13.2.
2. Or override the read_list andread_detail` methods.

If you think the documentation could be improved, feel free to submit a PR.

[miraculixx]

appreciate your feedback. thanks for the link to the doc, fair enough, my bad for missing that (please note the issue is assigned to v0.13.4 whereas the docs are in v0.13.2).

Let me make some final remarks from my pov:

We know that if a user can change something, then they can read it, that's how the Django admin does things.

Django admin uses the change permission because the admin interface is about changing objects. It makes sense there. The GET request on a REST API by definition is about reading/viewing. I would think that most devs quite simply don't expect DjangoAuthorization to refuse reading based on a missing permission to change.

This version is more secure, because it forces the developer to think about what they're doing.

One of tastypie's advertised features is to provide reasonable defaults. Wouldn't it be quite reasonable to assume that an API's GET (by definition: read) and PUT (change) methods, being different operations in all intent and purpose, also require different permissions?

I'll be happy to contribute a PR along the lines of what I wrote if you guys think it's a valuable addition.

[miraculixx]

The change permission is coming from Django itself. (...) The option view isn't.

there is a pending PR in Django to add a view permission which is why I used view.

[SeanHayes]

We're not going to allow public/global read operations by default. That's final. And we're not going to try and guess how developers have their permissions set up when there's not currently any standard way to do so. We're not even sure whether to call it "read" or "view", and I don't want a bunch of people coming in here saying "I do it this way" or "the new Django release calls it something else".

If and when Django supports a read/view permission out of the box we'll switch to that. For now developers are just going to have to write some custom code to handle the custom way they handle permissions.

[OOPMan]

I know this issue is closed but I just want to chime in and say that this change has basically blocked me from migrating an existing project to 0.13.x.

@miraculixx I think you're entirely correct with regards to the fact that requiring change permissions simply to view data is a little crazy. I guess we can blame Django for somehow, still, not having the concept of a view permission (Which blows my mine on a whole other level, I think it's insane that a project including a CRUD admin component simply doesn't have a permission for the READ part of CRUD).

[spanezz]

When you make production-breaking changes, can you at least reflect that in versioning?
Tastypie's x.y.z versioning looks very much like what most people expect, that usually is:

Given a version number MAJOR.MINOR.PATCH, increment the:
..
PATCH version when you make backwards-compatible bug fixes.

I just spent a very uncomfortable hour tracing this during a new production deployment.