feat: add opt-in authoritative superuser reconciliation#1709
feat: add opt-in authoritative superuser reconciliation#1709rohilsurana wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds audit recording for platform admin/member grant and revoke operations in ChangesPlatform Admin Audit Recording and relationName Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 28370424292Coverage increased (+0.3%) to 44.088%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/serviceuser/service.go (1)
518-531:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
memberrevocation idempotent inUnSudo.
UnSudocurrently returns an error when deleting a non-existentmemberrelation. In remove flows that revoke bothadminandmember, this can fail after successful admin demotion and leave the operation reported as failed.Suggested fix
- if err := s.relationService.Delete(ctx, relation.Relation{ + if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ ID: schema.PlatformID, Namespace: schema.PlatformNamespace, }, Subject: relation.Subject{ ID: currentUser.ID, Namespace: schema.ServiceUserPrincipal, }, RelationName: relationName, - }); err != nil { + }); err != nil && !errors.Is(err, relation.ErrNotExist) { return err }core/user/service.go (1)
255-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat missing
memberrelation as no-op duringUnSudo.When
relationService.Deletereturnsrelation.ErrNotExistformember,UnSudofails even though the desired end state is already satisfied. This can break platform-user removal flows that revoke both relations.Suggested fix
- if err := s.relationService.Delete(ctx, relation.Relation{ + if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ ID: schema.PlatformID, Namespace: schema.PlatformNamespace, }, Subject: relation.Subject{ ID: currentUser.ID, Namespace: schema.UserPrincipal, }, RelationName: relationName, - }); err != nil { + }); err != nil && !errors.Is(err, relation.ErrNotExist) { return err }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1ae0dde-555e-4c9a-b963-1eee98883348
📒 Files selected for processing (16)
cmd/serve.goconfig/sample.config.yamlcore/serviceuser/mocks/audit_record_repository.gocore/serviceuser/service.gocore/serviceuser/service_test.gocore/user/mocks/audit_record_repository.gocore/user/service.gocore/user/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/service_user_service.gointernal/api/v1beta1connect/mocks/user_service.gointernal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/platform_test.gointernal/bootstrap/service.gointernal/bootstrap/service_test.gopkg/auditrecord/consts.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/user/service.go (1)
185-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch
Sudoidempotency to relation-level checks.Line 186-Line 199 uses permission checks (
PlatformCheckPermission) formember. That can short-circuitSudo(..., member)for already-admin users without creating thememberrelation tuple, whileUnSudonow operates on exact relation tuples. This asymmetry can break downgrade flows that expect tuple-level state transitions.Proposed fix
- // check if already su - permissionName := "" - switch relationName { - case schema.MemberRelationName: - permissionName = schema.PlatformCheckPermission - case schema.AdminRelationName: - permissionName = schema.PlatformSudoPermission - } - if permissionName == "" { + // validate requested platform relation + switch relationName { + case schema.MemberRelationName, schema.AdminRelationName: + default: return fmt.Errorf("invalid relation name, possible options are: %s, %s", schema.MemberRelationName, schema.AdminRelationName) } - if ok, err := s.IsSudo(ctx, currentUser.ID, permissionName); err != nil { + // check if the exact relation already exists + if ok, err := s.IsSudo(ctx, currentUser.ID, relationName); err != nil { return err } else if ok { return nil }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6af69031-afee-4758-b2f3-fef5aec7469f
📒 Files selected for processing (5)
core/serviceuser/service.gocore/serviceuser/service_test.gocore/user/service.gocore/user/service_test.gopkg/auditrecord/consts.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/serviceuser/service_test.go
- core/serviceuser/service.go
- core/user/service_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/user/service_test.go (1)
703-704: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen audit matcher assertions to lock the full payload contract.
These matchers only validate
EventandTarget.ID. Please also assertResource(ID/Type),Target.Type, andMetadata["relation"]so payload regressions are caught.Proposed matcher shape
- return r.Event == pkgAuditRecord.PlatformAdminAddedEvent && r.Target != nil && r.Target.ID == "test-id" + return r.Event == pkgAuditRecord.PlatformAdminAddedEvent && + r.Target != nil && + r.Target.ID == "test-id" && + r.Target.Type == pkgAuditRecord.UserType && + r.Resource.ID == schema.PlatformID && + r.Resource.Type == pkgAuditRecord.PlatformType && + r.Metadata["relation"] == schema.AdminRelationNameAlso applies to: 817-818, 903-904, 958-959
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc102442-61a1-47be-b539-28d52a165871
📒 Files selected for processing (7)
core/serviceuser/service.gocore/serviceuser/service_test.gocore/user/service.gocore/user/service_test.gointernal/bootstrap/service.gointernal/bootstrap/service_test.gopkg/auditrecord/consts.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/serviceuser/service_test.go
- core/user/service.go
- internal/bootstrap/service_test.go
- internal/bootstrap/service.go
- core/serviceuser/service.go
2 participants
What
Adds an opt-in mode that makes the
app.admin.usersconfig list the authoritative source of truth for human superusers, and fixes a latent bug that prevented superusers from being demoted at all.Why
Today
MakeSuperUsersonly ever promotes the configured users at boot — it never demotes, so removing someone fromadmin.usershas no effect. And the existing demotion path (UnSudo/RemovePlatformUser) only ever deleted thememberrelation, so it could never actually strip a superuser (admin) — making manual demotion impossible.Changes
app.admin.authoritative(defaultfalse→ unchanged behavior). Whentrue, at boot any human holding the platformadminrelation that is not inadmin.usersis demoted. Service accounts and thememberrelation are never touched.UnSudo(ctx, id, relationName)now removes the specified relation (soadmincan actually be stripped), change-only.RemovePlatformUsernow strips bothadminandmemberfor users and service users — fixing the bug above. Applied to bothuserandserviceuserservices.ErrNotExist) is skipped, but any other resolve error (a transient backend/DB failure) aborts reconciliation rather than risk demoting a real admin that merely looked absent.MakeSuperUsersis a hard-fail bootstrap step, so that abort fails startup (the server won't boot until the error clears) — the intended safe-fail, consistent withMigrateSchema/MigrateRoles.platform.{admin,member}_{added,removed}events (and aplatformentity type) recorded via theauditrecordsystem on every real platform relation change — for both theadminandmemberrelations, on users and service accounts. None existed before. Names follow the existing membership-event convention (organization.added/removed,group.member_added/removed). Emitted insideSudo/UnSudo(change-only — only fires when the relation actually changes); the actor is the acting principal on request paths and the system actor (uuid.Nil) for config/boot-driven changes. (Config reconcile only ever touchesadmin, so at boot onlyadmin_*events fire.)With
authoritative: trueand an empty/misconfigureduserslist, all human superusers are demoted at boot (no guard, by design). Recovery: fixusersand restart, or use a service-account superuser. This is documented inconfig/sample.config.yaml.Testing
user/serviceuserSudo/UnSudo(admin & member add/remove + audit, no-op when the relation is absent, invalid relation), and theRemovePlatformUserhandler (both relations for user + service user).go build ./...,go vet,gofmt,golangci-lint(touched packages) all clean; full unit suite green. e2e (Docker) not run.Known gap / follow-up
AddPlatformUseris relation-specific (grantsadminormember), butRemovePlatformUseris relation-agnostic — it strips both relations becauseRemovePlatformUserRequesthas norelationfield. So there's currently no API to remove a single platform relation (e.g. demote an admin down tomember). The underlying service primitives (Sudo/UnSudo) are already relation-specific and symmetric; only the proto request is coarse. Tracked for a future proto change inraystack/proton: #1710.Notes