Feat: add auth subroutes capabilities #1056
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hey @IA-PieroCV 👋 Is the PR still a WIP? Or is it up for review? |
|
Hey @sansyrox ! |
| def default_secured(request : Request) -> str: | ||
| return "Authentication is set by include_router!" | ||
|
|
||
| app.include_router(sub_router, auth_required=True) |
There was a problem hiding this comment.
Hey @IA-PieroCV 👋
Thank you for the PR 😄 Overall I like the theme of the PR but will have to a review of the code.
However, why do we need an auth_required flag in include_router? Is it not the same as the SubRouter Class?
There was a problem hiding this comment.
Yes, actually it is!
This is just to give developers options for including auth_required due to their own criteria.
Of course we can remove any of the auth_required instance level parameters, from include_router or the SubRoute object instance.
However, include_router have higher precedence. My logic here is because developers face this method more often than the instantiation of the SubRoute. The same logic for endpoint decorators.
There was a problem hiding this comment.
@IA-PieroCV , apologies for the late revert. But, I think this is a redundancy. If someone wants to allow auth, they can do it the subrouter class. I'd suggest that we remove it from here.
Let me know if you have anymore thoughts 😄
|
|
||
| > **Important**: If any of these methods are used, the authentication for the endpoint is set to `False` unless explicitly overridden. | ||
| --- |
There was a problem hiding this comment.
I don't think we need this. Right?
| app.include_router(sub_router) | ||
| app.include_router(di_subrouter) | ||
| app.include_router(auth_subrouter_endpoint) | ||
| app.include_router(auth_subrouter_include, auth_required=True) | ||
| app.include_router(auth_subrouter_instance) | ||
| app.include_router(auth_subrouter_include_false, auth_required=False) | ||
| app.include_router(auth_subrouter_include_true, auth_required=True) |
There was a problem hiding this comment.
@IA-PieroCV , I believe auth_required should either be removed from include_router or the SubRouter() class
There was a problem hiding this comment.
@sansyrox @IA-PieroCV
i also think that auth_required should be in SubRouter than in include_router
I was also thinking to create new PR to provide auth_required functionality for all routes under a single router
so user just don't have to set auth_required to every end point, if they want then they can explicitly override the default auth_required setting set by the router, on endpoint itself
There was a problem hiding this comment.
I really like all the test cases!!
| def inner_handler(request: Request, *args): | ||
| if not self.authentication_handler: | ||
| raise AuthenticationNotConfiguredError() | ||
| identity = self.authentication_handler.authenticate(request) | ||
| if identity is None: | ||
| return self.authentication_handler.unauthorized_response | ||
| request.identity = identity | ||
| return request # Proceed to the next middleware or handler | ||
|
|
||
| self.add_route( | ||
| MiddlewareType.BEFORE_REQUEST, | ||
| endpoint, | ||
| inner_handler, | ||
| injected_dependencies, | ||
| ) |
There was a problem hiding this comment.
Hey @IA-PieroCV 👋
Thank you for the PR 😄
Overall the changes look great. I just have some suggestions/comments 😄
Merry shipmas! 🎄
sansyrox
force-pushed
the
main
branch
5 times, most recently
from
07f28de to
0b766c9
Compare
|
|
1 similar comment
|
|
|
✨ No issues found! Your code is sparkling clean! ✨ |
3 participants
Description
This PR fixes #708
Summary
This PR does handle authentication for subroutes having different levels of precedence:
include_routerhas the second highest precedence.SubRouteinstance has the lowest precedence.False.This PR change auth logic:
auth_requiredparameter forRouteobjects.Several integration tests were provided. MDX documentation was also updated. To be discussed on #1026
PR Checklist
Please ensure that:
Pre-Commit Instructions: