New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply_limits giving false sense of security? #332

Closed
gingerlime opened this Issue Jan 17, 2012 · 15 comments

Comments

Projects
None yet
10 participants
@gingerlime
Contributor

gingerlime commented Jan 17, 2012

I've been looking into the apply_limits authorization mechanism. This is a very powerful authorization method in Tastypie, and a great feature I really like. However, I felt like I was either trusting it too much, or just not paying enough attention (although I couldn't find anything in particular I was missing in the docs or elsewhere).

apply_limits works great for read access (via GET) to resources, by adding an optional filter to the queryset. It also seems to work well with related resources, and DELETEs.

The problem lies with POST/PUT requests.

From what I gathered, and please correct me if I'm wrong, the apply_limits is not used at all with obj_create. If you understand django querysets etc, you could expect this. When a new object is created and saved, there's no direct use for the filter method, and there's no object_list... (well, not as such). This is the first and main limitation I see currently. It might be (or probably is?) by-design, but it still seems like a potential blind-spot for API developers using and trusting apply_limits to also work on POSTs.

The second, related issue, is with PUT. Whilst obj_update does use the apply_limits method (via the obj_get method), if the apply_limits, erm, applies limits, and e.g. returns an empty list of objects, put_detail will fall back to obj_create - which as mentioned will not apply any such limits. Whilst it won't let you update the object you expected to update, it will create a new object for you.

Is this by-design? Should the documentation highlight this more clearly? As I see it, it might give a false sense of security and leave APIs open to authorization issues they don't necessarily anticipate.

If this is not desired, then I wonder what's the best approach to resolve this in a generic way? I was thinking that maybe adding an apply_write_limits method can/should be added specifically for object creates/updates (e.g. allowing to override certain fields with different values), or perhaps the current validation methods are sufficient?

@toastdriven

This comment has been minimized.

Show comment
Hide comment
@toastdriven

toastdriven Jan 17, 2012

Contributor

I've been working on a new API implementation, one where there are lots of users using the API & while GET access should be very open, there's lots of private data & writes should be carefully guarded. To be quite honest, I have many of the same concerns (blind-spots & actually considering it kind of weak in actual use).

