feat(security): named RBAC roles (phase 1)

[malickyeu]

Summary

Phase 1 of design/rbac-roles-and-host-scoping.md,
implementing decisions D2 (per-section write), D5 (built-in Viewer/Operator with
Duplicate) and part of D1. Per-host scoping is phase 2 and is not here.

A role is a reusable bundle of section grants — an admin no longer ticks
thirteen checkboxes per account. Each section in a role is independently
read-only or writable, which is finer-grained than the account-level flag.

checkAccess now resolves effective grants instead of reading u.Sections
directly: the union of the account's roles and its own section list, capped by the
read-only flag, with app-wide disabled sections removed last. It remains the single
gate for both REST and MCP, and MCP token narrowing is untouched — it runs
before this and is additive.

Type of change

• Bug fix
• New feature
• Docs only
• Refactor / chore

Checklist

• go test -short ./... and go vet ./... pass
• gofmt gate is clean (gofmt -l $(git ls-files '*.go') after staging)
• Frontend type-checks — N/A (no UI in this PR; see "Deliberately not here")
• Rebuilt and committed web/dist — N/A (nothing under web/src changed)
• Added/updated tests for the change
• Updated docs/ and added a CHANGELOG.md entry for user-facing changes

Notes for reviewers

Start with internal/api/access_middleware.go. It's the only behavioural
change to an existing security path; everything else is additive. The precedence
is explicit and each rule has a test:

1. admin bypasses (unchanged).
2. Grants are the union of roles and the per-account section list.
3. The account-level read-only flag caps everything — a writable role cannot
   lift it. This is what makes D2's migration mapping safe.
4. An app-wide disabled section is removed last, so a role can never re-enable
   a feature an admin turned off.

It also fails closed: if grants can't be computed, access is denied rather
than falling through.

Backwards compatibility is asserted, not assumed.
TestEffectiveGrants_NoRolesMatchesLegacyBehaviour pins that a user with no roles
resolves exactly as before, and the pre-existing suite passes untouched — which is
the real evidence, since it already covered the old semantics.

25 tests, 5 of them new pentests. The ones worth reading:

• built-ins immutable at both layers — store (ErrBuiltinRole) and HTTP (403) —
  because Viewer silently gaining write would escalate everyone holding it;
• a non-admin holding every section is still denied __admin, so no
  combination of grants reaches role management;
• sectionForPath is asserted __admin for every /api/roles… shape, since one
  falling through to "" would make role editing reachable by any authenticated
  user — the single worst failure mode here;
• a read-only account survives a writable role;
• revoking a role takes effect at once — the test re-uses the same
  *store.User value deliberately, to prove grants aren't baked into it.

Design choices worth flagging:

• admin stays a string on the user, not a row in roles. It's the lockout
  safety valve; making it data invites a migration that locks the operator out of
  their own instance.
• Seeding never overwrites an existing built-in row, so an edited description
  survives a restart and grants can't be silently reset on upgrade.
• Unknown section keys in a payload are dropped, not stored — an unenforceable
  permission sitting in the database is worse than a validation error.
• Deleting a role removes its assignments, so no dangling grants remain.

Deliberately not here

• The role-management UI. Roles are configurable over the API only for now, so
  this PR is complete and testable but not yet usable from the browser. UI next.
• LDAP group→role mapping (D6) — it needs the same UI to be configurable, so it
  ships with it rather than dead.
• Per-host scoping — phase 2, unchanged from the design note.

Both are recorded in NEXT.md so they aren't lost.

[Copilot]

Pull request overview

Implements phase 1 of the RBAC “named roles” design: adds reusable roles with per-section write grants, keeps the account-level read-only cap, and updates authorization to compute effective grants (union of per-user sections + assigned roles, minus disabled sections). Adds role CRUD API (admin-only), seeds built-in Viewer/Operator roles, and updates docs/changelog accordingly.

Changes:

• Add roles, role_sections, and user_roles tables plus built-in role seeding (Viewer/Operator) and store APIs for role CRUD + effective grant computation.
• Update access enforcement (checkAccess) to use effective grants and differentiate “account read-only” vs “section read-only”.
• Add admin-only role-management HTTP endpoints + tests/pentests; extend user APIs to return/accept role assignments.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
NEXT.md	Marks RBAC phase 1 (named roles) as shipped and notes follow-ups.
internal/store/store.go	Adds new role-related tables and triggers seeding during migration.
internal/store/roles.go	Implements role CRUD, role assignment, and effective grant computation; seeds built-ins.
internal/store/roles_test.go	Adds unit tests and store-level pentests for roles + effective grants semantics.
internal/api/user_handlers.go	Extends user list/create/update to include role IDs and effective sections.
internal/api/server.go	Registers /api/roles… routes (admin-only).
internal/api/role_pentest_test.go	Adds API-level pentests covering admin-only routes, immutability of built-ins, read-only caps, disabled sections, and revocation immediacy.
internal/api/role_handlers.go	Implements role CRUD + duplicate endpoints and auditing.
internal/api/access_middleware.go	Updates routing-to-section mapping and checkAccess to use effective grants.
docs/users.md	Documents named roles, built-ins, precedence rules, and admin-only role management.
CHANGELOG.md	Adds user-facing release notes for named RBAC roles (phase 1).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

