added Seznam SSO provider #194
Conversation
WalkthroughThe pull request introduces the Seznam Single Sign-On (SSO) integration into a FastAPI application. It updates the Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- README.md (1 hunks)
- examples/seznam.py (1 hunks)
- fastapi_sso/sso/seznam.py (1 hunks)
- tests/test_providers.py (2 hunks)
Additional comments not posted (7)
examples/seznam.py (1)
23-27: LGTM!The
auth_initfunction is implemented correctly and follows the expected flow for initiating the Seznam authentication process. It uses theget_login_redirectmethod of theSeznamSSOinstance to handle the redirection logic and is decorated with the appropriate endpoint.fastapi_sso/sso/seznam.py (3)
18-23: LGTM!The class attributes are correctly defined:
provideris set to "seznam".base_urlis specified for API interactions.scopeoutlines the permissions requested during the OAuth flow.
25-31: LGTM!The
get_discovery_documentmethod correctly constructs a dictionary containing URLs for authorization, token exchange, and user information retrieval based on the definedbase_url.
33-43: LGTM!The
openid_from_responsemethod correctly processes the response from Seznam after user authentication, extracting user details such as email, first name, last name, display name, user ID, and avatar URL. It returns anOpenIDobject populated with the extracted user information.README.md (1)
79-79: LGTM!The new entry for the Seznam login provider is consistent with the format used for other contributed providers. It provides relevant information and credits the contributor appropriately.
tests/test_providers.py (2)
27-27: LGTM!The import statement for
SeznamSSOis correctly written and is necessary to include it in the list of tested providers.
55-55: LGTM!Adding
SeznamSSOto thetested_providerstuple ensures that all the tests defined in this file are run against theSeznamSSOprovider. This change is consistent with the objective of expanding the test coverage for the new SSO provider.
examples/seznam.py
Outdated
| @app.get("/auth/callback") | ||
| async def auth_callback(request: Request): | ||
| """Verify login""" | ||
| with sso: | ||
| user = await sso.verify_and_process(request, params={"client_secret": CLIENT_SECRET}) | ||
| return user |
There was a problem hiding this comment.
Verify the login implementation and consider filtering sensitive information.
The auth_callback function is implemented correctly and follows the expected flow for verifying the login credentials. It uses the verify_and_process method of the SeznamSSO instance to process the request and is decorated with the appropriate endpoint.
However, passing the client_secret as a parameter to the verify_and_process method and directly returning the user object could potentially expose sensitive information. Consider filtering out any sensitive fields from the user object before returning it to the client.
There was a problem hiding this comment.
Hi @TomasKoutek and thanks for the contribution! I've attached a single comment, may I ask you to either explain it to me or change it if it's not necessary?
Nenapadlo by mě, že bude poptávka po Seznam SSO! 😁 Díky!
examples/seznam.py
Outdated
| async def auth_callback(request: Request): | ||
| """Verify login""" | ||
| with sso: | ||
| user = await sso.verify_and_process(request, params={"client_secret": CLIENT_SECRET}) |
There was a problem hiding this comment.
Is passing the client secret explicitly in params neccessary? I would guess oauthlib adds the client secret to the request uri when doing prepare_token_request in verify_login method of SSOBase 🤔 If this is specific to Seznam, I'd probably also add it to documentation, or maybe emphasize it in the comment here so that users who come to documented examples actually notice that this is some specialty :)
There was a problem hiding this comment.
Yes, unfortunately it is needed. It took me a while to figure it out, but it's also described in the documentation - "2. Převod kódu na token" contains a "client_secret" field. Without it, Seznam API returns the response "{'error': 'invalid_client'}".
I added a comment to the example - 3be5d2e
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #194 +/- ##
==========================================
- Coverage 94.82% 94.80% -0.03%
==========================================
Files 20 21 +1
Lines 483 500 +17
==========================================
+ Hits 458 474 +16
- Misses 25 26 +1
|
|
Why is that so? I don't think I've ever seen this, black, isort, ruff, flake, they all seem to make the imports more concise, this is probably the first time I see the tendency to do otherwise :) |
It's probably just personal preference and it's pycharm's default behavior (ctrl+o). |
Added support for authentication using Seznam.cz.
Docs: https://vyvojari.seznam.cz/oauth/doc
Summary by CodeRabbit
New Features
Documentation
Tests