feat(perms): route-level permissions (backend gates + frontend wiring)#142
Merged
Conversation
Extract pydantic schemas into per-package schema.py modules where applicable (submission, user). team/export/settings have no inline schemas. Each package __init__.py explicitly re-exports endpoints so the forms_pro.api.<family>.<endpoint> whitelist URLs continue to resolve. Also update test_submission_validation.py to import private helpers from forms_pro.api.submission.endpoints (the public __init__.py exposes only whitelisted endpoints).
- test_form_field.py → doctype/form_field/ (Frappe doctype-test convention). - test_invitations.py → api/team/, test_submission_validation.py → api/submission/, test_export.py → api/export/ (co-located with their endpoint families). - test_roles.py and tests/factories/ stay central (cross-cutting). Also update test_export.py's monkeypatch path to forms_pro.api.export.endpoints.DataExporter — the public __init__.py no longer re-exports the imported symbol.
Apply @require_permission decorator to get_form_shared_with, remove_form_access, add_form_access, set_form_permission. Removes the manual frappe.has_permission + frappe.throw block in each. Behaviour diffs: - Missing form now raises DoesNotExistError (HTTP 404). Previously the endpoint would fall through to a downstream LinkValidationError or a ValidationError-as-deny via has_permission(missing). - get_form_shared_with and remove_form_access now raise PermissionError (HTTP 403) on deny. Previously both raised the default ValidationError (HTTP 417) because frappe.throw was called without an exc class. Same Form, same ptype, same docname source — the decorator is a strict behaviour-preserving wrapper save for the two corrections above.
Apply @require_permission to get_team_members, invite_team_members, toggle_can_edit_team, save, and remove_member_from_team. Each preserves its existing doctype + ptype semantics. Adds HTTP 404 on missing team via the decorator's existence check (previously a downstream Frappe LinkValidationError or DoesNotExistError from frappe.get_doc). Skipped (intentionally): create_team (no docname yet), switch_team (session mutation; existing read check kept inline), get_team_forms (no current gate), add_member_to_team_via_invitation (invitation-token auth model differs).
…sion
Apply @require_permission("Form", "read", param="form_id") to
get_user_submissions and get_all_submissions. Drop manual FP Team
write check + 404 throw from get_all_submissions, and the dead
Guest early-return from get_user_submissions (endpoint is not
allow_guest).
get_submission and get_submission_response retain their manual
permission checks: their target doctype is dynamic (passed as a
parameter), which the current decorator (fixed doctype string)
cannot express.
Adds forms_pro/api/submission/test_submission.py with allow/403/404
coverage for both retrofitted endpoints.
…sion
Apply @require_permission("Form", "write", param="form_id") to
export_submissions and drop the manual frappe.has_permission block.
Privilege-swap behavior, audit log, and session restoration remain
unchanged.
Tightens TestExportPermissions to assert http_status_code == 403 on
the deny path and adds an explicit 404 case for a missing form.
api/user endpoints (get_user, get_current_user, get_user_teams) gate on session state, not DocShare, so they take no @require_permission decorator. Adds an integration test file that pins their current behavior so future changes surface regressions: - get_user returns a basic payload for an existing user, None for a missing user. - get_current_user returns the session user's profile under set_user. - get_user_teams returns a list for a real user and [] for Guest.
Introduces the frontend plumbing for route-level permission resolution:
- src/types/router.d.ts augments vue-router's RouteMeta with optional
allowGuest and fetch fields so per-route data resolvers are typed.
- src/stores/routeData.ts is the single Pinia store driving navigation:
state {status, data, error}, plus resolve(route) which awaits the
meta.fetch resource and normalizes Frappe errors (exc_type,
HTTP status, messages) into a uniform RouteError shape.
- src/composables/useRouteData.ts exposes typed, computed accessors so
pages can read status/data/error without touching the store directly.
No router/guard wiring yet, that lands in D2.
Wires the gated pages to the routeData store introduced in D1: - Three resource factories (formForView, formForEdit, teamForManage) point at the new whitelisted endpoints. cache keys per id keep intra-session refetches cheap. - meta.fetch attached to "Manage Team", "Manage Form" (parent), and "Edit Form". For nested Manage Form children (Overview, Submissions) vue-router merges parent meta into the matched route, so the parent resolver applies to every sub-tab. - beforeEach simplified to return-style, gains a call to useRouteData().resolve(to) after the auth check. Login redirect and allowGuest paths preserved. Public submission routes keep their existing beforeEnter and stay guest-friendly; no fetch attached. Manual error UX (RouteError component, page integration) lands in D3; until then a denied page will surface the store's error state but without a styled fallback.
Closes the user-visible half of the permission system. - components/RouteError.vue: one component, titled per exc_type (PermissionError → Access Denied, DoesNotExistError → Not Found, AuthenticationError → Login Required, fallback → Something Went Wrong). Carries an optional first-line message and HTTP status, plus a Go to Dashboard button. - App.vue: global LoadingIndicator (top-right) bound to routeData.isNavigating so users see in-flight perm resolution. - pages/manage/ManageForm.vue, pages/EditForm.vue, pages/team/ManageTeam.vue: render <RouteError> when status === 'error', existing layout otherwise. - stores/form/manageForm.ts: dropped the primary useDoc fetch. The Form document now flows in via routeData (resolved by the guard against get_form_for_view); manageFormStore.formData / formFields / formOwner read from there. Sharing mutations and formAccessResource untouched. - pages/manage/overview/Overview.vue: removed the now-dead formResource.loading branch (loading is handled globally).
team_members property used the raw return of frappe.db.get_value("DocShare", share_name, "write"), which is None when no DocShare row exists for a member, in turn making GetTeamMembersResponse.model_validate raise a pydantic ValidationError ("Input should be a valid boolean").
Coerce to bool() and short-circuit when share_name itself is None. False is the correct semantic when no share exists, since the user has no write permission on the team.
Race condition: meta.fetch for /team builds the get_team_for_manage resource using useUser().currentTeam?.name. Until userTeamsResource finishes, currentTeam is null, so the resource fires with an empty team_id and require_permission's existence check throws 404. - stores/user.ts: cache the initialize() promise so repeated calls share the same in-flight fetches instead of racing. - router.ts: after the userResource check confirms login, await useUser().initialize() inside beforeEach so userTeams + currentTeam are populated before the route resolver runs. Login redirect path is unaffected: initialize() is only awaited when isLoggedIn is true, and a thrown error still flips isLoggedIn to false via the existing catch.
Two Playwright specs exercising the meta.fetch + RouteError pipeline through a real browser against the built SPA: 1. /forms/manage/<bogus-id> drives a 404 on get_form_for_view and asserts the RouteError "Not Found" heading renders. 2. The logged-in test user creates and opens their own form's /edit-form/:id, the get_form_for_edit call returns 200, and no RouteError heading appears. Read-only viewer → "Access Denied" (403) is deferred: it needs an admin-owned form shared read-only with the test user, which requires a second authenticated API context that the current fixture set does not provide. Inline comment notes this for follow-up.
|
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 (1)
📝 WalkthroughWalkthroughThis PR centralizes permission checks using a new require_permission decorator across backend endpoints, extracts response schemas to dedicated modules, standardizes package exports, and implements route-level data loading, error handling, and navigation progress on the frontend. Tests (unit/integration/e2e) were added or updated to validate permission and error flows. ChangesCentralized Permission Enforcement & Route-level Data Loading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/EditForm.vue (1)
24-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard store initialization on successful route-data resolution.
editFormStore.initialize(formId)still runs when route fetch failed (e.g., 403/404), which can fire extra failing requests and mutate store state while the error page is shown. Only initialize after route data is successful.Suggested fix
watch( - () => route.params.id, - (formId) => { - if (formId && typeof formId === "string") { - editFormStore.initialize(formId); - } - }, + [() => route.params.id, () => status.value], + ([formId, routeStatus]) => { + if (routeStatus !== "success") return; + if (formId && typeof formId === "string") { + editFormStore.initialize(formId); + } + }, { immediate: true } );🤖 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 `@frontend/src/pages/EditForm.vue` around lines 24 - 31, The watcher currently calls editFormStore.initialize(formId) as soon as route.params.id changes even if the route's data fetch failed; update the watcher to only call editFormStore.initialize when the route's data is successfully resolved — e.g., check a route success flag (such as route.meta?.routeDataResolved === true or absence of route.meta?.fetchError) or ensure route.name is not the error route before invoking editFormStore.initialize(formId); modify the watcher that observes () => route.params.id to include this guard so initialize is skipped when route data failed.
🧹 Nitpick comments (3)
frontend/src/pages/manage/ManageForm.vue (1)
13-20: ⚡ Quick winGuard manage-form initialization behind successful route data.
RouteErroris rendered onstatus === "error", butmanageFormStore.initialize(...)still runs immediately. That can cause redundant failing calls on already-denied/not-found routes. Gate initialization to success state.Suggested change
watch( - () => route.params.id, - (id) => { - manageFormStore.initialize(id as string); + () => [route.params.id, status.value] as const, + ([id, routeStatus]) => { + if (routeStatus !== "success") return; + manageFormStore.initialize(id as string); }, { immediate: true } );Also applies to: 25-31
🤖 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 `@frontend/src/pages/manage/ManageForm.vue` around lines 13 - 20, Guard the manageFormStore initialization so it only runs when route data loaded successfully: inside the watch on route.params.id (and the similar watcher at 25-31), check the route data status from useRouteData (the `status` value) and only call manageFormStore.initialize(id as string) when status === "success"; if status is "error" or not yet "success", skip the initialize call to avoid redundant/failed requests. Use the same guard for both watchers that reference route.params.id and manageFormStore.initialize.frontend/src/components/RouteError.vue (1)
15-19: ⚡ Quick winUse a concrete Vue component type for
META.iconinstead ofunknown.This keeps strict-mode/template typing reliable and avoids hiding wrong icon values.
Proposed change
<script setup lang="ts"> -import { computed } from "vue"; +import { computed, type Component } from "vue"; @@ -const META: Record<string, { title: string; icon: unknown }> = { +const META: Record<string, { title: string; icon: Component }> = {🤖 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 `@frontend/src/components/RouteError.vue` around lines 15 - 19, The META constant uses unknown for icon which breaks template typing; import a concrete Vue component type (e.g., add "import type { Component } from 'vue'") and change META's type to Record<string, { title: string; icon: Component }>, then ensure the icon values (Lock, FileQuestion, etc.) are actual Vue components or wrapped/cast to Component where they are defined or imported so template usage is strongly typed (refer to the META symbol and the icon entries like PermissionError and DoesNotExistError).forms_pro/api/user/schema.py (1)
22-28: ⚡ Quick winHarden
rolesnormalization to handle bothHasRoleobjects and plain strings.Current logic assumes every item has
.role; this can break if input is alreadylist[str].Proposed change
`@field_validator`("roles", mode="before") `@classmethod` - def extract_roles(cls, v: list[HasRole]) -> list[str]: + def extract_roles(cls, v: list[HasRole] | list[str] | None) -> list[str]: if not v: return [] - - return [role.role for role in v] + normalized: list[str] = [] + for role in v: + if isinstance(role, str): + normalized.append(role) + else: + normalized.append(role.role) + return normalized🤖 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 `@forms_pro/api/user/schema.py` around lines 22 - 28, The extract_roles field validator assumes every item has a .role attribute and will fail for plain strings; update the extract_roles(cls, v: list[HasRole]) -> list[str] validator to normalize inputs by returning [] for falsy v, iterating items and for each: if isinstance(item, str) use it, elif hasattr(item, "role") use item.role, elif isinstance(item, dict) try item.get("role"), otherwise skip or raise a clear ValueError; ensure the final return is a list[str]. Use the same function name extract_roles and keep the `@field_validator`("roles", mode="before") usage so the schema accepts both HasRole objects and list[str].
🤖 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 `@frontend/src/stores/user.ts`:
- Around line 43-50: The initialize function stores a single initPromise that,
if the first fetches reject, remains rejected and blocks future retries; modify
initialize (and the inner async IIFE that calls userResource.fetch() and
userTeamsResource.fetch()) to catch any error, reset initPromise to null on
failure, rethrow the error so callers still see the failure, and only keep the
resolved promise when both fetches succeed so subsequent initialize() calls can
retry after a failure.
---
Outside diff comments:
In `@frontend/src/pages/EditForm.vue`:
- Around line 24-31: The watcher currently calls
editFormStore.initialize(formId) as soon as route.params.id changes even if the
route's data fetch failed; update the watcher to only call
editFormStore.initialize when the route's data is successfully resolved — e.g.,
check a route success flag (such as route.meta?.routeDataResolved === true or
absence of route.meta?.fetchError) or ensure route.name is not the error route
before invoking editFormStore.initialize(formId); modify the watcher that
observes () => route.params.id to include this guard so initialize is skipped
when route data failed.
---
Nitpick comments:
In `@forms_pro/api/user/schema.py`:
- Around line 22-28: The extract_roles field validator assumes every item has a
.role attribute and will fail for plain strings; update the extract_roles(cls,
v: list[HasRole]) -> list[str] validator to normalize inputs by returning [] for
falsy v, iterating items and for each: if isinstance(item, str) use it, elif
hasattr(item, "role") use item.role, elif isinstance(item, dict) try
item.get("role"), otherwise skip or raise a clear ValueError; ensure the final
return is a list[str]. Use the same function name extract_roles and keep the
`@field_validator`("roles", mode="before") usage so the schema accepts both
HasRole objects and list[str].
In `@frontend/src/components/RouteError.vue`:
- Around line 15-19: The META constant uses unknown for icon which breaks
template typing; import a concrete Vue component type (e.g., add "import type {
Component } from 'vue'") and change META's type to Record<string, { title:
string; icon: Component }>, then ensure the icon values (Lock, FileQuestion,
etc.) are actual Vue components or wrapped/cast to Component where they are
defined or imported so template usage is strongly typed (refer to the META
symbol and the icon entries like PermissionError and DoesNotExistError).
In `@frontend/src/pages/manage/ManageForm.vue`:
- Around line 13-20: Guard the manageFormStore initialization so it only runs
when route data loaded successfully: inside the watch on route.params.id (and
the similar watcher at 25-31), check the route data status from useRouteData
(the `status` value) and only call manageFormStore.initialize(id as string) when
status === "success"; if status is "error" or not yet "success", skip the
initialize call to avoid redundant/failed requests. Use the same guard for both
watchers that reference route.params.id and manageFormStore.initialize.
🪄 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: e527fea1-71a9-4b38-99ea-22ff1e06bf58
📒 Files selected for processing (40)
forms_pro/api/export/__init__.pyforms_pro/api/export/endpoints.pyforms_pro/api/export/test_export.pyforms_pro/api/form/__init__.pyforms_pro/api/form/endpoints.pyforms_pro/api/form/schema.pyforms_pro/api/form/test_form.pyforms_pro/api/settings/__init__.pyforms_pro/api/settings/endpoints.pyforms_pro/api/submission/__init__.pyforms_pro/api/submission/endpoints.pyforms_pro/api/submission/schema.pyforms_pro/api/submission/test_submission.pyforms_pro/api/submission/test_submission_validation.pyforms_pro/api/team/__init__.pyforms_pro/api/team/endpoints.pyforms_pro/api/team/test_invitations.pyforms_pro/api/team/test_team.pyforms_pro/api/user/__init__.pyforms_pro/api/user/endpoints.pyforms_pro/api/user/schema.pyforms_pro/api/user/test_user.pyforms_pro/forms_pro/doctype/form_field/test_form_field.pyforms_pro/forms_pro/doctype/fp_team/fp_team.pyforms_pro/utils/permissions.pyforms_pro/utils/test_permissions.pyfrontend/e2e/specs/route-perms.spec.tsfrontend/src/App.vuefrontend/src/components/RouteError.vuefrontend/src/components/RouteProgress.vuefrontend/src/composables/useRouteData.tsfrontend/src/pages/EditForm.vuefrontend/src/pages/manage/ManageForm.vuefrontend/src/pages/manage/overview/Overview.vuefrontend/src/pages/team/ManageTeam.vuefrontend/src/router.tsfrontend/src/stores/form/manageForm.tsfrontend/src/stores/routeData.tsfrontend/src/stores/user.tsfrontend/src/types/router.d.ts
Comment on lines
+43
to
+50
| let initPromise: Promise<void> | null = null; | ||
| async function initialize(): Promise<void> { | ||
| if (initPromise) return initPromise; | ||
| initPromise = (async () => { | ||
| await userResource.fetch(); | ||
| await userTeamsResource.fetch(); | ||
| })(); | ||
| return initPromise; |
There was a problem hiding this comment.
Reset cached initialization promise on failure.
A failed first initialization leaves initPromise permanently rejected, so all later initialize() calls fail immediately and never retry.
Suggested fix
let initPromise: Promise<void> | null = null;
async function initialize(): Promise<void> {
if (initPromise) return initPromise;
initPromise = (async () => {
- await userResource.fetch();
- await userTeamsResource.fetch();
+ try {
+ await userResource.fetch();
+ await userTeamsResource.fetch();
+ } catch (error) {
+ initPromise = null;
+ throw error;
+ }
})();
return initPromise;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let initPromise: Promise<void> | null = null; | |
| async function initialize(): Promise<void> { | |
| if (initPromise) return initPromise; | |
| initPromise = (async () => { | |
| await userResource.fetch(); | |
| await userTeamsResource.fetch(); | |
| })(); | |
| return initPromise; | |
| let initPromise: Promise<void> | null = null; | |
| async function initialize(): Promise<void> { | |
| if (initPromise) return initPromise; | |
| initPromise = (async () => { | |
| try { | |
| await userResource.fetch(); | |
| await userTeamsResource.fetch(); | |
| } catch (error) { | |
| initPromise = null; | |
| throw error; | |
| } | |
| })(); | |
| return initPromise; | |
| } |
🤖 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 `@frontend/src/stores/user.ts` around lines 43 - 50, The initialize function
stores a single initPromise that, if the first fetches reject, remains rejected
and blocks future retries; modify initialize (and the inner async IIFE that
calls userResource.fetch() and userTeamsResource.fetch()) to catch any error,
reset initPromise to null on failure, rethrow the error so callers still see the
failure, and only keep the resolved promise when both fetches succeed so
subsequent initialize() calls can retry after a failure.
Backend permission messages embed <strong> tags around the user email. The template escaped them so they showed literally. Escape all HTML then re-allow a safe set of inline tags (strong/b/em/i) and render via v-html, keeping bold formatting without an XSS vector.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@require_permission(doctype, ptype, param)decorator gates Frappe whitelisted endpoints. Throwsfrappe.PermissionError(403) orfrappe.DoesNotExistError(404). Per-family API packages (forms_pro/api/<family>/) with co-located tests.get_form_for_view(read),get_form_for_edit(write),get_team_for_manage(read).api/form,api/team,api/submission,api/export.api/useraudited (session-scoped, no decorator needed) and covered with tests.routeDatastore +useRouteDatacomposable.meta.fetchon gated routes (/manage/:id,/edit-form/:id,/team) resolves under the router guard;<RouteError>component renders titled error states fromexc_type. Global top progress bar (RouteProgress.vue) reflects in-flight navigation.useUser().initialize()(idempotent) socurrentTeamis populated beforeget_team_for_managefires.FPTeam.team_memberscoercescan_edit_teamto bool so members without a DocShare row don't break the pydantic response schema.Test plan
bench --site forms.dev run-tests --app forms_prois greencd frontend && yarn typecheck && yarn lintis cleancd frontend && yarn build && yarn playwright test e2e/specs/route-perms.spec.tspasses/edit-form/<id>; non-owner sees "Access Denied";/manage/<bogus>shows "Not Found"Summary by CodeRabbit
New Features
Bug Fixes
Refactor