internal/api/mcp_token_handlers.go:123

• The new "filtered-to-nothing" guard only triggers when cleanSections yields a non-empty slice (len(requested) > 0). If the client sends a non-empty sections list made entirely of invalid section keys, cleanSections drops them all and the request falls through to sections == nil/empty, which means an unscoped token (inherits all owner rights). For narrowing tokens, any non-empty sections payload that results in an empty validated+filtered scope should be refused.

	// An explicit scope that filters down to nothing must NOT fall through to
	// "empty = inherit everything" — the caller asked to narrow, and silently
	// handing back an unrestricted token would widen their reach instead.
	if len(requested) > 0 && len(sections) == 0 {
		writeErr(w, http.StatusBadRequest,
			"none of the requested sections are granted to your account, so this token would not be scoped to anything")
		return

internal/store/roles.go:279

• DeleteRole deletes from user_roles and then deletes the role row, but it never deletes role_sections. That leaves orphaned grants rows behind indefinitely. Also, without a transaction, a failure deleting from roles would permanently strip assignments while leaving the role (and its sections) in place.

// DeleteRole removes a user-defined role and any assignments of it. Built-ins are
// refused.
func (s *Store) DeleteRole(ctx context.Context, id int64) error {
	existing, err := s.RoleByID(ctx, id)
	if err != nil {
		return err
	}
	if existing.Builtin {
		return ErrBuiltinRole
	}
	if _, err := s.db.ExecContext(ctx, `DELETE FROM user_roles WHERE role_id = ?`, id); err != nil {
		return err
	}
	_, err = s.db.ExecContext(ctx, `DELETE FROM roles WHERE id = ?`, id)
	return err

internal/store/roles.go:321

• SetUserRoles is not transactional: it deletes all existing assignments, then inserts the new ones. If an insert fails part-way through (DB error, context cancellation), the user can be left with a partially-applied role set. It also does an extra RoleByID query per role ID. Wrapping the rewrite in a transaction and using an EXISTS insert keeps the operation atomic and avoids N+1 queries.

func (s *Store) SetUserRoles(ctx context.Context, userID int64, roleIDs []int64) error {
	if _, err := s.db.ExecContext(ctx, `DELETE FROM user_roles WHERE user_id = ?`, userID); err != nil {
		return err
	}
	seen := map[int64]bool{}
	for _, id := range roleIDs {
		if id <= 0 || seen[id] {
			continue
		}
		seen[id] = true
		if _, err := s.RoleByID(ctx, id); err != nil {
			continue // unknown role: ignore
		}
		if _, err := s.db.ExecContext(ctx,
			`INSERT INTO user_roles (user_id, role_id) VALUES (?, ?)`, userID, id); err != nil {
			return err
		}
	}
	return nil

internal/api/role_handlers.go:177

• The duplicate-name probe calls s.roleByName inside a loop; roleByName calls ListRoles, which loads every role and its sections (N+1 queries inside the store) each time. On a transient DB error, roleByName returns a non-ErrNotFound error which is treated as a name collision, eventually surfacing as "too many copies" instead of 500. Consider reading the existing role names once and handling list errors explicitly.

	// Find a free name rather than failing on the first collision.
	name := src.Name + " copy"
	for i := 2; ; i++ {
		if _, err := s.roleByName(r, name); errors.Is(err, store.ErrNotFound) {
			break
		}
		name = src.Name + " copy " + strconv.Itoa(i)
		if i > 50 {
			writeErr(w, http.StatusConflict, "too many copies of that role")
			return
		}
	}

internal/api/mcp_token_handlers.go:104

• There is no regression test covering the case where the client provides a non-empty sections payload but all entries are invalid section keys. With the current semantics (empty stored scope = inherit), this is an important narrowing-safety case to pin with a pentest: it should be rejected (4xx) and must not persist an unscoped token.

This issue also appears on line 117 of the same file.

	// Section narrowing: a token may only reference sections the owner actually
	// has (admins may use any valid section). Empty = inherit all of the owner's.
	requested := cleanSections(b.Sections)
	sections := requested
	if !u.IsAdmin() {
		// Effective sections, not u.Sections: access may come from an assigned
		// role, and a token must be scopeable to those too.
		effective, err := s.store.EffectiveSections(r.Context(), u)
		if err != nil {
			writeErr(w, http.StatusInternalServerError, "could not determine your permissions")