feat(ldap): map LDAP groups to named roles (completes RBAC phase 1) - #91
Merged
Conversation
A group mapping can now grant named roles, not just raw sections, so an AD shop drives access the way it drives everything else: put someone in cn=deployers and they hold Deployer on their next login. Roles are re-derived on every login, so revocation is immediate rather than waiting for a session. Roles become directory-driven only once at least one mapping actually grants a role. Gating on "any mapping exists" — the rule sections use — would have stripped hand-assigned roles from every install whose mappings predate this release, which is exactly the silent access change the design note forbids. The trade-off is recorded in the note: emptying the roles from every mapping stops role sync rather than revoking; removing a role from one mapping still revokes normally. Two limits are deliberate and pinned by pentests: a mapping can never grant admin (only the admin group DN does, and no role reaches role management), and a stale id left behind by a deleted role grants nothing instead of failing the login — otherwise deleting a role would lock out everyone in the groups that referenced it. The directory bind moved behind a field on Service so the provisioning rules — what a login is allowed to hand out — are testable without a directory. The real bind still has its container-backed integration test. 11 tests, 4 of them pentests. Verified they fail on the wrong behaviour: with the naive "any mapping" rule the hand-assigned-roles test fails, and with role sync removed the two grant tests fail.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds phase-1 RBAC completion for LDAP by allowing LDAP group mappings to grant named roles (in addition to legacy per-section grants), with role membership re-derived on each login only after role-mapping is actually used—avoiding silent access changes on upgrade.
Changes:
- Backend: extend LDAP group mappings with
roleIds, derive/sync user roles from directory groups on login, and add pentests for non-escalation + stale role safety. - Frontend: extend the LDAP settings editor to show role “pills” above section pills and explain the current sync mode.
- Docs/release notes: document the new behavior, limits, and the “roles become authoritative only once used” rule.
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/Settings.tsx | Adds role selection UI and sync-mode hint to LDAP group mappings editor |
| web/src/lib/types.ts | Extends LDAP group mapping type with optional roleIds |
| web/dist/index.html | Rebuilt web bundle reference |
| web/dist/assets/CodeEditor-DVBww0HF.js | Rebuilt web bundle asset |
| README.md | Updates LDAP feature blurb to include group→role mapping |
| NEXT.md | Marks RBAC phase 1 complete (including LDAP group→role) |
| internal/store/ldap.go | Adds RoleIDs to stored LDAP mappings + cleaning rules |
| internal/store/ldap_test.go | Extends LDAP config round-trip cleaning test for role IDs |
| internal/auth/service.go | Makes LDAP bind swappable for tests; syncs mapped roles on login (gated) |
| internal/auth/ldap.go | Implements RolesForGroups and MapsRoles helpers |
| internal/auth/ldap_roles_test.go | Adds role-mapping unit tests + pentests for security invariants |
| docs/users.md | Documents LDAP-driven roles behavior and the authority gating |
| docs/settings.md | Documents LDAP settings semantics for roles vs sections |
| design/rbac-roles-and-host-scoping.md | Updates phase 1 status + records the gating rule decision |
| CHANGELOG.md | Adds release note entry for LDAP group→role mapping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
16
to
20
| type LDAPGroupMapping struct { | ||
| GroupDN string `json:"groupDn"` | ||
| Sections []string `json:"sections"` | ||
| RoleIDs []int64 `json:"roleIds"` | ||
| } |
Comment on lines
+11
to
+15
| // LDAPGroupMapping grants access to members of an LDAP group, matched on the | ||
| // group's full DN. A mapping can hand out named roles, a raw list of sections, | ||
| // or both; a user's effective access is the union over every mapping whose group | ||
| // they belong to. Roles are the intended way to use this — Sections predates | ||
| // them and stays for configs written before roles existed. |
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
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
An LDAP group mapping can now grant named roles, not just raw sections — the
last piece of phase 1 of
design/rbac-roles-and-host-scoping.md.
Put someone in
cn=deployersand they hold Deployer on their next login; takethem out and they lose it. Roles are re-derived on every login, so revocation
doesn't wait for a session to expire.
The mapping editor in Settings → LDAP now offers role pills above the existing
section pills, and states which of the two sync modes the current config is in.
Type of change
Checklist
go test -short ./...andgo vet ./...passgofmtgate is clean (gofmt -l $(git ls-files '*.go')after staging)cd web && npx tsc --noEmit)web/distdocs/and added aCHANGELOG.mdentry for user-facing changesNotes for reviewers
The one real design decision is when the directory becomes authoritative.
Sections use the rule "any mapping exists ⇒ LDAP owns them". Applying that to roles
would have stripped hand-assigned roles from every install whose mappings were
written before roles existed — a silent access change on the next login, which is
what invariant 6 of the design note forbids. So roles sync only once at least one
mapping actually grants a role.
The cost is stated rather than hidden, in the note and in
docs/users.md: emptyingthe roles from every mapping stops role sync instead of revoking. Removing a role
from one mapping revokes normally, which is the case that actually happens.
Two limits are deliberate and pinned by pentests:
builds the most powerful role possible — every section, all writable — maps it to
a group, and asserts the account is still
userand__adminis absent from itseffective grants.
deleting a role would lock out everyone in the groups referencing it. The test
deletes the role out from under a live mapping and asserts the next login still
succeeds, with no roles.
Also pinned: DN matching is exact, not substring (
cn=ops,dc=example,dc=org,dc=evilgets nothing), and roles follow the groups the directory reports — a login whose
username is literally a mapped group DN gets no roles.
I checked the tests fail on the wrong behaviour. With the naive "any mapping"
rule,
TestPentestSectionOnlyMappingsLeaveHandAssignedRolesAlonefails; with rolesync removed entirely, the two grant/re-sync tests fail. They aren't passing
vacuously.
One structural change: the directory bind moved behind a field on
Service, sothe provisioning rules — what a login is allowed to hand out — can be tested without
a directory. Production still calls
LDAPAuthenticate, which keeps itscontainer-backed integration test.
Verification
Beyond the suites: built the binary, set a mapping over the real API and read it
back. The DN is trimmed, an unknown section is dropped, and
0/-5/ a duplicateare dropped from
roleIds— while the unknown id999is deliberately kept,because role existence is re-checked at login rather than at save time.
I did not run a browser pass on the settings tab this time; it type-checks and the
round-trip above covers the payload, but the pill layout itself is unverified by eye.