Your first concern (POST to obj_create) is a known deficiency & you're correct in that it's non-obvious to the developer (but generally also doesn't usually affect them).

Your second concern (PUT falling back to obj_create) is actually mostly by-design, though I'm increasingly unhappy with it. It's suggested for REST architectures that, if the object being PUT to doesn't exist, it should create the object instead - http://en.wikipedia.org/wiki/REST#RESTful_web_services. In this case, one interpretation is that there is(are) no object(s) the user has access that meet that condition, so falling back to create is the correct thing to do. But when I put myself in those circumstances, it no longer feels like the right course of action.

I think the "fix" is rather easy but need to stew on it. There'd be two steps to change I'm contemplating.

The first is to move the self.is_authorized() check to a later point in cycle, to when we actually have access to a potential object (already existing or about to be saved). That method takes an optional object parameter, which confuses everyone. I feel like that method could/should be used for fine-grained (per-object checks) & given extra information about what the action about to be taken is.

The second is alter the PUT to be able to tell the difference between objects-that-exist-that-you-can't-touch & no-objects-so-create-away. The trick is doing this efficiently without triggering an extra query. I'm currently wondering if, since that's where the self.is_authorized check would move to, if that doesn't cover it. If it does, there's no need for the apply_limits at all, since you would just codify that into the other method.

Long story short, I'm actively thinking on this & agree that some change is needed. I'll experiment a bit & will try to develop a way to address the existing shortcomings.

Contributor

toastdriven commented Jan 17, 2012

I've been working on a new API implementation, one where there are lots of users using the API & while GET access should be very open, there's lots of private data & writes should be carefully guarded. To be quite honest, I have many of the same concerns (blind-spots & actually considering it kind of weak in actual use).

Your first concern (POST to obj_create) is a known deficiency & you're correct in that it's non-obvious to the developer (but generally also doesn't usually affect them).

Your second concern (PUT falling back to obj_create) is actually mostly by-design, though I'm increasingly unhappy with it. It's suggested for REST architectures that, if the object being PUT to doesn't exist, it should create the object instead - http://en.wikipedia.org/wiki/REST#RESTful_web_services. In this case, one interpretation is that there is(are) no object(s) the user has access that meet that condition, so falling back to create is the correct thing to do. But when I put myself in those circumstances, it no longer feels like the right course of action.

I think the "fix" is rather easy but need to stew on it. There'd be two steps to change I'm contemplating.

The first is to move the self.is_authorized() check to a later point in cycle, to when we actually have access to a potential object (already existing or about to be saved). That method takes an optional object parameter, which confuses everyone. I feel like that method could/should be used for fine-grained (per-object checks) & given extra information about what the action about to be taken is.

The second is alter the PUT to be able to tell the difference between objects-that-exist-that-you-can't-touch & no-objects-so-create-away. The trick is doing this efficiently without triggering an extra query. I'm currently wondering if, since that's where the self.is_authorized check would move to, if that doesn't cover it. If it does, there's no need for the apply_limits at all, since you would just codify that into the other method.

Long story short, I'm actively thinking on this & agree that some change is needed. I'll experiment a bit & will try to develop a way to address the existing shortcomings.

@gingerlime

This comment has been minimized.

Show comment
Hide comment
@gingerlime

gingerlime Jan 18, 2012

Contributor

Thanks for the detailed info @toastdriven

I think you're right that the "fix" could be rather easy, and I also agree it needs some time to work out the details to do it right. I'd like help if possible with the design (and maybe coding, although I'm not that familiar with the tastypie codebase as other contributors).

Taking a slightly more generic/theoretic view of authorization/access-control, an authorization-decision can usually be described as a function that takes 3 parameters and returns a yes/no answer. The parameters are: subject (who is the entity requesting access - e.g. a user account/identifier), object (what is the resource being accessed) and the action requested (e.g. what data I want to read/write). In tastypie's is_authorized, both the action and the subject are encapsulated in the request, and the object is, as you stated, optional - and doesn't seem to be used within the framework itself. The apply_limits call does get an object_list. However, as we see, apply_limits is not always called, and it seems to be more read-only centric.

I think it would therefore be nice if authorization functions accept 3 parameters:

  1. subject (can be the get_identifier value or function);
  2. object(s) - the bundle / object list or single object; and
  3. the request object - from which people can extract what this request is about, i.e. the action.

I do realise the subject can also be extracted manually from the request (and that in 90% of the time it is request.user), so there is some duplication, but doing it explicitly makes designing authorization decisions easier and clearer.

merging authorization into a single call might not be the best solution however. is_authorized and apply_limits operate in slightly different ways, return different data, and they both have merit. apply_limits is already hugely useful for GET/DELETE and can also be useful for PUT/POST. Simply put, it's the logical place to override/mangle any request-provided data with safer alternatives rather than rejecting the whole request. I therefore lean towards either adding a different method for POST/PUT (e.g. apply_write_limits) passing it the bundle data before any save() or even before full_hydrate, or perhaps using the same apply_limits and instead of passing an object_list, passing in the bundle. The latter will not be particularly backwards-compatible though and might break existing implementations that (usually) assume the object_list is a queryset, but it is perhaps more elegant overall.

Apologies for writing so much, but I tried to explain my thinking currently. Perhaps it's best if I try to make push some code changes instead, as the change could be rather minimal (but requires this kind of explanation I guess).

Contributor

gingerlime commented Jan 18, 2012

Thanks for the detailed info @toastdriven

I think you're right that the "fix" could be rather easy, and I also agree it needs some time to work out the details to do it right. I'd like help if possible with the design (and maybe coding, although I'm not that familiar with the tastypie codebase as other contributors).

Taking a slightly more generic/theoretic view of authorization/access-control, an authorization-decision can usually be described as a function that takes 3 parameters and returns a yes/no answer. The parameters are: subject (who is the entity requesting access - e.g. a user account/identifier), object (what is the resource being accessed) and the action requested (e.g. what data I want to read/write). In tastypie's is_authorized, both the action and the subject are encapsulated in the request, and the object is, as you stated, optional - and doesn't seem to be used within the framework itself. The apply_limits call does get an object_list. However, as we see, apply_limits is not always called, and it seems to be more read-only centric.

I think it would therefore be nice if authorization functions accept 3 parameters:

  1. subject (can be the get_identifier value or function);
  2. object(s) - the bundle / object list or single object; and
  3. the request object - from which people can extract what this request is about, i.e. the action.

I do realise the subject can also be extracted manually from the request (and that in 90% of the time it is request.user), so there is some duplication, but doing it explicitly makes designing authorization decisions easier and clearer.

merging authorization into a single call might not be the best solution however. is_authorized and apply_limits operate in slightly different ways, return different data, and they both have merit. apply_limits is already hugely useful for GET/DELETE and can also be useful for PUT/POST. Simply put, it's the logical place to override/mangle any request-provided data with safer alternatives rather than rejecting the whole request. I therefore lean towards either adding a different method for POST/PUT (e.g. apply_write_limits) passing it the bundle data before any save() or even before full_hydrate, or perhaps using the same apply_limits and instead of passing an object_list, passing in the bundle. The latter will not be particularly backwards-compatible though and might break existing implementations that (usually) assume the object_list is a queryset, but it is perhaps more elegant overall.

Apologies for writing so much, but I tried to explain my thinking currently. Perhaps it's best if I try to make push some code changes instead, as the change could be rather minimal (but requires this kind of explanation I guess).

gingerlime added a commit to gingerlime/django-tastypie that referenced this issue Jan 18, 2012

@gingerlime

This comment has been minimized.

Show comment
Hide comment
@gingerlime

gingerlime Jan 18, 2012

Contributor

see commit above for very quick & rough changes to authorization as mentioned above. All tests seem to pass. However any app that relies on the fact that object_list is a queryset inside apply_limits will likely fail when a bundle is provided instead.

Contributor

gingerlime commented Jan 18, 2012

see commit above for very quick & rough changes to authorization as mentioned above. All tests seem to pass. However any app that relies on the fact that object_list is a queryset inside apply_limits will likely fail when a bundle is provided instead.

@toastdriven

This comment has been minimized.

Show comment
Hide comment
@toastdriven

toastdriven Feb 14, 2012

Contributor

I took a slightly different approach, though including those fundamental pieces on information in each. I think it's a vast improvement, but before I commit to it, I'm curious to get some feedback. Commit is at a498621.

Contributor

toastdriven commented Feb 14, 2012

I took a slightly different approach, though including those fundamental pieces on information in each. I think it's a vast improvement, but before I commit to it, I'm curious to get some feedback. Commit is at a498621.

@hvdklauw

This comment has been minimized.

Show comment
Hide comment
@hvdklauw

hvdklauw Feb 29, 2012

I think it's a vast improvement of the current system.

However would I want even more finegrained control, how about you are allowed to change/delete single items but not delete them all through the list option?

Basically the request_method and request_type as used in: https://github.com/toastdriven/django-tastypie/blob/a4986212955ac177710ad7716e878ec23b631d21/tastypie/resources.py#L419

hvdklauw commented Feb 29, 2012

I think it's a vast improvement of the current system.

However would I want even more finegrained control, how about you are allowed to change/delete single items but not delete them all through the list option?

Basically the request_method and request_type as used in: https://github.com/toastdriven/django-tastypie/blob/a4986212955ac177710ad7716e878ec23b631d21/tastypie/resources.py#L419

@akoumjian

This comment has been minimized.

Show comment
Hide comment
@akoumjian

akoumjian Apr 24, 2012

master...permsL6L1969

Something about the related_resource or the related_bundle, or perhaps the save() method is incorrect here. When trying to save the 'parent' of an m2m object, I am getting an error where it claims that the parent does not have a field which refers to itself (from the child).

Here is the traceback: https://gist.github.com/4c607ffcccce3f38784b

Essentially, I have a Collection resource with a CollectionMembership resource as an m2m. While trying to save the first collectionmembership, it attempts to save the parent Collection as well. While trying to save the parent collection, it hiccups here: https://github.com/toastdriven/django-tastypie/blob/perms/tastypie/resources.py#L2032

akoumjian commented Apr 24, 2012

master...permsL6L1969

Something about the related_resource or the related_bundle, or perhaps the save() method is incorrect here. When trying to save the 'parent' of an m2m object, I am getting an error where it claims that the parent does not have a field which refers to itself (from the child).

Here is the traceback: https://gist.github.com/4c607ffcccce3f38784b

Essentially, I have a Collection resource with a CollectionMembership resource as an m2m. While trying to save the first collectionmembership, it attempts to save the parent Collection as well. While trying to save the parent collection, it hiccups here: https://github.com/toastdriven/django-tastypie/blob/perms/tastypie/resources.py#L2032

@achur

This comment has been minimized.

Show comment
Hide comment
@achur

achur Aug 8, 2012

Contributor

It should also be noted that apply_limits also can't secure DELETEs in many use-cases, as a PUT can overwrite an existing object (#507).

  • User A creates object 1 with POST
  • User B does a PUT to object 1's uri (which fails back to obj_create, since object 1 is filtered by apply_limits)
  • User B does a DELETE on object 1's uri

Now, user B has accomplished the equivalent of a DELETE on object 1, even though it is filtered out by apply_limits.

Contributor

achur commented Aug 8, 2012

It should also be noted that apply_limits also can't secure DELETEs in many use-cases, as a PUT can overwrite an existing object (#507).

  • User A creates object 1 with POST
  • User B does a PUT to object 1's uri (which fails back to obj_create, since object 1 is filtered by apply_limits)
  • User B does a DELETE on object 1's uri

Now, user B has accomplished the equivalent of a DELETE on object 1, even though it is filtered out by apply_limits.

@JustinSGray

This comment has been minimized.

Show comment
Hide comment
@JustinSGray

JustinSGray Aug 13, 2012

I have tested out the code on this commit:

a498621

I've found that it does indeed give me the necessary control over auth to do per-object authentication. There are a few challenges with this approach that I have not fully vetted out yet though:

  1. using this method, you're exposed a bit more to attacks, since your authorization check does not happen until after the appropriate bundle has been created. This is exactly what you need if you want to do per-object authentication, but a problem if you have some more fundamental checks that you can/should perform before the DB gets hit. There are two solutions to this that I see. The first is the authentication setup, where you ensure you know who the request it coming from. Bot's are pretty good about sigining up for accounts though. The other option is to provide an authorization along the way of the current mainline method as a first check, and the more granular setup as second.

  2. I ran into an issue where unauthorized requests were generating empty instances of my models. I had some unicode methods that were erring out and causing unauthorized actions to be allowed through. I'm still trying to figure out where in the code that was happening. But empty models probably shouldn't make it through to the model layer.

Is this branch still being considered? I can't really use tastypie without this change.

JustinSGray commented Aug 13, 2012

I have tested out the code on this commit:

a498621

I've found that it does indeed give me the necessary control over auth to do per-object authentication. There are a few challenges with this approach that I have not fully vetted out yet though:

  1. using this method, you're exposed a bit more to attacks, since your authorization check does not happen until after the appropriate bundle has been created. This is exactly what you need if you want to do per-object authentication, but a problem if you have some more fundamental checks that you can/should perform before the DB gets hit. There are two solutions to this that I see. The first is the authentication setup, where you ensure you know who the request it coming from. Bot's are pretty good about sigining up for accounts though. The other option is to provide an authorization along the way of the current mainline method as a first check, and the more granular setup as second.

  2. I ran into an issue where unauthorized requests were generating empty instances of my models. I had some unicode methods that were erring out and causing unauthorized actions to be allowed through. I'm still trying to figure out where in the code that was happening. But empty models probably shouldn't make it through to the model layer.

Is this branch still being considered? I can't really use tastypie without this change.

@tadeck

This comment has been minimized.

Show comment
Hide comment
@tadeck

tadeck Aug 13, 2012

I am also interested in the state of perms branch and permission checks in general. Any progress in it? Or we should consider it as final / almost final?

tadeck commented Aug 13, 2012

I am also interested in the state of perms branch and permission checks in general. Any progress in it? Or we should consider it as final / almost final?

@graingert

This comment has been minimized.

Show comment
Hide comment
@graingert

graingert Aug 16, 2012

Contributor

bump/subscribe on status of "perms" this looks exactly like what's needed

Contributor

graingert commented Aug 16, 2012

bump/subscribe on status of "perms" this looks exactly like what's needed

@philwo

This comment has been minimized.

Show comment
Hide comment
@philwo

philwo Aug 20, 2012

I just stumbled upon this issue, too. Could someone of the core committers comment on the state of this issue?

philwo commented Aug 20, 2012

I just stumbled upon this issue, too. Could someone of the core committers comment on the state of this issue?

@joshbohde

This comment has been minimized.

Show comment
Hide comment
@joshbohde

joshbohde Aug 20, 2012

Contributor

This requires a pretty big change of the Resource API, and will be present in the next release. When the branch is merged into master, this ticket will be updated.

Contributor

joshbohde commented Aug 20, 2012

This requires a pretty big change of the Resource API, and will be present in the next release. When the branch is merged into master, this ticket will be updated.

@graingert

This comment has been minimized.

Show comment
Hide comment
@graingert

graingert Aug 20, 2012

Contributor

What's the ETA on the next release?

Contributor

graingert commented Aug 20, 2012

What's the ETA on the next release?

@joshbohde

This comment has been minimized.

Show comment
Hide comment
@joshbohde

joshbohde Aug 20, 2012

Contributor

It's not the best answer, but whenever the milestone in the issues is reached: https://github.com/toastdriven/django-tastypie/issues?milestone=5&page=1&state=open

Contributor

joshbohde commented Aug 20, 2012

It's not the best answer, but whenever the milestone in the issues is reached: https://github.com/toastdriven/django-tastypie/issues?milestone=5&page=1&state=open

@toastdriven

This comment has been minimized.

Show comment
Hide comment
@toastdriven

toastdriven Feb 12, 2013

Contributor

Fixed as of SHA: d850758

Release within the next several days.

Contributor

toastdriven commented Feb 12, 2013

Fixed as of SHA: d850758

Release within the next several days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment