Develop#7
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a cookie-based refresh-token auth flow and an email-verification gate across the backend and frontend, aligning types/mocks/routes with the new auth contract.
Changes:
- Switch refresh tokens to HttpOnly cookies (frontend uses
withCredentials, backend sets/rotates refresh cookie). - Add email verification endpoints + UI flow (
/verify-email) and gate unverified users from non-auth APIs. - Add/extend backend auth infrastructure (async DB session, settings/security helpers, rate limiter, tests) and update SQL enums/docs.
Reviewed changes
Copilot reviewed 44 out of 47 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/utils/http.ts | Enable credentialed requests for cookie refresh flow |
| web/src/types/user.d.ts | Update user/auth types (email verification, register response) |
| web/src/store/user.ts | Remove stored refresh token; add logout API call; return login response |
| web/src/router/routes.ts | Add /verify-email route |
| web/src/router/gurad.ts | Redirect unverified users to verify-email |
| web/src/pages/verify-email/VerifyEmail.vue | New email verification + resend UI |
| web/src/pages/verify-email/index.ts | Export verify-email page entry |
| web/src/pages/register/Register.vue | Route to verify-email after register when required |
| web/src/pages/login/Login.vue | Route to verify-email if user not verified |
| web/src/mock/state.ts | Add email verification fields to mock users |
| web/src/mock/handlers/user.ts | Include email verification fields in mock payloads |
| web/src/mock/handlers/auth.ts | Add verify/resend endpoints; cookie-style refresh behavior in mocks |
| web/src/api/user.ts | Update register return type; add verify/resend API calls |
| docs/sql/postgres/10_identity.sql | Use user_role_enum for role column |
| docs/sql/postgres/00_base.sql | Define user_role_enum type |
| app/uv.lock | Update Python dependency lockfile for new auth stack |
| app/tests/test_settings.py | Test async DB URL conversion |
| app/tests/test_security.py | Test token/password hashing helpers |
| app/tests/conftest.py | Ensure test imports resolve src package |
| app/src/workers/DESIGN.md | Add worker architecture design doc (future work) |
| app/src/services/rate_limiter.py | Add Redis rate limiter (degrades on Redis failure) |
| app/src/services/messaging.py | Add auth event publisher abstraction (in-process impl) |
| app/src/services/auth.py | Implement register/login/refresh/logout + verify/resend flows |
| app/src/services/init.py | Export service layer components |
| app/src/schemas/user.py | Add email verification fields; allow nullable last_login |
| app/src/schemas/auth.py | Add register response schema + verify-email request; remove refresh_token from token response |
| app/src/schemas/init.py | Re-export new auth schemas |
| app/src/routers/me.py | Add /me/profile endpoint |
| app/src/routers/auth.py | Add auth endpoints + refresh-cookie set/clear logic |
| app/src/routers/init.py | Register auth + me routers |
| app/src/models/tables_identity.py | Make role a typed enum column |
| app/src/models/enums.py | Add UserRole enum |
| app/src/main.py | New FastAPI app setup (CORS, middleware, error handlers) |
| app/src/db/session.py | Switch to async sessionmaker |
| app/src/db/engine.py | Switch to async engine using Settings URL |
| app/src/db/deps.py | Provide async DB dependency |
| app/src/core/settings.py | Add Settings via pydantic-settings + URL conversion helpers |
| app/src/core/security.py | Add JWT + password hashing helpers |
| app/src/core/middleware.py | Add email verification gate middleware |
| app/src/core/errors.py | Add standard API error/response envelope |
| app/src/core/deps.py | Add DI helpers (auth service, current user, rate limiter) |
| app/src/core/init.py | Export core helpers |
| app/src/.env.example | Provide environment template for new settings |
| app/src/init.py | Declare backend package |
| app/pyproject.toml | Add dependencies for async DB, JWT, Redis, mail, settings |
| agents.md | Update cross-layer contract/development guide |
| .gitignore | Ignore Python caches and test/lint caches |
Comments suppressed due to low confidence (1)
web/src/utils/http.ts:20
- Setting
withCredentials: trueon the shared axios instance means cookies (including the refresh-token cookie) will be sent on every request tobaseURL. IfVITE_BASE_URLcan point to a different origin (or if additional cookie-scoped endpoints are added later), this increases the blast radius for CSRF/cookie leakage. Consider enablingwithCredentialsonly for the auth/refresh/logout calls (or conditionally whenbaseURLis same-origin) and ensure the backend CORS/CSRF posture matches this.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SessionLocal = async_sessionmaker( | ||
| bind=engine, | ||
| class_=AsyncSession, | ||
| autocommit=False, |
There was a problem hiding this comment.
async_sessionmaker/AsyncSession in SQLAlchemy 2.x does not support the autocommit kwarg (it was removed). Keeping it here will raise a TypeError at runtime when creating sessions. Remove autocommit=False and rely on explicit commit()/rollback() (the default behavior).
| autocommit=False, |
| try: | ||
| current = await client.incr(key) | ||
| if current == 1: | ||
| await client.expire(key, window_seconds) | ||
| return current <= limit |
There was a problem hiding this comment.
The rate-limit increment and TTL set are not atomic (INCR then EXPIRE). If the process crashes (or the EXPIRE fails) after INCR, the key can be left without a TTL and the client may be rate-limited indefinitely. Use a Lua script or a MULTI/EXEC pipeline that sets the TTL when the counter is first created (or use a single command pattern like SET key 1 EX <window> NX + INCR).
| samesite = COOKIE_SAMESITE_MAP.get(settings.refresh_cookie_samesite.strip().lower()) | ||
| if samesite is None: | ||
| raise ValueError(f"Invalid samesite value: {settings.refresh_cookie_samesite}") | ||
| response.set_cookie( | ||
| key=settings.refresh_cookie_name, | ||
| value=refresh_token, | ||
| max_age=settings.refresh_token_ttl_seconds, | ||
| httponly=True, | ||
| secure=settings.refresh_cookie_secure, |
There was a problem hiding this comment.
_set_refresh_cookie raises ValueError on invalid refresh_cookie_samesite, which will surface as a 500 at request time. Consider validating/normalizing this in Settings at startup (e.g., use a Literal field or a validator) so misconfiguration fails fast on boot. Also consider enforcing secure=True when SameSite=None, otherwise modern browsers will reject the cookie and refresh will silently break.
| samesite = COOKIE_SAMESITE_MAP.get(settings.refresh_cookie_samesite.strip().lower()) | |
| if samesite is None: | |
| raise ValueError(f"Invalid samesite value: {settings.refresh_cookie_samesite}") | |
| response.set_cookie( | |
| key=settings.refresh_cookie_name, | |
| value=refresh_token, | |
| max_age=settings.refresh_token_ttl_seconds, | |
| httponly=True, | |
| secure=settings.refresh_cookie_secure, | |
| raw_samesite = settings.refresh_cookie_samesite or "" | |
| normalized_samesite = raw_samesite.strip().lower() | |
| samesite = COOKIE_SAMESITE_MAP.get(normalized_samesite, "lax") | |
| secure = settings.refresh_cookie_secure | |
| if samesite == "none" and not secure: | |
| # Modern browsers require Secure=True when SameSite=None; enforce it here | |
| secure = True | |
| response.set_cookie( | |
| key=settings.refresh_cookie_name, | |
| value=refresh_token, | |
| max_age=settings.refresh_token_ttl_seconds, | |
| httponly=True, | |
| secure=secure, |
| }); | ||
|
|
||
| const canResend = computed(() => userStore.isAuthenticated && !userStore.user?.emailVerified); | ||
|
|
||
| async function executeVerify(tokenValue: string) { |
There was a problem hiding this comment.
canResend is true when userStore.user is still null/undefined (because !undefined is true), so the UI can show the resend button before the profile has loaded, leading to a likely 401/403 when clicked. Prefer a strict check like userStore.user?.emailVerified === false (and/or require userStore.user to be present).
| const isLoggedIn = !!userStore.token; | ||
| const requiresAdmin = Boolean(to.meta.requiresAdmin); | ||
| const hasAdminAccess = userStore.user?.role === 'admin'; | ||
| const needsVerification = isLoggedIn && userStore.user?.emailVerified === false; | ||
|
|
There was a problem hiding this comment.
needsVerification only triggers when emailVerified === false. On page reloads where a token is present but the profile hasn’t been fetched yet (so userStore.user is null or missing emailVerified), the guard won’t redirect and the user can navigate into routes that will then be blocked by the backend (403), causing inconsistent UX. Consider adding an explicit "profile loaded" state and awaiting fetchUserProfile() (or blocking navigation) before making the verification decision.
| authorization = request.headers.get("authorization", "") | ||
| if not authorization.lower().startswith("bearer "): | ||
| return await call_next(request) | ||
|
|
||
| token = authorization.split(" ", 1)[1].strip() | ||
| try: | ||
| payload = decode_access_token(token, self.settings) | ||
| user_id = int(payload["sub"]) | ||
| except (InvalidTokenError, KeyError, ValueError): | ||
| return await call_next(request) | ||
|
|
||
| async with SessionLocal() as session: | ||
| user = await session.get(User, user_id) | ||
| if user and not user.email_verified: | ||
| return JSONResponse( |
There was a problem hiding this comment.
This middleware performs a DB lookup on every /api/v1/* request that carries an Authorization header to re-check email_verified. Since the frontend attaches the bearer token to all API calls, this adds an extra query per request and can become a significant bottleneck. Consider reducing the check to only routes that require auth, caching the verified status for the token/session lifetime, or embedding an emailVerified claim in the access token (and forcing token refresh after verification) to avoid the per-request query.
- add Electron main/preload entry and desktop dev/build scripts - support Electron routing/assets via hash history and relative base - add batch upload preflight/complete/status API contracts and mock handlers - enhance sidebar preview with PDF pagination, HLS playback, and image viewer - update docs and ignore release artifacts
No description provided.