BaseHasAPIKey should not implement has_object_permission

[sparkyb]

https://github.com/florimondmanca/djangorestframework-api-key/blob/2a343eeaa32c616747e6aae3c4902f1ba55adeb9/src/rest_framework_api_key/permissions.py#L53-L56

Right now the permission BaseHasAPIKey implement has_object_permission and just calls has_permission. This is an incorrect implementation of BasePermission. has_object_permission is only called after has_permission has already passed, so this implementation is redundant (and inefficient since it will cause a redundant database lookup for the API key).

Also, the type of obj is annotated as AbstractAPIKey, but the object passed to has_object_permission would be an instance of whatever model the view is acting on where this permission is used, not the API key.

[florimondmanca]

Also, the type of obj is annotated as AbstractAPIKey, but the object passed to has_object_permission would be an instance of whatever model the view is acting on where this permission is used, not the API key.

Absolutely, sounds like a no brainer. I think a separate PR for this particular type hint tweak would be acceptable?

As for .has_object_permission() — I think your rationale makes sense. The current state of affairs comes from #21. Looks like at the time we added the default .has_object_permission() but OP's use case should have warranted them to create a custom HasAPIKey permission that happens to implement .has_object_permission() as we do currently (i.e. deferring to .has_permission()). Is this your analysis of the situation as well?

[sparkyb]

Ah, I did try to look why has_object_permission was implemented like that, but I failed to find that issue. That makes a lot of sense, and actually, though I haven't gotten there yet, I'm probably going to be doing something similar and will likely have the same issue. It does seem that what you suggest (omit has_object_permission and leave this workaround to a custom subclass for those who need it) is the right way to go based on the intent of BasePermission. This seems like it might be a common issue, but it is actually more of an issue for DRF itself. Any permission could have this problem when used with OR (for example, IsAdminUser|IsTheSameUser). I plan to report it to DRF because it really should be fixed there (either BasePermission could implement a default has_object_permission that delegates to has_permission like the workaround we have, or it could be handled in the OR class).

Any way it is handled, it's a shame that there's no good way to cache the result of has_permission so it doesn't have to do the DB lookup again. Since the request and view are parameters to the methods, not the constructor, it's not possible to save the result of has_permission on the permission class itself, and the same permission instance isn't reused anyway. Perhaps the looked up APIKey should be stored on the request? That would make checking has_permission a second time faster and would making getting the APIKey from within the view easier without a 2nd lookup.

[sparkyb]

Looks like there's already an issue for this against DRF https://github.com/encode/django-rest-framework/issues/7117 . It looks like there's a PR to fix it exactly as I would have suggested (in the OR class) which didn't make it into the recent 3.12 release, but hopefully should make it into the next one. So I'd say, yes, djangorestframework-api-key should remove this workaround and anyone who needs it prior to the upstream DRF fix should do it themselves.

And yes, a separate PR for fixing the type annotation would be a simple fix, but it is moot if the method gets removed.

[florimondmanca]

@sparkyb As I come back to this repo to move a few things forward, I noticed I had incorrectly assumed this was only an external DRF issue. There is a problem with how DRF handles bitwise permission operations, but we still make an inappropriate use of .has_object_permissions() by duplicating API key checking there on top of .has_permissions(). I was able to confirm in https://github.com/florimondmanca/djangorestframework-api-key/issues/173#issuecomment-1146677315 that this results in about 30-40% more time spent processing requests, on top of performing unnecessary CPU hash crunching.

I agree we should drop .has_object_permissions() and hint users to implement it in a custom permission class if they need it.

We basically need to undo #25*, and maybe add a note about https://github.com/encode/django-rest-framework/issues/7117 in the docs that mention bitwise operations. (* That "fix" is 3 years old. I'm thinking -- all this time, computers running rest_framework_api_key have been doing 2x more CPU work than they should have been doing. Uh!)

[florimondmanca]

The PR https://github.com/encode/django-rest-framework/issues/7117 was merged on the DRF side.

Now, bitwise OR checks for both .has_permission() and .has_object_permission(). Before this, and at the time of https://github.com/florimondmanca/djangorestframework-api-key/pull/25, DRF only called for .has_object_permission()which meant API key permissions (which only implemented.has_permission()` at the time) were not checked if used with a bitwise OR.

This does confirm that we can revert #25 and have folks rely on the new fix from the DRF.