feat(ldap): fallback role for mappings that no longer resolve - #92
Merged
Conversation
A group mapping can name a role that has since been deleted. Until now its members simply got nothing, which is a silent outage: the mapping still matches, the id no longer resolves, and nobody finds out until someone can't work. An admin can now nominate a fallback role that stands in for whatever failed to resolve, so those users degrade to a known baseline (Viewer) instead. The fallback applies ONLY to a mapping that matched and then failed to resolve. It deliberately does not apply to a user whose groups map to no role at all — that is the ordinary "not entitled" case, and granting a baseline there would hand a role to every account in the directory that can authenticate. Nor does it stack on a mapping that resolves fine. The fallback must not go stale itself, so deleting the role it points at is refused (409, telling the admin to repoint it first); the two built-ins were already undeletable, which makes Viewer a fallback that can never dangle. A fallback id that is somehow missing anyway grants nothing rather than failing the login. Also: link docker-commander.app from the README and the docs index, and add .github/FUNDING.yml. 5 tests, 2 of them pentests. Verified they fail on the wrong behaviour: with the fallback ignored the stand-in test fails; with it applied unconditionally the "not granted to unmapped users" pentest fails; with the delete guard gone the deletion pentest fails.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an LDAP “fallback role” that is applied only when an LDAP group mapping matches but the mapped role ID no longer resolves (e.g., the role was deleted), plus documentation/README updates and GitHub funding metadata.
Changes:
- Add
fallbackRoleIdto LDAP config, expose it via API, and surface it in Settings → LDAP when role mappings are in use. - Apply fallback role during LDAP login role sync when (and only when) at least one mapped role no longer exists; add tests/pentests to pin security boundaries.
- Prevent deletion of a role that is currently configured as the LDAP fallback; update docs/README/CHANGELOG and add
.github/FUNDING.yml.
Reviewed changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/Settings.tsx | Adds UI control and explanatory copy for selecting an LDAP fallback role. |
| web/src/lib/types.ts | Extends LdapConfig type with optional fallbackRoleId. |
| web/dist/index.html | Rebuilt frontend output referencing new asset hash. |
| web/dist/assets/CodeEditor-C92Q_C24.js | Rebuilt frontend asset output. |
| README.md | Adds prominent link to docker-commander.app and related links. |
| internal/store/store.go | Introduces store-level error for “role is LDAP fallback” delete guard. |
| internal/store/roles.go | Guards fallback role deletion; adds role-existence helper for LDAP mapping application. |
| internal/store/ldap.go | Adds persisted FallbackRoleID to LDAP config and normalizes negative values. |
| internal/auth/service.go | Threads fallback through LDAP login role sync; applies fallback for broken mappings only. |
| internal/auth/ldap_roles_test.go | Adds tests and pentests covering fallback behavior and delete guard. |
| internal/api/role_handlers.go | Returns 409 conflict with admin guidance when deleting the fallback role. |
| internal/api/ldap_handlers.go | Includes fallbackRoleId in LDAP config GET response. |
| docs/users.md | Documents fallback role behavior and its intentionally narrow scope. |
| docs/settings.md | Documents fallback role setting in the LDAP settings section. |
| docs/README.md | Adds project home page link to docs index header. |
| CHANGELOG.md | Adds unreleased entry describing LDAP fallback role behavior and constraints. |
| .github/FUNDING.yml | Adds GitHub Sponsors funding metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+278
to
+280
| if cfg, err := s.GetLDAP(ctx); err == nil && cfg.FallbackRoleID == id { | ||
| return ErrRoleInUseAsFallback | ||
| } |
Comment on lines
+288
to
+306
| // ExistingRoleIDs filters ids down to the ones that still name a role, keeping | ||
| // the caller's order. Used when applying LDAP mappings, where an id can outlive | ||
| // the role it referred to. | ||
| func (s *Store) ExistingRoleIDs(ctx context.Context, ids []int64) ([]int64, error) { | ||
| out := make([]int64, 0, len(ids)) | ||
| for _, id := range ids { | ||
| if id <= 0 { | ||
| continue | ||
| } | ||
| switch _, err := s.RoleByID(ctx, id); { | ||
| case errors.Is(err, ErrNotFound): | ||
| continue | ||
| case err != nil: | ||
| return nil, err | ||
| } | ||
| out = append(out, id) | ||
| } | ||
| return out, nil | ||
| } |
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
Two things:
been deleted; until now its members simply got nothing — the mapping still
matches, the id no longer resolves, and nobody finds out until someone can't
work. Nominate a fallback in Settings → LDAP and they degrade to that baseline
(Viewer being the obvious choice) instead.
README header and the docs index, and
.github/FUNDING.ymlis added.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 scope of the fallback is the whole design, and it's narrow on purpose. It
applies only to a mapping that matched and then failed to resolve. It does not
apply to a user whose groups map to no role at all — that's the ordinary "not
entitled" case, and granting a baseline there would hand a role to every account
in the directory that can authenticate, which is a far bigger change than the
convenience it buys.
TestPentestFallbackNotGrantedToUnmappedUserspins that, andit fails the moment the condition is loosened to
if fallback > 0. It doesn't stackon a mapping that resolves fine, either.
The fallback must not go stale itself, or the hole reopens one level up. So
deleting the role it points at is refused — 409 with a message telling the admin to
repoint it first. The two built-in roles were already undeletable, which makes
Viewer a fallback that can never dangle. And if an id goes missing anyway, it grants
nothing rather than failing the login.
I checked the tests fail on the wrong behaviour — ignoring the fallback breaks
the stand-in test, applying it unconditionally breaks the pentest above, and
removing the delete guard breaks the deletion pentest.
Verification
Over the real API: created a role, made it the fallback, and
DELETE /api/roles/{id}returned 409 with "this role is the LDAP fallback — pick a different fallback in
Settings → LDAP first". Clearing the fallback and repeating returned 200.
The settings form was type-checked but not viewed in a browser this run.