feat: GitOps platform-user management + bootstrap superuser SA (protected, config-only)#1709
feat: GitOps platform-user management + bootstrap superuser SA (protected, config-only)#1709rohilsurana wants to merge 25 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds audit recording for platform grant and revoke operations in ChangesPlatform audit recording and bootstrap superuser setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 28435616158Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.7%) to 44.459%Details
Uncovered Changes
Coverage Regressions74 previously-covered lines in 2 files 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
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0783d969-56a8-45e7-8f00-957f3e88825f
📒 Files selected for processing (5)
cmd/serve.goconfig/sample.config.yamlinternal/bootstrap/bootstrapuser.gointernal/bootstrap/bootstrapuser_test.gointernal/bootstrap/service.go
rohilsurana
changed the title
…r downgrade keeps member
…val and reconcile
…t by id, dropping the metadata flag
…rap service account
…euser and drop stale reconcile comment
rohilsurana
changed the title
|
Superseded by the 2-PR split of this work:
Together they equal this PR's final diff. Closing in favour of the smaller, independently-reviewable PRs. |
2 participants
What
Moves platform-superuser management to a GitOps model. Config no longer seeds human
superusers; instead it seeds a single bootstrap superuser service account, and all
ongoing platform-user management (humans and service accounts, admin and member) happens
out-of-band via a declarative
frontier reconcileCLI driven from the IAM repo. Thebootstrap SA is treated as a protected, config-only break-glass identity.
Changes
Remove the config human-superuser flow
app.admin.users+MakeSuperUsers+ the boot-timeauthoritativereconciliationthat an earlier revision of this PR had added. No human superusers are seeded from config.
Config-bootstrapped superuser service account (
app.admin.bootstrap.{client_id, client_secret})ClientSecretservice-account credential exists in theplatform org and is promoted to superuser; rotates the secret if it changed. This is the only
config-seeded superuser and the break-glass identity.
client_idmust be a UUID (it is thecredential id); validated with a fail-fast error.
(
00000000-0000-0000-0000-000000000001, a deliberately non-nil sentinel —uuid.Nilis theaudit "system" actor and must not be a real principal).
creds.Createfails after the service-user row is created, the orphan row is rolled back sorepeated boot failures don't accumulate credential-less service users.
Harden the bootstrap SA — immutable via the API (security)
app.admin.bootstrapat boot. The API refuses, by its well-knownid, to: remove its platform access (
RemovePlatformUser), delete it(
DeleteServiceUser), or mint credentials/tokens/keys for it(
CreateServiceUserCredential/Token/JWK). Not even platform superusers may — thisprevents a rotation-proof superuser backdoor on a publicly-known id.
remove it. (Validated end-to-end on a live instance, including the escalation path where the
per-object
manageauthz is satisfied — the guard still blocks it.)Fix platform-user grant/revoke correctness
Sudois now idempotent on the exact relation tuple, not the derived permission.checkis granted by both
adminandmember, so the old permission check skipped creating themembertuple for an existing admin — silently breaking admin→member downgrades.SudoandUnSudoare now symmetric (both relation-level).UnSudo(ctx, id, relationName)is parameterized and actually strips the requested relation(previously it could only remove
member, neveradmin).RemovePlatformUserhonors the new optionalrelationfield (feat(frontier): add optional relation field to RemovePlatformUserRequest proton#489): when set,removes just that relation (e.g. demote admin → member); empty keeps the remove-both default.
ListPlatformUsersreports all relations a principal holds (metadata.relations), not justthe last-seen one, so the reconciler converges to the desired state exactly (admin-only,
member-only, or both — extras are removed).
metadata.relationis retained for backwardcompatibility.
Audit platform grants/revokes
platform.{admin,member}_{added,removed}events via theauditrecordsystem, on both theRPC path (real principal) and boot/config path (system actor,
uuid.Nil). Change-only.Declarative reconcile framework + CLI
internal/reconcile: aReconcilerinterface + registry + kind-based (kind:) multi-doc YAML,with PlatformUser as the first kind (parse → ListPlatformUsers → diff
(principal, relation)→ apply). Future kinds (roles, plans, traits…) plug in without changing the command/format.
frontier reconcile -f <file> [--dry-run] -H <auth>— authoritative converge of the desiredstate; auth via the existing
--headermechanism.Supporting fix
serviceuser_credentialsorg_name made nullable-tolerant (sql.NullString) — a platform-levelservice account has no real organization row.
Testing
internal/reconcile, bootstrap SA, audit, relation-selectiveremoval, the Sudo/list correctness fixes, and the API immutability guards). Tests run at
bcrypt.MinCostso they don't blow the-race -count 2timeout.superuser the production way (authenticates as the bootstrap SA and grants the test admin via
AddPlatformUser). The e2e caught two real integration bugs the unit mocks couldn't (UUIDclient_id, nullableorg_name), both fixed here.removed, deleted, or have credentials/tokens/keys minted — as a superuser, a regular user, or
unauthenticated — and that the API guard still blocks minting even when the per-object
manageauthorization is deliberately satisfied.
go build ./...,go vet,gofmt,golangci-lintall clean.Related
RemovePlatformUserRequest.relationfield (merged; this PRbumps the proton pin and regenerates).
Notes
app.admin.bootstrapconfigured, boot is a no-op (no superusers seeded);existing platform relations are untouched.
app.admin.users: that config is removed — seed the bootstrapSA instead and manage humans via GitOps.