Fix/endpoints #167
Fix/endpoints #167
Conversation
📝 WalkthroughWalkthroughRemoved the /verify-token endpoint and TokenValidationResponse; auth verification now uses current-user profile fetching. User role typing standardized to UserRole. Duplicate-email conflict handling moved to user repository. DLQ statistics, event-aggregation, and related types/endpoints were removed. Frontend SDK/types, auth store, interceptors, OpenAPI and tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FrontendStore as Frontend\nAuth Store
participant API as Backend\nAPI (/api/v1/auth)
participant DB as User\nRepository/DB
Client->>FrontendStore: trigger verifyAuth / initialize
FrontendStore->>API: GET /api/v1/auth/me
API->>DB: validate session/token & fetch user profile
DB-->>API: user profile (username, email, role, is_active)
API-->>FrontendStore: 200 OK + profile
FrontendStore->>FrontendStore: set user context (userId, userEmail, role), read CSRF cookie
FrontendStore-->>Client: auth confirmed (state updated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/auth.py`:
- Around line 168-176: The code currently uses the request-supplied user.role
when building DomainUserCreate which allows privilege escalation; change the
creation to always set role to UserRole.USER (import UserRole from
app.domain.enums if missing) instead of user.role — e.g. keep hashed_password =
security_service.get_password_hash(user.password) and pass role=UserRole.USER
into DomainUserCreate so self-registered accounts cannot set elevated roles.
In `@backend/app/db/repositories/user_repository.py`:
- Around line 21-24: The DuplicateKeyError catch in the doc.insert() flow
currently always raises ConflictError("Email already registered"), which
mislabels non-email unique violations; modify the except block for
DuplicateKeyError to inspect e.details (or e.args) to determine the conflicting
key (e.g., check for "email", "username", "user_id" in the error detail) and
raise a contextual ConflictError like "Email already registered" or "Username
already registered" accordingly, falling back to a generic "User already exists"
if the key can't be determined; keep the original exception as the cause (raise
... from e) and update any tests that assert the exact message in
UserDocument-related operations.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
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/app/db/repositories/user_repository.py">
<violation number="1" location="backend/app/db/repositories/user_repository.py:24">
P3: DuplicateKeyError can be raised for unique username or user_id as well as email, so always returning "Email already registered" can mislead clients. Use a generic conflict message or detect which field is duplicated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…at regard - also fixed other small errors
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/openapi.json (1)
16017-16090:⚠️ Potential issue | 🟡 MinorDocument
is_superuseras legacy; role is the source of truth.The backend always returns
role,is_active, andis_superuseras required fields inUserResponse. However,is_superuseris always initialized tofalseand never modified in the service layer—authorization decisions should userole(UserRole enum) instead. Add documentation to clarify this legacy field and guide clients to preferrole.✍️ Recommended documentation clarification
"is_superuser": { "type": "boolean", - "title": "Is Superuser" + "title": "Is Superuser", + "description": "Legacy field; always false. Use `role` for authorization decisions." },
🧹 Nitpick comments (2)
frontend/src/stores/auth.svelte.ts (1)
219-219: CSRF cookie value may needdecodeURIComponent.The regex extracts the raw cookie value, but cookie values can be URI-encoded. If the CSRF token ever contains characters like
=,%, or;, the raw match won't give you the correct value.Suggested fix
- this.csrfToken = document.cookie.match(/(?:^|;\s*)csrf_token=([^;]*)/)?.[1] ?? null; + const rawCsrf = document.cookie.match(/(?:^|;\s*)csrf_token=([^;]*)/)?.[1]; + this.csrfToken = rawCsrf ? decodeURIComponent(rawCsrf) : null;frontend/src/stores/__tests__/auth.test.ts (1)
241-304: Consider adding a test for CSRF cookie extraction inverifyAuth.The
verifyAuthmethod now readscsrfTokenfromdocument.cookie(Line 219 ofauth.svelte.ts) instead of from the API response. None of the existing tests verify thatauthStore.csrfTokenis correctly set afterverifyAuthsucceeds. A test that setsdocument.cookiewith acsrf_tokenvalue and assertsauthStore.csrfTokenafter verification would close this gap.
…ger, extra endpoint isnt used at all - also register endpoint fix (instead of calling register with role allowing default users to create admins, - letting only admins in admin/users do it, also fix for conftest setting admin for CI tests)
…y policy conversion
…o removed uncalled endpoints and tests for them + regen of types+openapi spec
|
1 participant
Summary by cubic
Switched auth and execution status to enums, moved session checks to /auth/me, and centralized duplicate-user handling. Removed unused DLQ stats and events aggregation endpoints, restricted DLQ routes to admins, and simplified event list responses.
Refactors
Migration
Written for commit b0aed8d. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Schema Updates
Breaking Changes