Skip to content

feat: permission-aware layout (issue #37)#57

Merged
smartlabsAT merged 28 commits into
mainfrom
feature/issue-37-permission-filtering
May 18, 2026
Merged

feat: permission-aware layout (issue #37)#57
smartlabsAT merged 28 commits into
mainfrom
feature/issue-37-permission-filtering

Conversation

@smartlabsAT
Copy link
Copy Markdown
Owner

Summary

Fixes Issue #37 — adds permission-aware filtering across the entire Super Layout Table pipeline.

Closes #37.

Changes

  • New usePermissions composable + sanitizeFilter utility (single source of truth)
  • Translation language columns hidden when user can't read the language
  • Inline-edit popover gated on canUpdate
  • Filter sanitization with user notification
  • Bulk-action duplicate button hidden when no create permission
  • Display bug [object Object] fixed
  • 403 errors surface as notifications

Test plan

  • Unit tests: usePermissions, sanitizeFilter, resolveTranslationValue (120/120 passing)
  • Manual: bookmark 137 as issue37@test.com (limited user)
  • Manual: bookmark 137 as admin
  • Quality checks: pnpm run quality

@smartlabsAT smartlabsAT linked an issue May 10, 2026 that may be closed by this pull request
@smartlabsAT smartlabsAT mentioned this pull request May 10, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 10, 2026

Quality Check Results

TypeScript Type Check

Passed - No type errors found

ESLint

Passed - No linting errors

Prettier Format Check

Passed - Code is properly formatted

Build

Passed - Extension builds successfully


Updated: 2026-05-11T12:06:22.227Z

…elf (issue #37 bug B regression)

The previous Task 9 implementation only handled the case where a translation
field is added at the parent collection level. When the field metadata's
collection is already the junction (the path used by tableHeaders'
getTranslationFieldMetadata), the relations lookup failed silently and the
permission gate fell through, leaving the popover openable on locked cells.

Now: try parent->junction lookup first, fall back to treating 'collection'
as the junction itself. Removes the fail-permissive default.
…coverage

- PermissionAction union no longer includes 'share' (no helper consumed it)
- getEntry guards against array-shaped permission stores via isPermissionsMap
- Tests added for canUpdate/canCreate/canDelete (5 cases) and store-shape resilience (2 cases)
- 21/21 tests pass

Addresses code review feedback from PR #57 (non-blocking cleanups).
…effect

Pure computed (sanitizedFilterResult) carries both the cleaned filter and
removed-field-list. The user notification is now emitted from a dedicated
watcher, keeping side-effects out of the reactivity-sensitive computed.

Behavior unchanged for users; addresses code review feedback from PR #57.
…ssue #37 L2)

When the languages collection is unrestricted but the translations
junction collection has a row-level filter on languages_code (a common
asymmetric configuration), getAccessibleLanguages over the languages
collection returns the full list while the API only delivers a subset
of language rows — leaving `--` placeholder columns in the table.

Fix: aggregate-query the translations collection at mount time, group by
languages_code. The server enforces the row filter, so the result is the
exact set of accessible languages — including row-level restrictions the
client cannot otherwise inspect. Falls back to permission-store detection
when the probe is empty (fresh install) or fails (no read permission).

The language picker dialog continues to use the broader permission-store
list, since it represents what the user *could* see, not what currently
has data.

Why declared after `fields`: Vue's watcher tracks dependency reads
eagerly during setup. Since translationsCollectionRef → hasTranslationFields
→ fields, the new code must follow `fields` to avoid TDZ.
…ile drawer (issue #37 L5)

When inline-edit is enabled on a file/file-image field but the user lacks
read access to directus_files, the drawer would render empty (or appear
broken with no clear cause). Surface a warning notification instead so
the user understands why the action is unavailable.
Combined two non-overlapping import groups in src/super-table.vue:
- usePermissions, useTranslationLanguages (PR #57 — permission filtering)
- getTranslationFieldMetadata, buildSearchFilter (PR #56 — search hotfix)

All 171 tests pass post-merge (134 from PR #57 + 37 from PR #56).
Move `const permissions = usePermissions()` to before `languageItems`
computed (which references it on line 392). The forward-reference worked
by accident because Vue computeds are lazy-evaluated, but any future
watcher (or eager dependency tracker like `useTranslationLanguages`'
existing watch) reading `languageItems` during setup would have crashed
with `Cannot access 'permissions' before initialization`.

Surfaced by code review on PR #57.
No behaviour change. Same file, just earlier declaration order.
171/171 tests still green.
…s (I-1)

Previously `sanitizeFilter` only checked the root field of each filter
clause, so a search/filter like
  { translations: { _some: { description: { _icontains: 'foo' } } } }
slipped through unchanged when the user had read on `translations` but
not on the `description` sub-field of the junction collection. The API
then rejected the query with 403 and the layout showed an empty table —
exactly the symptom Bug E was supposed to eliminate.

Fix: extend `sanitizeFilter` with a `nestedScopes` option mapping
parent-field-name → nested-collection-name. The walker now:
  - treats _some / _none / _every as relation-match operators that
    recurse with the same scope (we are already inside the relation)
  - recurses into the value of a configured nested-scope field with
    the nested collection set as scope
  - calls canRead(field, scope) so the caller can dispatch to the
    right collection's permissions

Caller in super-table.vue passes
  { nestedScopes: { translations: <junction collection> } }
and uses (field, scope) => permissions.canRead(scope ?? collection, field).

Tests:
  - 6 baseline tests still pass (canRead's optional second arg is
    backward-compatible).
  - 7 new tests cover the nested cases: _some/_none/_every, mixed
    siblings (one allowed one denied), translation branch dropped
    inside _or, no-options backward-compat, and the trivial case
    where filter never touches the nested scope.

Live verification: as the test user, with description read removed
from the junction collection and the bookmark filter set to
{translations: {_some: {description: {_icontains: 'test'}}}}, the
items query is now sent without any filter clause and returns 200 OK
with all three items, instead of 403 and an empty table.

Surfaced by code review on PR #57.
@smartlabsAT
Copy link
Copy Markdown
Owner Author

smartlabsAT commented May 10, 2026

Code review

Found 1 issue:

  1. Primary key field can be silently dropped after the explicit "always include" guard. The PK is added to adjustedFields per the comment // CRITICAL: Always include the primary key field for navigation and identification, but permissions.sanitizeFields runs immediately after and removes any field where canRead(collection, field) returns false — including the PK if the user's read-permission fields[] whitelist does not contain it. This breaks :item-key="getPrimaryKeyFieldName()", editItem navigation (item[pkField]), and handleManualSort for any user whose permission entry is { access: 'partial', fields: [...] } without id. Fix by re-injecting the PK after sanitisation or exempting it inside sanitizeFields.

// CRITICAL: Always include the primary key field for navigation and identification
const pkField = getPrimaryKeyFieldName();
if (!adjustedFields.includes(pkField)) {
adjustedFields.unshift(pkField); // Add at the beginning
}
// Ensure language code field is included for translations
const languageFieldPath = getTranslationLanguageFieldPath(translationConfig.value);
if (hasTranslationFields.value && !adjustedFields.includes(languageFieldPath)) {
adjustedFields.push(languageFieldPath);
}
// Permission gate: drop fields/language-suffixed entries the user cannot read
const translationsCollection = relationsStore.getRelationsForField(
props.collection,
'translations'
)?.[0]?.collection;
return permissions.sanitizeFields(props.collection, adjustedFields, {
translationsCollection,
accessibleLanguages: effectiveAccessibleLanguages.value,
});
});

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Cleanup pass on the issue-37 permission-filtering work — no behaviour
changes, all 180 unit tests still pass, browser-verified across admin,
restricted-with-PK and restricted-without-PK scenarios.

- `fieldsWithRelational`: the previous `// CRITICAL: Always include the
  primary key field` comment was misleading because sanitize permission-
  filters the PK and language-code path immediately after. Comment now
  states the truth: PK + language code are added before the gate so
  sanitize can drop them when the user lacks read access, matching
  native Directus's permission-filtered `useCollection.primaryKeyField`
  (degraded interaction instead of an empty 403).
- Drop the unused `probing` ref from `useTranslationLanguages` (declared,
  set, never consumed).
- Strip issue/bug-ID references from in-source comments per CLAUDE.md —
  ticket pointers rot as the codebase evolves; PR descriptions own that
  history.
- Compress over-long JSDoc and inline blocks in `sanitizeFilter`,
  `resolveTranslationValue`, `useTranslationLanguages`, and several
  spots in `super-table.vue`. WHY preserved, WHAT removed.
- Add two regression tests documenting `sanitizeFields`' contract:
  primary key and `translations.languages_code` are both dropped when
  not in the user's read whitelist — callers rely on this to fall back
  gracefully.

Net: +84 / -109 (-25 lines), 8 files.
…l'` quirk

Critical-review finding that turned into a regression test instead of a
behaviour change.

Real `/permissions/me` payloads from Directus 11 set `access: 'full'`
even when `fields` is an explicit allow-list — e.g. a restricted user's
read entry on `issue_37_test`:

  { "access": "full", "fields": ["id", "title", "translations"] }

(no `thumbnail`). The natural-language reading of `'full'` suggests
unrestricted access, but in practice `fields` remains the authoritative
gate; Directus uses `access` as a coarser hint that does not override
the whitelist. Short-circuiting on `access === 'full'` would let denied
fields through and silently re-introduce the original Bug A-D class.

- Add a comment on `canAction` documenting why we deliberately don't
  trust `access` for the field check.
- Add a regression test mirroring the real payload shape so a future
  contributor's "obvious cleanup" cannot remove this guard unnoticed.

181/181 tests, type-check, lint, prettier all green.
…ayout

Closes the last "known limitation" from PR #57: users without read
permission on the primary key now see a populated table instead of an
empty 403 state.

Root cause: `useTableApi.fetchItems` requested
`meta=filter_count,total_count`, which Directus resolves server-side via
`countDistinct(<pk>)`. That aggregate hits the same field-permission
gate as a direct query, so it 403s for any user whose `read.fields`
whitelist omits the PK — even though the items themselves are fully
accessible. Native Directus's `useItems` avoids this by issuing a
separate count request; we now do the same.

- `fetchItems`: drop `meta=` from the items request entirely; it now
  returns just `{ data }` and never reads `response.data.meta`.
- New `fetchItemCount(collection, filter?, search?)`: separate request
  using `aggregate[count]=*` (SQL `COUNT(*)`), which does not touch
  any field and therefore does not require PK access. Returns the
  previous count on failure so a transient aggregate error doesn't
  reset pagination to zero.
- `super-table.vue` `getItems`: issue both calls in parallel via
  `Promise.all` so an aggregate failure cannot block items rendering
  (the count call is wrapped in `.catch(() => undefined)`).

Browser-verified end-to-end against a freshly-provisioned user whose
permission entry is `{ access: 'partial', fields: ['title', 'translations'] }`
(explicitly no `id`): super-table layout renders all three items with
zero console errors; the two API requests succeed (200/200) where the
old combined request would 403. Existing admin and partially-restricted
flows unaffected.

181/181 tests, type-check, lint, prettier, build clean.
Hardening for the count/items split in the previous commit. The table
and footer were gated on `itemCount > 0`, which now lives in a separate
request from the items themselves. If that aggregate call fails or
races behind the items call, `itemCount` stays at zero while
`items.value` is already populated — the table would then stay hidden
even though the rows are right there.

Both v-if guards now also pass when `items.length > 0`, so the items
render unconditionally; the per-page selector follows the same rule.
Pagination buttons are still gated on `totalPages > 1` and therefore
hide gracefully when the count is unknown.

Behaviour-equivalent for the common case (both calls succeed) and only
adds a fallback for the partial-failure case.
@smartlabsAT smartlabsAT merged commit a7b88e6 into main May 18, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Language columns

1 participant