fix(auth): remove incorrect request parameter from OAuth method calls #130
Conversation
Fixed a critical bug where framework implementations were passing a request parameter to OAuth.is_authenticated() and OAuth.get_user_info() methods that don't accept any parameters beyond self. Issue: - FastAPI and Flask framework routes were calling these methods with request - Methods were designed to get user context internally via framework.get_user_id() - This caused "takes 1 positional argument but 2 were given" errors - Authentication checks always failed, breaking the /user endpoint Changes: - kinde_fastapi/framework/fastapi_framework.py: Removed request parameter from is_authenticated() and get_user_info() calls in auto-registered routes - kinde_flask/framework/flask_framework.py: Removed request parameter from is_authenticated() and get_user_info() calls in auto-registered routes - kinde_sdk/auth/oauth.py: Updated docstrings to remove misleading documentation about non-existent request parameter Impact: - Fixes authentication checks for all FastAPI and Flask applications - /user endpoint now works correctly - No breaking changes - methods never used the parameter that was being passed - All dependent features (permissions, claims, feature flags) unaffected Resolves client-reported issue where authentication always showed as "not authenticated" and /user endpoint threw exceptions.
WalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
participant FastAPI as FastAPI handler
participant Flask as Flask handler
participant OAuth as OAuth module
rect rgb(240,248,255)
Note over FastAPI,OAuth: Updated flow — no request object passed
FastAPI->>OAuth: is_authenticated()
OAuth-->>FastAPI: bool (authenticated?)
alt not authenticated
FastAPI->>OAuth: login flow (redirect/init)
end
FastAPI->>OAuth: get_user_info()
OAuth-->>FastAPI: user info
end
sequenceDiagram
participant Flask as Flask handler
participant OAuth as OAuth module
rect rgb(240,248,255)
Note over Flask,OAuth: Updated flow — no request object passed
Flask->>OAuth: is_authenticated()
OAuth-->>Flask: bool
alt not authenticated
Flask->>OAuth: login flow (redirect/init)
end
Flask->>OAuth: get_user_info()
OAuth-->>Flask: user info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Look like it will fix the problem |
|
Can we add some tests to cover this? params have been removed and all tests are still passing unchanged, need to make sure this is covered |
Fixed a critical bug where framework implementations were passing a request parameter to OAuth.is_authenticated() and OAuth.get_user_info() methods that don't accept any parameters beyond self. Root Cause: - FastAPI and Flask framework routes were calling OAuth methods with a request parameter - These methods were designed to get user context internally via framework.get_user_id() - This caused "takes 1 positional argument but 2 were given" TypeError - Authentication checks always failed, breaking the /user endpoint and user flows Changes Made: - kinde_fastapi/framework/fastapi_framework.py: * Removed request parameter from is_authenticated() and get_user_info() calls * Removed unused Request parameter from get_current_user(), register(), and get_user() route handlers * Removed unused imports: Depends, Dict, Any - kinde_flask/framework/flask_framework.py: * Removed request parameter from is_authenticated() and get_user_info() calls * Removed unused imports: threading, logging, Session, Dict, Any - kinde_sdk/auth/oauth.py: * Updated docstrings to remove misleading documentation about non-existent request parameter * Removed unused imports: asyncio, List, Tuple, Union, urlparse, quote, GrantType, base64_url_encode - testv2/testv2_auth/test_oauth_method_signatures.py (NEW): * Added 8 comprehensive tests covering the bug fix * Tests verify method signatures don't accept parameters * Tests ensure TypeError is raised if parameters are passed incorrectly * Tests confirm framework routes are registered correctly Testing: - All 8 new tests pass - 200 existing tests continue to pass (no regressions) - Manual testing verified with python-starter-kit (Flask and FastAPI) - No linter errors Impact: - Fixes authentication for all FastAPI and Flask applications - /user endpoint now works correctly - No breaking changes - methods never used the parameter being passed - All dependent features (permissions, claims, feature flags) unaffected Resolves client-reported issue where authentication always showed as "not authenticated" and /user endpoint threw TypeError exceptions.
Added import checks for FastAPI and Flask to ensure the frameworks are available before proceeding. This change helps prevent runtime errors related to missing dependencies. Changes made: - In kinde_fastapi/framework/fastapi_framework.py: Added a line to check for FastAPI import. - In kinde_flask/framework/flask_framework.py: Added a line to check for Flask import. No functional changes were made to the existing methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
kinde_fastapi/framework/fastapi_framework.py (2)
122-128: Consider removing unused helper function.The
get_current_user()helper is defined but never invoked in the codebase. This appears to be dead code that can be removed.Would you like me to verify whether this function is used elsewhere in the codebase, or help generate a diff to remove it?
243-248: Minor: Consider alternative to no-op assignment.The
_ = fastapiassignment works but could be more explicit. Consider alternatives like checking a module attribute instead.Apply this diff for a clearer intent:
try: import fastapi - _ = fastapi # Import check only + return hasattr(fastapi, '__version__') - return True except ImportError: return FalseAlternatively, keep the current approach if you prefer simplicity—both work fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kinde_fastapi/framework/fastapi_framework.py(4 hunks)kinde_flask/framework/flask_framework.py(4 hunks)kinde_sdk/auth/oauth.py(1 hunks)testv2/testv2_auth/test_oauth_method_signatures.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kinde_sdk/auth/oauth.py
- kinde_flask/framework/flask_framework.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-03T09:38:46.175Z
Learnt from: brettchaldecott
Repo: kinde-oss/kinde-python-sdk PR: 83
File: kinde_flask/examples/example_app.py:25-44
Timestamp: 2025-07-03T09:38:46.175Z
Learning: The kinde_flask framework automatically creates and registers standard OAuth routes (/login, /logout, /callback, /register, /user) at runtime when the OAuth instance is initialized with a Flask app. These routes do not need to be manually defined in application code.
Applied to files:
kinde_fastapi/framework/fastapi_framework.py
🧬 Code graph analysis (2)
testv2/testv2_auth/test_oauth_method_signatures.py (3)
kinde_sdk/auth/oauth.py (3)
OAuth(23-634)is_authenticated(124-141)get_user_info(143-173)kinde_fastapi/framework/fastapi_framework.py (5)
start(54-67)stop(69-75)get_user_id(96-106)FastAPIFramework(17-248)get_name(36-43)kinde_flask/framework/flask_framework.py (5)
start(58-71)stop(73-79)get_user_id(100-110)get_name(40-47)FlaskFramework(14-262)
kinde_fastapi/framework/fastapi_framework.py (2)
kinde_sdk/auth/oauth.py (4)
is_authenticated(124-141)get_user_info(143-173)register(374-418)login(333-372)kinde_fastapi/storage/fastapi_storage.py (1)
get(20-36)
🪛 Ruff (0.14.3)
testv2/testv2_auth/test_oauth_method_signatures.py
38-38: Unused method argument: mock_storage_factory
(ARG002)
38-38: Unused method argument: mock_framework_factory
(ARG002)
55-55: Unused method argument: mock_storage_factory
(ARG002)
55-55: Unused method argument: mock_framework_factory
(ARG002)
212-212: Unused method argument: mock_middleware
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unittest (3.13)
- GitHub Check: unittest (3.9)
- GitHub Check: unittest (3.12)
- GitHub Check: unittest (3.11)
- GitHub Check: unittest (3.10)
🔇 Additional comments (8)
kinde_fastapi/framework/fastapi_framework.py (3)
1-2: LGTM! Import cleanup aligns with usage.The removal of unused imports (
Dict,Anyfrom typing andDependsfrom fastapi) is correct. The code no longer uses these types, andRequestis appropriately retained for routes that access the request object.
224-226: LGTM! Correctly removed unused parameter.The
requestparameter was never used in this route handler, andOAuth.register()doesn't accept it. The removal is correct and aligns with the method signature inoauth.py.
230-234: LGTM! This fixes the core authentication bug.The removal of the
requestparameter from both the route signature and the OAuth method calls directly addresses the reported issue. The OAuth methodsis_authenticated()andget_user_info()retrieve user context internally viaframework.get_user_id()and don't require a request parameter.testv2/testv2_auth/test_oauth_method_signatures.py (5)
1-34: LGTM! Well-structured test setup.The test module has a clear purpose statement, appropriate imports, and proper setup/teardown with environment variable mocking. This directly addresses the reviewer request for test coverage of the parameter fix.
38-68: LGTM! Signature validation prevents regression.These tests use
inspect.signatureto verify thatis_authenticated()andget_user_info()accept no parameters beyondself, directly preventing the regression of the bug where frameworks incorrectly passed request parameters.Note: The static analysis warnings about unused
mock_storage_factoryandmock_framework_factoryparameters are false positives—these are injected by the@patchdecorators and required in the signature.
73-133: LGTM! Functional tests validate parameterless invocation.These tests verify that the methods can be successfully invoked without parameters and exhibit correct behavior. The comprehensive mocking (framework, storage, OpenID config) ensures the tests accurately simulate the runtime environment.
138-201: LGTM! Parameter rejection tests ensure bug detection.These tests verify that passing extra parameters raises
TypeErrorwith a clear error message. This is crucial for catching regressions—if someone mistakenly adds back therequestparameter in framework code, these tests will immediately fail.
204-294: LGTM! Integration tests validate end-to-end fix.These integration tests verify that OAuth initialization and route registration complete successfully for both FastAPI and Flask frameworks. By ensuring no TypeError is raised during route registration, they confirm the parameter fix works in the actual framework context.
Note: The static analysis warning about unused
mock_middlewareis a false positive—it's injected by the@patchdecorator.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
testv2/testv2_auth/test_framework_route_handlers.py(1 hunks)testv2/testv2_auth/test_oauth_method_fix.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-03T09:38:46.175Z
Learnt from: brettchaldecott
Repo: kinde-oss/kinde-python-sdk PR: 83
File: kinde_flask/examples/example_app.py:25-44
Timestamp: 2025-07-03T09:38:46.175Z
Learning: The kinde_flask framework automatically creates and registers standard OAuth routes (/login, /logout, /callback, /register, /user) at runtime when the OAuth instance is initialized with a Flask app. These routes do not need to be manually defined in application code.
Applied to files:
testv2/testv2_auth/test_framework_route_handlers.py
🧬 Code graph analysis (2)
testv2/testv2_auth/test_framework_route_handlers.py (1)
kinde_sdk/auth/oauth.py (2)
OAuth(23-634)get_user_info(143-173)
testv2/testv2_auth/test_oauth_method_fix.py (1)
kinde_sdk/auth/oauth.py (2)
OAuth(23-634)get_user_info(143-173)
🪛 Ruff (0.14.3)
testv2/testv2_auth/test_framework_route_handlers.py
37-37: Unused method argument: mock_storage_factory
(ARG002)
93-93: Unused method argument: mock_storage_factory
(ARG002)
150-150: Unused method argument: mock_storage_factory
(ARG002)
223-223: Unused method argument: mock_storage_factory
(ARG002)
253-253: Possible hardcoded password assigned to: "secret_key"
(S105)
270-270: Unused method argument: mock_storage_factory
(ARG002)
300-300: Possible hardcoded password assigned to: "secret_key"
(S105)
testv2/testv2_auth/test_oauth_method_fix.py
40-40: Unused method argument: mock_framework_factory
(ARG002)
40-40: Unused method argument: mock_storage_factory
(ARG002)
56-56: Unused method argument: mock_framework_factory
(ARG002)
56-56: Unused method argument: mock_storage_factory
(ARG002)
73-73: Unused method argument: mock_framework_factory
(ARG002)
73-73: Unused method argument: mock_storage_factory
(ARG002)
103-103: Unused method argument: mock_framework_factory
(ARG002)
103-103: Unused method argument: mock_storage_factory
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unittest (3.12)
- GitHub Check: unittest (3.13)
- GitHub Check: unittest (3.10)
- GitHub Check: unittest (3.9)
- GitHub Check: unittest (3.11)
Add focused tests to verify that OAuth.is_authenticated() and OAuth.get_user_info() accept no parameters beyond self, preventing the bug where framework implementations were passing a request parameter these methods don't accept. Tests verify: - Method signatures have no parameters - Methods can be called without parameters - Passing extra parameters raises TypeError - Will catch any regression if request parameter is added back All tests properly mock requests.get to prevent network calls in CI/offline environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
testv2/testv2_auth/test_oauth_method_fix.py (3)
18-18: Remove unused import.
KindeConfigurationExceptionis imported but never used in this test file.Apply this diff:
from kinde_sdk.auth.oauth import OAuth -from kinde_sdk.core.exceptions import KindeConfigurationException
38-92: Previous critical issue resolved; consider extracting duplicate mock setup.The
requests.getpatch is now in place for both tests, which resolves the critical issue flagged in the previous review.However, the OpenID configuration mock setup (lines 48-56 and 76-84) is duplicated. Consider extracting this to a helper method or fixture to reduce duplication.
Note: Static analysis warnings about unused
mock_framework_factoryandmock_storage_factoryare false positives—these parameters are required by the@patchdecorators to prevent actual factory instantiation during OAuth construction.Example refactor using a helper method:
def _create_mock_openid_response(self): """Helper to create mock OpenID configuration response.""" mock_response = Mock() mock_response.status_code = 200 mock_response.json = Mock(return_value={ 'authorization_endpoint': 'https://test.kinde.com/oauth2/auth', 'token_endpoint': 'https://test.kinde.com/oauth2/token', 'end_session_endpoint': 'https://test.kinde.com/logout', 'userinfo_endpoint': 'https://test.kinde.com/oauth2/userinfo' }) return mock_response @patch('kinde_sdk.auth.oauth.requests.get') @patch('kinde_sdk.auth.oauth.StorageFactory') @patch('kinde_sdk.auth.oauth.FrameworkFactory') def test_is_authenticated_signature_has_no_parameters(self, mock_framework_factory, mock_storage_factory, mock_get): """...""" mock_get.return_value = self._create_mock_openid_response() # rest of test...
94-152: Good regression coverage; same duplication opportunity.These tests provide valuable regression protection by ensuring
TypeErroris raised when extra parameters are passed, directly addressing the PR objective to prevent the bug from being reintroduced.The same mock setup duplication noted in lines 38-92 also applies here (lines 105-113 and 135-143). The helper method suggestion from the previous comment would eliminate this duplication as well.
Note: The static analysis warnings about unused mock parameters are false positives for the same reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testv2/testv2_auth/test_oauth_method_fix.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
testv2/testv2_auth/test_oauth_method_fix.py (1)
kinde_sdk/auth/oauth.py (2)
OAuth(23-634)get_user_info(143-173)
🪛 Ruff (0.14.3)
testv2/testv2_auth/test_oauth_method_fix.py
41-41: Unused method argument: mock_framework_factory
(ARG002)
41-41: Unused method argument: mock_storage_factory
(ARG002)
69-69: Unused method argument: mock_framework_factory
(ARG002)
69-69: Unused method argument: mock_storage_factory
(ARG002)
97-97: Unused method argument: mock_framework_factory
(ARG002)
97-97: Unused method argument: mock_storage_factory
(ARG002)
127-127: Unused method argument: mock_framework_factory
(ARG002)
127-127: Unused method argument: mock_storage_factory
(ARG002)
🔇 Additional comments (2)
testv2/testv2_auth/test_oauth_method_fix.py (2)
21-36: LGTM!The test setup and teardown correctly manage environment variable patching for OAuth configuration.
154-157: LGTM!Standard unittest runner for direct execution.
Tests added. |
…od signature tests Deleted the outdated test file `test_oauth_method_fix.py` and integrated its tests into `test_oauth.py`. The new tests ensure that `OAuth.is_authenticated()` and `OAuth.get_user_info()` methods accept no parameters beyond `self`, preventing issues with framework implementations passing unnecessary request parameters. This change enhances test organization and maintains coverage for critical functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testv2/testv2_auth/test_oauth.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
testv2/testv2_auth/test_oauth.py (2)
kinde_sdk/auth/oauth.py (3)
OAuth(23-634)is_authenticated(124-141)get_user_info(143-173)kinde_sdk/auth/user_session.py (1)
is_authenticated(130-146)
🪛 GitHub Actions: CI
testv2/testv2_auth/test_oauth.py
[error] 1-1: Step 'pytest --cov=kinde_sdk --cov-branch --cov-report=xml' failed due to timeout. Test 'TestOAuthExtended.test_is_authenticated_rejects_extra_parameters' timed out (pytest-timeout). Investigate potential blocking operations or increase timeout.
🪛 Ruff (0.14.4)
testv2/testv2_auth/test_oauth.py
142-142: Unused method argument: mock_framework_factory
(ARG002)
142-142: Unused method argument: mock_storage_factory
(ARG002)
177-177: Unused method argument: mock_framework_factory
(ARG002)
177-177: Unused method argument: mock_storage_factory
(ARG002)
212-212: Unused method argument: mock_framework_factory
(ARG002)
212-212: Unused method argument: mock_storage_factory
(ARG002)
248-248: Unused method argument: mock_framework_factory
(ARG002)
248-248: Unused method argument: mock_storage_factory
(ARG002)
🔇 Additional comments (3)
testv2/testv2_auth/test_oauth.py (3)
6-7: LGTM!The new imports are appropriate for the signature verification tests.
133-167: LGTM!The signature inspection test correctly verifies that
is_authenticated()accepts no parameters beyondself. The unused mock parameters flagged by static analysis are intentionally present to prevent side effects during OAuth initialization.
168-202: LGTM!The signature inspection test correctly verifies that
get_user_info()accepts no parameters beyondself.
Introduced a new test class `TestOAuthMethodSignatures` to validate the method signatures of OAuth functions, ensuring they accept no parameters beyond `self`. This addition includes mocks to prevent blocking during OAuth initialization and verifies that passing extra parameters raises a TypeError. These tests enhance coverage and help prevent regressions related to request parameter handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testv2/testv2_auth/test_oauth.py (1)
134-294: Consider extracting shared mock setup to reduce duplication.All four tests share identical mock setup patterns (environment variables, factory configuration, OpenID config mocking). Extracting this to a
setUpmethod or helper function would improve maintainability.Example refactor:
class TestOAuthMethodSignatures(unittest.TestCase): """Test OAuth method signatures to verify the fix for incorrect request parameter passing.""" def _setup_oauth_mocks(self, mock_get, mock_framework_factory, mock_storage_factory): """Helper to configure OAuth mocks consistently.""" # Mock OpenID configuration mock_response = Mock() mock_response.status_code = 200 mock_response.json = Mock(return_value={ 'authorization_endpoint': 'https://test.kinde.com/oauth2/auth', 'token_endpoint': 'https://test.kinde.com/oauth2/token', 'end_session_endpoint': 'https://test.kinde.com/logout', 'userinfo_endpoint': 'https://test.kinde.com/oauth2/userinfo' }) mock_get.return_value = mock_response # Configure factory mocks mock_storage_factory.create_storage.return_value = MagicMock() mock_framework_factory.create_framework.return_value = None return OAuth(framework=None)Then use it in each test:
@patch.dict('os.environ', {...}) @patch('kinde_sdk.auth.oauth.requests.get') @patch('kinde_sdk.auth.oauth.StorageFactory') @patch('kinde_sdk.auth.oauth.FrameworkFactory') def test_is_authenticated_signature_has_no_parameters(self, mock_framework_factory, mock_storage_factory, mock_get): oauth = self._setup_oauth_mocks(mock_get, mock_framework_factory, mock_storage_factory) sig = inspect.signature(oauth.is_authenticated) # ... rest of test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testv2/testv2_auth/test_oauth.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
testv2/testv2_auth/test_oauth.py (2)
kinde_sdk/auth/oauth.py (2)
is_authenticated(124-141)get_user_info(143-173)kinde_sdk/auth/user_session.py (1)
is_authenticated(130-146)
🔇 Additional comments (5)
testv2/testv2_auth/test_oauth.py (5)
6-7: LGTM!The new imports (
inspectfor signature inspection andMockfor HTTP response mocking) are appropriate for the test requirements.
146-174: Good regression test with proper mock configuration.This test correctly validates that
is_authenticated()accepts no parameters beyondself, which prevents the bug where FastAPI/Flask were passing arequestparameter. The factory mock configuration at lines 164-166 addresses the timeout concern from past reviews.
185-213: LGTM!This test correctly validates that
get_user_info()accepts no parameters beyondself, with proper factory mock configuration to prevent initialization issues.
224-253: Past review concern addressed.The factory mock configuration at lines 242-244 correctly resolves the timeout issue flagged in past reviews. The test effectively prevents regression by ensuring
TypeErroris raised when extra parameters are passed.
264-293: Past review concern addressed.The factory mock configuration at lines 282-284 resolves the timeout concern from past reviews. The test correctly validates that
get_user_info()rejects extra parameters with aTypeError.
5 participants
Fixed a critical bug where framework implementations were passing a request parameter to OAuth.is_authenticated() and OAuth.get_user_info() methods that don't accept any parameters beyond self.
Issue:
Changes:
Impact:
Resolves client-reported issue where authentication always showed as "not authenticated" and /user endpoint threw exceptions.
Explain your changes
Suppose there is a related issue with enough detail for a reviewer to understand your changes fully. In that case, you can omit an explanation and instead include either “Fixes #XX” or “Updates #XX” where “XX” is the issue number.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.