feat(ui): structured editor for ACL rules, tags and groups - #608
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The policy model and editors are well-built, but three edge cases should be fixed before merge: the undeclared-tag warning fires as a false positive whenever the policy can't be read, group edits from the Users page silently drop policy comments, and "Edit groups" isn't gated on policy writability so it fails silently in file mode.
Reviewed changes
- ACL policy model (
app/utils/acl-policy.ts) — typedparsePolicy/serializePolicyover the HuJSON policy with round-tripping of unknown top-level keys,:*port defaulting (IPv6-aware), and name validation. - Rules / Tags & Groups editors (
app/routes/acls/) — structured editors and dialogs that write into the samecodePolicybuffer as the file editor, keeping the diff view and Save on a single source of truth. - Users page (
app/routes/users/) — group membership shown under each user, plus an "Edit groups" dialog andupdate_user_groupsaction. - Machines page (
app/routes/machines/) — tags not declared undertagOwnersare flagged in the tag dialog. - Docs & tests — new
docs/features/acls.mdand 26 new unit tests for the policy model.
ℹ️ Nitpicks
serializePolicyalways emitsgroups→tagOwners→hosts→acls→ssh, so "key order preserved" only holds within a section: a hand-written policy that lists sections in a different order gets reshuffled on the first structured edit, producing a noisy diff that undercuts the stated "diff shows only what changed" goal.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Thanks — all three findings were real, and the nitpick was right too. Fixed in Undeclared-tag warning on an unreadable policy. Correct, and the diagnosis is "Edit groups" not gated on writability. Also correct, including the detail Comments dropped from the Users page. Right, the asymmetry was Nitpick — section order. Fair, and it undercut the stated goal. 218 unit tests passing, typecheck and lint clean. One thing worth flagging for a human reviewer rather than the bot: this PR |
There was a problem hiding this comment.
✅ No new issues found.
The three edge cases flagged in the prior review are all addressed cleanly, along with the top-level key-ordering nitpick. I verified the two Headscale-facing assumptions against v0.29.0 source: GetPolicy returns a nil updatedAt in file mode (so updatedAt !== null correctly detects database mode), and SetPolicy in file mode returns ErrPolicyUpdateIsDisabled ("update is disabled for modes other than 'database'"), which the new catch block matches.
Reviewed changes
- Fixed the undeclared-tag false positive — both machine loaders now pass
policyTagsasundefinedwhen the policy could not be read, so the tag dialog no longer flags every tag whenapi.policy.get()rejects. - Gated "Edit groups" on policy writability —
canEditGroupsnow also requiresupdatedAt !== null, andupdate_user_groupscatchesapi.policy.setfailures, returning a friendly 403 in file mode instead of failing silently. - Warned before dropping policy comments —
policyHasCommentsis surfaced from the users loader and shown as a notice in the group-edit dialog before a save rewrites the policy. - Preserved top-level key order —
parsePolicyrecordskeyOrderandserializePolicyre-emits sections in that order, appending newly created sections at the end so the diff only shows real changes. - Added tests — two new unit tests cover section-order preservation and appending a new section.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
4976aa5 to
3ddc8cf
Compare
|
Fix your merge conflicts and I'll take a look |
3ddc8cf to
4a0199e
Compare
|
Rebased onto The only real overlap was #554 (key expiry toggle), which threads a Verified locally: Ready for review whenever you have a moment. |
|
Hi @tale! Have you had the opportunity to check my pr? |
|
Will look at it tomorrow. There are more conflicts, this is the last one I promise, sorry |
Headscale stores the policy as an opaque HuJSON string. Parse it into a typed model that the UI can edit, and serialize it back in a shape that stays close to a hand-written policy: rules on a single line, key order preserved, empty sections omitted. Top-level keys Headplane does not model (autoApprovers, nodeAttrs, ...) are round-tripped untouched so editing never silently drops them. Also exposes helpers the editors need: the source/destination catalog, group membership lookups, name validation, and `withDefaultPort`, which appends `:*` to a destination that has no port spec since Headscale rejects those. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Access Control page gains two tabs in front of the file editor: - Rules renders `acls`, `ssh` and `hosts` as editable lists. Sources and destinations are built from chips, suggested from the groups, tags, hosts and Headscale users that actually exist in the tailnet. A destination typed without a port gets `:*` appended. - Tags & Groups manages `groups` and `tagOwners`, showing which machines currently carry each tag. Both write into the same buffer the file editor uses, so the diff view and the save button keep working on a single source of truth and nothing reaches Headscale until Save is pressed. A policy that fails to parse falls back to a notice pointing at the file editor, and a policy with comments warns that the structured editors will drop them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Group membership is stored in the ACL policy, which made it invisible from the page where users are actually managed. Show the groups a user belongs to under their name, and add an "Edit groups" entry to the row menu that rewrites the `groups` section of the policy. Editing requires `write_policy` on top of `write_users`, and the loader treats the policy as optional: an unreadable one just hides the UI instead of breaking the page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Headscale accepts any forced tag on a node, but a tag that is missing from `tagOwners` will never match a rule, which is easy to miss. Mark those tags in the machine tag dialog and point at the Access Control page where they can be declared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The undeclared-tag warning fired for every tag whenever the policy could not be read at all: `extractTagOwnerTags` returns `[]` for a missing policy, so the `undefined` guard in the tag dialog was dead code. The machine loaders now pass `undefined` when `api.policy.get()` rejects, so the warning only appears for a policy that was actually read. - "Edit groups" was offered whenever the role allowed it, ignoring whether the policy is writable at all. In `file` mode Headscale rejects the write with "update is disabled", and the failure never reached the operator. The users loader now gates on the same `updatedAt !== null` signal the Access Control page uses, and the action returns a readable error instead of throwing. - Editing groups from the Users page rewrites the whole policy, which drops HuJSON comments, but only the Access Control page warned about it. The group dialog now shows the same notice. - `serializePolicy` emitted sections in a fixed order, so a policy whose sections were written in a different order was reshuffled on the first structured edit. The parsed key order is now preserved, with new sections appended after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4a0199e to
d856366
Compare
|
@tale don't miss this time!! 😂 |
tale
left a comment
tale
left a comment
There was a problem hiding this comment.
I left quite a bit of changes, overall this seems to work so that's good. I'd also appreciate if you can trim down your AI comments a bit (do that manually) as a chunk of your conversation has leaked into some of your comments and they are slightly excessive.
|
|
||
| return { | ||
| ok: true, | ||
| hasComments: stripped.length !== raw.length, |
There was a problem hiding this comment.
stripped also removes trailing commas so the warning will show up for people who do not have comments but simply trailing commas.
| if (destination.includes("::") && !destination.includes("]")) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I don't think this works on bracketless IPv6 addresses if they already have a port?
For example: fd7a::1:22 is technically valid?
| <TableList> | ||
| {selected.length === 0 ? ( | ||
| <TableList.Item className="flex flex-col items-center gap-2.5 py-4 opacity-70"> | ||
| <UsersRound /> | ||
| <p className="font-semibold">This user is not in any group</p> | ||
| </TableList.Item> | ||
| ) : ( | ||
| selected.map((group) => ( | ||
| <TableList.Item className="font-mono" id={group} key={group}> | ||
| {group} | ||
| <Button | ||
| className="rounded-md p-0.5" | ||
| onClick={() => setSelected(selected.filter((entry) => entry !== group))} | ||
| type="button" | ||
| > | ||
| <X className="p-1" /> | ||
| </Button> | ||
| </TableList.Item> | ||
| )) | ||
| )} | ||
| </TableList> | ||
|
|
||
| <div className="flex items-center gap-2"> | ||
| <Input | ||
| aria-label="Add a group" | ||
| className="w-full" | ||
| invalid={draft.length > 0 && draftIsInvalid} | ||
| label="Group" | ||
| labelHidden | ||
| onChange={setDraft} | ||
| placeholder="group:example" | ||
| value={draft} | ||
| /> | ||
| <Button | ||
| className={cn("rounded-md p-1", draftIsInvalid && "cursor-not-allowed opacity-50")} | ||
| disabled={draftIsInvalid} | ||
| onClick={() => { | ||
| setSelected([...selected, draft]); | ||
| setDraft("group:"); | ||
| }} | ||
| type="button" | ||
| > | ||
| <Plus className="p-1" size={30} /> | ||
| </Button> | ||
| </div> | ||
| {options.length > 0 ? ( | ||
| <div className="flex flex-wrap gap-2"> | ||
| {options.map((group) => ( | ||
| <Button | ||
| className="px-2 py-1 font-mono text-xs" | ||
| key={group} | ||
| onClick={() => setSelected([...selected, group])} | ||
| type="button" | ||
| variant="ghost" | ||
| > | ||
| {group} | ||
| </Button> | ||
| ))} | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
Can't we just use TokenList to replace all of this? Seems like it's just duplicate logic
| function Section({ title, description, isDisabled, onAdd, children }: SectionProps) { | ||
| return ( | ||
| <section> | ||
| <div className="mb-3 flex items-end justify-between gap-4"> | ||
| <div> | ||
| <h2 className="text-lg font-medium">{title}</h2> | ||
| <p className="max-w-prose text-sm text-mist-600 dark:text-mist-300">{description}</p> | ||
| </div> | ||
| <Button className="shrink-0" disabled={isDisabled} onClick={onAdd} type="button"> | ||
| <Plus className="h-4 w-4" /> | ||
| Add | ||
| </Button> | ||
| </div> | ||
| <TableList>{children}</TableList> | ||
| </section> | ||
| ); | ||
| } | ||
|
|
||
| function Empty({ text }: { text: string }) { | ||
| return <TableList.Item className="justify-center py-6 text-sm opacity-70">{text}</TableList.Item>; | ||
| } | ||
|
|
||
| function RowActions({ | ||
| isDisabled, | ||
| onEdit, | ||
| onDelete, | ||
| }: { | ||
| isDisabled: boolean; | ||
| onEdit: () => void; | ||
| onDelete: () => void; | ||
| }) { | ||
| return ( | ||
| <div className="flex shrink-0 items-center gap-1"> | ||
| <Button | ||
| aria-label="Edit" | ||
| className="rounded-md p-1" | ||
| disabled={isDisabled} | ||
| onClick={onEdit} | ||
| type="button" | ||
| > | ||
| <Pencil className="h-4 w-4" /> | ||
| </Button> | ||
| <Button | ||
| aria-label="Delete" | ||
| className="rounded-md p-1" | ||
| disabled={isDisabled} | ||
| onClick={onDelete} | ||
| type="button" | ||
| > | ||
| <Trash2 className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Aren't these identical to the stuff in rules-editor.tsx? Just make it shared?
| }, [isOpen, name, members, copy.prefix]); | ||
|
|
||
| const isDuplicate = draftName !== name && existingNames.includes(draftName); | ||
| const nameIsInvalid = !copy.validate(draftName) || isDuplicate; |
There was a problem hiding this comment.
I tested and if I open the dialog up to type it shows it as red/invalid before I even type anything at all. User validation needs that grace. We generally check to make sure the trimmed length is more than 0.
| // `undefined` when the policy could not be read at all, which is what | ||
| // stops the tag dialog from flagging every tag as undeclared. | ||
| policyTags: policy === undefined ? undefined : extractTagOwnerTags(policy), |
There was a problem hiding this comment.
I don't think this does what it's supposed to. extractTagOwnerTags can still fail here
| function toAclRules(value: unknown): AclRule[] { | ||
| if (!Array.isArray(value)) { | ||
| return []; | ||
| } | ||
|
|
||
| return value | ||
| .filter((entry): entry is Record<string, unknown> => entry != null && typeof entry === "object") | ||
| .map((entry) => { | ||
| const rule: AclRule = { | ||
| action: "accept", | ||
| src: toStringList(entry.src), | ||
| dst: toStringList(entry.dst), | ||
| }; | ||
| if (typeof entry.proto === "string" && entry.proto.length > 0) { | ||
| rule.proto = entry.proto; | ||
| } | ||
| return rule; | ||
| }); | ||
| } | ||
|
|
||
| function toSshRules(value: unknown): SshRule[] { | ||
| if (!Array.isArray(value)) { | ||
| return []; | ||
| } | ||
|
|
||
| return value | ||
| .filter((entry): entry is Record<string, unknown> => entry != null && typeof entry === "object") | ||
| .map((entry) => { | ||
| const rule: SshRule = { | ||
| action: entry.action === "check" ? "check" : "accept", | ||
| src: toStringList(entry.src), | ||
| dst: toStringList(entry.dst), | ||
| users: toStringList(entry.users), | ||
| }; | ||
| if (typeof entry.checkPeriod === "string" && entry.checkPeriod.length > 0) { | ||
| rule.checkPeriod = entry.checkPeriod; | ||
| } | ||
| return rule; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The keys here are hardcoded, so if something exists that the functions don't know then it'll just default to "accept" which is probably a security issue or a rendering issue.
- Only warn about dropped comments when the policy actually has some. The previous check compared lengths before and after stripping, which also fires on trailing commas. - Follow Headscale when deciding whether a destination carries a port. It splits on the last colon, so `fd7a::1:22` is `fd7a::1` on port 22 and must not get another `:*` appended, while `fd7a::1` still does. - Keep rule actions and unknown rule fields as they were written instead of defaulting every action to `accept`, which could widen a policy the editor does not understand. Unknown SSH actions are offered as-is in the dialog and unknown actions are shown verbatim in the list. - Return `undefined` from `extractTagOwnerTags` when the policy cannot be parsed, not just when it is missing, so an unreadable policy no longer flags every machine tag as undeclared. - Give the group and tag name field the usual grace period: nothing is flagged until the prefilled prefix is edited. - Reuse `TokenList` for the user group dialog and move it to `app/components` now that it is shared outside the ACL routes. - Share the section, empty and row action pieces between the rules and the tags/groups editors. - Trim the comments across the feature down to the non-obvious parts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed in 70d34c7. All seven review comments are addressed.
On the comments: trimmed across the feature, 79 added lines down to 62. What went was the rationale prose and anything restating the function name; what stayed are the Headscale behaviours that are not visible from the code. That leaves Typecheck, lint and build are clean, 253 unit tests pass. |
There was a problem hiding this comment.
✅ No new issues found.
The new commit addresses every thread from the maintainer review, and the resulting code is clean and well-tested. No prior Pullfrog threads were left open (the three from the initial review were already resolved), and the maintainer's seven threads are each handled by this commit.
Reviewed changes
- Shared editor chrome —
Section/Empty/RowActionswere extracted intoapp/routes/acls/components/editor-section.tsxand shared by the rules and tags/groups editors, and the chip editor moved toapp/components/token-list.tsxto replace the duplicated group picker in the Users dialog. - Unknown-rule handling —
AclRule.actionandSshRule.actionare now kept verbatim instead of being coerced toaccept, and unknown per-rule keys (srcPosture,acceptEnv, ...) round-trip through anextrafield, so editing a rule can no longer silently widen it. - Bracketless IPv6 port detection —
hasPortSpecnow splits on the last colon and only treats the tail as a port when the head is a complete destination, sofd7a::1:22keeps its port rather than gaining:*. - Comment detection —
scanHuJsonreports comments separately from trailing commas, so the "comments will be dropped" warning no longer fires for a policy that only has trailing commas. - Undeclared-tag false positive —
extractTagOwnerTagsreturnsundefinedfor an unreadable or unparseable policy and[]for one that declares no tags, so the machine tag dialog only warns on genuinely undeclared tags. - Input validation grace — the name field in the tag/group dialog no longer shows red before it is edited, and the SSH action select preserves unknown actions as selectable options.
- Tests — new unit tests cover action/unknown-key round-tripping, bracketless IPv6 ports, trailing-comma-only policies, and the
extractTagOwnerTagsundefined/empty distinction.
pnpm run test:unit passes (253 tests). pnpm run typecheck is red, but only on app/routes/ssh/ghostty.client.tsx (ResttyConfig / implicit-any errors) — a file this PR does not touch, so that is pre-existing on main and unrelated to these changes.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Great work! |
2 participants
Closes #607.
The Access Control page is a raw HuJSON editor today. This adds a structured
editor in front of it for the parts that change most often, and surfaces tags
and groups on the pages where the objects they apply to already live.
Nothing is sent to Headscale until Save is pressed, and the structured
editors write into the same buffer the file editor uses — so the file editor,
the diff view and the save button all keep operating on a single source of
truth.
Access Control page
Two tabs in front of
Edit file:acls,sshandhostsas editable lists. Sources anddestinations are built from chips and suggested from the groups, tags, hosts
and Headscale users that actually exist in the tailnet. A destination entered
without a port gets
:*appended, since Headscale rejects one that has none.groupsandtagOwners, showing which machinescurrently carry each tag.
A policy that fails to parse falls back to a notice pointing at the file editor
instead of breaking the page. A policy containing comments warns that the
structured editors will drop them — the file editor stays the way to keep them.
Users page
Group membership lives in the policy, which made it invisible from the page
where users are managed. The groups a user belongs to are now shown under their
name, and the row menu gains Edit groups. Editing requires
write_policyontop of
write_users; if the policy cannot be read the UI simply does not appear.Machines page
A tag assigned to a node but missing from
tagOwnersis accepted by Headscaleyet will never match a rule. The tag dialog now flags those and links to Access
Control.
Policy model
app/utils/acl-policy.tsparses the policy into a typed model and serializes itback close to a hand-written policy: rules on one line, key order preserved,
empty sections omitted, so the diff shows only what changed. Unknown top-level
keys (
autoApprovers,nodeAttrs, ...) are round-tripped untouched.stripJsonCommentsAndTrailingCommasis exported fromnode-info.tsand reusedrather than duplicated.
Verification
pnpm run typecheck,pnpm run lint,pnpm run formatcleanpnpm run test:unit— 216 passing, 26 of them new for the policy model(parsing, round-trip, group membership, port defaulting, validation)
databasepolicy mode: creatingand editing rules, tags and groups from the UI, editing a user's groups from
the Users page, and confirming the resulting policy with
headscale policy getOverlap with existing work
This overlaps with #548 / #549 / #550 and sits next to #603 (which is about
visualizing reachability rather than editing). I would rather not step on
anyone's toes: happy to close this in favour of that work, split it into smaller
PRs, or rebase on top of whichever direction you prefer.
🤖 Generated with Claude Code