test authorization in blog apps#46
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds authorization policies for posts/categories/tags/comments; enforces per-resource authorize() checks across admin routes (Next/Nuxt/SvelteKit); threads framework-specific authorization error factories into adapters and core; updates generated authorization types, tests, and DB connection/context handling. ChangesAuthorization Enforcement Implementation
Database & Type Improvements
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant Route as Admin Route
participant Action as Action/Handler
participant authorize as authorize()
participant Policy as Policy
participant Database as Database
Admin->>Route: POST /admin/posts
Route->>Action: readRequestBody()
Action->>Action: compute status
Action->>authorize: authorize('create', Post)
authorize->>Policy: check class.create
alt permitted
Policy-->>authorize: true
authorize-->>Action: ✓
Action->>Action: status==='published' ?
opt if published
Action->>authorize: authorize('publish', Post)
authorize->>Policy: check class.publish
Policy-->>authorize: true
authorize-->>Action: ✓
end
Action->>Database: createPost(fields)
Database-->>Route: success
Route-->>Admin: 303 redirect to /admin/posts
else denied
Policy-->>authorize: deny(message, status)
authorize->>Route: framework error (403/404)
Route-->>Admin: forbidden()/notFound()/createError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/project/registry.ts (1)
580-582:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImport
HoloAuthUserwhenever policy/ability actor types are emitted.
actor: HoloAuthUseris now always generated for policies/abilities, but the import is still gated byguardNames.length > 0. In projects with policies/abilities and no configured guards, generatedauthorization/types.d.tswill reference an undefined type.💡 Proposed fix
- ...(guardNames.length > 0 + ...(guardNames.length > 0 || authorizationPolicies.length > 0 || authorizationAbilities.length > 0 ? ['import type { AuthUser as HoloAuthUser } from \'`@holo-js/auth`\''] : []),Also applies to: 603-604, 617-618, 633-639
🤖 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 `@packages/cli/src/project/registry.ts` around lines 580 - 582, The import of HoloAuthUser is currently wrapped by the guardNames.length > 0 condition, so when policies/abilities emit "actor: HoloAuthUser" but no guards are configured the generated authorization/types.d.ts references an undefined type; update the import logic where the import array is built (the expression using guardNames.length > 0 and the other similar blocks around lines that construct import lists) to include "import type { AuthUser as HoloAuthUser } from '`@holo-js/auth`'" whenever policy/ability actor types are emitted (e.g., when you generate authorization types or when a flag/variable indicating emitted actor types is true) rather than only when guardNames.length > 0 so the type is always imported when referenced.
🧹 Nitpick comments (3)
apps/blog-sveltekit/tests/blog-logic.mjs (1)
274-292: ⚡ Quick winApply editor auth setup consistently for direct route-action assertions.
Lines 284 and 291 call
signInEditor()only throughcreatePost/updatePosthelpers, but other direct admin page-action calls in this file bypass that setup. Prefer one shared setup point so these assertions don’t depend on ambient auth state.♻️ One way to make auth setup deterministic
async function createPost(fields) { - await signInEditor() await expectRedirect(() => createPostPageActions.create({ request: createActionRequest(fields), })) } async function updatePost(id, fields) { - await signInEditor() await expectRedirect(() => updatePostPageActions.update({ params: { id: String(id) }, request: createActionRequest(fields), })) } try { + await signInEditor() const verifyEmailRouteSource = await readFile(new URL('../src/routes/verify-email/+page.server.ts', import.meta.url), 'utf8') ...🤖 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/blog-sveltekit/tests/blog-logic.mjs` around lines 274 - 292, Tests call signInEditor only inside helpers (createPost/updatePost) but some tests call page-action functions (createPostPageActions.create, updatePostPageActions.update) directly and thus skip the editor auth setup; make auth deterministic by invoking signInEditor for all tests that call page-actions directly—either add a top-level beforeEach that calls signInEditor or update the direct assertions to call signInEditor before calling createPostPageActions.create or updatePostPageActions.update (and similarly ensure expectRedirect wrappers run after signInEditor). Reference signInEditor, createPost, updatePost, createPostPageActions.create, updatePostPageActions.update, and expectRedirect when making the change.apps/blog-next/tests/admin-actions.test.mjs (1)
89-143: ⚡ Quick winStrengthen per-action authorization assertions in the table-driven test.
Line 170 asserts the same
('viewAny', mocks.Post)tuple for every action case, so Category/Tag authorization regressions can slip through if that Post check still exists. Add expected authorization per case (ability + subject) and assert against it.♻️ Suggested structure
const cases = [ { name: 'createPostAction', mutate: mocks.createPost, run: () => actions.createPostAction(createPostFormData()), redirectTo: '/admin/posts', + expectedAuthorize: ['create', mocks.Post], }, + // ...add expectedAuthorize for each case ] -it.each(cases)('runs authenticated $name before redirecting', async ({ mutate, run, redirectTo }) => { +it.each(cases)('runs authenticated $name before redirecting', async ({ mutate, run, redirectTo, expectedAuthorize }) => { mocks.auth.mockResolvedValue({ authenticated: true, user: { id: 1, email: 'editor@example.com' } }) mutate.mockResolvedValue(undefined) await expect(run()).rejects.toThrow(`NEXT_REDIRECT:${redirectTo}`) expect(mocks.auth).toHaveBeenCalledTimes(1) - expect(mocks.authorize).toHaveBeenCalledWith('viewAny', mocks.Post) + expect(mocks.authorize.mock.calls).toContainEqual(expectedAuthorize) expect(mutate).toHaveBeenCalledTimes(1) expect(mocks.redirect).toHaveBeenCalledWith(redirectTo) })Also applies to: 163-171
🤖 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/blog-next/tests/admin-actions.test.mjs` around lines 89 - 143, The table-driven test currently asserts the same authorization tuple ('viewAny', mocks.Post) for every case, which can mask Category/Tag authorization regressions; update the cases array (each entry with name, mutate, run, redirectTo) to include an expected authorization field (e.g., expectedAuth: ['abilityName', subjectMock]) per case, setting the correct ability and subject for Post, Category, and Tag cases, then change the test assertion that currently checks ('viewAny', mocks.Post) to assert against each case's expectedAuth so each action validates the correct (ability, subject) pair.apps/blog-next/tests/run.mjs (1)
355-360: ⚡ Quick winMake form selection resilient to button attribute/whitespace changes.
Exact string matching on
<button type="submit">...</button>is fragile and can cause false test failures on harmless markup updates.♻️ Proposed fix
function findFormByButtonText(pageHtml, buttonText) { const formPattern = /<form\b[\s\S]*?<\/form>/gi for (const [form] of pageHtml.matchAll(formPattern)) { - if (form.includes(`<button type="submit">${buttonText}</button>`)) { + const hasSubmitButton = new RegExp( + `<button\\b[^>]*type=(["'])submit\\1[^>]*>\\s*${buttonText}\\s*<\\/button>`, + 'i', + ).test(form) + if (hasSubmitButton) { return form } }🤖 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/blog-next/tests/run.mjs` around lines 355 - 360, The function findFormByButtonText uses exact string matching for `<button type="submit">...</button>`, which is brittle; replace the includes check with a case-insensitive RegExp that matches a <button> element whose attributes may vary/contain whitespace/order (e.g. /<button\b[^>]*\btype\s*=\s*["']?submit["']?[^>]*>\s*BUTTON_TEXT\s*<\/button>/i), build that RegExp with the escaped buttonText variable, and use regex.test(form) to decide when to return the form (update function findFormByButtonText accordingly).
🤖 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/blog-next/app/admin/actions.ts`:
- Around line 79-82: The handlers (e.g., updateCategoryAction and the
corresponding delete handlers and tag handlers referenced) currently call
authorize('manage', Category) at the class level; change them to load the
specific model instance (e.g., fetch the Category or Tag by the provided id
inside updateCategoryAction / deleteCategoryAction / updateTagAction /
deleteTagAction) and then call authorize('update', category) for updates and
authorize('delete', category) for deletions on that loaded record; also handle
the not-found case (return 404 or throw) before authorizing to avoid null deref
and ensure correct record-level permission checks.
In `@apps/blog-next/server/policies/CategoryPolicy.ts`:
- Around line 6-9: The before hook in CategoryPolicy (before({ user, guard }))
currently allows access when guard === 'admin' even if no actor is resolved;
change the condition to require authenticated as well (e.g., only return true
when guard === 'admin' && authenticated) and keep existing user check for the
super-admin email; update the same pattern in other blog policy files that use
before({ user, guard }) so the guard-based bypass is only granted when
authenticated is true.
In `@apps/blog-next/tests/run.mjs`:
- Around line 44-45: The test imports encodeReply via a hard-coded internal path
using createRequire and
require('next/dist/compiled/react-server-dom-turbopack/client.node'); replace
this unsupported internal import by using Next's supported testing utilities
(e.g., next/experimental/testmode/playwright) or exercising server-actions
through public UI/HTTP flows instead of RSC internals. Locate the
require/createRequire usage and the symbol encodeReply in tests/run.mjs, remove
the direct internal import, and update the test to use the official test helper
APIs or perform assertions by driving the app over HTTP or Playwright so
behavior is validated via public interfaces.
In `@apps/blog-nuxt/server/routes/admin/categories/`[id]/delete.post.ts:
- Around line 6-8: The current handler calls authorize('manage', Category) which
skips the record-level delete policy; instead, load the Category record for the
id from event.context.params (use the same id used by deleteCategory), call
authorize('delete', category) against that instance, and only then invoke
deleteCategory(Number(event.context.params?.id || 0)) (or pass the loaded
category/id to the deletion function) so authorization uses the category
record-level policy.
In `@apps/blog-nuxt/server/routes/admin/posts/`[id]/update.post.ts:
- Around line 8-14: The publish authorization uses the raw status input, so
callers can bypass publish checks by sending arbitrary non-empty values;
normalize the status before the publish check by applying the same coercion used
in updatePost(): derive a normalizedStatus = (body.status === 'draft') ? 'draft'
: 'published' (defaulting to 'published' when absent) and use normalizedStatus
for the if-check and any subsequent logic (keep Post.findOrFail(id),
authorize('update', post) as-is, but call authorize('publish', post) only when
normalizedStatus === 'published').
In `@apps/blog-nuxt/server/routes/admin/posts/create.post.ts`:
- Around line 8-12: The current status assignment keeps arbitrary non-empty
values which bypasses publish authorization even though createPost() normalizes
any non-'draft' to 'published'; change the status normalization before
authorization (e.g., set status = body.status === 'draft' ? 'draft' :
'published') and then call authorize('create', Post) and, if status ===
'published', authorize('publish', Post) so the authorization logic matches
createPost()'s persisted behavior.
In `@packages/adapter-sveltekit/tests/client.test.ts`:
- Around line 9-18: The waitForActionHydration helper currently silently returns
if the predicate never becomes true; update the function waitForActionHydration
to throw an error after the retry loop instead of returning silently so tests
fail fast—after the for-loop completes, throw a descriptive Error (e.g.,
"waitForActionHydration: predicate not satisfied after N attempts" or include
the predicate context) so failing tests immediately surface the timeout
condition.
In `@packages/db/src/model/ModelRepository.ts`:
- Around line 83-89: The current resolveActiveConnection only checks
connectionName and can incorrectly swap different DatabaseContext instances that
share a name; change the guard to ensure the active context is the same logical
context before returning active.connection — e.g., require either
active.connection === connection (object identity) or compare a stable unique
identifier on DatabaseContext (add/use a method like getContextId()/getId() on
DatabaseContext and check active.connection.getId() === connection.getId()) in
addition to matching connectionName; update resolveActiveConnection (and add the
identifier accessor if missing) so repositories are not redirected to unrelated
active contexts.
---
Outside diff comments:
In `@packages/cli/src/project/registry.ts`:
- Around line 580-582: The import of HoloAuthUser is currently wrapped by the
guardNames.length > 0 condition, so when policies/abilities emit "actor:
HoloAuthUser" but no guards are configured the generated
authorization/types.d.ts references an undefined type; update the import logic
where the import array is built (the expression using guardNames.length > 0 and
the other similar blocks around lines that construct import lists) to include
"import type { AuthUser as HoloAuthUser } from '`@holo-js/auth`'" whenever
policy/ability actor types are emitted (e.g., when you generate authorization
types or when a flag/variable indicating emitted actor types is true) rather
than only when guardNames.length > 0 so the type is always imported when
referenced.
---
Nitpick comments:
In `@apps/blog-next/tests/admin-actions.test.mjs`:
- Around line 89-143: The table-driven test currently asserts the same
authorization tuple ('viewAny', mocks.Post) for every case, which can mask
Category/Tag authorization regressions; update the cases array (each entry with
name, mutate, run, redirectTo) to include an expected authorization field (e.g.,
expectedAuth: ['abilityName', subjectMock]) per case, setting the correct
ability and subject for Post, Category, and Tag cases, then change the test
assertion that currently checks ('viewAny', mocks.Post) to assert against each
case's expectedAuth so each action validates the correct (ability, subject)
pair.
In `@apps/blog-next/tests/run.mjs`:
- Around line 355-360: The function findFormByButtonText uses exact string
matching for `<button type="submit">...</button>`, which is brittle; replace the
includes check with a case-insensitive RegExp that matches a <button> element
whose attributes may vary/contain whitespace/order (e.g.
/<button\b[^>]*\btype\s*=\s*["']?submit["']?[^>]*>\s*BUTTON_TEXT\s*<\/button>/i),
build that RegExp with the escaped buttonText variable, and use regex.test(form)
to decide when to return the form (update function findFormByButtonText
accordingly).
In `@apps/blog-sveltekit/tests/blog-logic.mjs`:
- Around line 274-292: Tests call signInEditor only inside helpers
(createPost/updatePost) but some tests call page-action functions
(createPostPageActions.create, updatePostPageActions.update) directly and thus
skip the editor auth setup; make auth deterministic by invoking signInEditor for
all tests that call page-actions directly—either add a top-level beforeEach that
calls signInEditor or update the direct assertions to call signInEditor before
calling createPostPageActions.create or updatePostPageActions.update (and
similarly ensure expectRedirect wrappers run after signInEditor). Reference
signInEditor, createPost, updatePost, createPostPageActions.create,
updatePostPageActions.update, and expectRedirect when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce9b7ae6-c406-4754-9040-57079f4f268a
📒 Files selected for processing (62)
apps/blog-next/app/admin/actions.tsapps/blog-next/app/admin/auth.tsapps/blog-next/server/policies/CategoryPolicy.tsapps/blog-next/server/policies/CommentPolicy.tsapps/blog-next/server/policies/PostPolicy.tsapps/blog-next/server/policies/TagPolicy.tsapps/blog-next/tests/admin-actions.test.mjsapps/blog-next/tests/blog-logic.mjsapps/blog-next/tests/run.mjsapps/blog-nuxt/server/policies/CategoryPolicy.tsapps/blog-nuxt/server/policies/CommentPolicy.tsapps/blog-nuxt/server/policies/PostPolicy.tsapps/blog-nuxt/server/policies/TagPolicy.tsapps/blog-nuxt/server/routes/admin/categories/[id]/delete.post.tsapps/blog-nuxt/server/routes/admin/categories/[id]/update.post.tsapps/blog-nuxt/server/routes/admin/categories/create.post.tsapps/blog-nuxt/server/routes/admin/posts/[id]/delete.post.tsapps/blog-nuxt/server/routes/admin/posts/[id]/update.post.tsapps/blog-nuxt/server/routes/admin/posts/create.post.tsapps/blog-nuxt/server/routes/admin/tags/[id]/delete.post.tsapps/blog-nuxt/server/routes/admin/tags/[id]/update.post.tsapps/blog-nuxt/server/routes/admin/tags/create.post.tsapps/blog-nuxt/tests/blog-logic.mjsapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/server/policies/CategoryPolicy.tsapps/blog-sveltekit/server/policies/CommentPolicy.tsapps/blog-sveltekit/server/policies/PostPolicy.tsapps/blog-sveltekit/server/policies/TagPolicy.tsapps/blog-sveltekit/src/routes/admin/categories/+page.server.tsapps/blog-sveltekit/src/routes/admin/categories/[id]/edit/+page.server.tsapps/blog-sveltekit/src/routes/admin/posts/+page.server.tsapps/blog-sveltekit/src/routes/admin/posts/[id]/edit/+page.server.tsapps/blog-sveltekit/src/routes/admin/posts/new/+page.server.tsapps/blog-sveltekit/src/routes/admin/tags/+page.server.tsapps/blog-sveltekit/src/routes/admin/tags/[id]/edit/+page.server.tsapps/blog-sveltekit/tests/blog-logic.mjsapps/blog-sveltekit/tests/run.mjspackages/adapter-next/src/config.tspackages/adapter-next/src/runtime.tspackages/adapter-nuxt/src/module.tspackages/adapter-nuxt/src/runtime/authorization-error.tspackages/adapter-nuxt/src/runtime/composables/index.tspackages/adapter-nuxt/src/runtime/plugins/init.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/adapter-sveltekit/src/index.tspackages/adapter-sveltekit/tests/adapter.test.tspackages/adapter-sveltekit/tests/client.test.tspackages/authorization/src/runtime.tspackages/authorization/tests/package.test.tspackages/cli/src/project/registry-svelte.tspackages/cli/src/project/registry.tspackages/cli/tests/authorization-registry.test.tspackages/cli/tests/cli.test.tspackages/core/src/adapter.tspackages/core/src/portable/holo.tspackages/core/tests/adapter.test.tspackages/db/src/index.tspackages/db/src/model/ModelRepository.tspackages/db/src/model/index.tspackages/db/tests/query-cache.test.tstests/example-app-auth-flow.mjs
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/blog-sveltekit/src/routes/admin/categories/[id]/edit/+page.server.ts (1)
8-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the edit-page load path too.
The
updateaction is protected now, butload()still fetches and returns the category without a policy check. That means a caller who cannot update categories can still request/admin/categories/[id]/editand read the record data.🤖 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/blog-sveltekit/src/routes/admin/categories/`[id]/edit/+page.server.ts around lines 8 - 28, The load handler returns category data without a policy check; add the same authorization used in the update action so only permitted users can load the edit page: after fetching category with getAdminCategoryById (and after the null check that throws error(404,...)), call authorize('update', category) (importing authorize if missing) and only then return { category } from load; this mirrors the update action's protection and prevents unauthorized reads.
🧹 Nitpick comments (3)
packages/cli/tests/authorization-registry.test.ts (1)
252-266: ⚡ Quick winAdd the no-guards ability-only regression case.
This new test only exercises the policy branch. If
renderGeneratedAuthorizationTypes()regresses when a project defines only abilities and no guards, this suite still passes.🧪 Suggested coverage add-on
it('imports auth user types when authorization entries exist without guards', () => { const output = renderGeneratedAuthorizationTypes([ { sourcePath: 'server/policies/posts.ts', @@ expect(output).toContain('import type { AuthUser as HoloAuthUser } from \'`@holo-js/auth`\'') expect(output).toContain('actor: HoloAuthUser') }) + + it('imports auth user types for ability-only registries without guards', () => { + const output = renderGeneratedAuthorizationTypes([], [ + { + sourcePath: 'server/abilities/reports.export.ts', + name: 'reports.export', + exportName: 'default', + }, + ] satisfies readonly GeneratedAuthorizationAbilityRegistryEntry[], []) + + expect(output).toContain('import type { AuthUser as HoloAuthUser } from \'`@holo-js/auth`\'') + expect(output).toContain('actor: HoloAuthUser') + })🤖 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 `@packages/cli/tests/authorization-registry.test.ts` around lines 252 - 266, The new test only exercises the policy branch and misses the regression where a project defines abilities without guards; add a test case that calls renderGeneratedAuthorizationTypes with a registry entry (e.g., the existing GeneratedAuthorizationPolicyRegistryEntry used in this test) but simulate an ability-only project by passing in empty guards/ability-maps (the second and third args) or a separate test that supplies only abilities and no guards, then assert that the generated output still imports AuthUser and includes "actor: HoloAuthUser"; update or add a test near the existing it('imports auth user types when authorization entries exist without guards') to cover the ability-only/no-guards scenario using renderGeneratedAuthorizationTypes so the function handles that regression.apps/blog-sveltekit/tests/blog-logic.mjs (1)
274-280: ⚡ Quick winAdd a positive authenticated admin-guard assertion.
This new check only proves that
guard('admin')fails without an actor. It does not verify that a real authenticated admin-guard request still succeeds after adding theauthenticatedrequirement, so broken admin-guard binding would slip through.Also applies to: 479-482
🤖 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/blog-sveltekit/tests/blog-logic.mjs` around lines 274 - 280, The test config only verifies that guard('admin') fails without an actor but doesn't assert a successful admin-guard when an actor is provided; update signInEditor (and the similar block around lines 479-482) so configureAuthorizationAuthIntegration's resolveGuardActor returns the editor for 'admin' (e.g., guardName === 'web' || guardName === 'admin') and add a positive assertion that the admin guard succeeds (call the same guard('admin') authorization check you use elsewhere and assert it authorizes with the editor present).apps/blog-next/tests/admin-actions.test.mjs (1)
182-190: ⚡ Quick winAssert authorization happens before the mutation.
This only proves both calls happened, not that the action enforced auth before the write. A regression that mutates first and authorizes second would still pass this test.
Suggested assertion hardening
await expect(run()).rejects.toThrow(`NEXT_REDIRECT:${redirectTo}`) expect(mocks.auth).toHaveBeenCalledTimes(1) + expect(mocks.authorize).toHaveBeenCalledTimes(1) expect(mocks.authorize).toHaveBeenCalledWith(...expectedAuth) expect(mutate).toHaveBeenCalledTimes(1) + expect(mocks.authorize.mock.invocationCallOrder[0]).toBeLessThan( + mutate.mock.invocationCallOrder[0], + ) expect(mocks.redirect).toHaveBeenCalledWith(redirectTo)🤖 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/blog-next/tests/admin-actions.test.mjs` around lines 182 - 190, The test currently only asserts both authorize and mutate were called but not their order; update the test to assert mocks.authorize was invoked before mutate by checking their mock invocation order (use mocks.authorize.mock.invocationCallOrder and mutate.mock.invocationCallOrder) and expect the authorize call order value to be less than the mutate call order value so the authorization happens prior to the write.
🤖 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/blog-sveltekit/src/routes/admin/categories/`[id]/edit/+page.server.ts:
- Around line 8-28: The load handler returns category data without a policy
check; add the same authorization used in the update action so only permitted
users can load the edit page: after fetching category with getAdminCategoryById
(and after the null check that throws error(404,...)), call authorize('update',
category) (importing authorize if missing) and only then return { category }
from load; this mirrors the update action's protection and prevents unauthorized
reads.
---
Nitpick comments:
In `@apps/blog-next/tests/admin-actions.test.mjs`:
- Around line 182-190: The test currently only asserts both authorize and mutate
were called but not their order; update the test to assert mocks.authorize was
invoked before mutate by checking their mock invocation order (use
mocks.authorize.mock.invocationCallOrder and mutate.mock.invocationCallOrder)
and expect the authorize call order value to be less than the mutate call order
value so the authorization happens prior to the write.
In `@apps/blog-sveltekit/tests/blog-logic.mjs`:
- Around line 274-280: The test config only verifies that guard('admin') fails
without an actor but doesn't assert a successful admin-guard when an actor is
provided; update signInEditor (and the similar block around lines 479-482) so
configureAuthorizationAuthIntegration's resolveGuardActor returns the editor for
'admin' (e.g., guardName === 'web' || guardName === 'admin') and add a positive
assertion that the admin guard succeeds (call the same guard('admin')
authorization check you use elsewhere and assert it authorizes with the editor
present).
In `@packages/cli/tests/authorization-registry.test.ts`:
- Around line 252-266: The new test only exercises the policy branch and misses
the regression where a project defines abilities without guards; add a test case
that calls renderGeneratedAuthorizationTypes with a registry entry (e.g., the
existing GeneratedAuthorizationPolicyRegistryEntry used in this test) but
simulate an ability-only project by passing in empty guards/ability-maps (the
second and third args) or a separate test that supplies only abilities and no
guards, then assert that the generated output still imports AuthUser and
includes "actor: HoloAuthUser"; update or add a test near the existing
it('imports auth user types when authorization entries exist without guards') to
cover the ability-only/no-guards scenario using
renderGeneratedAuthorizationTypes so the function handles that regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae516bc5-16a3-4470-b18f-205f56d9e454
📒 Files selected for processing (34)
apps/blog-next/app/admin/actions.tsapps/blog-next/server/policies/CategoryPolicy.tsapps/blog-next/server/policies/CommentPolicy.tsapps/blog-next/server/policies/PostPolicy.tsapps/blog-next/server/policies/TagPolicy.tsapps/blog-next/tests/admin-actions.test.mjsapps/blog-next/tests/blog-logic.mjsapps/blog-next/tests/run.mjsapps/blog-nuxt/server/policies/CategoryPolicy.tsapps/blog-nuxt/server/policies/CommentPolicy.tsapps/blog-nuxt/server/policies/PostPolicy.tsapps/blog-nuxt/server/policies/TagPolicy.tsapps/blog-nuxt/server/routes/admin/categories/[id]/delete.post.tsapps/blog-nuxt/server/routes/admin/categories/[id]/update.post.tsapps/blog-nuxt/server/routes/admin/posts/[id]/update.post.tsapps/blog-nuxt/server/routes/admin/posts/create.post.tsapps/blog-nuxt/server/routes/admin/tags/[id]/delete.post.tsapps/blog-nuxt/server/routes/admin/tags/[id]/update.post.tsapps/blog-nuxt/tests/blog-logic.mjsapps/blog-sveltekit/server/policies/CategoryPolicy.tsapps/blog-sveltekit/server/policies/CommentPolicy.tsapps/blog-sveltekit/server/policies/PostPolicy.tsapps/blog-sveltekit/server/policies/TagPolicy.tsapps/blog-sveltekit/src/routes/admin/categories/+page.server.tsapps/blog-sveltekit/src/routes/admin/categories/[id]/edit/+page.server.tsapps/blog-sveltekit/src/routes/admin/tags/+page.server.tsapps/blog-sveltekit/src/routes/admin/tags/[id]/edit/+page.server.tsapps/blog-sveltekit/tests/blog-logic.mjspackages/adapter-sveltekit/tests/client.test.tspackages/cli/src/project/registry.tspackages/cli/tests/authorization-registry.test.tspackages/db/src/core/DatabaseContext.tspackages/db/src/model/ModelRepository.tspackages/db/tests/query-cache.test.ts
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/blog-sveltekit/tests/blog-logic.mjs (1)
276-280: ⚡ Quick winMake the admin-guard assertion independent from default-actor fallback.
resolveDefaultActorandresolveGuardActor('admin')both returneditor, so the Line 484 check still passes ifauthorization.guard('admin')accidentally falls back to the default actor. Use a dedicated helper or a different default actor for that assertion so the test actually proves guard-specific wiring.🤖 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/blog-sveltekit/tests/blog-logic.mjs` around lines 276 - 280, The test is accidentally passing because resolveDefaultActor and resolveGuardActor('admin') both return the same editor; change the setup so the admin guard uses a distinct actor (or use a dedicated helper actor) rather than the default fallback: modify resolveDefaultActor to return a different default (e.g., viewer) and/or make resolveGuardActor return a specific adminActor for guardName === 'admin' (keep resolveGuardActor logic for 'web'/'admin' but return adminActor for 'admin'), then update any assertions that reference authorization.guard('admin') to expect that distinct adminActor so the test proves guard-specific wiring.
🤖 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.
Nitpick comments:
In `@apps/blog-sveltekit/tests/blog-logic.mjs`:
- Around line 276-280: The test is accidentally passing because
resolveDefaultActor and resolveGuardActor('admin') both return the same editor;
change the setup so the admin guard uses a distinct actor (or use a dedicated
helper actor) rather than the default fallback: modify resolveDefaultActor to
return a different default (e.g., viewer) and/or make resolveGuardActor return a
specific adminActor for guardName === 'admin' (keep resolveGuardActor logic for
'web'/'admin' but return adminActor for 'admin'), then update any assertions
that reference authorization.guard('admin') to expect that distinct adminActor
so the test proves guard-specific wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3944bdb9-2fd0-4c5f-9656-18e2e6fa6772
📒 Files selected for processing (4)
apps/blog-next/tests/admin-actions.test.mjsapps/blog-sveltekit/src/routes/admin/categories/[id]/edit/+page.server.tsapps/blog-sveltekit/tests/blog-logic.mjspackages/cli/tests/authorization-registry.test.ts
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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 by CodeRabbit
New Features
Bug Fixes
Tests