feat: self-service sign-up to tenant creation end-to-end flow#2126
Merged
feat: self-service sign-up to tenant creation end-to-end flow#2126
Conversation
…ant registration When a user self-registers and the tenant requires async schema provisioning, the registration handler now stores the admin email and password hash in tenant metadata instead of attempting to create the identity immediately (which would fail because the tenant schema doesn't exist yet). A new post-provisioning hook reads the stored credentials after schema provisioning completes and creates the self-registered admin identity with tenant-owner role, then clears the credentials from metadata. This completes the end-to-end self-service sign-up flow: 1. User submits registration form 2. Tenant created with provisioning_pending status + credentials in metadata 3. Provisioning worker creates schemas, runs migrations, seeds reference data 4. Self-registered admin hook creates the admin identity 5. Platform admin hook provisions platform admin (existing behavior) 6. Tenant becomes active 7. User sees provisioning progress page, auto-redirects to login on completion
Contributor
📝 WalkthroughWalkthroughTenant creation now accepts registration metadata and returns a result with TenantID and ProvisioningPending. A new post-provisioning hook creates self-registered admin identities from tenant metadata. Tenant metadata read/update support and wiring/tests were added across gateway, handler, bootstrap, and persistence layers. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Registration Handler
participant TenantCreator
participant TenantDB as Tenant DB
participant ProvisioningWorker as Provisioning Worker
participant AdminHook as Admin Bootstrap Hook
participant IdentityDB as Identity DB
Client->>Handler: Register (email, password)
Handler->>Handler: Hash password
Handler->>TenantCreator: CreateTenant(metadata: email, password_hash, verify_flag)
TenantCreator->>TenantDB: Create tenant with metadata
TenantDB-->>TenantCreator: CreateTenantResult (TenantID, ProvisioningPending)
TenantCreator-->>Handler: CreateTenantResult
alt ProvisioningPending == true
Handler-->>Client: Return provisioning_pending: true
Note over ProvisioningWorker: Provisioning runs asynchronously
ProvisioningWorker->>AdminHook: Run post-provisioning hook for TenantID
AdminHook->>TenantDB: GetMetadata(TenantID)
AdminHook->>IdentityDB: Create/activate admin identity (email, password_hash)
IdentityDB-->>AdminHook: Identity created
AdminHook->>TenantDB: UpdateMetadata (clear registration keys)
TenantDB-->>AdminHook: Metadata updated
else ProvisioningPending == false
Handler->>IdentityDB: Provision admin inline (email, password_hash)
IdentityDB-->>Handler: Identity created
Handler-->>Client: Return provisioning_pending: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummarySolid implementation of deferred identity creation for async tenant provisioning. The latest commit (c9e59a2) addressed the critical email verification bypass from the previous review - the async path now reads The architecture is clean - Risk Assessment
Findings
Bot Review NotesCodeRabbit unresolved threads (5 found):
Previously Flagged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/api-gateway/registration_handler.go (1)
228-262:⚠️ Potential issue | 🟠 MajorPassword hash persists in tenant metadata for sync provisioning path.
In the sync provisioning path (lines 251-262), the handler:
- Creates tenant with metadata containing
_registration_password_hash(line 235)- Creates identity inline (line 253)
- Does NOT clear the metadata
The post-provisioning hook only runs for async provisioning. For sync/active tenants, the password hash remains in tenant metadata indefinitely.
Consider clearing metadata after successful inline identity creation:
🔒️ Proposed fix
} else { // Tenant is immediately active - create identity inline. regErr := h.provisionAdminIdentity(ctx, result.TenantID, req.Email, passwordHash) if regErr != nil { // Compensate: delete orphaned tenant to allow the user to retry. if delErr := h.tenantCreator.DeleteTenant(ctx, result.TenantID); delErr != nil { h.logger.ErrorContext(ctx, "registration: failed to compensate (delete tenant)", "tenant_id", result.TenantID, "error", delErr) } return regErr.status, nil, regErr } + // Clear registration credentials from tenant metadata (best-effort). + // Note: This requires TenantCreator to expose a metadata update method, + // or accept that sync-provisioned tenants briefly retain the hash. }Alternatively, for sync provisioning, skip including credentials in metadata entirely since identity is created inline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/api-gateway/registration_handler.go` around lines 228 - 262, The tenant metadata currently includes MetaKeyRegistrationPasswordHash when calling h.tenantCreator.CreateTenant (metadata map), but in the sync provisioning branch where h.provisionAdminIdentity is called inline the password hash is never removed; update the flow so credentials are not left in tenant metadata: either (A) do not add MetaKeyRegistrationPasswordHash to the metadata map for the immediate-provisioning path (i.e., build metadata without the password hash when result.ProvisioningPending is false), or (B) after successful h.provisionAdminIdentity(ctx, result.TenantID, req.Email, passwordHash) call, call the tenant metadata update/remove API on h.tenantCreator to delete MetaKeyRegistrationPasswordHash (and handle/log any update errors, compensating if needed); reference CreateTenant, provisionAdminIdentity, and DeleteTenant to locate the code to change.
🧹 Nitpick comments (3)
cmd/meridian/wire_services.go (1)
246-255: Duplicate tenant repository instantiation.Line 249 creates a new
tenantpersistence.Repositoryinstance, but line 205 already creates one (repo). While both use the same underlying*gorm.DBconnection, creating multiple Repository instances is slightly wasteful.Consider reusing the existing
repovariable:♻️ Suggested refactor
- tenantRepo := tenantpersistence.NewRepository(conns.gormDB("tenant")) - selfRegHook, hookErr := identitybootstrap.NewSelfRegisteredAdminHook(identityRepo, tenantRepo, logger) + selfRegHook, hookErr := identitybootstrap.NewSelfRegisteredAdminHook(identityRepo, repo, logger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/meridian/wire_services.go` around lines 246 - 255, The code creates a second tenant repository (tenantRepo := tenantpersistence.NewRepository(...)) even though an existing repo variable was already constructed earlier; replace the duplicate instantiation by reusing repo when calling identitybootstrap.NewSelfRegisteredAdminHook (i.e., pass repo instead of creating tenantRepo) and remove the tenantRepo variable to avoid the redundant NewRepository call while keeping the call to NewSelfRegisteredAdminHook, handling hookErr and registering the post-provisioning hook (selfRegHook.AsPostProvisioningHook()) as before.services/identity/bootstrap/self_registered_admin_test.go (2)
15-25: Consider adding test for nil tenant repo validation.The test validates
NewSelfRegisteredAdminHookrejects a nil identity repo but doesn't test the nil tenant repo case. The comment at lines 22-24 mentions this limitation.While constructing a non-nil
domain.Repositoryrequires a DB, you could test the nil tenant repo case by passing a non-nil identity repo mock:💡 Suggestion to improve test coverage
// Mock identity repo for testing nil tenant repo case type mockIdentityRepo struct { identitydomain.Repository } func TestNewSelfRegisteredAdminHook_NilTenantRepoRejected(t *testing.T) { _, err := NewSelfRegisteredAdminHook(&mockIdentityRepo{}, nil, slog.Default()) assert.ErrorIs(t, err, ErrNilTenantRepo) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/identity/bootstrap/self_registered_admin_test.go` around lines 15 - 25, Add a unit test that verifies NewSelfRegisteredAdminHook rejects a nil tenant repository by creating a small mock identity repo (e.g., type mockIdentityRepo struct { identitydomain.Repository }) and calling NewSelfRegisteredAdminHook(&mockIdentityRepo{}, nil, slog.Default()), then assert the returned error is ErrNilTenantRepo; name the test TestNewSelfRegisteredAdminHook_NilTenantRepoRejected and place it alongside the existing TestNewSelfRegisteredAdminHook_Validation.
27-37: Placeholder test provides no coverage.This test only logs a message and doesn't execute any code. Consider either:
- Removing it (since integration tests cover this)
- Adding a real unit test with mocks
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/identity/bootstrap/self_registered_admin_test.go` around lines 27 - 37, The placeholder test TestSelfRegisteredAdminHook_NoMetadata_IsNoop provides no coverage; either delete this test or replace it with a real unit test that constructs a tenant object with no registration metadata, invokes the Provision method on the self-registered-admin hook (call Provision on the hook implementation referenced in your codebase), and assert that Provision returns nil/no-op and does not modify the tenant or produce side effects; ensure you use appropriate mocks for any dependencies and assert expected outcomes rather than only logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/meridian/gateway.go`:
- Around line 226-234: The metadata conversion using
structpb.NewStruct(metadata) can fail and currently only logs a warning
(a.logger.Warn) which leads to creating a tenant with nil req.Metadata and
broken async provisioning; change the handler so that if
structpb.NewStruct(metadata) returns an error you abort registration and return
an error to the caller instead of continuing — i.e., replace the
warn-and-continue behavior around structpb.NewStruct with a failure flow that
returns an appropriate error from the Register handler (so ProvisioningPending
tenants are not created without credentials), keeping the check tied to
structpb.NewStruct, req.Metadata and the ProvisioningPending code path.
In `@services/identity/bootstrap/self_registered_admin.go`:
- Around line 108-118: Currently the handler swallows errors from
h.clearRegistrationMetadata (logged via h.logger.WarnContext) which leaves the
bcrypt password hash in tenant metadata; change the flow so a failure to clear
registration metadata is treated as fatal: return the error from the enclosing
hook (propagate it) instead of continuing to the success log
(h.logger.InfoContext). Ensure you reference and handle failures from
clearRegistrationMetadata( ctx, tenantID, t.Metadata ) by returning that error
(or wrap it) so the caller can retry or abort; if necessary, perform
compensating cleanup of the created identity before returning to avoid
partially-provisioned state.
---
Outside diff comments:
In `@services/api-gateway/registration_handler.go`:
- Around line 228-262: The tenant metadata currently includes
MetaKeyRegistrationPasswordHash when calling h.tenantCreator.CreateTenant
(metadata map), but in the sync provisioning branch where
h.provisionAdminIdentity is called inline the password hash is never removed;
update the flow so credentials are not left in tenant metadata: either (A) do
not add MetaKeyRegistrationPasswordHash to the metadata map for the
immediate-provisioning path (i.e., build metadata without the password hash when
result.ProvisioningPending is false), or (B) after successful
h.provisionAdminIdentity(ctx, result.TenantID, req.Email, passwordHash) call,
call the tenant metadata update/remove API on h.tenantCreator to delete
MetaKeyRegistrationPasswordHash (and handle/log any update errors, compensating
if needed); reference CreateTenant, provisionAdminIdentity, and DeleteTenant to
locate the code to change.
---
Nitpick comments:
In `@cmd/meridian/wire_services.go`:
- Around line 246-255: The code creates a second tenant repository (tenantRepo
:= tenantpersistence.NewRepository(...)) even though an existing repo variable
was already constructed earlier; replace the duplicate instantiation by reusing
repo when calling identitybootstrap.NewSelfRegisteredAdminHook (i.e., pass repo
instead of creating tenantRepo) and remove the tenantRepo variable to avoid the
redundant NewRepository call while keeping the call to
NewSelfRegisteredAdminHook, handling hookErr and registering the
post-provisioning hook (selfRegHook.AsPostProvisioningHook()) as before.
In `@services/identity/bootstrap/self_registered_admin_test.go`:
- Around line 15-25: Add a unit test that verifies NewSelfRegisteredAdminHook
rejects a nil tenant repository by creating a small mock identity repo (e.g.,
type mockIdentityRepo struct { identitydomain.Repository }) and calling
NewSelfRegisteredAdminHook(&mockIdentityRepo{}, nil, slog.Default()), then
assert the returned error is ErrNilTenantRepo; name the test
TestNewSelfRegisteredAdminHook_NilTenantRepoRejected and place it alongside the
existing TestNewSelfRegisteredAdminHook_Validation.
- Around line 27-37: The placeholder test
TestSelfRegisteredAdminHook_NoMetadata_IsNoop provides no coverage; either
delete this test or replace it with a real unit test that constructs a tenant
object with no registration metadata, invokes the Provision method on the
self-registered-admin hook (call Provision on the hook implementation referenced
in your codebase), and assert that Provision returns nil/no-op and does not
modify the tenant or produce side effects; ensure you use appropriate mocks for
any dependencies and assert expected outcomes rather than only logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45a0b3d6-2e67-48ba-927f-eb22ca3bfb2c
📒 Files selected for processing (8)
cmd/meridian/gateway.gocmd/meridian/registration_wiring_test.gocmd/meridian/wire_services.goservices/api-gateway/registration_handler.goservices/api-gateway/registration_handler_test.goservices/identity/bootstrap/self_registered_admin.goservices/identity/bootstrap/self_registered_admin_test.goservices/tenant/adapters/persistence/repository.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Make metadata conversion failure fatal in loopbackTenantCreator (prevents silent registration failure for async provisioning) - Add ClearTenantMetadata to TenantCreator interface and clear credentials from tenant metadata after sync identity provisioning - Make clearRegistrationMetadata failure fatal in post-provisioning hook (enforces minimal credential retention) - Export metadata key constants from bootstrap package and add cross-package sync test against gateway constants - Reuse existing tenant repo in wire_services.go - Add nil tenant repo validation test - Remove placeholder test with no coverage
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/meridian/gateway.go (1)
260-269: Silent no-op when tenantRepo is nil may mask configuration issues.When
tenantRepois nil,ClearTenantMetadatasilently returnsnil. This could mask a wiring bug where the creator was constructed without a tenant repository. Consider logging a warning or returning an error when this is called with a nil repo, since clearing metadata is an explicit action that should fail visibly if unconfigured.However, given this is a "chill" review and the sync path calls this after successful identity creation, the impact is limited to leaving credentials in metadata longer than intended (which the async hook would eventually clear anyway).
💡 Optional: Add a warning log when repo is nil
func (a *loopbackTenantCreator) ClearTenantMetadata(ctx context.Context, tenantID string) error { if a.tenantRepo == nil { + a.logger.Warn("ClearTenantMetadata called but tenant repository not configured", + "tenant_id", tenantID) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/meridian/gateway.go` around lines 260 - 269, The method loopbackTenantCreator.ClearTenantMetadata currently silently returns nil when its tenantRepo is nil, which can hide wiring/configuration bugs; modify ClearTenantMetadata to surface this by either returning a descriptive error (e.g., "tenant repository not configured") or logging a warning before returning, referencing the tenantRepo field to detect the nil case, and ensure callers can observe the failure (prefer returning an error if callers expect clearing to be explicit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/meridian/gateway.go`:
- Around line 260-269: The method loopbackTenantCreator.ClearTenantMetadata
currently silently returns nil when its tenantRepo is nil, which can hide
wiring/configuration bugs; modify ClearTenantMetadata to surface this by either
returning a descriptive error (e.g., "tenant repository not configured") or
logging a warning before returning, referencing the tenantRepo field to detect
the nil case, and ensure callers can observe the failure (prefer returning an
error if callers expect clearing to be explicit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb3bfd36-f298-4aa8-b8d7-b1bca781db32
📒 Files selected for processing (6)
cmd/meridian/gateway.gocmd/meridian/wire_services.goservices/api-gateway/registration_handler.goservices/api-gateway/registration_handler_test.goservices/identity/bootstrap/self_registered_admin.goservices/identity/bootstrap/self_registered_admin_test.go
✅ Files skipped from review due to trivial changes (1)
- services/identity/bootstrap/self_registered_admin_test.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…face Replace direct dependency on *tenantpersistence.Repository with a TenantMetadataStore interface defined in the identity bootstrap package. This fixes the cross-service domain import architecture violation (identity -> tenant/adapters/persistence). Add GetMetadata convenience method on tenant repository to satisfy the new interface without exposing full domain objects.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/identity/bootstrap/self_registered_admin.go`:
- Around line 89-109: The hook currently treats missing/invalid
MetaKeyRegistrationEmail or MetaKeyRegistrationPasswordHash as a no-op and
returns nil; instead detect partial or malformed registration metadata in the
handler (the code checking MetaKeyRegistrationEmail,
MetaKeyRegistrationPasswordHash and the variables emailRaw/hashRaw) and either
(a) return a non-nil error to fail provisioning when one or both keys are
present but invalid, or (b) scrub both registration keys from metadata (remove
MetaKeyRegistrationEmail and MetaKeyRegistrationPasswordHash and persist the
cleaned metadata) before returning, so you do not silently leave a password hash
or half-registration state; implement one consistent behavior and update the
code paths around the email/passwordHash type checks and the hook return to
reflect this.
- Around line 133-143: The idempotency check in the self-registered admin flow
uses h.identityRepo.FindByEmail and returns success if any identity exists,
which misses cases where the existing identity lacks RoleTenantOwner or
tenant-owner assignment; instead, when FindByEmail returns an identity, validate
that its role and tenant-owner metadata include RoleTenantOwner and the correct
tenant ID and, if missing or incorrect, update the identity (e.g., via
identityRepo.Update or a helper like ensureTenantOwnerAssignment) to set the
role to RoleTenantOwner and repair tenant owner links before returning nil; add
logging on repair and any update errors and only treat the hook as complete
after successful verification/repair.
- Around line 123-125: The InfoContext calls in self_registered_admin.go are
logging raw user emails (e.g., the call to h.logger.InfoContext with
"tenant_id", tenantID, "email", email); remove the raw email from these logs
(also the similar call at the other block around lines 139-141) and replace it
with a non-PII form such as a redacted value or a one-way hash (e.g., compute a
sha256 hex or mask the local part) and log that under a descriptive key like
"email_hash" or "email_redacted" while keeping "tenant_id" unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8314d83-74a9-44ed-b92d-8e525876eaaa
📒 Files selected for processing (3)
services/identity/bootstrap/self_registered_admin.goservices/identity/bootstrap/self_registered_admin_test.goservices/tenant/adapters/persistence/repository.go
✅ Files skipped from review due to trivial changes (1)
- services/identity/bootstrap/self_registered_admin_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/tenant/adapters/persistence/repository.go
- Store emailVerificationRequired in tenant metadata so the post-provisioning hook creates PENDING_VERIFICATION identities when email verification is enabled (fixes bypass on async path) - Fail hook on partial/malformed registration metadata instead of silently succeeding (prevents orphaned tenants without admin) - Remove raw email addresses from hook log messages (PII) - Add MetaKeyRegistrationEmailVerifyRequired constant with sync test
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/identity/bootstrap/self_registered_admin.go (1)
140-149:⚠️ Potential issue | 🟡 MinorIdempotency check does not verify the tenant-owner role exists.
When an identity with the same email already exists (line 145), the hook skips creation and returns success without verifying that
RoleTenantOwneris assigned. In scenarios where the identity was created via other means (manual provisioning, previous failed attempts that created identity but failed before role assignment), the tenant could be left without an owner.The codebase uses role reconciliation elsewhere (see
bootstrap.gofor the admin provisioning pattern): when an identity already exists, check for missing required roles and persist them viaSaveRoleAssignments. Consider adopting the same pattern here to ensure idempotency handles partial-state scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/identity/bootstrap/self_registered_admin.go` around lines 140 - 149, The idempotency branch that returns early when an existing identity is found (using h.identityRepo.FindByEmail) must also ensure the identity has RoleTenantOwner assigned; instead of immediately returning when existing != nil, load the current role assignments for that identity and, if RoleTenantOwner is missing, call the repository method used elsewhere (SaveRoleAssignments) to add the tenant-owner role for tenantID, logging actions via h.logger and preserving existing roles; reuse the same reconciliation pattern as in bootstrap.go for admin provisioning to persist missing roles rather than skipping creation outright.
🧹 Nitpick comments (2)
services/api-gateway/registration_handler.go (2)
203-210: Duplication of metadata key constants.These constants are defined identically in
services/identity/bootstrap/self_registered_admin.go(lines 32-36). While the comment mentions a test for sync, consider defining them in a shared location (e.g., ashared/registrationpackage) to eliminate duplication and the need for sync tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/api-gateway/registration_handler.go` around lines 203 - 210, The three metadata constants (MetaKeyRegistrationEmail, MetaKeyRegistrationPasswordHash, MetaKeyRegistrationEmailVerifyRequired) are duplicated; extract them into a single shared package (for example a new package like shared/registration) and remove the duplicate definitions in registration_handler.go and services/identity/bootstrap/self_registered_admin.go; then import that package from both locations and reference the centralized constants (preserve the exact constant names to avoid downstream changes). Ensure the new package contains the same const block and update any imports/usages to use the shared package identifier.
225-230: Error message is misleading for password hash failure.The error returned is
errIdentityCreationFailed, but the actual failure is password hashing. Consider a more specific error or log message for clarity during debugging.💡 Suggested improvement
passwordHash, err := credentials.HashPassword(req.Password) if err != nil { - h.logger.ErrorContext(ctx, "registration: failed to hash password", "error", err) - return http.StatusInternalServerError, nil, errIdentityCreationFailed + h.logger.ErrorContext(ctx, "registration: failed to hash password", "error", err) + return http.StatusInternalServerError, nil, fmt.Errorf("failed to prepare credentials: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/api-gateway/registration_handler.go` around lines 225 - 230, The returned error on password hashing failure is misleading: when credentials.HashPassword(req.Password) fails in the registration flow (see passwordHash and credentials.HashPassword), replace the generic errIdentityCreationFailed with a specific error (e.g., errPasswordHashFailed) and update the returned HTTP status if needed; keep or refine the h.logger.ErrorContext call's message to explicitly mention password hashing failure and include the error details so callers and logs clearly indicate the actual cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@services/identity/bootstrap/self_registered_admin.go`:
- Around line 140-149: The idempotency branch that returns early when an
existing identity is found (using h.identityRepo.FindByEmail) must also ensure
the identity has RoleTenantOwner assigned; instead of immediately returning when
existing != nil, load the current role assignments for that identity and, if
RoleTenantOwner is missing, call the repository method used elsewhere
(SaveRoleAssignments) to add the tenant-owner role for tenantID, logging actions
via h.logger and preserving existing roles; reuse the same reconciliation
pattern as in bootstrap.go for admin provisioning to persist missing roles
rather than skipping creation outright.
---
Nitpick comments:
In `@services/api-gateway/registration_handler.go`:
- Around line 203-210: The three metadata constants (MetaKeyRegistrationEmail,
MetaKeyRegistrationPasswordHash, MetaKeyRegistrationEmailVerifyRequired) are
duplicated; extract them into a single shared package (for example a new package
like shared/registration) and remove the duplicate definitions in
registration_handler.go and
services/identity/bootstrap/self_registered_admin.go; then import that package
from both locations and reference the centralized constants (preserve the exact
constant names to avoid downstream changes). Ensure the new package contains the
same const block and update any imports/usages to use the shared package
identifier.
- Around line 225-230: The returned error on password hashing failure is
misleading: when credentials.HashPassword(req.Password) fails in the
registration flow (see passwordHash and credentials.HashPassword), replace the
generic errIdentityCreationFailed with a specific error (e.g.,
errPasswordHashFailed) and update the returned HTTP status if needed; keep or
refine the h.logger.ErrorContext call's message to explicitly mention password
hashing failure and include the error details so callers and logs clearly
indicate the actual cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d521078f-75f4-4067-beb1-524d62a5bd61
📒 Files selected for processing (4)
services/api-gateway/registration_handler.goservices/api-gateway/registration_handler_test.goservices/identity/bootstrap/self_registered_admin.goservices/identity/bootstrap/self_registered_admin_test.go
✅ Files skipped from review due to trivial changes (1)
- services/identity/bootstrap/self_registered_admin_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/api-gateway/registration_handler_test.go
bjcoombs
dismissed stale reviews from coderabbitai[bot], claude[bot], claude[bot], and coderabbitai[bot]
Stale review - issues addressed in subsequent commits
bjcoombs
added a commit
that referenced
this pull request
Apr 4, 2026
Consolidates the database-per-service plus schema-per-tenant topology into a single reference covering service ownership, tenant isolation mechanism, reference data replication, and the one cross-tenant access pattern. Reflects PR #2125 (public removed from search_path) and PR #2126 (self-service signup). Updates ADR-0003 with a pointer to the new doc and marks the CockroachDB compatibility section as historical. Archives the completed database-per-service migration runbook.
5 tasks
bjcoombs
added a commit
that referenced
this pull request
Apr 4, 2026
* docs: Add Postgres data model reference and tidy stale schema docs Consolidates the database-per-service plus schema-per-tenant topology into a single reference covering service ownership, tenant isolation mechanism, reference data replication, and the one cross-tenant access pattern. Reflects PR #2125 (public removed from search_path) and PR #2126 (self-service signup). Updates ADR-0003 with a pointer to the new doc and marks the CockroachDB compatibility section as historical. Archives the completed database-per-service migration runbook. * docs: Update links after archiving database-per-service migration runbook * docs: Fix relative links in archived database-per-service runbook --------- Co-authored-by: Ben Coombs <bjcoombs@users.noreply.github.com>
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
1 participant
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
provisioning_pendingfield to registration response so the frontend knows the tenant is being set upChanges
Registration handler (
services/api-gateway/registration_handler.go):TenantCreatorinterface to accept metadata and returnCreateTenantResultwith provisioning statusProvisioningPending=true: store credentials in metadata, skip inline identity creationProvisioningPending=false: create identity inline (existing behavior preserved)Self-registered admin hook (
services/identity/bootstrap/self_registered_admin.go):Wiring (
cmd/meridian/wire_services.go,cmd/meridian/gateway.go):InitiateTenantgRPC call via protobuf StructTenant.Statusinstead of deprecatedProvisioningHintfieldTenant repo (
services/tenant/adapters/persistence/repository.go):UpdateMetadatamethod for clearing registration credentials post-provisioningEnd-to-end flow
provisioning_pendingstatus + admin credentials in metadataTest plan
loopbackTenantCreator