feat!: utilize async lock to overcome dangerous race conditions #189
Conversation
tomasvotava
added
bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #189 +/- ##
==========================================
- Coverage 94.82% 93.15% -1.67%
==========================================
Files 20 20
Lines 483 526 +43
==========================================
+ Hits 458 490 +32
- Misses 25 36 +11
|
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update enhances the FastAPI SSO library by transitioning to asynchronous context management for handling Single Sign-On (SSO) processes, addressing potential race conditions that could lead to security issues. Key changes include the use of Changes
Assessment against linked issues
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: 3
Outside diff range, codebase verification and nitpick comments (4)
tests/test_providers_individual.py (1)
2-2: Stylistic Improvement: Added blank line after imports.The addition of a blank line enhances readability but has no impact on functionality.
tests/test_race_condition.py (1)
36-36: Annotate mutable class attribute withClassVar.The
post_responsesattribute should be annotated withClassVarto indicate it is a class-level attribute.- post_responses = [] # list of the responses which a client will return for the `POST` requests + post_responses: ClassVar[List[Response]] = [] # list of the responses which a client will return for the `POST` requestsTools
Ruff
36-36: Mutable class attributes should be annotated with
typing.ClassVar(RUF012)
tests/test_providers.py (2)
158-159: Remove useless expression.The expression
sso.oauth_clientis not used and can be removed.- with pytest.warns(SecurityWarning, match="Please make sure you are using SSO provider in an async context"): - sso.oauth_clientTools
Ruff
159-159: Found useless expression. Either assign it to a variable or remove it.
(B018)
160-160: Avoid hardcoded sensitive information.The
_refresh_tokenis being set to a hardcoded value. Consider using a mock or fixture for testing purposes.- sso._refresh_token = "test" + sso._refresh_token = mock_refresh_token()def mock_refresh_token(): return "mocked_refresh_token"Tools
Ruff
160-160: Possible hardcoded password assigned to: "_refresh_token"
(S105)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
Files selected for processing (30)
- .gitignore (1 hunks)
- CONTRIBUTING.md (1 hunks)
- docs/how-to-guides/additional-query-params.md (1 hunks)
- docs/how-to-guides/additional-scopes.md (1 hunks)
- docs/how-to-guides/redirect-uri-request-time.md (1 hunks)
- docs/how-to-guides/state-return-url.md (1 hunks)
- docs/how-to-guides/use-with-fastapi-security.md (2 hunks)
- docs/tutorials.md (1 hunks)
- examples/facebook.py (1 hunks)
- examples/fitbit.py (2 hunks)
- examples/generic.py (2 hunks)
- examples/github.py (2 hunks)
- examples/gitlab.py (2 hunks)
- examples/google.py (1 hunks)
- examples/kakao.py (2 hunks)
- examples/line.py (2 hunks)
- examples/linkedin.py (2 hunks)
- examples/microsoft.py (2 hunks)
- examples/naver.py (2 hunks)
- examples/notion.py (2 hunks)
- examples/twitter.py (2 hunks)
- examples/yandex.py (2 hunks)
- fastapi_sso/sso/base.py (12 hunks)
- pyproject.toml (2 hunks)
- tests/test_base.py (3 hunks)
- tests/test_openid_responses.py (3 hunks)
- tests/test_providers.py (7 hunks)
- tests/test_providers_individual.py (1 hunks)
- tests/test_race_condition.py (1 hunks)
- tox.ini (1 hunks)
Additional context used
Ruff
tests/test_base.py
60-60: Found useless expression. Either assign it to a variable or remove it.
(B018)
tests/test_race_condition.py
36-36: Mutable class attributes should be annotated with
typing.ClassVar(RUF012)
tests/test_providers.py
118-118: Argument name
Providershould be lowercase(N803)
123-123: Argument name
Providershould be lowercase(N803)
128-128: Argument name
Providershould be lowercase(N803)
154-154: Argument name
Providershould be lowercase(N803)
159-159: Found useless expression. Either assign it to a variable or remove it.
(B018)
160-160: Possible hardcoded password assigned to: "_refresh_token"
(S105)
GitHub Check: codecov/patch
fastapi_sso/sso/base.py
[warning] 78-78: fastapi_sso/sso/base.py#L78
Added line #L78 was not covered by tests
[warning] 405-405: fastapi_sso/sso/base.py#L405
Added line #L405 was not covered by tests
Additional comments not posted (62)
tox.ini (3)
4-4: Ensure the availability of all interpreters.Changing
skip_missing_interpreterstofalsewill cause the environment to fail if any specified Python interpreters are missing. This enhances robustness but requires all interpreters to be available.Ensure that all specified Python interpreters (
py38,py39,py310,py311,py312) are available in your testing environment to avoid failures.
11-12: Clarify dependency management approach.Switching from
poetry installtopoetry exportfollowed bypip installallows for more explicit dependency management. This can improve reproducibility but requires ensuring that therequirements.txtfile is up-to-date and correctly reflects the project's dependencies.Ensure that the
requirements.txtfile is generated correctly and includes all necessary dependencies for the project.
15-19: Direct tool invocation enhances modularity.The change from using
poetry runto direct invocation of tools likeruff,black,mypy,pytest, andcoverageenhances modularity and control over the testing process. This approach allows for better customization and integration of each tool's features.docs/how-to-guides/redirect-uri-request-time.md (1)
14-14: Correct use of asynchronous context management.The transition to
async with google_ssoensures proper asynchronous resource management, aligning with best practices for handling I/O-bound tasks in web applications.examples/fitbit.py (2)
24-24: Correct use of asynchronous context management.The transition to
async with ssoinauth_initensures proper asynchronous handling of the SSO context, enhancing the responsiveness of the application.
31-31: Correct use of asynchronous context management.The transition to
async with ssoinauth_callbackensures proper asynchronous handling of the SSO context, enhancing the responsiveness of the application.docs/how-to-guides/additional-query-params.md (2)
14-14: Correct use of asynchronous context management.The transition to
async with google_ssoensures proper asynchronous resource management, aligning with best practices for handling I/O-bound tasks in web applications.
22-22: Correct use of asynchronous context management.The transition to
async with google_ssoingoogle_callbackensures proper asynchronous handling of the SSO context, enhancing the responsiveness of the application.examples/github.py (2)
24-24: Correct use of asynchronous context management.The transition to
async with ssoinauth_initensures proper asynchronous handling of the SSO context, enhancing the responsiveness of the application.
31-31: Correct use of asynchronous context management.The transition to
async with ssoinauth_callbackensures proper asynchronous handling of the SSO context, enhancing the responsiveness of the application.examples/yandex.py (2)
24-25: Correct use ofasync withfor non-blocking operations.The transition to
async with sso:ensures that the login redirect process is handled asynchronously, which is crucial for performance in web applications using FastAPI.
31-33: Appropriate use ofasync withfor asynchronous verification.Using
async with sso:in theauth_callbackfunction allows the verification process to be non-blocking, improving the application's ability to handle concurrent requests efficiently.examples/notion.py (2)
24-25: Adoption ofasync withfor asynchronous context management.The use of
async with sso:in theauth_initfunction is a necessary change for handling login redirects asynchronously, enhancing performance and scalability.
31-33: Effective use ofasync withfor non-blocking verification.The
async with sso:statement in theauth_callbackfunction ensures that the user verification process is handled asynchronously, which is essential for maintaining responsiveness in high-concurrency environments.examples/linkedin.py (2)
24-25: Implementation ofasync withfor asynchronous login handling.The change to
async with sso:inauth_initis appropriate for managing login redirects asynchronously, which is beneficial for application performance.
31-33: Use ofasync withfor asynchronous user verification.The
async with sso:inauth_callbackensures that the verification process is non-blocking, aligning with best practices for handling concurrent requests.examples/line.py (2)
24-26: Transition toasync withfor asynchronous login initialization.The use of
async with sso:inauth_initis a crucial change for handling login redirects in a non-blocking manner, improving scalability and performance.
31-34: Effective asynchronous context management for user verification.The
async with sso:inauth_callbackensures that the verification process is handled asynchronously, which is essential for maintaining application responsiveness.examples/twitter.py (2)
24-25: Appropriate use ofasync withfor asynchronous login handling.The transition to
async with sso:inauth_initensures that login redirects are processed in a non-blocking manner, which is crucial for performance in asynchronous environments.
31-33: Implementation ofasync withfor non-blocking user verification.Using
async with sso:inauth_callbackallows the verification process to be asynchronous, enhancing the application's ability to handle multiple concurrent requests.examples/kakao.py (2)
24-25: LGTM: Asynchronous context management inauth_init.The transition to
async with sso:allows for non-blocking operations, improving performance and responsiveness.
31-32: LGTM: Asynchronous context management inauth_callback.Using
async with sso:ensures that the verification and processing of requests are handled efficiently without blocking the event loop.examples/naver.py (2)
24-25: LGTM: Asynchronous context management inauth_init.The use of
async with sso:is appropriate for handling asynchronous operations in FastAPI.
31-32: LGTM: Asynchronous context management inauth_callback.This change supports better scalability and responsiveness by allowing other tasks to proceed while waiting for the SSO operations to complete.
examples/microsoft.py (2)
26-27: LGTM: Asynchronous context management inauth_init.The use of
async with sso:supports non-blocking operations, enhancing performance.
33-34: LGTM: Asynchronous context management inauth_callback.This change ensures efficient request handling without blocking the event loop.
examples/google.py (2)
24-25: LGTM: Asynchronous context management inauth_init.The transition to
async with sso:is suitable for handling non-blocking operations in FastAPI.
31-33: LGTM: Asynchronous context management inauth_callback.This change enhances the control flow and logic of the authentication process, promoting better scalability.
examples/facebook.py (2)
26-26: Good use ofasync withfor non-blocking context management.The transition to
async with ssoin theauth_initfunction ensures that the login redirection operation does not block the event loop, which is crucial for maintaining responsiveness in asynchronous applications.
33-33: Effective use ofasync withfor asynchronous verification.The use of
async with ssoin theauth_callbackfunction allows for non-blocking verification and processing of login requests, enhancing the application's ability to handle concurrent authentication processes efficiently.examples/gitlab.py (2)
26-26: Appropriate use ofasync withfor login initialization.The change to
async with ssoin theauth_initfunction is well-suited for asynchronous environments, preventing blocking during the login redirect process.
33-33: Correct application ofasync withfor login verification.The modification to use
async with ssoin theauth_callbackfunction supports non-blocking verification, which is essential for handling multiple login requests concurrently.docs/how-to-guides/state-return-url.md (2)
18-18: Excellent demonstration ofasync withfor login redirection.Using
async with google_ssoin thegoogle_loginfunction effectively illustrates how to manage asynchronous contexts, ensuring that login redirection does not block the event loop.
23-23: Clear example ofasync withfor request verification.The
async with google_ssoin thegoogle_callbackfunction is a good example of handling asynchronous verification, promoting efficient and responsive authentication workflows.docs/how-to-guides/additional-scopes.md (2)
17-17: Appropriate use ofasync withfor login redirection.The use of
async with ssoin thegoogle_loginfunction ensures that the login process is handled asynchronously, aligning with best practices for non-blocking operations.
22-22: Effective use ofasync withfor request processing and data access.The
async with ssoin thegoogle_callbackfunction, combined withhttpx.AsyncClient(), demonstrates a comprehensive approach to handling asynchronous operations, from authentication to data retrieval.examples/generic.py (2)
48-48: Good implementation ofasync withfor login URL generation.Using
async with ssoin thesso_loginfunction ensures that the login URL generation is non-blocking, which is important for maintaining application responsiveness.
55-55: Correct use ofasync withfor processing login responses.The change to
async with ssoin thesso_callbackfunction supports non-blocking processing of login responses, which is essential for handling multiple requests concurrently..gitignore (1)
1-5: LGTM! The.gitignoreupdates are appropriate.The addition of
requirements.txtand.python-versionto.gitignorealigns with modern dependency management practices usingpyproject.toml. This helps maintain a clean repository by excluding environment-specific files.docs/tutorials.md (1)
29-34: LGTM! Transition to async context management is beneficial.The switch to
async with google_ssoensures non-blocking operations, improving the application's ability to handle concurrent requests effectively. This aligns with best practices in modern web frameworks like FastAPI.pyproject.toml (1)
117-117: LGTM! Dependency updates are appropriate.The removal of
toxand addition oftyping-extensionsreflect a strategic refinement in dependency management, ensuring compatibility and potentially simplifying the testing process.tests/test_base.py (1)
4-8: LGTM! Security warnings enhance test robustness.The addition of
SecurityWarningensures that the SSO provider is used in an async context, which is critical for maintaining security and preventing race conditions.CONTRIBUTING.md (1)
Line range hint
1-67: LGTM! Simplified testing instructions are clear.The removal of
toxinstructions in favor of usingpoestreamlines the testing process, making it easier for contributors to follow a single method for running tests.tests/test_race_condition.py (3)
23-32: LGTM!The
Responseclass is a well-implemented mock for simulating HTTP responses with token data.
35-55: LGTM!The
AsyncClientclass effectively simulates an asynchronous HTTP client for testing purposes.Tools
Ruff
36-36: Mutable class attributes should be annotated with
typing.ClassVar(RUF012)
12-82: LGTM!The
test__race_conditionfunction effectively tests for race conditions by simulating concurrent logins and verifying token handling.Tools
Ruff
36-36: Mutable class attributes should be annotated with
typing.ClassVar(RUF012)
docs/how-to-guides/use-with-fastapi-security.md (2)
74-74: LGTM!The transition to
async within theloginfunction aligns with best practices for asynchronous operations.
89-89: LGTM!The transition to
async within thelogin_callbackfunction ensures proper asynchronous handling and enhances responsiveness.tests/test_openid_responses.py (1)
230-231: LGTM!The use of
async withintest_provider_openid_by_responseensures proper asynchronous context management.tests/test_providers.py (10)
Line range hint
73-77: LGTM!The use of
async withintest_discovery_documentensures proper asynchronous context management.Tools
Ruff
70-70: Wrong values type in
@pytest.mark.parametrizeexpectedlistoftupleUse
listoftuplefor parameter values(PT007)
71-71: Argument name
Providershould be lowercase(N803)
Line range hint
82-87: LGTM!The use of
async withintest_login_url_request_timeensures proper asynchronous context management.Tools
Ruff
80-80: Argument name
Providershould be lowercase(N803)
95-100: LGTM!The use of
async withintest_login_url_construction_timeensures proper asynchronous context management.
Line range hint
103-109: LGTM!The use of
async withinassert_get_login_url_and_redirectensures proper asynchronous context management.Tools
Ruff
92-92: Argument name
Providershould be lowercase(N803)
113-116: LGTM!The use of
async withintest_login_url_additional_paramsensures proper asynchronous context management.
120-121: LGTM!The use of
async withintest_login_url_state_at_request_timeensures proper asynchronous context management.
125-126: LGTM!The use of
async withintest_login_url_scope_defaultensures proper asynchronous context management.
130-131: LGTM!The use of
async withintest_login_url_scope_additionalensures proper asynchronous context management.
148-153: LGTM!The use of
async withintest_process_loginensures proper asynchronous context management.
154-162: LGTM!The use of
async withintest_context_manager_behaviorensures proper asynchronous context management.Tools
Ruff
154-154: Argument name
Providershould be lowercase(N803)
159-159: Found useless expression. Either assign it to a variable or remove it.
(B018)
160-160: Possible hardcoded password assigned to: "_refresh_token"
(S105)
fastapi_sso/sso/base.py (3)
75-88: LGTM!The
requires_async_contextdecorator effectively ensures methods are used within an async context, enhancing safety.Tools
GitHub Check: codecov/patch
[warning] 78-78: fastapi_sso/sso/base.py#L78
Added line #L78 was not covered by tests
119-120: LGTM!The introduction of
_login_lockand_in_stackeffectively manages concurrency and prevents race conditions.
421-442: LGTM!The
__aenter__and__aexit__methods provide robust async context management, ensuring safe resource handling.
| with pytest.raises(NotImplementedError): | ||
| with pytest.raises(NotImplementedError), pytest.warns( | ||
| SecurityWarning, match="Please make sure you are using SSO provider in an async context" | ||
| ): | ||
| sso.oauth_client |
There was a problem hiding this comment.
Remove the useless expression.
There's a useless expression that should either be assigned to a variable or removed to clean up the code.
- sso.oauth_clientCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sso.oauth_client |
Tools
Ruff
60-60: Found useless expression. Either assign it to a variable or remove it.
(B018)
| def requires_async_context(func: Callable[P, T]) -> Callable[P, T]: | ||
| def wrapper(*args: P.args, **kwargs: P.kwargs) -> T: | ||
| if not args or not isinstance(args[0], SSOBase): | ||
| return func(*args, **kwargs) |
There was a problem hiding this comment.
Consider adding test coverage.
The line where args is checked for its first element being an instance of SSOBase is not covered by tests.
Would you like assistance in creating a test case to cover this scenario?
Tools
GitHub Check: codecov/patch
[warning] 78-78: fastapi_sso/sso/base.py#L78
Added line #L78 was not covered by tests
| warnings.warn( | ||
| "SSO Providers are supposed to be used in async context, please change 'with provider' to " | ||
| "'async with provider'. See https://github.com/tomasvotava/fastapi-sso/issues/186 for more information.", | ||
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) |
There was a problem hiding this comment.
Consider adding test coverage.
The warning for using SSO providers in a non-async context is not covered by tests.
Would you like assistance in creating a test case to cover this scenario?
Tools
GitHub Check: codecov/patch
[warning] 405-405: fastapi_sso/sso/base.py#L405
Added line #L405 was not covered by tests
Brought into light by @parikls in !186
tomasvotava
force-pushed
the
186-insecure-race-conditions
branch
from
6dfdd08
to
a8fc745
Compare
Brought into light by @parikls in #186
Closes #186
Summary by CodeRabbit
New Features
toxtopoefor running tests, streamlining the testing process.Bug Fixes
Documentation
CONTRIBUTING.mdto remove redundant test instructions and simplify contributor guidelines.Chores
toxdependency and includetyping-extensions.Tests