Update GET_IS_AUTHENTICATED_PREDICATE #84
Conversation
logston
force-pushed
the
logston/update-custom-authentication
branch
from
b2bab4c
to
255d091
Compare
| if not hasattr(request, 'user') or not get_is_authenticated_predicate()(request.user): | ||
| if not get_is_authenticated_predicate()(request): | ||
| if request.path.startswith(self.reverse_url): |
There was a problem hiding this comment.
These tests should happen in the other order, to reduce unnecessary extra work for all the other endpoints in the project.
For that matter, though, why does SCIM even need to have a middleware? I would think it’d be enough to just check authentication in a decorator used on all the SCIM endpoints. Then we wouldn’t need to slow down the rest of the project at all.
There was a problem hiding this comment.
For that matter, though, why does SCIM even need to have a middleware?
This project was built under specific constraints and having a middleware fit those constraints. PR's are welcome if you'd like to speed things up.
There was a problem hiding this comment.
These tests should happen in the other order, to reduce unnecessary extra work for all the other endpoints in the project.
Not sure I follow here. For the default predicate, not hasattr(request, 'user') or request.user.is_annoymous is faster than the request.path.startswith. But this is Python and web framework so we are talking microseconds compared to multiple millseconds of a request. I think clarity of code makes more sense in these situations. IMO, the failing soon for non-SCIM specific reasons makes more sense to me.
There was a problem hiding this comment.
This project was built under specific constraints and having a middleware fit those constraints. PR's are welcome if you'd like to speed things up.
Okay, here’s one proposal: we could move SCIMAuthCheckMiddleware out of the global project settings and apply it directly to SCIMView.dispatch. This fixes the performance issue on non-SCIM views with minimal restructuring.
There was a problem hiding this comment.
Not sure I follow here. For the default predicate,
not hasattr(request, 'user') or request.user.is_annoymousis faster than therequest.path.startswith.
No, it isn’t. request.user is a lazy object. Forcing its evaluation will typically involve a round trip to a database server or at least a cache server. On some endpoints that would have happened anyway, but on others it would not.
logston
force-pushed
the
logston/update-custom-authentication
branch
from
255d091
to
3409849
Compare
This commit updates the default GET_IS_AUTHENTICATED_PREDICATE function to accept a request object rather than a user object. Usages are updated accordingly.
logston
force-pushed
the
logston/update-custom-authentication
branch
from
3409849
to
f9c4d4a
Compare
This commit updates the default GET_IS_AUTHENTICATED_PREDICATE function
to accept a request object rather than a user object. Usages are updated
accordingly.
Closes #78