feat: manage platform users via GitOps (relation-level grants, bootstrap superuser SA, reconcile CLI)#1719
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 persistence for relation grants and revokes, makes platform removal relation-selective, replaces bootstrap promotion with bootstrap seeding and immutability guards, and adds platform reconciliation plus a new ChangesPlatform audit, bootstrap seeding, and reconciliation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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 28569278944Coverage increased (+0.8%) to 44.833%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines 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: 3
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)
163-189: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
relationNamebefore auto-creating users.For an unknown valid email,
Sudocreates the user before rejecting an invalid relation, leaving persisted user data after a failed request. Move the relation validation to the top of the method.Proposed fix
func (s Service) Sudo(ctx context.Context, id string, relationName string) error { + // validate the 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) + } + currentUser, err := s.GetByID(ctx, id) if errors.Is(err, ErrNotExist) { if isValidEmail(id) { // create a new user @@ if err != nil { return err } - - // validate the 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) - }
🧹 Nitpick comments (3)
internal/api/v1beta1connect/platform_test.go (1)
101-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the deterministic legacy relation value.
The handler sorts relations, so this test can lock both the full ordered list and the backward-compatible first relation instead of only checking non-empty.
🧪 Proposed test tightening
- assert.ElementsMatch(t, []string{schema.AdminRelationName, schema.MemberRelationName}, got) + assert.Equal(t, []string{schema.AdminRelationName, schema.MemberRelationName}, got) // "relation" stays populated for backward compatibility. - assert.NotEmpty(t, fields["relation"].GetStringValue()) + assert.Equal(t, schema.AdminRelationName, fields["relation"].GetStringValue())core/serviceuser/service_test.go (1)
32-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a no-op
Sudotest for an already-present service-user relation.This block only covers the grant path. A regression that re-creates an existing
admin/membertuple—or emits a duplicate grant audit—would still pass for service users.Example shape
+ t.Run("does not recreate an existing member relation", func(t *testing.T) { + svc, repo, _, rel, _, _ := newTestService(t) + repo.On("GetByID", ctx, suID).Return(serviceuser.ServiceUser{ID: suID, Title: "svc"}, nil) + rel.On("CheckPermission", ctx, platformRel(schema.MemberRelationName)).Return(true, nil) + + if err := svc.Sudo(ctx, suID, schema.MemberRelationName); err != nil { + t.Fatalf("Sudo() error = %v", err) + } + })core/user/service_test.go (1)
753-820: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the admin idempotency check for the member
Sudopath.The admin branch has an already-present no-op case, but the member branch only covers creation. A member-specific regression could still duplicate the relation or grant audit without failing this suite.
Example shape
+ { + name: "don't create user member if already created", + wantErr: false, + args: args{ + id: "test-id", + relationName: schema.MemberRelationName, + }, + setup: func() *user.Service { + repo, relationService, sessionService, auditRecordRepository := mockService(t) + repo.EXPECT().GetByName(mock.Anything, "test-id").Return(user.User{ + ID: "test-id", + Name: "test", + }, nil) + relationService.EXPECT().BatchCheckPermission(mock.Anything, []relation.Relation{ + { + Subject: relation.Subject{ID: "test-id", Namespace: schema.UserPrincipal}, + Object: relation.Object{ID: schema.PlatformID, Namespace: schema.PlatformNamespace}, + RelationName: schema.MemberRelationName, + }, + }).Return([]relation.CheckPair{{ + Relation: relation.Relation{ + Subject: relation.Subject{ID: "test-id", Namespace: schema.UserPrincipal}, + Object: relation.Object{ID: schema.PlatformID, Namespace: schema.PlatformNamespace}, + RelationName: schema.MemberRelationName, + }, + Status: true, + }}, nil) + return user.NewService(repo, relationService, sessionService, auditRecordRepository) + }, + },
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6195a1d0-b561-4445-bf51-63b0c50fdb39
⛔ Files ignored due to path filters (1)
proto/v1beta1/admin.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (14)
Makefilecmd/serve.gocore/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.gopkg/auditrecord/consts.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/api/v1beta1connect/platform_test.go (2)
46-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd parallel service-user cases for the new branch-specific behavior.
These tests only exercise the
userServicepath, butplatform.gonow has separateserviceUserServicebranches for relation-scopedUnSudoand normalizedSudo. A regression in the service-user path would still pass this suite.Also applies to: 94-106
76-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the Connect error code for invalid relations.
assert.Errorwill also pass on an unexpected internal failure. These cases should pin the API contract toconnect.CodeInvalidArgument.Suggested assertion tightening
assert.Error(t, err) + assert.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err)) assert.Nil(t, resp)Also applies to: 108-115
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62415c8c-90df-4847-8c6c-1a8231613429
📒 Files selected for processing (2)
internal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/v1beta1connect/platform.go
whoAbhishekSah
left a comment
whoAbhishekSah
left a comment
There was a problem hiding this comment.
Overall LGTM. It might be cleaner to just log the audit log error instead of failing the call.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
cmd/reconcile.go (2)
36-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on the admin-API context used for reconcile RPCs.
RunErunsreconcile.Runwithcmd.Context()directly, which has no deadline. If the admin API hangs (network partition, slow server), the CLI blocks indefinitely with no way out except Ctrl-C, and any half-appliedAddPlatformUser/RemovePlatformUsersequence inplatformuser_reconciler.gostalls mid-way.♻️ Suggested fix
RunE: func(cmd *cli.Command, args []string) error { + ctx, cancel := context.WithTimeout(cmd.Context(), 60*time.Second) + defer cancel() data, err := os.ReadFile(filePath) if err != nil { return fmt.Errorf("read desired-state file: %w", err) } adminClient, err := createAdminClient(cliConfig.Host) if err != nil { return err } registry := map[string]reconcile.Reconciler{ reconcile.KindPlatformUser: reconcile.NewPlatformUserReconciler(adminClient, header), } - reports, runErr := reconcile.Run(cmd.Context(), registry, data, dryRun) + reports, runErr := reconcile.Run(ctx, registry, data, dryRun)
58-58: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAuth secret passed as a plain CLI flag.
--headerputs the rawAuthorization: Basic <base64>value on the command line, which is visible in shell history and to any local user viaps. This mirrors a common CLI pattern (curl-H, kubectl), so it's not a blocker, but consider also accepting the header value via an env var (e.g.FRONTIER_RECONCILE_HEADER) to avoid shell-history/psexposure for operators who prefer it.internal/reconcile/platformuser_reconciler.go (1)
144-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMalformed
--headervalue is silently ignored instead of erroring.If
headerdoesn't contain a:,authReqjust skips setting it — requests then go out unauthenticated with no warning, surfacing later as a confusing auth error from the server instead of a clear CLI usage error.♻️ Suggested fix
-func authReq[T any](msg *T, header string) *connect.Request[T] { +func authReq[T any](msg *T, header string) (*connect.Request[T], error) { req := connect.NewRequest(msg) if header != "" { - if k, v, ok := strings.Cut(header, ":"); ok { - req.Header().Set(strings.TrimSpace(k), strings.TrimSpace(v)) + k, v, ok := strings.Cut(header, ":") + if !ok { + return nil, fmt.Errorf("invalid header %q, want key:value", header) } + req.Header().Set(strings.TrimSpace(k), strings.TrimSpace(v)) } - return req + return req, nil }internal/reconcile/platformuser_reconciler_test.go (1)
14-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test simulates an API error mid-reconcile.
fakePlatformUserAPIalways succeeds. SinceReconcilereturns a partialReport(withAppliedcorrectly reflecting ops before the failure) on anapplyerror, a test exercising that path would both document the intended behavior and catch the report-discarding issue inreconcile.go'sRun.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c26acc5-6d59-4cca-90f2-2e83b27d5bf5
📒 Files selected for processing (24)
cmd/reconcile.gocmd/root.gocmd/serve.goconfig/sample.config.yamlcore/serviceuser/errors.gocore/serviceuser/service.gocore/serviceuser/service_test.gointernal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/platform_test.gointernal/api/v1beta1connect/serviceuser.gointernal/api/v1beta1connect/serviceuser_test.gointernal/bootstrap/bootstrapuser.gointernal/bootstrap/bootstrapuser_test.gointernal/bootstrap/schema/schema.gointernal/bootstrap/service.gointernal/reconcile/platformuser.gointernal/reconcile/platformuser_reconciler.gointernal/reconcile/platformuser_reconciler_test.gointernal/reconcile/platformuser_test.gointernal/reconcile/reconcile.gointernal/reconcile/reconcile_test.gointernal/store/postgres/serviceuser_repository.gotest/e2e/testbench/helper.gotest/e2e/testbench/testbench.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/serviceuser/service_test.go
- internal/api/v1beta1connect/platform.go
- core/serviceuser/service.go
…rrectness # Conflicts: # core/serviceuser/service_test.go
…aller can see what applied
…redential so boot self-heals
…rviceUsers to avoid confusion with human users
rohilsurana
changed the title
…user's credential
3 participants
What
This PR moves platform-user (admin and member) management to a file-based (GitOps) flow, and fixes how grants and revokes work along the way.
It was originally split out of #1709 as two PRs. The second one (#1720 — the bootstrap service account and the
reconcileCLI) has since been merged into this branch, so this PR now carries the whole change.Changes
Platform-user correctness
SudoandUnSudonow act on the exact relation. This fixes a bug where turning an admin into a member did nothing: the old check saw that an admin could already pass, so it skipped addingmember. It also letsUnSudoremoveadmin, not justmember.RemovePlatformUsernow reads the new optionalrelationfield (feat(frontier): add optional relation field to RemovePlatformUserRequest proton#489). Leave it empty to remove both admin and member, as before. Closes RemovePlatformUser is not relation-selective (asymmetric with AddPlatformUser) #1710.ListPlatformUsersnow returns all the relations a user or service account holds, inmetadata.relations. The oldmetadata.relationfield stays for backward compatibility.platform.{admin,member}_{added,removed}on each grant and revoke.GitOps flow
app.admin.bootstrap.{client_id, client_secret}) — created and promoted to superuser at boot, with a fixed id. You rotate the secret by changing the config.client_idmust be a UUID. Boot recovery is safe to repeat: if the account row exists but its credential is missing, the next boot reuses the row and writes the credential. It never creates a duplicate row or a second credential.frontier reconcilecommand — reads a file that lists who should have access and makes the platform match it. Anyone listed is added; anyone not listed is removed. Adds run before removes, so a partial failure never leaves someone with no access. If an apply fails part-way, the report still shows what was applied. It lives ininternal/reconcileand can support more kinds later; PlatformUser is the first.app.admin.usersandMakeSuperUsers.org_namelookup in the service-user store can now be null, since the platform service account's org id points to no real organization row.Testing
Unit tests pass (
-race -count 2). Vet, lint, and format are clean. Full e2e tests pass (the test bench seeds data using the bootstrap service account). A live security check confirmed no one can remove or delete the service account, or create credentials for it, even when the per-object permission check is made to pass.Related