Skip to content

Fix admin permissions for external plugin LiveViews - #521

Merged
ddon merged 2 commits into
BeamLabEU:devfrom
timujinne:fix/external-plugin-admin-permissions
May 8, 2026
Merged

Fix admin permissions for external plugin LiveViews#521
ddon merged 2 commits into
BeamLabEU:devfrom
timujinne:fix/external-plugin-admin-permissions

Conversation

@timujinne

Copy link
Copy Markdown
Contributor

Summary

Custom roles granted plugin permission keys (e.g. entities, billing, ai) were denied access to those plugins' admin pages — even when the role explicitly had the permission and the module was enabled — because PhoenixKitWeb.Users.Auth.infer_permission_key_from_module/1 only resolved the core PhoenixKit.Modules.* namespace.

For an external plugin LiveView like PhoenixKitEntities.Web.Entities, all three resolution fallbacks in permission_key_for_admin_view/1 (static map, custom-tabs map, namespace inference) returned nil. The fail-closed branch in enforce_admin_view_permission/2 then redirected every non-system role with "You do not have permission to access this section.", regardless of the role's actual permissions.

The fix adds a registry-backed fourth option: when the LiveView's top-level namespace doesn't match PhoenixKit.Modules, ask PhoenixKit.ModuleRegistry whether any registered module starts with that namespace, and return its declared module_key/0. Owner/Admin behaviour and the fail-closed default for genuinely unknown views are preserved.

Changes

lib/phoenix_kit/module_registry.ex

  • Add get_module_key_for_namespace/1 — symmetric with the existing get_by_key/1. Iterates all_modules/0, matches on Module.split(mod) head, returns the module's module_key/0 via the existing safe_call/3. No new state, no caching layer.
  • Reference it in the moduledoc Query API list.

lib/phoenix_kit_web/users/auth.ex

  • infer_permission_key_from_module/1 gets a new [top | _rest] clause that calls ModuleRegistry.get_module_key_for_namespace(top).
  • The previous _ -> nil fallback is removed: Module.split/1 always returns a non-empty list of binaries, so it was unreachable (Dialyzer flags this on strict).
  • permission_key_for_admin_view/1 is exposed as @doc false def (was defp) so unit tests can reach it without LiveView mounting machinery.
  • alias PhoenixKit.ModuleRegistry added.

Tests

  • New test/phoenix_kit_web/users/auth_test.exs (4 cases): static-map hit, PhoenixKit.Modules.* namespace, registered-plugin namespace, unknown view → nil.
  • New describe block in test/phoenix_kit/module_registry_test.exs for get_module_key_for_namespace/1 (3 cases). Uses Module.create/3 with explicit top-level fixture names to avoid defmodule X getting auto-nested under the test module's namespace.

Tightening (second commit, post-internal-review)

Initial implementation used [^top_namespace | _] which matched any registered module whose Module.split path starts with the segment, not just modules whose top-level is the segment. Live repro on a parent app: get_module_key_for_namespace("PhoenixKit") => "db" because PhoenixKit.Modules.DB happens to be the first registered module beginning with "PhoenixKit".

Today this is masked by infer_permission_from_custom_tabs/1 for every existing PhoenixKit.<X>.Web.* admin view, so no user-visible regression exists. But it's a footgun for any future PhoenixKit.* view that doesn't register a tab.

Fix: tightened to [^top_namespace] (exact, single segment) and narrowed safe_call's return via is_binary/1 so @spec is truthful even if a misbehaving plugin returns a non-string module_key/0. Added regression test pinning get_module_key_for_namespace("PhoenixKit") == nil.

Affected scope

Every plugin shipping its own admin UI under a top-level PhoenixKit<Name> namespace was unreachable for custom roles. Verified plugins in our deps tree: PhoenixKitEntities, PhoenixKitCRM, PhoenixKitStaff, PhoenixKitProjects, PhoenixKitBilling, PhoenixKitCatalogue, PhoenixKitDocumentCreator, PhoenixKitLocations, PhoenixKitEcommerce. After the fix, any registered module is auto-resolved by namespace.

Test Plan

  • mix format
  • mix credo --strict — 0 issues on the changed files
  • mix dialyzer — 0 new warnings
  • mix test test/phoenix_kit_web/users/auth_test.exs test/phoenix_kit/module_registry_test.exs — 45 tests, 0 failures (DB optional; integration tests excluded automatically)
  • Pre-commit hook passed end-to-end (compile + docs + format + credo + dialyzer)
  • Live verification on a parent app via Tidewave: external plugin views now resolve to their module key (PhoenixKitEntities.Web.Entities → "entities", PhoenixKitAI.Web.Index → "ai", etc.); static map (Dashboard → "dashboard", Users → "users") and PhoenixKit.Modules.* namespace (Languages → "languages") preserved; genuinely unknown views still return nil (fail-closed)

