Rejecting anonymous in DjangoModelPermissions *before* the get_queryset call

[theoden-dd]

Description

There can be a situation when get_queryset uses request.user to determine a queryset filtration to apply. If DjangoModelPermissions has authenticated_users_only equal to True, one could assume that there'll be no anonymous in the get_queryset. However, that was not the case before this PR.

Before

There were workarounds which you had to apply if you wanted to avoid anonymous inside the get_queryset method:

1. Either do a manually check like

   if request.user.is_anonymous():
       return Model.objects.None()

   in every concerned viewset
2. or add the IsAuthenticated permission to every concerned viewset.

Both seem to be reasonably redundant.

After

No additional tuning is required. Having authenticated_users_only equal to True is enough.

[rpkilby]

Hi @theoden-dd. I'm not familiar with this permissions class, but off the cuff, this issue seems valid. DjangoModelPermissions can ensure that anonymous users are not permitted, so the call to get_queryset() can be seen as breaking expectations.

That all said, my general advice is to always include a test case that demonstrates the desired behavior.

• It shows that there is indeed a bug that needs to be fixed.
• It prevents future changes from accidentally reintroducing this bug.

[rpkilby]

Are the test changes necessary? The changes to DjangoModelPermissions seems like it wouldn't affect existing behavior.

[theoden-dd]

Unfortunately, yes. It was a surprise for me, too.

These tests became broken, since they used anonymous users (accidentally, I think) and started to retrieve "403 forbidden" instead of "405 not allowed". That's connected with the priority of permission checks, which is higher than most of other checks.

So, all I did - just changed users to authenticated ones in these tests.

[rpkilby]

Will need to review it, but thanks for the explanation.

[rpkilby]

So, it is possible to retain the existing behavior by moving the HTTP method check out of get_required_permissions() and into has_permission() before the request.user check.

That said, I think the test changes are appropriate, as I'd argue that users shouldn't really be relying on the permissions class to perform 405 checks. 405 checking should happen earlier during dispatch.

[theoden-dd]

Hi @rpkilby .

my general advice is to always include a test case that demonstrates the desired behavior

I totally agree and understand the reasons, but I missed that requirement while making a patch and had no time left when finished. If that's an absolute requirement, just say it - I'll add it later on.

[rpkilby]

If that's an absolute requirement, just say it

Nope, just advice. Requirements for tests are more or less flexible given the scope of changes, although I always prefer to add them because...

• It prevents future changes from accidentally reintroducing this bug.

😉

[rpkilby]

Hey @theoden-dd - closing this in favor of the updated PR that has a failing test + your commit. Thanks!