Admin UI and i18n corrections; surface sign-in-as-user in the users menus - #683
Conversation
The core <.input> component already renders the required marker itself (components/core/input.ex:87 emits a text-error asterisk whenever the required attribute is set, with aria-hidden so screen readers don't announce it twice). Six labels also hand-appended " *" into the label string, so both markers rendered side by side. Drops the hand-written asterisk and keeps the component-rendered one: it takes its colour from the theme and stays out of the msgid, which a literal in the label string cannot do. The component's own comment already told callers not to append it. user_form.html.heex Email, Password, New Password, Organization Name roles.html.heex Role Name (create and edit modals) Organization Name had the hand-written asterisk but no required attr at all, so the marker was decorative while the changeset does require the field for organization accounts (users/auth/user.ex:491). It gains the attribute, which also lines the browser up with the server-side rule. Catalogues follow the msgid change: "Role Name *" becomes "Role Name" carrying its existing translations, and "Organization Name *" is dropped because a bare, fully translated "Organization Name" already existed -- it just inherits the user_form reference. Edited by hand rather than via gettext.extract --merge, which would have swept in ~22 unrelated un-extracted msgids sitting on this branch.
The Edit action sat in the breadcrumb bar as an icon-only chip via app_layout's page_action attribute, detached from the "..." menu right below it that already held Roles, Confirm Email and Deactivate. It landed there in 208697d, the admin header deduplication sweep. Before that it was a labelled button in the page's in-body <.admin_page_header> :actions slot; when that duplicate header was removed the action was parked in the breadcrumb instead of being folded into the menu that was already on the page. So it never was a menu item -- this makes it one. Placed first in the menu, ahead of Roles, and labelled with the existing "Edit User" msgid rather than the list view's bare "Edit": this menu is page-scoped rather than row-scoped, and it also offers "Settings", which targets the viewer's own account instead of the user being viewed, so an unqualified "Edit" would be ambiguous about its subject. This was page_action's only call site in the workspace; the attribute and its :action slot counterpart are now unused.
…ings
"Person" named a human being where the label names a kind of account, the
counterpart of "Organization". The msgid becomes "Personal" and each locale
gets the idiomatic term for the private-individual-vs-legal-entity split
rather than a calque: Eraisik, Privatperson, Particulier, Particular,
Privato, Osoba prywatna, Личный. Four call sites; the stored account_type
value stays "person".
Auditing the Estonian catalogue for similar defects turned up 38 more. It
is mechanically healthy -- 2158 entries, none empty, none fuzzy -- so these
are all semantic. The provable class: strings carrying a *different*
message's translation, detectable without knowing Estonian by looking for
one translation serving two unrelated msgids.
Current Page said "Praegune aeg" (Current time)
Not set said "Märkus" (Note)
Field not found said "Faili ei leitud" (File not found)
Failed to save custom field said "...schedule"
Failed to update field/user role/roles/status all said "...setting"
Cannot delete this user said "Cannot delete the last system owner"
"Not set" is the one on the user detail page: every unset timezone read
"Note". Alongside those, ten labels had lost their qualifier ("Available
Fields" -> "Available", "Revoke All User Sessions" -> "Revoke all"), and
two pairs of distinct menu entries rendered identically -- Live Sessions
could not be told from Active Sessions, Dimensions from Dimension Presets.
The rest are word-choice and grammar: Healthy read "Tervislik" (wholesome,
as of food), Override read "ülekate" (an overlay), "Unnamed contact" read
as the imperative "name the contact", plus case and number agreement fixes.
The same twelve copy-paste defects were then found in Russian and fixed
there too; de/en/es/fr/it/pl are clean, so this pairing is specific to the
et and ru catalogues.
The badge under the email derived its label from `Scope.can_access_admin_area?/1`, which is true for Owner, Admin *or any single permission holder*. A Client holds `client_portal`, so every client was labelled "Admin" — and custom roles (Manager, …) were flattened into the same three buckets. After an account switch the badge therefore kept describing the account as an admin regardless of who was now active. `MultiSession.role_label/1` already computes the right thing and is what the account list in the same dropdown renders, which is why the list said "Client" while the header above it said "Admin". It becomes public and both badge sites in this nav now share it, so the two cannot disagree. Colour still tracks the system roles; everything else renders neutral. niinastarmo (Client, client_portal) Admin -> Client fotkin (Admin, Manager) Admin -> Admin sergei (User) User -> User
`MultiSession.impersonate/2`, its authority rules, the controller action and the POST route all shipped already — with no way to reach any of it from the UI. This adds the entry points, and nothing else: the rules are untouched. Owner and Admin are exactly who the existing rules admit, so no policy changed. They are enforced server-side as before; the menu only decides what to *offer*. To keep offer and outcome from drifting, `impersonable?/2` runs the same private predicate the request runs, rather than restating it in a template. Its companion `impersonation_actor/1` names the session's ROOT account — the one the decision is judged against, which after an account switch is not the active one. Both LiveViews hold that as a `User` struct, so no session token enters a socket. The item is a real POST, not a `phx-click`: it rewrites the session cookie, which a LiveView event cannot do. `table_row_menu_link` therefore had to stop dropping `method`, `csrf_token` and `data-confirm` from its global include. Verified against live accounts. Authority matrix: Owner -> Admin, Client, User (never itself) Admin -> Client, User (never an Owner, never another Admin) Client -> nothing and rendered: with each row present on the page, the option appears on the client's row, and is absent on the owner's row and the actor's own. Posting admin -> client makes niinastarmo the active account and the stack 2 deep; admin -> owner is refused with the session left as it was.
The menu asked only the authority rules, which never look at settings. The controller opens with `with_gate` → `gate_allowed?/1`, which also requires `multi_session_enabled`. With that setting off the item still rendered, and using it bounced the operator to the home page with "Multi-account switching is not available." `impersonation_actor/1` now returns nil unless the gate is open, so `impersonable?/2` answers false for every target and the item disappears. One place, and it keeps the property that offer and outcome are decided by the same code rather than by two lists of conditions kept in step by hand. Found by a GLM security review of the branch, which traced the controller's three layers and named the gate as the first of them. The earlier claim that impersonation is independent of `multi_session_enabled` came from reading `add_authenticated_user/3`, which indeed does not consult it — but that is the library call underneath, not the path the UI takes. Verified by flipping the setting live: offered while on, absent while off, offered again once restored.
…or the badge
Two findings from a GLM review of the branch, both confirmed against the code
before acting on them.
"Sign in as user" and "Sign in as %{email}?" went in with no catalogue entries
at all, so every non-English operator got the menu label and the confirm()
dialog in English — an odd omission two commits after an i18n audit. Added to
the .pot and all eight locales, placed in the existing sort order.
`current_role_badge/1` called `MultiSession.role_label/1`, which runs
`Auth.User.get_roles/1` and therefore queries. The scope already carries those
names in `cached_roles`, loaded once in `Scope.for_user/1` — and the predicates
the badge replaced (`Scope.owner?/1` and friends) read exactly that, so the
rewrite added a query where there had been none, on a component that renders on
every admin page and twice per render. `role_label_from_roles/1` splits the
labelling rule from the lookup; `role_label/1` stays as the fetching wrapper for
callers holding only a user.
Verified live: the strings resolve in en/et/ru/de, and the badge still reads
Owner / Admin / Client for the three accounts, now off the cached list.
`impersonable?/2` reads roles from the database, and `authorize_impersonation/2` did so three times per call: the actor, the target, and the actor again inside `staff?/1`. Calling it from a `:if` on every row of the users list therefore turned one page render into an N+1. Measured on a 20-row page: 47 role-assignment queries before this feature, 108 after — 61 of them added by the menu item. The rule moves into `decide_impersonation/4`, over role names already in hand, and `impersonable_uuids/2` reads the actor once and each target from the `:roles` preload `list_users_paginated/1` already returns, falling back to a lookup only for a row that arrives without one. The LiveView computes the set in `load_users/1` — so it follows pagination, search and filters — and the template asks `user.uuid in @impersonable_uuids`. Both the request path and the menus still funnel through the same predicate, so offer and outcome cannot diverge. `impersonable?/2` stays for the detail page, where one user means one lookup. Same page after: 49 queries, 2 more than the baseline instead of 61, and the option still appears on the client's row while staying absent on the owner's and the actor's own.
…email `clean_username/1` keeps `[a-zA-Z0-9_]` and drops everything else, so an accented local part lost letters rather than being folded: `ülo.kask@` became `lo_kask`, missing the first letter of the name, and a wholly non-Latin local part collapsed to nothing before the `user_` fallback caught it. `Utils.Slug.transliterate/1`, which arrived in 1.7.231, already maps lowercase Cyrillic to Latin and strips Latin diacritics via NFD. Running it after the downcase — the order it requires — reuses that instead of adding a second character map. ülo.kask@ lo_kask -> ulo_kask märt.õun@ mrt_un -> mart_oun jürgen@ jrgen -> jurgen šveits@ veits -> sveits Дмитрий@ user_ -> dmitrii john.doe@ john_doe -> john_doe (unchanged) The uniqueness walk is unaffected: it runs after this, so two names that fold to the same ASCII still get the numeric suffix.
timujinne
left a comment
There was a problem hiding this comment.
Code review
Three review passes over this branch — two by GLM (glm-5.2, an elixir-review
and a security-review persona) and one by me. Findings below are the ones that
survived checking against the code; each fix is a commit in this PR.
Fixed during review
Impersonation offered while the feature is off. The menu asked only the
authority rules, which never consult settings, but Session.impersonate/2 opens
with with_gate → gate_allowed?/1, which also requires multi_session_enabled.
With that off the item still rendered and using it bounced the operator to the
home page. impersonation_actor/1 now returns nil unless the gate is open, so
impersonable?/2 answers false for every target — one place, keeping offer and
outcome decided by the same code rather than two condition lists kept in step by
hand. (207cae49)
A query per row on the users list. impersonable?/2 reads roles from the
database, and authorize_impersonation/2 did so three times per call — actor,
target, and the actor again inside staff?/1. On an :if for every row that is
an N+1. Measured with telemetry on a 20-row page: 47 role-assignment queries
before the feature, 108 after. The rule moved into decide_impersonation/4
over role names already in hand; impersonable_uuids/2 reads the actor once and
each target from the :roles preload list_users_paginated/1 already returns.
Same page after: 49. (a94a8d89)
A query per render on the role badge. Same class, caught first:
current_role_badge/1 called role_label/1, which queries, on a component that
renders on every admin page and twice per render — while the predicates it
replaced read scope.cached_roles, already loaded once in Scope.for_user/1.
role_label_from_roles/1 splits the rule from the lookup. (1b722a54)
The new menu strings had no catalogue entries at all, so every non-English
operator saw the label and the confirm() dialog in English — a poor omission
in a branch that also audits translations. Added to the .pot and all eight
locales. (1b722a54)
Verified, not merely compiled
Against a running application:
- Authority matrix — Owner → Admin/Client/User, never itself; Admin →
Client/User, never an Owner or another Admin; Client → nothing. - Rendering — with each row actually present on the page, so the negatives
are not vacuous: offered on the client's row, absent on the owner's and on the
actor's own. An earlier run of this check was misleading becauseper_pageis
not overridable by query param and the forbidden rows simply were not on page
one. - A real POST — admin → client makes that account active with a two-deep
stack; admin → owner is refused with the session untouched. - The feature flag — toggled live: offered, absent, offered again.
- Catalogues — no
%{}placeholder differs between msgid and msgstr in any
of the nine files (a mismatch raises at runtime rather than rendering oddly).
The security pass traced the three authority layers and returned PASS: no
non-staff path reaches the POST; no escalation chain, since impersonation
appends to the stack and the root stays the admin; CSRF enforced through the
host's :browser pipeline, with session-id rotation on success; the template
predicate leaks nothing and enables no enumeration; the widened :global list
is a three-name allowlist admitting no event handlers, with the interpolated
email escaped.
The final pass confirmed the load-bearing detail that <.link method="post">
only takes effect on the href clause, not navigate/patch — the impersonate
links use href while Edit and View use navigate, so there is no silent
no-op — and that the host loads the phoenix_html UJS that converts
[data-method] anchors into real POST forms.
Not addressed here
- The role badge renders system role names in English regardless of locale
("Owner"/"Admin"/"User"). Pre-existing — the code this replaces hardcoded the
same strings — but it sits awkwardly in a PR that also fixes translations. Left
alone because the account list directly below rendersaccount.rolethe same
way, so translating one without the other would introduce a new inconsistency. user_details.html.heexstill callsimpersonable?/2per render.
Deliberate: one user is one lookup, and the batching helper exists for lists.- Audit logging is fail-open —
Activity.log/1is wrapped in
rescue _ -> :okwhile the session mutates before the write, so a logging
failure leaves a successful impersonation with no audit row. Codebase-wide
pattern, not introduced here, but worth its own issue for a privileged action.
mix compile --warnings-as-errors and mix format --check-formatted are clean.
Menu predicates for "Sign in as user" now exclude deactivated targets — the `:inactive` refusal is raised by `add_authenticated_user/2`, not by the authority rules the predicates asked, so every deactivated row offered an action whose POST always came back "That account is deactivated." `impersonable?/2` reads the target's roles from the `:roles` preload when the caller has one, so the user detail page stops paying two role queries per render. Tests for the three new public functions, and for the username transliteration fix, which shipped with none. i18n: `mix gettext.extract --check-up-to-date` was failing. 18 msgids were in the source but in no .pot file (SEO/robots.txt settings, the hex.pm package browser, the referral gate) and four reworded entries were still rendering their old translation — German showed "online" on a Continue button. Full round-trip, then all 22 translated across the seven locales; 0 untranslated, 0 fuzzy, 0 placeholder mismatches. Russian `Personal` goes back to "Физическое лицо", the legal counterpart of "Организация", rather than the adjective calque of the English word. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two related pieces of admin-surface work. They are in one PR because the
impersonation menu item is anchored on the Edit item the first part moves into
that menu, and both touch
multi_session.exand the catalogues — split apart,the second PR's diff would simply contain the first. Happy to break it up if
you would rather review them separately.
1. Required-field markers, page headers, account-type label
Doubled required markers.
<.input>already renders the marker itself(
core/input.ex:87emits atext-errorasterisk whenrequiredis set, witharia-hiddenso it is not announced twice). Six labels also hand-appended" *", so both showed. The literal ones are gone.Organization Namehad thehand-written asterisk but no
requiredattribute, while the changeset doesrequire it for organization accounts (
users/auth/user.ex:491) — it gains theattribute, so the marker is now real and the browser agrees with the server.
Edit action on the user detail page moves from the breadcrumb bar into the
"..." menu that already sits on the page holding Roles, Confirm Email and
Deactivate. It had been parked in the breadcrumb by the header-deduplication
sweep (208697d) when the in-body header that used to own it was removed. That
was
page_action's only call site in the workspace; the attribute and its:actionslot counterpart are now unused, left in place as public API.Personwas a wrong source string, not a bad translation: it named a humanbeing where the label names a kind of account, the counterpart of
Organization. The msgid becomesPersonal, and each locale takes theidiomatic private-individual-vs-legal-entity term rather than a calque —
Eraisik, Privatperson, Particulier, Particular, Privato, Osoba prywatna. The
stored
account_typevalue stays"person".The role badge in the admin nav derived its label from
Scope.can_access_admin_area?/1, which is true for Owner, Admin or any singlepermission holder. A Client holds
client_portal, so clients were labelled"Admin", and custom roles were flattened into three buckets. The account list
directly below it renders
MultiSession.role_label/1and said "Client" whilethe header said "Admin". Both now share that function, reading the names the
scope already loaded in
cached_rolesrather than re-querying on a componentthat renders on every admin page.
2. Entry points for sign-in-as-user
MultiSession.impersonate/2, its authority rules, the controller action andthe POST route all shipped in #672 with nothing in the UI reaching them — a
grep for
impersonateacross every template returned nothing. This adds theentry points and changes no rules.
The item appears in the users-list row menu, the card menu, and the user detail
menu.
impersonable?/2decides what to offer by running the same privatepredicate the request runs, rather than restating the rules in a template, so
the offer cannot drift from the outcome. Its companion
impersonation_actor/1names the session's ROOT account — the one authority is judged against, which
after an account switch is not the active one — and returns nil unless
gate_allowed?/1passes, so the item is hidden whenmulti_session_enabledisoff instead of offering an action that would bounce to the home page.
It is a real POST rather than a
phx-click, because it rewrites the sessioncookie and a LiveView event cannot;
table_row_menu_linktherefore stopsdropping
method,csrf_tokenanddata-confirmfrom its:globalinclude.Translation fixes found along the way
The Estonian catalogue is mechanically healthy — 2158 entries, none empty, none
fuzzy — and still had 38 strings saying the wrong thing. Grouping msgids by
shared msgstr finds the provable class without judging the language:
Not setcarried Note's translation, so every unset timezone read "Note";
Current Pageread "Current time";
Field not foundread "File not found"; fourFailed to update …messages all read "Failed to update setting";Cannot delete this userread "Cannot delete the last system owner". Ten morelabels had lost their qualifier, and two pairs of distinct menu entries rendered
identically (Live Sessions / Active Sessions, Dimensions / Dimension Presets).
The rest are word choice and grammar —
Healthyread "Tervislik", wholesome asof food;
Overrideread "ülekate", an overlay;Unnamed contactread as theimperative "name the contact".
The same check then found twelve of those defects in Russian, fixed there too.
de/en/es/fr/it/pl are clean, so the pairing is specific to et and ru.
Verification
Against a running application rather than compilation alone: the authority
matrix (Owner → Admin/Client/User, never itself; Admin → Client/User, never an
Owner or another Admin; Client → nothing), the rendered menus with each row
present on the page so the negatives are not vacuous, and a real POST — admin →
client makes that account active with a two-deep stack, admin → owner is
refused with the session untouched. Toggling
multi_session_enabledliveremoves and restores the item.
A GLM security review of the branch traced the controller's three authority
layers and returned PASS; it also caught the feature-flag gap above, which is
fixed here.
mix compile --warnings-as-errorsandmix format --check-formattedare clean.