Custom roles granted plugin permission keys (entities, billing, ai, …)
were denied access to those plugins' admin pages because
infer_permission_key_from_module/1 only resolved the core
PhoenixKit.Modules.* namespace. Add a registry-backed fallback so
top-level plugin namespaces (PhoenixKitEntities, PhoenixKitBilling, …)
resolve to their declared module_key, while preserving the
fail-closed default for genuinely unmapped views.
- Add ModuleRegistry.get_module_key_for_namespace/1 next to get_by_key/1
- Wire it into Auth.infer_permission_key_from_module/1 fallback
- Expose Auth.permission_key_for_admin_view/1 as @doc false def
- Add unit tests for both helpers (4 + 2 cases)
Reviewer caught that get_module_key_for_namespace/1 used
[^top_namespace | _], which matches any registered module whose
Module.split path *starts with* the segment, not just modules
whose top-level *is* the segment. Live repro:
get_module_key_for_namespace("PhoenixKit") returned "db" because
PhoenixKit.Modules.DB.Module.split begins with "PhoenixKit".
Today this is masked by infer_permission_from_custom_tabs for every
existing PhoenixKit.<X>.Web.* admin view, so no user-visible
regression exists. But it's a footgun for any future view in the
PhoenixKit.* namespace that doesn't register a tab.
Tighten the match to [^top_namespace] (exact, single segment) and
narrow safe_call's return via is_binary/1 so the @SPEC is truthful
even if a misbehaving plugin returns a non-string key. Add a
regression test pinning get_module_key_for_namespace("PhoenixKit")
to nil.
@ddon
ddon merged commit 937adae into BeamLabEU:devMay 8, 2026
ddon added a commit that referenced this pull request May 8, 2026
Eight post-merge reviews covering V111 PDF tables, the DB-module
extraction, MediaBrowser modal + LV login return_to, external-plugin
admin permissions, dashboard sidebar gettext, live Hex.pm known-packages
fetch, the publishing routing-strategy shim, and the LanguageSwitcher
per_translation_urls attr (plus the bundled DnD work).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon added a commit that referenced this pull request May 8, 2026
…525
Code/doc fixes addressing one finding per PR (or several where
trivial). Each closes a NITPICK or IMPROVEMENT-LOW from the
matching CLAUDE_REVIEW.md; design-level / breaking / risky items
deferred per the FOLLOW_UP.md "Skipped" sections.
- #516: Drop dead `String.to_atom` fallback in OAuth interpolate_url
- #518: Delete stray 0-byte pages_html.ex
- #519: Fix stale `viewer={true}` template comment + login_path
trailing-slash self-loop guard
- #521: Resolution-order doc on permission_key_for_admin_view/1
- #522: Hot-reload safety pitfall in per-module-i18n.md
- #523: KnownPackages — max-pages cap, ensure_table race comment,
Logger-levels operational signals in moduledoc
- #524: __mix_recompile__?/0 note next to apply/3 explanation
- #525: LanguageSwitcher attr doc atom/string keys + DRY resolve_url
per-language + JS sortable:flash defensive status check
Plus FOLLOW_UP.md per PR enumerating closed vs deferred items.
PR #525's FOLLOW_UP also captures the bundled DnD audit trail
(table_default drag-handle scoping, sortable:flash, TR cell-width
preservation) absent from the original PR body.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon added a commit that referenced this pull request May 8, 2026
Three-day window of accumulated work since 1.7.105 (2026-05-05):
PRs #516, #518, #519, #521, #522, #523, #524, #525, plus the
review-doc suite and post-merge triage closing nitpicks across all
eight.
Headline changes — V111 PDF library tables, DB module extracted to
phoenix_kit_db, MediaBrowser modal viewer, sidebar gettext API, live
Hex.pm catalog, publishing routing-strategy shim closing the
/:locale/<literal>/... host-route shadowing bug, LanguageSwitcher
:per_translation_urls, and bundled DnD improvements (drag-handle
scoping, sortable:flash, TR cell-width preservation).
All changes are strictly additive / non-breaking; one transitional
extraction (DB → phoenix_kit_db) requires the paired Hex package
once it ships.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@timujinne
timujinne deleted the fix/external-plugin-admin-permissions branch June 25, 2026 08:42
Sign up for freeto 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.

2 participants

@timujinne@ddon