Conversation
|
@igorbenav please have a look, I think the pattern looks OK, but perhaps is trusting fastapi-sso too much?? l do like the use of the OpenID object though, but for example there is a bug in the microsoft provider that will require a PR in fastapi-sso (I will do that soon). |
…user password i null/None
src/app/api/v1/oauth.py
Outdated
| """ | ||
| if not oauth_user.email: | ||
| raise UnauthorizedException(f"Invalid response from {self.provider_name.title()} OAuth.") | ||
| username = oauth_user.email.split("@")[0] |
There was a problem hiding this comment.
Isn't there any sort of collision handling here?
| router.include_router(tasks_router) | ||
| router.include_router(tiers_router) | ||
| router.include_router(rate_limits_router) | ||
| router.include_router(users_router) |
There was a problem hiding this comment.
Any reason for this order change?
There was a problem hiding this comment.
alphabetical order
src/app/api/v1/oauth.py
Outdated
| } | ||
|
|
||
|
|
||
| class GithubSSOProvider(BaseOAuthProvider): |
There was a problem hiding this comment.
Why is github the only one named with "SSO" instead of OAuth?
There was a problem hiding this comment.
not really, fixed it by renaming SSO to OAuth
src/app/api/v1/oauth.py
Outdated
| ) | ||
| return access_token | ||
|
|
||
| async def _login_handler(self): |
igorbenav
left a comment
igorbenav
left a comment
There was a problem hiding this comment.
Just a few things that need addressing or answering, plus remove inline comments please (you can use docstrings to document decisions if necessary).
Good PR overall, thanks
|
@igorbenav done |
3 participants
This is a WIP because there are some bugs in the underlying fastapi-sso library and there is still missing code in this PR to address robustness, since oauth has to be quite air tight there are a log of exceptions to consider.
For the moment this draft PR adds the patterns to create an oauth provider to the application.
Google, GitHub oauth should be working as is, I tested it.