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

Already on GitHub? Sign in to your account

Add option to use custom responses for authorization exceptions. #626

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
7 participants

In the main branch, it's possible to return custom output for authorization problems while processing a request, which is very useful for e.g. authorization as an OAuth provider and needing to return certain headers as required by the OAuth spec. This is missing with the enhanced authorization hooks in the perms branch but can be added back relatively easily.

Contributor

gingerlime commented on a498621 Feb 16, 2012

From a rather shallow look through the changes it looks good. I like the approach you took separating different actions into different authorization methods. The only thing I'm not too sure about is whether it's possible/sensible to do something similar with apply_limits, but I guess that getting the authorization foundation first (i.e. authorized / not-authorized binary decisions) is more important. Some clever apply_limits functionality can still get added by the user app if they want to.

One thing I couldn't quite work out (hope it's not a dumb question) - when you, for example, call authorized_to_change and pass in the bundle, the bundle does not actually include the object, i.e. bundle.obj is None. I'm guessing it's safer not to hydrate the object before the authorization decision is made, but wouldn't it make it difficult to do a row-level authorization without the actual object? (or the user app needs to effectively hydrate the object)??

Contributor

toastdriven replied Feb 16, 2012

I'm not sure altering/duplicating apply_limits to the non-read types helps, since as you pointed out before, they aren't really using the ORM in those circumstances. I think doing the boolean check for those types makes the most sense.

As for the bundle getting passed to the authorized_to_* methods, it will almost always have an obj (only exceptions are the delete_list/detail & get_schema methods. Before it's in master, the delete_list/detail will have it too, at the expense of not being quite as efficient). That bundle.obj may be unsaved (like in the case of POST), but everything should at least be there for the user to check authorization on it.

Contributor

gingerlime replied Feb 20, 2012

Thanks @toastdriven !

Haven't had a chance to play with the code yet, but in principle as I already said I think it looks good and I do like the approach.

Re apply_limits - I agree, just that it might create some false expectations for less-experienced django/tastypie users (I'm just thinking they might expect that the same magic that limits the result-set for GET/DELETE will somehow happen for POST/PUT if they use it inside apply_limits). I guess that's still the user's responsibility after all, and nothing really stops them from creating their own apply_write_limits or any other function to achieve the same thing (e.g. via hydrate_x).

Don't know what I was thinking about the bundle.obj. I see that it does indeed get set correctly.

I am definitely getting some kind of error where bundle.obj is None during the single PUT/change requests inside of to_change. My guess is that something is going awry in update_in_place. Will try to debug.

@toastdriven , so I am not sure what the expected behavior is, but when put_detail is called to update a resource, the bundle that it passes to self.authorized_to_change contains a generic object of the same class as the data, but not the actual obj from the database.

This prevents you from doing object-level permission checks inside of to_change because bundle.obj is an empty instance of the _meta.object_class.

Is this intentional, or should the user be able to do permission checks against bundle.obj?

Contributor

toastdriven replied Feb 28, 2012

@akoumjian That's an error, as it should be checking bundle.obj. It likely needs to get pushed down into obj_update, except that might cause issues for people with overrides. It'll be corrected before this branch gets merged.

Thanks. Feel free to ping me if you need testers since this feature will be immensely useful for us.

Contributor

dericcrago commented on 0991975 Mar 6, 2012

line 1992 returns unauthorized on resources with ReadOnlyAuthorization

This pull request fails (merged 39dbedc into c4e7d86).

@alexleigh alexleigh closed this Jan 8, 2013

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