Is it good practice to access the request from a model manager?

[Antonis Christofides]

Hello,

The problem

It has happened to me more than once to need to check the user's permissions before allowing access to an object. Rather than worry whether I've remembered to do that in all my views and in all pieces of code there might be access, I'd prefer to make this check at the model level, that is, for the model manager to return filtered results—only these objects that the user is allowed to access. But model managers don't have access to the request object (and, likewise, I don't want to create a manager that needs the request object to be passed to it, because likewise I won't be certain I remembered to pass the request object in all places in the code).

One solution

One solution that I've implemented in an app is to have middleware that stores the request in a thread local variable. The model manager gets the request from there and filters the results accordingly. If there is no request object stored in the thread local variable, the manager assumes this query does not come from the web and does not filter it.

Is this a good way to do it? Is there any alternative?

Regards,

A.

-- 
Antonis Christofides
http://djangodeployment.com

[Melvyn Sopacua]

On Thursday 09 March 2017 12:51:26 Antonis Christofides wrote:

 

> One solution that I've implemented in an app is to have middleware

> that stores the request in a thread local variable. The model manager

> gets the request from there and filters the results accordingly. If

> there is no request object stored in the thread local variable, the

> manager assumes this query does not come from the web and does not

> filter it.

>

> Is this a good way to do it? Is there any alternative?

 

So you've rebranded your problem from "remembering to do this in views" to "remembering you've used the right model manager".

The only way to solve it, is to use a default-deny authentication backend, where the view needs to whitelist permissions. That way never anything leaks and your memory is no longer a factor in the problem.

But....it assumes one url handles one object, which isn't the case in most projects.

 

The obvious reason for not tying a model to a request, is that you cannot obtain instances without a request (management commands, among which data migrations).

 

You have to remember something somewhere and tests are the place to assert you did (if you remember to write a test for it ...).

--

Melvyn Sopacua

[Antonis Christofides]

So you've rebranded your problem from "remembering to do this in views" to "remembering you've used the right model manager".

No, because I do this in the default manager, "objects", and I create a new manager, "all_objects", for the exceptional case I actually want to get all objects in a view, without checking permissions.

The obvious reason for not tying a model to a request, is that you cannot obtain instances without a request (management commands, among which data migrations).

Of course you can. I explained it in my original message: "If there is no request object stored in the thread local variable, the manager assumes this query does not come from the web and does not filter it."

tests are the place to assert you did (if you remember to write a test for it ...).

Exactly, "if you remember to write a test". Unit tests can help, but when you do the same kind of permissions checking 10 times in 10 different places of the app, it's really hard to know that you failed to check edge case XYZ.

Antonis Christofides
http://djangodeployment.com

--
You received this message because you are subscribed to the Google Groups "Django users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-users...@googlegroups.com.
To post to this group, send email to django...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-users.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-users/1570464.pXdMm1KXXq%40devstation.
For more options, visit https://groups.google.com/d/optout.

[Daniel Hepper]

I think it is a good idea to do the permission check on the manager level. I would prefer a more explicit approach, e.g. something like this:

MyModel.objects.for_user(request.user).all()

MyModel.objects.for_all_users().all()

MyModel.objects.all() -> raises Exception

This way you don't need a middleware, have less global state and you keep your manager decoupled from the request. It's a bit more typing though :)

Cheers,

Daniel Hepper
https://consideratecode.com

[Melvyn Sopacua]

On Thursday 09 March 2017 14:36:55 Antonis Christofides wrote:

 

> Exactly, "if you remember to write a test". Unit tests can help, but

> when you do the same kind of permissions checking 10 times in 10

> different places of the app, it's really hard to know that you failed

> to check edge case XYZ.

 

This sounds more and more like you haven't got a good inheritance chain for your CBV's.

Take a look at Django Guardian: it has a different method for fetching objects a user has access to. If you inject that in a class that overrides SingleObjectMixin.get_object() and it's counterpart MultipleObjectMixin.get_queryset(), you can setup an inheritance chain that does all the work for you.

But without some code examples for those edge cases and their view inheritance, it's hard to predict and it may be comfy to have a 2nd line of defence at the manager level.

 

--

Melvyn Sopacua

[Antonis Christofides]

MyModel.objects.for_user(request.user).all()

MyModel.objects.for_all_users().all()

MyModel.objects.all() -> raises Exception

I like this idea.

    def get_queryset(self):
        result = super(MyManager, self).get_queryset()
        try:
            if self.user is None:
                return result
            else:
                return result.filter(user=self.user)
        except AttributeError:
            raise SomeError("Specify user")

However, I'm worried about the rest of the Django code (or of third-party apps), which might assume that the manager's get_queryset() method is supposed to work (without needing to do something else first). I think that the decision to have get_queryset() raise an exception (unless something else is done first) is a modification of the Django API, which will lead to trouble.

Antonis Christofides
http://djangodeployment.com

To view this discussion on the web visit https://groups.google.com/d/msgid/django-users/27fd4278-d439-49e3-8217-9b129aff3a84%40googlegroups.com.

[Antonis Christofides]

Let's talk with a concrete (real but simplified) example. There's a database of documents. Some documents are publicly available, whereas some are available only to logged-on users.

The "document list" view shows all documents if you're logged-on, and a subset if not.

The "document detail" view will return 404 if the document is private and you're not logged on.

The "document download" view will do the same.

Edge case: Besides documents, the database also contains projects. The "project detail" view displays among other things this note: "25 reports are available for this project". The "project detail" view definitely has very little to do with the "document detail" view and it is a different view, and the programmer forgets to include the mix-in. The number displayed may be wrong (this is relatively harmless but it illustrates the problem).

By implementing what I proposed, I solve this problem once and for all, regardless what functionality will be developed in the future and whether the programmer who develops that functionality is trying to cure a broken heart with hard work after a sleepless night of heavy drinking.

I'm playing the devil's advocate here. Many people seem to dislike my proposal, and I'm trying to understand why.

Antonis Christofides
http://djangodeployment.com

--

You received this message because you are subscribed to the Google Groups "Django users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-users...@googlegroups.com.
To post to this group, send email to django...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-users.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-users/2599535.PjUGXTrGZr%40devstation.