Improve error messages for user authentication#297
Conversation
WalkthroughThis change refactors authentication error handling in the user dependency layer. The 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider whether
fetch_usershould still be a pureUser | Nonehelper and move theAuthenticationFailedErrorraising logic into a separate dependency (e.g.fetch_user_or_fail) so that optional-auth endpoints can opt-in more explicitly to the new behavior without surprising existing call sites. - The error detail strings (
"Invalid API key provided.","No API key provided.") are now used in multiple places and tests; you may want to centralize them (e.g. on the error classes or as constants) to avoid future drift between implementation and expectations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider whether `fetch_user` should still be a pure `User | None` helper and move the `AuthenticationFailedError` raising logic into a separate dependency (e.g. `fetch_user_or_fail`) so that optional-auth endpoints can opt-in more explicitly to the new behavior without surprising existing call sites.
- The error detail strings (`"Invalid API key provided."`, `"No API key provided."`) are now used in multiple places and tests; you may want to centralize them (e.g. on the error classes or as constants) to avoid future drift between implementation and expectations.
## Individual Comments
### Comment 1
<location path="tests/dependencies/fetch_user_test.py" line_range="34-38" />
<code_context>
+ await fetch_user(api_key=ApiKey.INVALID, user_data=user_test)
+
+
+async def test_fetch_user_or_raise_raises_if_no_user() -> None:
+ # This function calls `fetch_user` through dependency injection,
+ # so it only needs to correctly handle possible output of `fetch_user`.
+ with pytest.raises(AuthenticationRequiredError):
+ fetch_user_or_raise(user=None)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a positive test for `fetch_user_or_raise` when a valid user is provided.
Right now we only cover the error path (no user → `AuthenticationRequiredError`). Please also add a test passing a valid `User` instance and asserting it’s returned unchanged and no exception is raised. This will guard against regressions where the function might start altering or re-validating the user.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| async def test_fetch_user_or_raise_raises_if_no_user() -> None: | ||
| # This function calls `fetch_user` through dependency injection, | ||
| # so it only needs to correctly handle possible output of `fetch_user`. | ||
| with pytest.raises(AuthenticationRequiredError): | ||
| fetch_user_or_raise(user=None) |
There was a problem hiding this comment.
suggestion (testing): Add a positive test for fetch_user_or_raise when a valid user is provided.
Right now we only cover the error path (no user → AuthenticationRequiredError). Please also add a test passing a valid User instance and asserting it’s returned unchanged and no exception is raised. This will guard against regressions where the function might start altering or re-validating the user.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/dependencies/fetch_user_test.py (1)
34-38: Consider removing unnecessaryasyncfrom this test function.
fetch_user_or_raiseis a synchronous function, so this test doesn't need to be async. While pytest handles this gracefully, removingasyncwould be more accurate.♻️ Optional fix
-async def test_fetch_user_or_raise_raises_if_no_user() -> None: +def test_fetch_user_or_raise_raises_if_no_user() -> None: # This function calls `fetch_user` through dependency injection, # so it only needs to correctly handle possible output of `fetch_user`. with pytest.raises(AuthenticationRequiredError): fetch_user_or_raise(user=None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dependencies/fetch_user_test.py` around lines 34 - 38, The test function is declared async but exercises the synchronous function fetch_user_or_raise; change the test signature from async def test_fetch_user_or_raise_raises_if_no_user() to a regular def so it is synchronous, leaving the body (the pytest.raises block calling fetch_user_or_raise(user=None)) unchanged and ensuring there are no awaits or async utilities used inside the test.src/routers/dependencies.py (1)
39-45: Minor edge case in error message accuracy.If
api_keyis provided butuser_dataisNone(database connection unavailable),fetch_userreturnsNone, andfetch_user_or_raisewould report "No API key provided." which isn't accurate.In practice this is unlikely since
user_datacomes fromDepends(userdb_connection), but for defensive accuracy you could track whyuserisNone.This is a minor concern given that dependency injection should always provide the connection. If you want to be pedantic about error accuracy, one option would be to pass through more context from
fetch_user, but the current implementation is reasonable for practical use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routers/dependencies.py` around lines 39 - 45, fetch_user_or_raise currently treats any None from Depends(fetch_user) as "No API key provided." — update the flow so you distinguish "no API key" vs "user lookup failed/DB unavailable": modify fetch_user to either raise a specific exception when the api_key is missing (so fetch_user_or_raise can let that propagate) or return a discriminated result (e.g. (user, reason) or a sentinel) and then change fetch_user_or_raise to inspect that reason and raise AuthenticationRequiredError when the api key is absent and a different error (e.g. ServiceUnavailableError or a new DatabaseUnavailableError) when the downstream user_data lookup failed; reference fetch_user_or_raise and fetch_user to implement this change and ensure AuthenticationRequiredError is only used for the missing API key case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/routers/dependencies.py`:
- Around line 39-45: fetch_user_or_raise currently treats any None from
Depends(fetch_user) as "No API key provided." — update the flow so you
distinguish "no API key" vs "user lookup failed/DB unavailable": modify
fetch_user to either raise a specific exception when the api_key is missing (so
fetch_user_or_raise can let that propagate) or return a discriminated result
(e.g. (user, reason) or a sentinel) and then change fetch_user_or_raise to
inspect that reason and raise AuthenticationRequiredError when the api key is
absent and a different error (e.g. ServiceUnavailableError or a new
DatabaseUnavailableError) when the downstream user_data lookup failed; reference
fetch_user_or_raise and fetch_user to implement this change and ensure
AuthenticationRequiredError is only used for the missing API key case.
In `@tests/dependencies/fetch_user_test.py`:
- Around line 34-38: The test function is declared async but exercises the
synchronous function fetch_user_or_raise; change the test signature from async
def test_fetch_user_or_raise_raises_if_no_user() to a regular def so it is
synchronous, leaving the body (the pytest.raises block calling
fetch_user_or_raise(user=None)) unchanged and ensuring there are no awaits or
async utilities used inside the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ae56a6d-dbe8-4abd-b1b1-45a95324afc7
📒 Files selected for processing (8)
src/core/errors.pysrc/routers/dependencies.pytests/dependencies/__init__.pytests/dependencies/fetch_user_test.pytests/routers/openml/migration/datasets_migration_test.pytests/routers/openml/setups_tag_test.pytests/routers/openml/setups_untag_test.pytests/routers/openml/users_test.py
💤 Files with no reviewable changes (1)
- tests/routers/openml/users_test.py
1 participant
(work in progress, but I want to run CI)
Improve error messages for user authentication by differentiating between failed authentication (of a provided key) or required authentication (if no key is provided but one is required).
Previous behavior would be inconsistent depending on endpoint. For user-optional endpoints a mistake with the apikey would fail silently (if it passed the constraints of ApiKey), which may be hard to spot as a user. For user required endpoints there would always be an AuthenticationFailed.
And that is if dependencies were used.
WIP because I have to update the sites that use
fetch_userwherefetch_user_or_raiseshould be used.