[fix] Fix AttributeError for filter classes #229 #230
Conversation
purhan
force-pushed
the
issues/229-allow-filter-classes-without-auth
branch
from
11aa4a1
to
c71362a
Compare
purhan
force-pushed
the
issues/229-allow-filter-classes-without-auth
branch
from
c71362a
to
b6e3efc
Compare
There was a problem hiding this comment.
Great @purhan, see my comments below.
2 questions:
- is it possible to add 1 test which fails without the
IsAuthenticatedpermission? Like the failure we had in ow-controller - can you please update the docs in the README to mention
IsAuthenticatedis shipped by default in these classes, since it requires the user to be authenticated to work.
tests/testapp/urls.py
Outdated
| @@ -37,4 +37,14 @@ | |||
| path( | |||
| 'owner/shelf', views.shelf_list_owner_view, name='test_shelf_list_owner_view', | |||
| ), | |||
| path( | |||
| 'owner/shelf/<str:shelf_id>/books', | |||
There was a problem hiding this comment.
there's a missing trailing slash here
There was a problem hiding this comment.
All other paths in this file don't have a trailing slash, should I just add it to all?
openwisp_users/api/mixins.py
Outdated
| @@ -17,6 +18,9 @@ def _user_attr(self): | |||
|
|
|||
| organization_lookup = 'organization__in' | |||
|
|
|||
| def get_permissions(self): | |||
| return super().get_permissions() + [IsAuthenticated()] | |||
There was a problem hiding this comment.
what about defining it in permission_classes? Eg:
permission_classes = [IsAuthenticated]This way we can override it if needed: we may need a custom IsAuthenticated or anything else, in that case we can override the behavior, while with this code overriding will be harder.
What do you think?
There was a problem hiding this comment.
My idea was to make it robust so that the developers wouldn't have to worry about adding this permission class separately while creating views. But sure, we can do this instead.
For the time being, I'm keeping it simple as you said:
permission_classes = [IsAuthenticated]Also, I had this other idea, we could check if the user is authenticated, manually:
# ...
if not (request.user and request.user.is_authenticated):
raise NotFound()
# ...Let me know if this is a better alternative.
This comment has been minimized.
This comment has been minimized.
purhan
moved this from In progress
to Ready for review/testing
in OpenWISP Priorities for next releases
There was a problem hiding this comment.
Thanks @purhan, I'm pushing some improvements. See also my comments below.
|
|
||
| def test_unauthorized_user_failing(self): | ||
| del FilterByOrganization.permission_classes | ||
| del FilterByParent.permission_classes |
There was a problem hiding this comment.
I think I didn't explain myself properly. This is not required.
The test above is enough, we just have to ensure it fails without the changes to the code in the PR.
I tried this and it works, here's how to do it in this case:
- comment out
permission_classesin the filter classes - run tests
- expect the test above to fail with "AttributeError: 'AnonymousUser' object has no attribute 'organizations_dict'"
If it fails, it means the test is correct, if it doesn't it mean the test is missing something.
I confirm the test above is correct, this one below is not necessary and I'm removing it.
Fixes #229