Feature/cookies #1179
Feature/cookies #1179
Conversation
WalkthroughAdds cookie-based v2 auth endpoints (login, refresh, logout, iframe token), updates v1 token response, changes JWT middleware to prefer cookies (except login_token path), tightens CORS/cookie/CSRF settings, and adds typing/log message tweaks in charts API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FE as Frontend
participant API as /api/v2/login
participant DB as User/Org Store
participant Auth as Token Issuer
FE->>API: POST /api/v2/login (username/password)
API->>DB: Validate credentials
DB-->>API: User, Org
API->>Auth: Issue access_token & refresh_token
Auth-->>API: Tokens
API-->>FE: 200 {user/org data}
Note right of API: Set httpOnly cookies: access_token, refresh_token (attrs from settings)
sequenceDiagram
autonumber
participant FE as Frontend
participant API as /api/v2/token/refresh
participant Auth as Token Issuer
FE->>API: GET/POST (no body)
Note right of API: Reads `refresh_token` from httpOnly cookie
API->>Auth: Validate refresh, mint new access_token
Auth-->>API: access_token
API-->>FE: 200
Note right of API: Set httpOnly access_token cookie
sequenceDiagram
autonumber
participant FE as Frontend
participant API as /api/v2/iframe-token
participant Auth as Token Issuer
FE->>API: GET (authenticated)
API->>Auth: Issue iframe_token (expires in 2 minutes)
Auth-->>API: iframe_token
API-->>FE: 200 {iframe_token, org_slug}
sequenceDiagram
autonumber
participant Client as Client
participant MW as CustomJwtAuthMiddleware
participant API as API View
participant Auth as Token Validator
Client->>MW: HTTP Request
alt Path == /api/login_token/
MW->>MW: Prefer Authorization header (Bearer)
else Other paths
MW->>MW: Try cookie `access_token`
opt cookie missing
MW->>MW: Fallback to Authorization header
end
end
alt Token present
MW->>Auth: authenticate(token)
Auth-->>MW: user/orguser context
MW->>API: Forward request with context
else No token
MW-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1179 +/- ##
==========================================
- Coverage 52.65% 52.53% -0.12%
==========================================
Files 97 97
Lines 11415 11463 +48
==========================================
+ Hits 6010 6022 +12
- Misses 5405 5441 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
ddpui/settings.py (1)
125-137: Duplicate CommonMiddleware and ordering.CommonMiddleware appears twice (Lines 128 and 131). Keep one and ensure CorsMiddleware is as high as possible and before CommonMiddleware, per django-cors-headers docs.
MIDDLEWARE = [ "django_prometheus.middleware.PrometheusBeforeMiddleware", "corsheaders.middleware.CorsMiddleware", - "django.middleware.common.CommonMiddleware", "django.middleware.security.SecurityMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", - "django.middleware.common.CommonMiddleware", + "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", ... ]ddpui/api/user_org_api.py (3)
639-688: Set cookie expiries and review SameSite.
- Tie cookie lifetime to token lifetime to avoid lingering stale cookies.
- SameSite="Lax" works for same-site subdomains; if you ever need true cross-site (third-party) requests, switch to "None" + Secure and add CSRF protections accordingly.
Apply:
- # Set access token cookie + # Set access token cookie (align with token lifetime) + access_max_age = int(settings.SIMPLE_JWT.get("ACCESS_TOKEN_LIFETIME", timedelta(hours=12)).total_seconds()) response.set_cookie( "access_token", token_data["access"], httponly=True, secure=secure_cookies, samesite="lax", path="/", + max_age=access_max_age, ) - # Set refresh token cookie + # Set refresh token cookie (align with token lifetime) + refresh_max_age = int(settings.SIMPLE_JWT.get("REFRESH_TOKEN_LIFETIME", timedelta(days=30)).total_seconds()) response.set_cookie( "refresh_token", token_data["refresh"], httponly=True, secure=secure_cookies, samesite="lax", path="/", + max_age=refresh_max_age, )
690-715: Avoid bare except/pass; log non-TokenError cases.Catching Exception and passing hides real issues. Limit exceptions and log.
- if refresh_token: - try: - token = RefreshToken(refresh_token) - token_user_id = token.payload.get("user_id") - if request.user and request.user.id == token_user_id: - token.blacklist() - except (TokenError, Exception): - # Token is already invalid or expired, continue with logout - pass + if refresh_token: + try: + token = RefreshToken(refresh_token) + token_user_id = token.payload.get("user_id") + if request.user and request.user.id == token_user_id: + token.blacklist() + except TokenError: + # Already invalid/expired; continue with logout + pass + except Exception as err: + logger.warning("Logout v2: unexpected error while blacklisting token", exc_info=err)
717-748: Refresh v2: also set max_age on the access_token cookie.Keep cookie expiry aligned with the new access token.
- response.set_cookie( + access_max_age = int(settings.SIMPLE_JWT.get("ACCESS_TOKEN_LIFETIME", timedelta(hours=12)).total_seconds()) + response.set_cookie( "access_token", token_data["access"], httponly=True, secure=secure_cookies, samesite="lax", path="/", + max_age=access_max_age, )ddpui/auth.py (1)
97-106: Use suffix match for/login_token/path check
Replace the hard-coded prefix equality with a suffix check to handle any mounted prefix:- if request.path == "/api/login_token/": + if request.path.endswith("/login_token/"): logger.info("CustomJwtAuthMiddleware: prioritizing Authorization header for /login_token/") return super().__call__(request)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ddpui/api/user_org_api.py(4 hunks)ddpui/auth.py(1 hunks)ddpui/settings.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ddpui/auth.py (1)
ddpui/utils/custom_logger.py (1)
info(25-29)
ddpui/api/user_org_api.py (7)
ddpui/models/org_user.py (3)
LoginPayload(206-210)OrgUserResponse(123-135)OrgUser(66-86)ddpui/auth.py (3)
CustomTokenObtainSerializer(199-236)CustomTokenRefreshSerializer(239-253)get_token(203-232)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)ddpui/models/org.py (3)
Org(61-84)OrgWarehouse(125-146)base_plan(82-84)ddpui/models/org_preferences.py (1)
OrgPreferences(7-39)ddpui/models/dashboard.py (1)
Dashboard(48-157)ddpui/models/role_based_access.py (2)
Role(5-14)RolePermission(28-38)
🪛 Ruff (0.13.3)
ddpui/api/user_org_api.py
639-639: Unused function argument: request
(ARG001)
703-705: try-except-pass detected, consider logging the exception
(S110)
703-703: Do not catch blind exception: Exception
(BLE001)
752-752: Redefinition of unused get_current_user_v2 from line 65
(F811)
752-752: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (1)
ddpui/api/user_org_api.py (1)
175-176: Casting request.token to str is correct.Ensures consistent token type in response payload.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ddpui/settings.py (1)
128-140: Remove duplicate CommonMiddleware.
django.middleware.common.CommonMiddlewareappears twice (lines 132 and 134), which will cause it to execute twice per request, adding unnecessary overhead and potentially causing unexpected behavior.Apply this diff:
MIDDLEWARE = [ "django_prometheus.middleware.PrometheusBeforeMiddleware", "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware", - "django.middleware.common.CommonMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", "django_prometheus.middleware.PrometheusAfterMiddleware", ]
🧹 Nitpick comments (2)
ddpui/api/user_org_api.py (2)
668-685: Consider setting cookie max_age to align with token lifetimes.The cookies are set without
max_age, making them session cookies that clear on browser close. However, the JWT tokens have longer lifetimes (12 hours for access, 30 days for refresh fromsettings.SIMPLE_JWT). This means users must re-login after closing their browser, even though their tokens remain valid.If persistent login across browser sessions is desired, apply:
# Set access token cookie response.set_cookie( "access_token", token_data["access"], + max_age=int(settings.SIMPLE_JWT["ACCESS_TOKEN_LIFETIME"].total_seconds()), httponly=True, secure=secure_cookies, samesite="lax", path="/", ) # Set refresh token cookie response.set_cookie( "refresh_token", token_data["refresh"], + max_age=int(settings.SIMPLE_JWT["REFRESH_TOKEN_LIFETIME"].total_seconds()), httponly=True, secure=secure_cookies, samesite="lax", path="/", )Alternatively, if session-only cookies are intentional for security, document this behavior and ensure frontend handles re-authentication gracefully.
698-705: Log exceptions instead of silently passing.Catching all exceptions without logging can hide real issues during token blacklisting, making debugging difficult.
Apply this diff:
try: token = RefreshToken(refresh_token) token_user_id = token.payload.get("user_id") if request.user and request.user.id == token_user_id: token.blacklist() - except (TokenError, Exception): + except TokenError as e: + # Token is already invalid or expired, continue with logout + logger.info(f"Token already invalid during logout: {e}") + except Exception as e: # Token is already invalid or expired, continue with logout - pass + logger.warning(f"Unexpected error during token blacklist: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ddpui/api/user_org_api.py(4 hunks)ddpui/settings.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/user_org_api.py (3)
ddpui/models/org_user.py (2)
LoginPayload(206-210)OrgUser(66-86)ddpui/auth.py (4)
CustomTokenObtainSerializer(199-236)CustomTokenRefreshSerializer(239-253)has_permission(30-51)get_token(203-232)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)
🪛 Ruff (0.13.3)
ddpui/api/user_org_api.py
639-639: Unused function argument: request
(ARG001)
703-705: try-except-pass detected, consider logging the exception
(S110)
703-703: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (3)
ddpui/api/user_org_api.py (3)
175-176: LGTM!Stringifying the token ensures consistent string representation in the response, aligning with the v1 login endpoint behavior.
770-776: Verify intended iframe token lifetime.The current implementation consistently uses 2 minutes for the iframe token (comment, code, and
expires_invalue). However, a previous review suggested changing this to 5 minutes. Please confirm which lifetime is correct for iframe tokens.If 5 minutes is the intended lifetime, apply:
- # Override access token expiration to 2 minutes for iframe use - access_token.set_exp(lifetime=timedelta(minutes=2)) + # Override access token expiration to 5 minutes for iframe use + access_token.set_exp(lifetime=timedelta(minutes=5)) return { "success": True, "iframe_token": str(access_token), - "expires_in": 120, # 2 minutes in seconds + "expires_in": 300, # 5 minutes in seconds "org_slug": orguser.org.slug, }
638-640: False positive:requestparameter is used by the framework.The static analysis tool flags
requestas unused (ARG001), but it's actively used by the authentication middleware to populaterequest.userandrequest.orguser(accessed at lines 759-763). This is a standard Django/django-ninja pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ddpui/settings.py (1)
132-134: Duplicate CommonMiddleware and non-standard orderCommonMiddleware appears twice (Line 132 and Line 134). Also, recommended order is SecurityMiddleware → SessionMiddleware → CommonMiddleware. Apply:
MIDDLEWARE = [ "django_prometheus.middleware.PrometheusBeforeMiddleware", "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware", - "django.middleware.common.CommonMiddleware", - "django.contrib.sessions.middleware.SessionMiddleware", - "django.middleware.common.CommonMiddleware", + "django.contrib.sessions.middleware.SessionMiddleware", + "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", "django_prometheus.middleware.PrometheusAfterMiddleware", ]
🧹 Nitpick comments (5)
ddpui/api/user_org_api.py (5)
639-639: Lint: unusedrequestparameterRename to
_requestor add a small use (e.g.,del request) to satisfy ARG001.
707-713: Don’t swallow exceptions silently; log and narrow catchThe bare
except Exception: passhides useful context. Limit to TokenError and log unexpected errors:- if refresh_token: - try: - token = RefreshToken(refresh_token) - token_user_id = token.payload.get("user_id") - if request.user and request.user.id == token_user_id: - token.blacklist() - except (TokenError, Exception): - # Token is already invalid or expired, continue with logout - pass + if refresh_token: + try: + token = RefreshToken(refresh_token) + token_user_id = token.payload.get("user_id") + if request.user and request.user.id == token_user_id: + token.blacklist() + except TokenError: + # Already invalid/expired; continue + logger.debug("Refresh token invalid/expired during logout") + except Exception as err: + logger.warning(f"Unexpected error blacklisting refresh token: {err}")
717-747: CSRF also applies to refresh; ensure header/cookie flowPOST /v2/token/refresh will also require X-CSRFToken under CsrfViewMiddleware. After adding a CSRF bootstrap endpoint, document that clients must send the header here too. Cookie setting looks fine; rotation of refresh isn’t attempted, which is acceptable given your serializer behavior.
752-752: PEP 604 typing for optional paramUse
str | Nonefor Optional per modern typing:-def get_currentuser_v2(request, org_slug: str = None): +def get_currentuser_v2(request, org_slug: str | None = None):
844-851: Iframe token TTL is consistent (2 minutes)Comment and expires_in now align. Consider making TTL configurable via env for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ddpui/api/user_org_api.py(4 hunks)ddpui/settings.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/user_org_api.py (7)
ddpui/models/org_user.py (3)
LoginPayload(206-210)OrgUserResponse(123-135)OrgUser(66-86)ddpui/auth.py (3)
CustomTokenObtainSerializer(199-236)CustomTokenRefreshSerializer(239-253)get_token(203-232)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)ddpui/models/org.py (4)
Org(61-84)OrgWarehouse(125-146)base_plan(82-84)OrgType(8-18)ddpui/models/org_preferences.py (1)
OrgPreferences(7-39)ddpui/models/dashboard.py (1)
Dashboard(48-157)ddpui/models/role_based_access.py (2)
Role(5-14)RolePermission(28-38)
🪛 Ruff (0.13.3)
ddpui/api/user_org_api.py
639-639: Unused function argument: request
(ARG001)
703-705: try-except-pass detected, consider logging the exception
(S110)
703-703: Do not catch blind exception: Exception
(BLE001)
752-752: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (2)
ddpui/settings.py (2)
78-91: Good move to origin-allowlist and credentials enabledExplicit CORS_ALLOWED_ORIGINS with CORS_ALLOW_CREDENTIALS=True is required for cookie auth. Looks correct.
100-103: CSRF settings align with cookie-based flowTrusting explicit origins and allowing X-CSRFToken header is right. Tuple merge with default_headers is fine.
Please confirm frontend sends the X-CSRFToken header on unsafe requests, and that a CSRF cookie is issued prior to login (e.g., via an “ensure CSRF” GET endpoint).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ddpui/api/user_org_api.py (2)
700-731: Refresh endpoint: add max_age and consider CSRF bootstrap
- Set access_token cookie max_age to ACCESS_TOKEN_LIFETIME, as above.
- With CsrfViewMiddleware, POST to cookie-auth endpoints needs a CSRF cookie + X-CSRFToken; add a GET /v2/csrf/ bootstrap (see below).
Apply:
# Determine if we should use secure cookies secure_cookies = not settings.DEBUG + + # Align cookie lifetime with SIMPLE_JWT settings if available + cfg = getattr(settings, "SIMPLE_JWT", {}) + access_max_age = int(cfg["ACCESS_TOKEN_LIFETIME"].total_seconds()) if cfg.get("ACCESS_TOKEN_LIFETIME") else None # Set new access token cookie response.set_cookie( "access_token", token_data["access"], httponly=True, secure=secure_cookies, - samesite="lax", + samesite="lax", path="/", + max_age=access_max_age, )Add CSRF bootstrap endpoint (same module):
from django.views.decorators.csrf import ensure_csrf_cookie @user_org_router.get("/v2/csrf/", auth=None) @ensure_csrf_cookie def get_csrf(_request): return JsonResponse({"success": True})
663-669: Response fields don’t match lookup_user; return all user fieldsorg_users and wtype aren’t provided by lookup_user, while real fields (email_verified, active, can_create_orgs, is_consultant, is_platform_admin) are omitted. Include the full payload.
Apply:
- # Don't include tokens in response body - response_data = { - "success": True, - "email": retval.get("email"), - "org_users": retval.get("org_users"), - "wtype": retval.get("wtype"), - } + # Don't include tokens in response body; include all user data + response_data = { + "success": True, + **retval, + }
🧹 Nitpick comments (2)
ddpui/api/user_org_api.py (2)
648-649: Silence ARG001: unused request paramRuff flags unused request. Since it’s not used, rename to _request (consistent with other endpoints using pylint disables).
Apply:
-@user_org_router.post("/v2/login/", auth=None, response={200: dict}) -def post_login_v2(request, payload: LoginPayload): +@user_org_router.post("/v2/login/", auth=None, response={200: dict}) +def post_login_v2(_request, payload: LoginPayload):
677-695: Harden cookie settings: align lifetimes and verify SameSite
- Set cookie max_age to JWT lifetimes so browser eviction aligns with token TTLs.
- Verify samesite="lax" works with your frontend origin; cross-site XHR generally requires samesite="none" and secure=True.
Apply:
# Create JsonResponse and set cookies response = JsonResponse(response_data) # Determine if we should use secure cookies (HTTPS only in production) secure_cookies = not settings.DEBUG + + # Align cookie lifetimes with SIMPLE_JWT settings if available + cfg = getattr(settings, "SIMPLE_JWT", {}) + access_max_age = int(cfg["ACCESS_TOKEN_LIFETIME"].total_seconds()) if cfg.get("ACCESS_TOKEN_LIFETIME") else None + refresh_max_age = int(cfg["REFRESH_TOKEN_LIFETIME"].total_seconds()) if cfg.get("REFRESH_TOKEN_LIFETIME") else None # Set access token cookie response.set_cookie( "access_token", token_data["access"], httponly=True, secure=secure_cookies, - samesite="lax", + samesite="lax", path="/", + max_age=access_max_age, ) # Set refresh token cookie response.set_cookie( "refresh_token", token_data["refresh"], httponly=True, secure=secure_cookies, - samesite="lax", + samesite="lax", path="/", + max_age=refresh_max_age, )To verify SameSite needs with your deployment, confirm whether the frontend origin differs from the API origin and if cross-site XHR is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/api/user_org_api.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/user_org_api.py (3)
ddpui/models/org_user.py (3)
LogoutPayload(213-216)LoginPayload(206-210)OrgUser(66-86)ddpui/auth.py (4)
CustomTokenObtainSerializer(199-236)CustomTokenRefreshSerializer(239-253)has_permission(30-51)get_token(203-232)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)
🪛 Ruff (0.14.0)
ddpui/api/user_org_api.py
202-204: try-except-pass detected, consider logging the exception
(S110)
202-202: Do not catch blind exception: Exception
(BLE001)
649-649: Unused function argument: request
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (2)
ddpui/api/user_org_api.py (2)
175-176: Good fix: ensure token is a stringCasting request.token to str prevents serialization issues. LGTM.
753-760: Iframe token TTL consistentComment, token lifetime, and expires_in all set to 2 minutes. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ddpui/api/user_org_api.py (3)
186-208: Don’t swallow logout errors; log and narrow exceptions. Also align delete_cookie attrsCatching Exception and passing masks real issues; log and split excepts. Ensure delete_cookie uses the same samesite as when set.
- if refresh_token: - try: - token = RefreshToken(refresh_token) - token_user_id = token.payload.get("user_id") - if request.user and request.user.id == token_user_id: - token.blacklist() - except (TokenError, Exception): - # Token is already invalid/expired or other error, continue with logout - pass + if refresh_token: + try: + token = RefreshToken(refresh_token) + token_user_id = token.payload.get("user_id") + if request.user and request.user.id == token_user_id: + token.blacklist() + except TokenError as exc: + # Token already invalid/expired; continue + logger.info("post_logout: refresh token invalid/expired: %s", exc) + except Exception: + # Unexpected error; continue but record it + logger.exception("post_logout: unexpected error during token blacklist")Tip: if cookies were set with samesite="None", delete_cookie(samesite="lax") won’t match. Use the same samesite value for deletion.
638-646: CSRF will 403 on POST /v2/login without a prior CSRF cookieWith CsrfViewMiddleware, clients need a CSRF cookie + X-CSRFToken. Add a GET /v2/csrf/ endpoint to issue the cookie (preferred), or exempt login (less secure).
from django.views.decorators.csrf import ensure_csrf_cookie @user_org_router.get("/v2/csrf/", auth=None) @ensure_csrf_cookie def get_csrf(_request): return JsonResponse({"success": True})
653-659: Response fields don’t match lookup_user; returns incomplete/incorrect datalookup_user returns email, email_verified, active, can_create_orgs, is_consultant, is_platform_admin. You’re returning org_users/wtype which aren’t present.
- response_data = { - "success": True, - "email": retval.get("email"), - "org_users": retval.get("org_users"), - "wtype": retval.get("wtype"), - } + response_data = { + "success": True, + **retval, # include all fields from lookup_user + }
🧹 Nitpick comments (4)
ddpui/api/user_org_api.py (4)
15-15: Remove unused AccessToken importAccessToken isn’t used; drop it to appease linters.
-from rest_framework_simplejwt.tokens import RefreshToken, TokenError, AccessToken +from rest_framework_simplejwt.tokens import RefreshToken, TokenError
664-685: Cookie attributes: consider settings-driven samesite/secure and add max_ageSameSite=lax may block set-cookie/send on cross-site flows; many SPAs require SameSite=None; also align cookie lifetime with token TTL.
- # Determine if we should use secure cookies (HTTPS only in production) - secure_cookies = not settings.DEBUG + # Use settings to drive cookie attributes + secure_cookies = getattr(settings, "SESSION_COOKIE_SECURE", not settings.DEBUG) + samesite = getattr(settings, "SESSION_COOKIE_SAMESITE", "lax") # set to "None" for cross-site + # Optional: align cookie lifetime with SIMPLE_JWT + sj = getattr(settings, "SIMPLE_JWT", {}) + access_max_age = int(sj.get("ACCESS_TOKEN_LIFETIME", timedelta(minutes=5)).total_seconds()) + refresh_max_age = int(sj.get("REFRESH_TOKEN_LIFETIME", timedelta(days=1)).total_seconds()) # Set access token cookie response.set_cookie( "access_token", token_data["access"], httponly=True, - secure=secure_cookies, - samesite="lax", + secure=secure_cookies, + samesite=samesite, + max_age=access_max_age, path="/", ) # Set refresh token cookie response.set_cookie( "refresh_token", token_data["refresh"], httponly=True, - secure=secure_cookies, - samesite="lax", + secure=secure_cookies, + samesite=samesite, + max_age=refresh_max_age, path="/", )Please confirm your deployment is same-site (Lax OK). If cross-site, switch to SameSite=None and ensure HTTPS.
639-639: Unused parameter in handlerRename to _request or silence linter; framework still injects the request.
-def post_login_v2(request, payload: LoginPayload): +def post_login_v2(_request, payload: LoginPayload): # pylint: disable=unused-argument
710-718: Mirror cookie attrs and TTLs on refresh responseUse the same samesite/secure and add max_age for access cookie to match SIMPLE_JWT ACCESS_TOKEN_LIFETIME.
- # Determine if we should use secure cookies - secure_cookies = not settings.DEBUG + # Mirror login cookie settings + secure_cookies = getattr(settings, "SESSION_COOKIE_SECURE", not settings.DEBUG) + samesite = getattr(settings, "SESSION_COOKIE_SAMESITE", "lax") + sj = getattr(settings, "SIMPLE_JWT", {}) + access_max_age = int(sj.get("ACCESS_TOKEN_LIFETIME", timedelta(minutes=5)).total_seconds()) # Set new access token cookie response.set_cookie( "access_token", token_data["access"], httponly=True, - secure=secure_cookies, - samesite="lax", + secure=secure_cookies, + samesite=samesite, + max_age=access_max_age, path="/", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/api/user_org_api.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/user_org_api.py (3)
ddpui/models/org_user.py (2)
LoginPayload(206-210)OrgUser(66-86)ddpui/auth.py (3)
CustomTokenObtainSerializer(199-236)CustomTokenRefreshSerializer(239-253)get_token(203-232)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)
🪛 Ruff (0.14.0)
ddpui/api/user_org_api.py
196-198: try-except-pass detected, consider logging the exception
(S110)
196-196: Do not catch blind exception: Exception
(BLE001)
639-639: Unused function argument: request
(ARG001)
🔇 Additional comments (2)
ddpui/api/user_org_api.py (2)
174-176: Good fix: ensure token is stringCoercing request.token to str avoids serialization issues.
743-750: Iframe token TTL is consistent2-minute lifetime matches comment and expires_in. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ddpui/settings.py (2)
77-99: Add missing CSRF settings for cookie-based authentication.While CORS is correctly configured for credentials, the CSRF protection settings are still missing. Django's
CsrfViewMiddleware(enabled at line 131) requires these settings to safely handle cross-origin POST requests with cookies.Add after line 90:
] + +# CSRF settings for cookie-based authentication +CSRF_TRUSTED_ORIGINS = CORS_ALLOWED_ORIGINS +CSRF_COOKIE_SECURE = not DEBUG +CSRF_COOKIE_SAMESITE = "Lax" +CSRF_COOKIE_HTTPONLY = False # Must be False so JavaScript can read the tokenNote:
CSRF_COOKIE_HTTPONLYmust beFalsebecause the frontend needs to read the CSRF token from the cookie and send it in theX-CSRFTokenheader. This is different from the session cookies (access_token, refresh_token) which should remain httpOnly.Based on past review comments.
124-136: Remove duplicate CommonMiddleware entry.
CommonMiddlewareappears twice (lines 128 and 130), which will cause it to run twice per request.Apply this diff to remove the duplicate at line 128:
MIDDLEWARE = [ "django_prometheus.middleware.PrometheusBeforeMiddleware", "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware", - "django.middleware.common.CommonMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", "django_prometheus.middleware.PrometheusAfterMiddleware", ]Based on past review comments.
♻️ Duplicate comments (2)
ddpui/api/user_org_api.py (2)
662-668: Fix response data to match lookup_user return values.The response references
org_usersandwtypefields that are not returned byorguserfunctions.lookup_user(). According to the code snippet fromddpui/core/orguserfunctions.py(lines 41-72),lookup_userreturns:email_verified,active,can_create_orgs,is_consultant,is_platform_admin.This will cause a runtime error when accessing these non-existent fields.
Apply this diff to return the correct fields:
# Don't include tokens in response body response_data = { "success": True, - "email": retval.get("email"), - "org_users": retval.get("org_users"), - "wtype": retval.get("wtype"), + **retval, # Include all fields from lookup_user }Based on past review comments.
199-207: Log exceptions instead of silently swallowing them.Catching all exceptions with
passmasks real errors and makes debugging difficult. The refresh token might fail for expected reasons (invalid/expired) or unexpected errors (database issues, etc.).Apply this diff to add proper logging:
+import logging + +logger_auth = logging.getLogger(__name__) + # Try to blacklist the refresh token if we have one if refresh_token: try: token = RefreshToken(refresh_token) token_user_id = token.payload.get("user_id") if request.user and request.user.id == token_user_id: token.blacklist() - except (TokenError, Exception): - # Token is already invalid/expired or other error, continue with logout - pass + except TokenError as exc: + # Token already invalid/expired; continue with logout + logger_auth.info("post_logout: refresh token invalid/expired: %s", exc) + except Exception: + # Unexpected error; log but continue with logout + logger_auth.exception("post_logout: unexpected error during token blacklist")Based on past review comments and static analysis hints (S110, BLE001).
🧹 Nitpick comments (3)
ddpui/settings.py (1)
294-297: Consider renaming for clarity and note the CSRF cookie distinction.These settings control application cookies (access_token, refresh_token), but the naming could be clearer.
Consider renaming to make the purpose explicit:
-# Cookie settings -COOKIE_SECURE = os.getenv("ENVIRONMENT", "") in ["production", "staging"] -COOKIE_SAMESITE = "Lax" -COOKIE_HTTPONLY = True +# Application cookie settings (for access_token and refresh_token) +SESSION_COOKIE_SECURE = os.getenv("ENVIRONMENT", "") in ["production", "staging"] +SESSION_COOKIE_SAMESITE = "Lax" +SESSION_COOKIE_HTTPONLY = TrueThen update references in
ddpui/api/user_org_api.pyto use these renamed settings (e.g.,settings.SESSION_COOKIE_SECUREinstead ofsettings.COOKIE_SECURE).Important: Keep these separate from CSRF cookie settings. CSRF cookies must have
httponly=Falseso JavaScript can read the token, while session cookies should remainhttponly=Truefor security.ddpui/api/user_org_api.py (2)
674-674: Remove unused variable.The variable
secure_cookiesis assigned but never used.Apply this diff:
- # Determine if we should use secure cookies (HTTPS only in production) - secure_cookies = not settings.DEBUG - # Set access token cookieBased on static analysis (F841).
716-718: Remove unused variable.The variable
secure_cookiesis assigned but never used.Apply this diff:
- # Determine if we should use secure cookies - secure_cookies = not settings.DEBUG - # Set new access token cookieBased on static analysis (F841).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ddpui/api/user_org_api.py(4 hunks)ddpui/settings.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/user_org_api.py (2)
ddpui/auth.py (3)
CustomTokenObtainSerializer(199-236)get_token(203-232)CustomTokenRefreshSerializer(239-253)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)
🪛 Ruff (0.14.0)
ddpui/api/user_org_api.py
205-207: try-except-pass detected, consider logging the exception
(S110)
205-205: Do not catch blind exception: Exception
(BLE001)
648-648: Unused function argument: request
(ARG001)
674-674: Local variable secure_cookies is assigned to but never used
Remove assignment to unused variable secure_cookies
(F841)
717-717: Local variable secure_cookies is assigned to but never used
Remove assignment to unused variable secure_cookies
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (1)
ddpui/api/user_org_api.py (1)
752-758: Verify comment matches implementation.The implementation sets a 2-minute expiration (line 753) with
expires_in: 120seconds (line 758), which is consistent. However, if the comment at line 752 still mentions "5 minutes" (as noted in past reviews), it should be updated to match the implementation.If the comment says "5 minutes", update it:
- # Override access token expiration to 5 minutes for iframe use + # Override access token expiration to 2 minutes for iframe use access_token.set_exp(lifetime=timedelta(minutes=2))Based on past review comments.
ddpui/api/user_org_api.py
Outdated
| @user_org_router.post("/v2/login/", auth=None, response={200: dict}) | ||
| def post_login_v2(request, payload: LoginPayload): | ||
| """Login endpoint that sets httpOnly cookies instead of returning tokens in response""" | ||
| serializer = CustomTokenObtainSerializer( | ||
| data={ | ||
| "username": payload.username, | ||
| "password": payload.password, | ||
| } | ||
| ) | ||
| serializer.is_valid(raise_exception=True) | ||
| token_data = serializer.validated_data | ||
|
|
||
| # Get user data (same as v1) | ||
| retval = orguserfunctions.lookup_user(payload.username) | ||
|
|
||
| # Don't include tokens in response body | ||
| response_data = { | ||
| "success": True, | ||
| "email": retval.get("email"), | ||
| "org_users": retval.get("org_users"), | ||
| "wtype": retval.get("wtype"), | ||
| } | ||
|
|
||
| # Create JsonResponse and set cookies | ||
| response = JsonResponse(response_data) | ||
|
|
||
| # Determine if we should use secure cookies (HTTPS only in production) | ||
| secure_cookies = not settings.DEBUG | ||
|
|
||
| # Set access token cookie | ||
| response.set_cookie( | ||
| "access_token", | ||
| token_data["access"], | ||
| httponly=settings.COOKIE_HTTPONLY, | ||
| secure=settings.COOKIE_SECURE, | ||
| samesite=settings.COOKIE_SAMESITE, | ||
| path="/", | ||
| ) | ||
|
|
||
| # Set refresh token cookie | ||
| response.set_cookie( | ||
| "refresh_token", | ||
| token_data["refresh"], | ||
| httponly=settings.COOKIE_HTTPONLY, | ||
| secure=settings.COOKIE_SECURE, | ||
| samesite=settings.COOKIE_SAMESITE, | ||
| path="/", | ||
| ) | ||
|
|
||
| return response |
There was a problem hiding this comment.
Add CSRF protection handling for v2 login endpoint.
The POST /v2/login/ endpoint has auth=None (line 647), but Django's CsrfViewMiddleware is enabled (settings.py line 131). This means:
- Current state: The endpoint will reject POST requests without a valid CSRF token, which clients won't have before logging in.
- Solution options:
- Option A (Recommended): Add a public CSRF token endpoint that clients call before login
- Option B: Exempt the login endpoint with
@csrf_exempt(less secure but acceptable for login endpoints)
Recommended approach (Option A):
Add a CSRF bootstrap endpoint:
from django.views.decorators.csrf import ensure_csrf_cookie
@user_org_router.get("/v2/csrf/", auth=None)
@ensure_csrf_cookie
def get_csrf_token(request):
"""Bootstrap CSRF token for cookie-based authentication"""
return JsonResponse({"success": True})Then update client flow to:
GET /v2/csrf/to receive CSRF cookiePOST /v2/login/withX-CSRFTokenheader
Alternative approach (Option B):
from django.views.decorators.csrf import csrf_exempt
@user_org_router.post("/v2/login/", auth=None, response={200: dict})
@csrf_exempt
def post_login_v2(request, payload: LoginPayload):
# ... existing codeBased on past review comments.
🧰 Tools
🪛 Ruff (0.14.0)
648-648: Unused function argument: request
(ARG001)
674-674: Local variable secure_cookies is assigned to but never used
Remove assignment to unused variable secure_cookies
(F841)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ddpui/api/user_org_api.py (2)
202-207: Log and narrow exception handling.This broad exception handling (catching both
TokenErrorandExceptionwithpass) masks real errors and was flagged in a previous review but remains unaddressed.Apply this diff to log exceptions while preserving logout behavior:
- try: - token = RefreshToken(refresh_token) - token_user_id = token.payload.get("user_id") - if request.user and request.user.id == token_user_id: - token.blacklist() - except (TokenError, Exception): - # Token is already invalid/expired or other error, continue with logout - pass + try: + token = RefreshToken(refresh_token) + token_user_id = token.payload.get("user_id") + if request.user and request.user.id == token_user_id: + token.blacklist() + except TokenError as exc: + # Token already invalid/expired; continue with logout + logger.info("post_logout: refresh token invalid/expired: %s", exc) + except Exception: + # Unexpected error; continue but record it + logger.exception("post_logout: unexpected error during token blacklist")Based on past review comments.
647-685: CSRF protection required for v2 login endpoint.This POST endpoint with
auth=Nonewill be rejected by Django'sCsrfViewMiddleware(enabled in settings). Clients won't have a CSRF token before logging in. This critical issue was flagged in a previous review but remains unaddressed.Recommended approach (Option A): Add a CSRF bootstrap endpoint:
from django.views.decorators.csrf import ensure_csrf_cookie @user_org_router.get("/v2/csrf/", auth=None) @ensure_csrf_cookie def get_csrf_token(request): """Bootstrap CSRF token for cookie-based authentication""" return JsonResponse({"success": True})Then update client flow to:
GET /v2/csrf/to receive CSRF cookiePOST /v2/login/withX-CSRFTokenheaderAlternative approach (Option B): Exempt the endpoint:
from django.views.decorators.csrf import csrf_exempt @user_org_router.post("/v2/login/", auth=None) @csrf_exempt def post_login_v2(request, payload: LoginPayload): # ... existing codeBased on past review comments.
🧹 Nitpick comments (2)
ddpui/api/user_org_api.py (2)
648-648: Remove unusedrequestparameter or use it for logging.The
requestparameter is unused. If you don't need it for logging or other purposes, remove it from the signature.-def post_login_v2(request, payload: LoginPayload): +def post_login_v2(payload: LoginPayload):
706-706: Remove unused variable.The
secure_cookiesvariable is assigned but never used.- # Determine if we should use secure cookies - secure_cookies = not settings.DEBUG - # Set new access token cookie
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/api/user_org_api.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/user_org_api.py (3)
ddpui/auth.py (3)
CustomTokenObtainSerializer(199-236)get_token(203-232)CustomTokenRefreshSerializer(239-253)ddpui/core/orguserfunctions.py (1)
lookup_user(42-73)ddpui/models/org_user.py (2)
LoginPayload(206-210)OrgUser(66-86)
🪛 Ruff (0.14.0)
ddpui/api/user_org_api.py
205-207: try-except-pass detected, consider logging the exception
(S110)
205-205: Do not catch blind exception: Exception
(BLE001)
648-648: Unused function argument: request
(ARG001)
706-706: Local variable secure_cookies is assigned to but never used
Remove assignment to unused variable secure_cookies
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (2)
ddpui/api/user_org_api.py (2)
167-186: LGTM!The token generation logic correctly creates a new session token with standard expiry. The use of
CustomTokenObtainSerializer.get_token()ensures custom claims (likeorguser_role_key) are included in the token.
721-749: LGTM!The iframe token implementation correctly:
- Generates a short-lived token (2 minutes) for iframe communication
- Uses the same token generation logic as login to include custom claims
- Returns consistent expiry information (120 seconds matches 2-minute lifetime)
- Includes org_slug for context
Summary by CodeRabbit
New Features
Security
Chores