feat: #842 - oauth2 with discord #847
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Code Coverage SummaryDiff against mainResults for commit: a99a16a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
dazy-ds
requested changes
Oct 16, 2025
| ) | ||
|
|
||
| def setup_routes(self) -> None: | ||
| def setup_routes(self) -> None: # noqa: PLR0915 |
There was a problem hiding this comment.
I think this should be refactored (extract the methods). This function grows too big and may be to complex in the future.
| content={"success": False, "error_message": "Internal server error"}, | ||
| ) | ||
|
|
||
| async def _handle_oauth2_callback( # noqa: PLR6301 |
There was a problem hiding this comment.
This method feels overly complex for its purpose. Generating dynamic HTML directly in the backend isn’t ideal, especially since we have a full SPA handling user flows.
I’d suggest re-architecting this so the backend only validates the code and state, then redirects to a frontend route such as /login?code=.... The frontend can then exchange that code via a dedicated API endpoint to obtain the token.
Alternatively, we could consider using an HTTP-only cookie for storing the session token, which would improve security compared to localStorage.
If we decide to keep the dynamic HTML approach for now, it would be good to at least apply consistent styling so it aligns visually with the rest of the application.
Comment on lines
+608
to
+629
| async def authenticate_with_oauth2(self, oauth_credentials: OAuth2Credentials) -> AuthenticationResponse: | ||
| """ | ||
| Try to authenticate with OAuth2 using all OAuth2 backends. | ||
| Args: | ||
| oauth_credentials: OAuth2 credentials | ||
| Returns: | ||
| AuthenticationResponse from the first successful backend | ||
| """ | ||
| errors = [] | ||
|
|
||
| for backend in self.get_oauth2_backends(): | ||
| result = await backend.authenticate_with_oauth2(oauth_credentials) | ||
| if result.success: | ||
| return result | ||
| if result.error_message: | ||
| errors.append(result.error_message) | ||
|
|
||
| # All backends failed | ||
| error_msg = "; ".join(errors) if errors else "OAuth2 authentication failed" | ||
| return AuthenticationResponse(success=False, error_message=error_msg) |
There was a problem hiding this comment.
The naming may be confusing me (provider vs backend, as they seem to be used interchangeably), but as far as I understand the code, we do have a provider name from the callback URL. Instead of iterating over each of the backends/providers we can instead pass it directly.
This class could implement the registry pattern and register handlers for each of the provider with example syntax: instance.registerHandler('discord', DiscordOAuth2Provider)
Comment on lines
+565
to
+575
| # Use the first backend's JWT configuration | ||
| first_backend = backends[0] | ||
| if hasattr(first_backend, "jwt_secret"): | ||
| self.jwt_secret = first_backend.jwt_secret | ||
| self.jwt_algorithm = first_backend.jwt_algorithm | ||
| self.token_expiry_minutes = first_backend.token_expiry_minutes | ||
| else: | ||
| # Fallback if first backend doesn't have JWT config | ||
| self.jwt_secret = get_secret_key() | ||
| self.jwt_algorithm = default_options.jwt_algorithm | ||
| self.token_expiry_minutes = default_options.token_expiry_minutes |
There was a problem hiding this comment.
I think we shouldn’t naively use the first backend’s JWT configuration, since each backend may have its own (and perhaps should).
It might be better to dynamically select the correct backend and use its validate_token implementation, for example by routing based on a provider claim in the JWT.
Comment on lines
+81
to
+100
| provider === "discord" && ( | ||
| <svg | ||
| width="20" | ||
| height="20" | ||
| viewBox="0 0 71 55" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| > | ||
| <g clipPath="url(#clip0)"> | ||
| <path | ||
| d="M60.1045 4.8978C55.5792 2.8214 50.7265 1.2916 45.6527 0.41542C45.5603 0.39851 45.468 0.440769 45.4204 0.525289C44.7963 1.6353 44.105 3.0834 43.6209 4.2216C38.1637 3.4046 32.7345 3.4046 27.3892 4.2216C26.905 3.0581 26.1886 1.6353 25.5617 0.525289C25.5141 0.443589 25.4218 0.40133 25.3294 0.41542C20.2584 1.2888 15.4057 2.8186 10.8776 4.8978C10.8384 4.9147 10.8048 4.9429 10.7825 4.9795C1.57795 18.7309 -0.943561 32.1443 0.293408 45.3914C0.299005 45.4562 0.335386 45.5182 0.385761 45.5576C6.45866 50.0174 12.3413 52.7249 18.1147 54.5195C18.2071 54.5477 18.305 54.5139 18.3638 54.4378C19.7295 52.5728 20.9469 50.6063 21.9907 48.5383C22.0523 48.4172 21.9935 48.2735 21.8676 48.2256C19.9366 47.4931 18.0979 46.6 16.3292 45.5858C16.1893 45.5041 16.1781 45.304 16.3068 45.2082C16.679 44.9293 17.0513 44.6391 17.4067 44.3461C17.471 44.2926 17.5606 44.2813 17.6362 44.3151C29.2558 49.6202 41.8354 49.6202 53.3179 44.3151C53.3935 44.2785 53.4831 44.2898 53.5502 44.3433C53.9057 44.6363 54.2779 44.9293 54.6529 45.2082C54.7816 45.304 54.7732 45.5041 54.6333 45.5858C52.8646 46.6197 51.0259 47.4931 49.0921 48.2228C48.9662 48.2707 48.9102 48.4172 48.9718 48.5383C50.038 50.6034 51.2554 52.5699 52.5959 54.435C52.6519 54.5139 52.7526 54.5477 52.845 54.5195C58.6464 52.7249 64.529 50.0174 70.6019 45.5576C70.6551 45.5182 70.6887 45.459 70.6943 45.3942C72.1747 30.0791 68.2147 16.7757 60.1968 4.9823C60.1772 4.9429 60.1437 4.9147 60.1045 4.8978ZM23.7259 37.3253C20.2276 37.3253 17.3451 34.1136 17.3451 30.1693C17.3451 26.225 20.1717 23.0133 23.7259 23.0133C27.308 23.0133 30.1626 26.2532 30.1066 30.1693C30.1066 34.1136 27.28 37.3253 23.7259 37.3253ZM47.3178 37.3253C43.8196 37.3253 40.9371 34.1136 40.9371 30.1693C40.9371 26.225 43.7636 23.0133 47.3178 23.0133C50.9 23.0133 53.7545 26.2532 53.6986 30.1693C53.6986 34.1136 50.9 37.3253 47.3178 37.3253Z" | ||
| fill="currentColor" | ||
| /> | ||
| </g> | ||
| <defs> | ||
| <clipPath id="clip0"> | ||
| <rect width="71" height="55" fill="white" /> | ||
| </clipPath> | ||
| </defs> | ||
| </svg> |
There was a problem hiding this comment.
This could grow in complexity when other providers arrive. We can extract this into some kind of map outside of component which would map provider -> icon
Comment on lines
+397
to
+414
| async with httpx.AsyncClient() as client: | ||
| response = await client.post( | ||
| self.provider.token_url, | ||
| data={ | ||
| "client_id": self.client_id, | ||
| "client_secret": self.client_secret, | ||
| "grant_type": "authorization_code", | ||
| "code": code, | ||
| "redirect_uri": self.redirect_uri, | ||
| }, | ||
| headers={"Content-Type": "application/x-www-form-urlencoded"}, | ||
| ) | ||
|
|
||
| if response.status_code != 200: # noqa: PLR2004 | ||
| return None | ||
|
|
||
| token_data = response.json() | ||
| return token_data.get("access_token") |
There was a problem hiding this comment.
I think the httpx.AsyncClient() should be able to be passed as a dependency (with reasonable default), either to this method or the whole class. This would allow users to reuse the default logic with ability to add some logging, etc. without redefining the whole function.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.