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

Authorization example is misleading. #826

Closed
vinodc opened this Issue Feb 19, 2013 · 4 comments

Comments

Projects
None yet
5 participants
@vinodc

vinodc commented Feb 19, 2013

The example at http://django-tastypie.readthedocs.org/en/latest/authorization.html#implementing-your-own-authorization shows that a boolean value (False) returned on *_detail methods is sufficient to indicate the user is not authorized to access the resource. However, the boolean value is ignored in practice and the code must raise an Unauthorized exception to prevent access to the detail view (in read_detail, for example).

@orion

This comment has been minimized.

Show comment
Hide comment
@orion

orion Feb 19, 2013

Agreed. What is the boolean value for if not to indicate whether access is allowed or denied?

orion commented Feb 19, 2013

Agreed. What is the boolean value for if not to indicate whether access is allowed or denied?

@nikmolnar

This comment has been minimized.

Show comment
Hide comment
@nikmolnar

nikmolnar Feb 21, 2013

Contributor

I ran into this as well. In rewriting my Authorization classes after upgrading to v0.9.12, I at first wrote my functions something like:

def read_detail(self, object_list, bundle):
    return user_is_authorized(bundle.request.user, bundle.obj)

But then I noticed that one part of the docs mentioned that read_detail should raise Unauthorized(), even though the example suggests otherwise. I quick look at the code confirmed that the value returned by *_detail is in fact ignored, meaning that these methods must raise Unauthorized() in order to prevent the action. I then rewrote my functions so something like:

def read_detail(self, object_list, bundle):
    if user_is_authorized(bundle.request.user, bundle.obj):
        return True
    raise Unauthorized()

Am I understanding the issue correctly?

Contributor

nikmolnar commented Feb 21, 2013

I ran into this as well. In rewriting my Authorization classes after upgrading to v0.9.12, I at first wrote my functions something like:

def read_detail(self, object_list, bundle):
    return user_is_authorized(bundle.request.user, bundle.obj)

But then I noticed that one part of the docs mentioned that read_detail should raise Unauthorized(), even though the example suggests otherwise. I quick look at the code confirmed that the value returned by *_detail is in fact ignored, meaning that these methods must raise Unauthorized() in order to prevent the action. I then rewrote my functions so something like:

def read_detail(self, object_list, bundle):
    if user_is_authorized(bundle.request.user, bundle.obj):
        return True
    raise Unauthorized()

Am I understanding the issue correctly?

@toastdriven

This comment has been minimized.

Show comment
Hide comment
@toastdriven

toastdriven Feb 21, 2013

Contributor

You all are correct, the docs are incorrect (https://github.com/toastdriven/django-tastypie/blob/master/tastypie/resources.py#L597-L607). An earlier version of Authorization that I was working on used booleans, but it caused a lot of duplicate & painful code, so I moved purely to exceptions or returning True. The docs will need to be updated to match.

Returning True isn't required, but makes writing unittests easier (explicit success rather than a potential silent failure), hence that recommendation in the docs.

Contributor

toastdriven commented Feb 21, 2013

You all are correct, the docs are incorrect (https://github.com/toastdriven/django-tastypie/blob/master/tastypie/resources.py#L597-L607). An earlier version of Authorization that I was working on used booleans, but it caused a lot of duplicate & painful code, so I moved purely to exceptions or returning True. The docs will need to be updated to match.

Returning True isn't required, but makes writing unittests easier (explicit success rather than a potential silent failure), hence that recommendation in the docs.

@gingerlime

This comment has been minimized.

Show comment
Hide comment
@gingerlime

gingerlime Mar 19, 2013

Contributor

I believe this is now fixed in 0.9.13, see 2dff249.

Both returning False or throwing an Unauthorized() exception from *_detail methods should block the request as unauthorized.

Contributor

gingerlime commented Mar 19, 2013

I believe this is now fixed in 0.9.13, see 2dff249.

Both returning False or throwing an Unauthorized() exception from *_detail methods should block the request as unauthorized.

@gingerlime gingerlime closed this Mar 19, 2013

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