Improve error messages for user authentication#297
Conversation
WalkthroughAuthentication behavior was changed: Possibly related PRs
Suggested labels
🚥 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 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
===========================================
+ Coverage 52.43% 93.04% +40.60%
===========================================
Files 38 71 +33
Lines 1661 2991 +1330
Branches 154 221 +67
===========================================
+ Hits 871 2783 +1912
+ Misses 788 152 -636
- Partials 2 56 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/routers/openml/study.py (1)
65-71: Remove unreachableuser is Nonebranch inattach_to_study.After switching to
Depends(fetch_user_or_raise), this guard is dead in normal route execution and preserves an outdated message path.Suggested cleanup
async def attach_to_study( study_id: Annotated[int, Body()], entity_ids: Annotated[list[int], Body()], user: Annotated[User, Depends(fetch_user_or_raise)], expdb: Annotated[AsyncConnection, Depends(expdb_connection)] = None, ) -> AttachDetachResponse: assert expdb is not None # noqa: S101 - if user is None: - msg = "Authentication required." - raise AuthenticationRequiredError(msg) study = await _get_study_raise_otherwise(study_id, user, expdb)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routers/openml/study.py` around lines 65 - 71, In attach_to_study, remove the now-unreachable guard that checks "if user is None" (the route uses Depends(fetch_user_or_raise) so user will never be None); delete that branch and its AuthenticationRequiredError raise (and clean up any now-unused imports if necessary) so the function relies on fetch_user_or_raise to enforce authentication and no longer preserves the stale message path.
🤖 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/openml/study.py`:
- Around line 65-71: In attach_to_study, remove the now-unreachable guard that
checks "if user is None" (the route uses Depends(fetch_user_or_raise) so user
will never be None); delete that branch and its AuthenticationRequiredError
raise (and clean up any now-unused imports if necessary) so the function relies
on fetch_user_or_raise to enforce authentication and no longer preserves the
stale message path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d9f4e4a-7de2-4b39-8ecb-a1bb0484a014
📒 Files selected for processing (2)
src/routers/openml/datasets.pysrc/routers/openml/study.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
45-45: Scope coverage to project code explicitly (--cov=src).Using bare
--covcan make coverage include unintended modules and reduce report stability across environments.Suggested diff
- docker compose exec python-api pytest -n "$jobs" -v -m "$marker" --cov --cov-report=xml + docker compose exec python-api pytest -n "$jobs" -v -m "$marker" --cov=src --cov-report=xml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml at line 45, Update the pytest invocation that currently uses a bare "--cov" (the command starting with "docker compose exec python-api pytest -n \"$jobs\" -v -m \"$marker\" --cov --cov-report=xml") to scope coverage to the project code by replacing "--cov" with "--cov=src" so pytest explicitly measures the src package only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/tests.yml:
- Line 45: Update the pytest invocation that currently uses a bare "--cov" (the
command starting with "docker compose exec python-api pytest -n \"$jobs\" -v -m
\"$marker\" --cov --cov-report=xml") to scope coverage to the project code by
replacing "--cov" with "--cov=src" so pytest explicitly measures the src package
only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ccfbd7c-4a23-4d10-9ac5-2a37931b4e7c
📒 Files selected for processing (2)
.github/workflows/tests.ymlpyproject.toml
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.