fix: address review feedback - credential cleanup and metadata safety

- 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

Author: bjcoombs

cmd/meridian/gateway.go

@@ -17,6 +17,7 @@ import (
 	platformauth "github.com/meridianhub/meridian/shared/platform/auth"
 	"github.com/meridianhub/meridian/shared/platform/env"
 	platformgateway "github.com/meridianhub/meridian/shared/platform/gateway"
+	"github.com/meridianhub/meridian/shared/platform/tenant"
 
 	"google.golang.org/grpc"
 	"google.golang.org/protobuf/types/known/structpb"
@@ -205,6 +206,7 @@ var errNilTenantResponse = fmt.Errorf("InitiateTenant returned nil tenant")
 // gateway.TenantCreator interface used by RegistrationHandler.
 type loopbackTenantCreator struct {
 	client     tenantv1.TenantServiceClient
+	tenantRepo *tenantpersistence.Repository
 	baseDomain string
 	logger     *slog.Logger
 }
@@ -224,13 +226,14 @@ func (a *loopbackTenantCreator) CreateTenant(ctx context.Context, tenantID, slug
 	}
 
 	// Pass registration metadata through to the tenant record.
+	// Metadata conversion must succeed - async provisioning relies on credentials
+	// being present in the tenant record for the post-provisioning hook.
 	if len(metadata) > 0 {
 		pbMeta, err := structpb.NewStruct(metadata)
 		if err != nil {
-			a.logger.Warn("failed to convert registration metadata to protobuf struct", "error", err)
-		} else {
-			req.Metadata = pbMeta
+			return nil, fmt.Errorf("failed to convert registration metadata: %w", err)
 		}
+		req.Metadata = pbMeta
 	}
 
 	resp, err := a.client.InitiateTenant(ctx, req)
@@ -254,6 +257,17 @@ func (a *loopbackTenantCreator) DeleteTenant(ctx context.Context, tenantID strin
 	return err
 }
 
+func (a *loopbackTenantCreator) ClearTenantMetadata(ctx context.Context, tenantID string) error {
+	if a.tenantRepo == nil {
+		return nil
+	}
+	tid, err := tenant.NewTenantID(tenantID)
+	if err != nil {
+		return fmt.Errorf("invalid tenant ID: %w", err)
+	}
+	return a.tenantRepo.UpdateMetadata(ctx, tid, map[string]interface{}{})
+}
+
 // loopbackSlugChecker adapts the tenant persistence repository to the
 // gateway.SlugChecker interface used by RegistrationHandler.
 type loopbackSlugChecker struct {
@@ -307,8 +321,9 @@ func wireRegistration(identityDB, tenantDB *gorm.DB, rawConn *grpc.ClientConn, b
 	identityRepo := identitypersistence.NewRepository(identityDB)
 	tenantClient := tenantv1.NewTenantServiceClient(rawConn)
 
-	creator := &loopbackTenantCreator{client: tenantClient, baseDomain: baseDomain, logger: logger}
-	slugChecker := &loopbackSlugChecker{repo: tenantpersistence.NewRepository(tenantDB)}
+	tenantRepo := tenantpersistence.NewRepository(tenantDB)
+	creator := &loopbackTenantCreator{client: tenantClient, tenantRepo: tenantRepo, baseDomain: baseDomain, logger: logger}
+	slugChecker := &loopbackSlugChecker{repo: tenantRepo}
 
 	emailVerificationRequired := env.GetEnvAsBool("EMAIL_VERIFICATION_REQUIRED", false)
 

cmd/meridian/wire_services.go

@@ -246,8 +246,7 @@ func startProvisioningWorker(ctx context.Context, prov *tenantprovisioner.Postgr
 	// Self-registered admin: creates the admin identity from registration metadata
 	// stored by the registration handler. Must run after all reference data hooks
 	// so the identity schema has instruments, account types, etc. available.
-	tenantRepo := tenantpersistence.NewRepository(conns.gormDB("tenant"))
-	selfRegHook, hookErr := identitybootstrap.NewSelfRegisteredAdminHook(identityRepo, tenantRepo, logger)
+	selfRegHook, hookErr := identitybootstrap.NewSelfRegisteredAdminHook(identityRepo, repo, logger)
 	if hookErr != nil {
 		_ = prov.Close()
 		return nil, nil, fmt.Errorf("self-registered admin hook: %w", hookErr)

services/api-gateway/registration_handler.go

@@ -55,6 +55,9 @@ type TenantCreator interface {
 	// DeleteTenant removes a tenant. Used as compensation when identity provisioning
 	// fails after tenant creation, preventing orphaned tenants.
 	DeleteTenant(ctx context.Context, tenantID string) error
+	// ClearTenantMetadata removes all metadata from a tenant record.
+	// Used to clean up registration credentials after sync identity provisioning.
+	ClearTenantMetadata(ctx context.Context, tenantID string) error
 }
 
 // SlugChecker abstracts slug availability checks for the registration handler.
@@ -259,6 +262,11 @@ func (h *RegistrationHandler) executeRegistration(ctx context.Context, req *regi
 			}
 			return regErr.status, nil, regErr
 		}
+		// Clear registration credentials from tenant metadata (best-effort).
+		if clearErr := h.tenantCreator.ClearTenantMetadata(ctx, result.TenantID); clearErr != nil {
+			h.logger.WarnContext(ctx, "registration: failed to clear credentials from tenant metadata",
+				"tenant_id", result.TenantID, "error", clearErr)
+		}
 	}
 
 	loginURL := fmt.Sprintf("https://%s.%s/login", req.Slug, h.baseDomain)

services/api-gateway/registration_handler_test.go

@@ -24,8 +24,9 @@ import (
 // --- Stub implementations ---
 
 type stubTenantCreator struct {
-	createFn func(ctx context.Context, tenantID, slug, displayName string, metadata map[string]interface{}) (*gateway.CreateTenantResult, error)
-	deleteFn func(ctx context.Context, tenantID string) error
+	createFn        func(ctx context.Context, tenantID, slug, displayName string, metadata map[string]interface{}) (*gateway.CreateTenantResult, error)
+	deleteFn        func(ctx context.Context, tenantID string) error
+	clearMetadataFn func(ctx context.Context, tenantID string) error
 }
 
 func (s *stubTenantCreator) CreateTenant(ctx context.Context, tenantID, slug, displayName string, metadata map[string]interface{}) (*gateway.CreateTenantResult, error) {
@@ -39,6 +40,13 @@ func (s *stubTenantCreator) DeleteTenant(ctx context.Context, tenantID string) e
 	return nil
 }
 
+func (s *stubTenantCreator) ClearTenantMetadata(ctx context.Context, tenantID string) error {
+	if s.clearMetadataFn != nil {
+		return s.clearMetadataFn(ctx, tenantID)
+	}
+	return nil
+}
+
 type stubIdentityRepo struct {
 	saveIdentityWithRolesFn            func(ctx context.Context, identity *identitydomain.Identity, roles []*identitydomain.RoleAssignment) error
 	findByIDFn                         func(ctx context.Context, id uuid.UUID) (*identitydomain.Identity, error)

services/identity/bootstrap/self_registered_admin.go

@@ -19,10 +19,13 @@ import (
 	"github.com/meridianhub/meridian/shared/platform/tenant"
 )
 
-// Metadata keys matching the registration handler constants.
+// Metadata keys for self-registered admin credentials stored on the tenant record.
+// These MUST match the exported constants in services/api-gateway/registration_handler.go
+// (MetaKeyRegistrationEmail, MetaKeyRegistrationPasswordHash). The test file verifies
+// they stay in sync.
 const (
-	metaKeyRegistrationEmail        = "_registration_email"
-	metaKeyRegistrationPasswordHash = "_registration_password_hash"
+	MetaKeyRegistrationEmail        = "_registration_email"
+	MetaKeyRegistrationPasswordHash = "_registration_password_hash"
 )
 
 // ErrNilTenantRepo is returned when a nil tenant repository is passed to NewSelfRegisteredAdminHook.
@@ -77,8 +80,8 @@ func (h *SelfRegisteredAdminHook) Provision(ctx context.Context, tenantID tenant
 		return fmt.Errorf("reading tenant %s: %w", tenantID, err)
 	}
 
-	emailRaw, hasEmail := t.Metadata[metaKeyRegistrationEmail]
-	hashRaw, hasHash := t.Metadata[metaKeyRegistrationPasswordHash]
+	emailRaw, hasEmail := t.Metadata[MetaKeyRegistrationEmail]
+	hashRaw, hasHash := t.Metadata[MetaKeyRegistrationPasswordHash]
 
 	if !hasEmail || !hasHash {
 		h.logger.InfoContext(ctx, "self-registered admin hook: no registration metadata, skipping",
@@ -106,11 +109,9 @@ func (h *SelfRegisteredAdminHook) Provision(ctx context.Context, tenantID tenant
 	}
 
 	// Clear registration credentials from tenant metadata.
+	// This is fatal: leaving a bcrypt hash in metadata violates minimal credential retention.
 	if err := h.clearRegistrationMetadata(ctx, tenantID, t.Metadata); err != nil {
-		// Non-fatal: identity was created successfully. Log and continue.
-		h.logger.WarnContext(ctx, "self-registered admin hook: failed to clear registration metadata",
-			"tenant_id", tenantID,
-			"error", err)
+		return fmt.Errorf("clearing registration metadata for tenant %s: %w", tenantID, err)
 	}
 
 	h.logger.InfoContext(ctx, "self-registered admin identity provisioned",
@@ -171,7 +172,7 @@ func (h *SelfRegisteredAdminHook) clearRegistrationMetadata(ctx context.Context,
 	// Copy metadata without registration keys.
 	cleaned := make(map[string]interface{}, len(metadata))
 	for k, v := range metadata {
-		if k == metaKeyRegistrationEmail || k == metaKeyRegistrationPasswordHash {
+		if k == MetaKeyRegistrationEmail || k == MetaKeyRegistrationPasswordHash {
 			continue
 		}
 		cleaned[k] = v

services/identity/bootstrap/self_registered_admin_test.go

@@ -5,56 +5,42 @@ import (
 	"log/slog"
 	"testing"
 
+	gateway "github.com/meridianhub/meridian/services/api-gateway"
 	"github.com/meridianhub/meridian/services/identity/domain"
 	tenantpersistence "github.com/meridianhub/meridian/services/tenant/adapters/persistence"
 	"github.com/meridianhub/meridian/shared/platform/tenant"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
-func TestNewSelfRegisteredAdminHook_Validation(t *testing.T) {
-	logger := slog.Default()
-
-	_, err := NewSelfRegisteredAdminHook(nil, &tenantpersistence.Repository{}, logger)
+func TestNewSelfRegisteredAdminHook_NilIdentityRepo(t *testing.T) {
+	_, err := NewSelfRegisteredAdminHook(nil, &tenantpersistence.Repository{}, slog.Default())
 	assert.ErrorIs(t, err, ErrNilRepository)
+}
 
-	// Can't easily test with a real identity repo since it requires a DB,
-	// but we verify that nil tenant repo is rejected.
-	// Note: We can't construct a non-nil domain.Repository without a DB connection,
-	// so we test the nil-identity-repo case only.
+// mockIdentityRepo satisfies domain.Repository for testing nil tenant repo validation.
+type mockIdentityRepo struct {
+	domain.Repository
 }
 
-func TestSelfRegisteredAdminHook_NoMetadata_IsNoop(t *testing.T) {
-	// This tests the core logic: when tenant has no registration metadata,
-	// the hook should be a no-op.
-	//
-	// Full integration test would require a DB. Unit test verifies the
-	// metadata extraction logic by checking that Provision returns nil
-	// for tenants without registration metadata.
-	//
-	// This is tested indirectly through the provisioning worker integration tests.
-	t.Log("self-registered admin hook with no metadata is a no-op - tested via integration")
+func TestNewSelfRegisteredAdminHook_NilTenantRepo(t *testing.T) {
+	_, err := NewSelfRegisteredAdminHook(&mockIdentityRepo{}, nil, slog.Default())
+	assert.ErrorIs(t, err, ErrNilTenantRepo)
 }
 
-func TestMetadataKeyConstants(t *testing.T) {
-	// Verify the metadata keys match between the registration handler and hook.
-	// These must stay in sync.
-	assert.Equal(t, "_registration_email", metaKeyRegistrationEmail)
-	assert.Equal(t, "_registration_password_hash", metaKeyRegistrationPasswordHash)
+func TestMetadataKeyConstants_InSyncWithGateway(t *testing.T) {
+	// These constants are duplicated across packages (identity/bootstrap and api-gateway)
+	// because a circular import prevents direct sharing. This test ensures they stay in sync.
+	assert.Equal(t, gateway.MetaKeyRegistrationEmail, MetaKeyRegistrationEmail,
+		"bootstrap.MetaKeyRegistrationEmail must match gateway.MetaKeyRegistrationEmail")
+	assert.Equal(t, gateway.MetaKeyRegistrationPasswordHash, MetaKeyRegistrationPasswordHash,
+		"bootstrap.MetaKeyRegistrationPasswordHash must match gateway.MetaKeyRegistrationPasswordHash")
 }
 
 func TestSelfRegisteredAdminHook_AsPostProvisioningHook(t *testing.T) {
 	// Verify the hook function signature is compatible with the provisioning worker.
-	// We can't create a real hook without DB connections, but we verify the
-	// function type matches what the worker expects.
 	var hookFn func(ctx context.Context, tenantID tenant.TenantID) error
 	_ = hookFn // proves the type signature
 
-	// Also verify the Identity creation logic with known constants.
-	tid, err := tenant.NewTenantID("test_tenant")
-	require.NoError(t, err)
-	assert.Equal(t, "test_tenant", tid.String())
-
 	// Verify RoleTenantOwner is the correct role for self-registered admins.
 	assert.Equal(t, domain.Role("TENANT_OWNER"), domain.RoleTenantOwner)
 }