fix: allow team members to access auth page and improve auth API response handling#349
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR standardizes error handling for the project auth flow: ChangesAuth page access and response flow
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant AuthPage
participant DashboardAPI
participant loadProjectForAdmin
AuthPage->>DashboardAPI: request project/auth details
DashboardAPI->>loadProjectForAdmin: validate projectId format and access
loadProjectForAdmin-->>DashboardAPI: project or 400/404 AppError
DashboardAPI-->>AuthPage: sanitized project or { success: false, data: {}, message }
AuthPage->>AuthPage: unwrapApiResponse and check ok flag
AuthPage-->>AuthPage: render project/users or detailed error toast
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web-dashboard/src/pages/Auth.jsx (1)
101-139: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStale/unmounted requests can still surface a toast error.
The success path for the users fetch correctly guards against races with
if (!isMounted || requestId !== latestUsersRequestId.current) return;(Line 127) before updating state. But the!usersResult.okbranch (Lines 123-125) throws immediately, without this guard, so a stale or post-unmount failure still reaches the outercatchand firestoast.error(Line 137) for a request that's no longer relevant (e.g. user changedpage/limitquickly, or navigated away). Thefinallyblock already guardssetLoadingwithisMountedbut the toast call has no equivalent guard.🐛 Proposed fix
const usersResult = unwrapApiResponse(usersRes.data); - if (!usersResult.ok) { + if (!isMounted || requestId !== latestUsersRequestId.current) return; + + if (!usersResult.ok) { throw new Error(usersResult.message || 'Failed to load auth users'); } - if (!isMounted || requestId !== latestUsersRequestId.current) return; setUsers(normalizeUsersResponse(usersResult.data));And guard the toast itself as a defense-in-depth measure:
} catch (err) { - toast.error(err?.response?.data?.message || err?.response?.data?.error || err.message || "Failed to load auth details"); + if (isMounted) { + toast.error(err?.response?.data?.message || err?.response?.data?.error || err.message || "Failed to load auth details"); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-dashboard/src/pages/Auth.jsx` around lines 101 - 139, The Auth.jsx data-loading effect can still show a toast for stale or unmounted requests because the error path in the users fetch throws before the request-race guard is checked. Update the useEffect fetch logic around fetchData, unwrapApiResponse, and latestUsersRequestId so that failures from the admin users request are ignored when isMounted is false or the requestId no longer matches, instead of bubbling to the outer catch. Also add an isMounted check before calling toast.error in the catch block as a defense-in-depth guard.sdks/urbackend-react/src/components/UrAuth.tsx (2)
490-499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrong/non-localized subtitle text on forgot-password screen.
For
mode === 'forgot', the subtitle displaystext.loginTitle— the sign-in headline — under a "Forgot Password" heading, which is confusing content unrelated to the flow. Additionally, the reset-mode text`Enter the code sent to ${email}`is hardcoded inline rather than sourced from thetextlabels object like every other string in this component, breaking the established i18n/customization pattern.🐛 Proposed fix
- {mode === 'forgot' ? text.loginTitle : `Enter the code sent to ${email}`} + {mode === 'forgot' ? 'Enter your email to receive a reset code' : `Enter the code sent to ${email}`}Consider reintroducing a dedicated
forgotSubtitle/resetSubtitlefield inAuthLabelsso both strings remain customizable/localizable like the rest of the component's text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/urbackend-react/src/components/UrAuth.tsx` around lines 490 - 499, The subtitle logic in UrAuth is using the wrong label for the forgot flow and hardcoding the reset flow copy. Update the conditional inside the forgot/reset header block to use dedicated localized fields from the AuthLabels/text object instead of text.loginTitle and the inline email string. Add or restore forgotSubtitle and resetSubtitle in the labels type/config, and reference those fields in the UrAuth component so both modes stay customizable and consistent with the rest of the i18n pattern.
451-465: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHeader hidden by default when no
brandingis provided.The header block is gated on
branding?.logo || branding?.brandName || branding?.title || branding?.subtitle. Sincebrandinghas no default value, most consumers who don't pass any branding prop will never see the header at all — even thoughbrandName(defaults to'urBackend'),headerTitle, andheaderSubtitle(mode-aware fallback text) are always computed with sensible defaults. Those defaults become dead code in the common no-branding case.🐛 Proposed fix: gate on auth-method availability instead of branding presence
- {(branding?.logo || branding?.brandName || branding?.title || branding?.subtitle) && ( + {(hasPasswordAuth || hasSocialAuth) && ( <div style={styles.header}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/urbackend-react/src/components/UrAuth.tsx` around lines 451 - 465, The header in UrAuth is incorrectly hidden when no branding prop is provided, because the current conditional only checks branding fields and bypasses the always-available fallback text. Update the header render guard in UrAuth to depend on auth-method/header availability instead of branding presence, so the computed defaults in brandName, headerTitle, and headerSubtitle are still shown for the common no-branding case. Keep the existing header content and fallback calculations, but move the visibility condition to the appropriate auth-mode logic used by UrAuth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Line 456: The success response in the project controller is still returning
the raw sanitized project object instead of the standard `{ success, data,
message }` envelope. Update the success path in `project.controller.js` (the
`res.json(sanitizeProjectResponse(projectObj))` return) so it matches the same
response contract used by the catch block, wrapping
`sanitizeProjectResponse(projectObj)` under `data` with `success: true` and an
appropriate `message`, keeping the API shape consistent for `Auth.jsx` and other
callers.
- Around line 458-466: The generic error path in project.controller.js is
exposing raw internal exception text to clients. Update the catch handling
around the existing AppError branch in the controller action to avoid returning
err.message for non-AppError cases; instead log the original error server-side
and send a safe generic message (or wrap it in a new AppError) from the response
path. Keep the AppError response behavior unchanged, and use the controller’s
existing error handling flow to ensure MongoDB/Mongoose details never reach the
client.
In `@sdks/urbackend-react/src/components/UrAuth.tsx`:
- Around line 313-326: The primaryBtn gradient in UrAuth.tsx contains a dead
ternary where both branches return the same end color, so the theme check has no
effect. Update the gradient logic in the primaryBtn style to use a distinct
dark-mode end color in the theme === 'dark' branch, and make sure the chosen
value still provides sufficient contrast with themeColors.primaryText in dark
mode. Verify the button appearance in both themes after adjusting the
theme-aware background.
---
Outside diff comments:
In `@apps/web-dashboard/src/pages/Auth.jsx`:
- Around line 101-139: The Auth.jsx data-loading effect can still show a toast
for stale or unmounted requests because the error path in the users fetch throws
before the request-race guard is checked. Update the useEffect fetch logic
around fetchData, unwrapApiResponse, and latestUsersRequestId so that failures
from the admin users request are ignored when isMounted is false or the
requestId no longer matches, instead of bubbling to the outer catch. Also add an
isMounted check before calling toast.error in the catch block as a
defense-in-depth guard.
In `@sdks/urbackend-react/src/components/UrAuth.tsx`:
- Around line 490-499: The subtitle logic in UrAuth is using the wrong label for
the forgot flow and hardcoding the reset flow copy. Update the conditional
inside the forgot/reset header block to use dedicated localized fields from the
AuthLabels/text object instead of text.loginTitle and the inline email string.
Add or restore forgotSubtitle and resetSubtitle in the labels type/config, and
reference those fields in the UrAuth component so both modes stay customizable
and consistent with the rest of the i18n pattern.
- Around line 451-465: The header in UrAuth is incorrectly hidden when no
branding prop is provided, because the current conditional only checks branding
fields and bypasses the always-available fallback text. Update the header render
guard in UrAuth to depend on auth-method/header availability instead of branding
presence, so the computed defaults in brandName, headerTitle, and headerSubtitle
are still shown for the common no-branding case. Keep the existing header
content and fallback calculations, but move the visibility condition to the
appropriate auth-mode logic used by UrAuth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c7dade0-d841-4915-8f4c-0b912be088ae
📒 Files selected for processing (3)
apps/dashboard-api/src/controllers/project.controller.jsapps/web-dashboard/src/pages/Auth.jsxsdks/urbackend-react/src/components/UrAuth.tsx
There was a problem hiding this comment.
why there are changes in SDK.
@Nitin-kumar-yadav1307
| if (!getProjectRole(projectObj, req.user._id)) { | ||
| throw new AppError(403, "Access denied."); | ||
| } | ||
|
|
There was a problem hiding this comment.
why removed role check?
isnt this incorrect??
|
@yash-pouranik i think i over exxeratted the PR, little bit confused in urAuth.tsx, thats actually unrelated to the project auth page , thnks for pointing it out bro, my bad |
This reverts commit 03ffc5d.
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)
apps/dashboard-api/src/controllers/project.controller.js (1)
431-454: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winReapply project access checks on cache hits.
getProjectById()returns a shared cached project, butgetProjectAccessQuery(req.user._id)only runs on the cache-miss path. Add an access check before returning cached data so one user’s cached project can’t be read by another authenticated user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 431 - 454, getSingleProject currently trusts getProjectById() on cache hits without rechecking ownership or access, so the cached project can be returned to an unauthorized user. Add a permission check in getSingleProject before using a cached projectObj, using getProjectAccessQuery(req.user._id) or equivalent logic to verify the current user can access req.params.projectId. Keep the existing Project.findOne fallback for cache misses, and only return cached data after the access check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 431-454: getSingleProject currently trusts getProjectById() on
cache hits without rechecking ownership or access, so the cached project can be
returned to an unauthorized user. Add a permission check in getSingleProject
before using a cached projectObj, using getProjectAccessQuery(req.user._id) or
equivalent logic to verify the current user can access req.params.projectId.
Keep the existing Project.findOne fallback for cache misses, and only return
cached data after the access check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 151262fe-74f5-4600-bf60-4fbe1f0cdfe6
📒 Files selected for processing (2)
apps/dashboard-api/src/controllers/project.controller.jssdks/urbackend-react/src/components/UrAuth.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- sdks/urbackend-react/src/components/UrAuth.tsx
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
still u have not rreverted changes done in sdk.
@Nitin-kumar-yadav1307
| if (!getProjectRole(projectObj, req.user._id)) { | ||
| throw new AppError(403, "Access denied."); | ||
| } | ||
|
|
|
@yash-pouranik i did git revert HEAD but doesnt it didnt appllied? |
|
@yash-pouranik |
| if (!projectId) return next(new AppError(400, "Project ID is required")); | ||
|
|
||
| const project = await Project.findOne({ _id: projectId, owner: req.user._id }); | ||
| const project = await Project.findOne({ | ||
| _id: projectId, | ||
| ...getProjectAccessQuery(req.user._id), |
Summary
This PR fixes the auth page accessibility issue for project team members.
Previously, when a project admin or viewer opened the auth page, the server could return errors such as:
Instead, authorized team members can now retrieve the project auth details correctly.
Additionally, the auth API response handling has been improved on the frontend to properly process both success and error responses.
Changes
Backend
Frontend
SDK
UrAuthcomponent to work with the new API response flow.Files Changed
apps/dashboard-api/src/controllers/project.controller.jsapps/web-dashboard/src/pages/Auth.jsxsdks/urbackend-react/src/components/UrAuth.tsxIssue
Closes #348
Summary by CodeRabbit