feat: add support to multiple authentication#901
Conversation
|
@yomaaf is attempting to deploy a commit to the sparckles Team on Vercel. A member of the Team first needs to authorize it. |
CodSpeed Performance ReportMerging #901 will not alter performanceComparing Summary
Benchmarks breakdown
|
|
Hey @yomaaf 👋 Thanks for the PR. However, I do not understand the problem you are addressing here. What do you mean by "multiple authentication" and why is it needed in Robyn? |
Hi @sansyrox. "Multiple authentication" refers to the capability of a system to support different methods or mechanisms for verifying the identity of a user. This can include a variety of authentication methods such as:
Why is it needed in Robyn?
Addressing fina-joy's Issue #708 |
|
Hey @yomaaf 👋 There is only one authentication handler in Robyn so the users can override the implementation. I believe your PR will serve well as a Robyn Plugin like the following - https://robyn.tech/documentation/plugins However, I don't know enough about auth and need some time to think about it. I will get back to you after the weekend 😄 Meanwhile, if you have any items to help me with my research, I'd highly appreciate them. Thanks again for all the hard work :D |
07f28de to
0b766c9
Compare
|
|
|
|
||
| class BasicAuthHandler(AuthenticationHandler): | ||
| def authenticate(self, request: Request) -> Optional[Identity]: | ||
| username, password = self.token_getter.get_credentials(request) |
There was a problem hiding this comment.
The code attempts to call get_credentials() on BasicGetter, but the BasicGetter class only implements get_token() and does not have a get_credentials() method. This will result in an AttributeError when the /sync/auth/basic or /async/auth/basic endpoints are accessed.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| def get_credentials(cls, request: Request) -> Union[Tuple[str, str], Tuple[None, None]]: | ||
| basic_token = cls.get_token(request) | ||
| try: | ||
| basic_token_decoded = b64decode(basic_token).decode() |
There was a problem hiding this comment.
The get_credentials method in BasicGetter attempts to decode the base64 token without first checking if basic_token is None. This will raise a TypeError when basic_token is None (which can happen when get_token() returns None). According to Python's base64 documentation, b64decode requires a non-None input. The code should check if basic_token is None before attempting to decode it.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| request.headers["Authorization"] = f"Basic {token}" | ||
|
|
||
| @classmethod | ||
| def get_credentials(cls, request: Request) -> Union[Tuple[str, str], Tuple[None, None]]: |
There was a problem hiding this comment.
The code introduces a potentially dangerous pattern by adding get_credentials as a method to BasicGetter without properly implementing it in BearerGetter. Since get_credentials is a newly added method in the TokenGetter base class (an abstract class), all derived classes including BearerGetter must implement it. Currently, BearerGetter does not implement this method, which will lead to AttributeError at runtime when trying to use BearerGetter.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 3 issues. Time to roll up your sleeves! 😱 |
5fb08e0 to
0472e42
Compare
sansyrox
left a comment
There was a problem hiding this comment.
Thanks @yomaaf — multiple named auth handlers is a feature I want, and the one-default validation in auth_handler_configured() plus the new BasicGetter are nice touches. Two things before this can land: first, name is currently a required positional on AuthenticationHandler.__init__, which breaks every existing Handler(token_getter=...) call — can we default it (e.g. to the class name) so it's backward compatible? Second, main has drifted: add_auth_middleware now takes (endpoint, route_type) for method-aware middleware and the route decorators gained openapi_name/openapi_tags, so this needs a rebase to apply. A clearer error when auth_middleware_name doesn't match any registered handler would also help. The test coverage across basic/bearer is great — once it's rebased and the constructor stays compatible, I'm happy to take it.
Description
This PR fixes #708
Hello @sansyrox. This is a split PR for support for multiple authentication, however it does not include support for subrouter. Because, as previously explained. When decorating is called, it registers the route. So the authentication handler is not registered on the subrouter because the subrouter already register the route before the authentication handler is registered. I'll include support for the subrouter in the nested router PR. But it will require this PR merge first. So check out this PR first.