Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile OverviewGreptile SummaryChanged the Key Changes:
Why this matters: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Endpoint as /me Endpoint
participant OptUser as optional_user
participant FastAPIAuth as FastAPI Users Auth
participant AnonCheck as Anonymous Check
Client->>Endpoint: GET /me
Endpoint->>OptUser: Depends(optional_user)
OptUser->>FastAPIAuth: Get current user
FastAPIAuth-->>OptUser: User or None
alt User is authenticated anonymous user
OptUser->>AnonCheck: is_anonymous and anonymous_user_enabled
AnonCheck-->>OptUser: returns True
OptUser-->>Endpoint: Return fresh anonymous user object
else User authenticated via other methods
OptUser->>OptUser: Check authentication methods
OptUser-->>Endpoint: Return authenticated user
else No authentication provided
OptUser-->>Endpoint: Return None
end
alt user is None
Endpoint-->>Client: 403 Unauthorized
else user is anonymous
Endpoint-->>Client: Return fake UserInfo
else user OIDC token expired
Endpoint-->>Client: 403 Token expired
else user verified or unverified
Endpoint-->>Client: Return UserInfo with status
end
|
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/server/manage/users.py">
<violation number="1" location="backend/onyx/server/manage/users.py:660">
P2: /me no longer allows anonymous access when anonymous users are enabled, because optional_user returns None and the new 401 check blocks the anonymous flow. This makes the anonymous branch unreachable and changes behavior from current_chat_accessible_user.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) -> UserInfo: | ||
| # User can no longer be None. However, we need to use optional_user dependency | ||
| # to allow unverified users to access this endpoint | ||
| if user is None: |
There was a problem hiding this comment.
P2: /me no longer allows anonymous access when anonymous users are enabled, because optional_user returns None and the new 401 check blocks the anonymous flow. This makes the anonymous branch unreachable and changes behavior from current_chat_accessible_user.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/onyx/server/manage/users.py, line 660:
<comment>/me no longer allows anonymous access when anonymous users are enabled, because optional_user returns None and the new 401 check blocks the anonymous flow. This makes the anonymous branch unreachable and changes behavior from current_chat_accessible_user.</comment>
<file context>
@@ -652,9 +652,13 @@ def get_current_token_creation(user: User, db_session: Session) -> datetime | No
) -> UserInfo:
+ # User can no longer be None. However, we need to use optional_user dependency
+ # to allow unverified users to access this endpoint
+ if user is None:
+ raise HTTPException(status_code=401, detail="Unauthorized")
# If anonymous user, return the fake UserInfo (maintains backward compatibility)
</file context>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/server/manage/users.py">
<violation number="1" location="backend/onyx/server/manage/users.py:662">
P2: Unauthenticated /me requests now return 403 because BasicAuthenticationError maps to HTTP 403. This contradicts the intended 401 behavior and can break clients that rely on 401 to trigger re-auth. Use HTTPException(status_code=401) (or a 401-specific error type) for the unauthenticated branch.
(Based on your team's feedback about FastAPI endpoint error handling.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@greptile @cubic-dev-ai review |
@wenxi-onyx I have started the AI code review. It will take a few minutes to complete. |
| # User should no longer be None (unless not auth-ed). | ||
| # However, we need to use optional_user dependency | ||
| # to allow unverified users to access this endpoint |
There was a problem hiding this comment.
comment contradicts itself - says "should no longer be None" but the whole point of using optional_user is that user CAN be None (you handle it on line 661)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/server/manage/users.py
Line: 658:660
Comment:
comment contradicts itself - says "should no longer be None" but the whole point of using `optional_user` is that user CAN be None (you handle it on line 661)
How can I resolve this? If you propose a fix, please make it concise.
Description
How Has This Been Tested?
Additional Options
Summary by cubic
Allow unverified users to call the /me endpoint by switching to an optional auth dependency, while still returning 403 for unauthenticated requests. Keeps existing behavior for anonymous users to avoid breaking clients.
Written for commit 8e6b49a. Summary will update on new commits.