From 85b2049a619b1e4b30d3f3ca5ad5a532db0ae0f7 Mon Sep 17 00:00:00 2001 From: chan Date: Thu, 26 Mar 2026 12:46:33 +0900 Subject: [PATCH] fix(backend): improve LoginID synchronization from custom metadata fields - Centralize LoginID sync logic in syncLoginID helper - Support namespaced metadata in CreateUser, UpdateUser, and BulkCreateUsers - Ensure UpdateUser and UpdateMe always sync LoginID from configured field even if not in update request - Add phone number normalization consistency for custom LoginIDs - Add unit tests for namespaced metadata LoginID sync --- backend/internal/handler/auth_handler.go | 45 ++-- backend/internal/handler/user_handler.go | 108 ++++++---- backend/internal/handler/user_handler_test.go | 199 ++++++++++++++++++ 3 files changed, 289 insertions(+), 63 deletions(-) diff --git a/backend/internal/handler/auth_handler.go b/backend/internal/handler/auth_handler.go index 0cb9f0a8..cb7c26b9 100644 --- a/backend/internal/handler/auth_handler.go +++ b/backend/internal/handler/auth_handler.go @@ -5272,37 +5272,30 @@ func (h *AuthHandler) UpdateMe(c *fiber.Ctx) error { for k, v := range req.Metadata { // Do not overwrite core fields if _, isCore := map[string]bool{"email": true, "phone_number": true, "name": true, "department": true, "grade": true, "companyCode": true, "affiliationType": true, "id": true, "role": true, "tenant_id": true}[k]; !isCore { - traits[k] = v + // [Fix] Support merging namespaced metadata maps + if incomingMap, ok := v.(map[string]any); ok { + if existingMap, ok := traits[k].(map[string]interface{}); ok { + for subK, subV := range incomingMap { + existingMap[subK] = subV + } + traits[k] = existingMap + } else { + traits[k] = incomingMap + } + } else { + traits[k] = v + } } } } // [LoginID Sync based on Tenant Settings] - schemaCompCode := extractTraitString(traits, "companyCode") - if schemaCompCode != "" && h.TenantService != nil { - if tenant, err := h.TenantService.GetTenantBySlug(c.Context(), schemaCompCode); err == nil && tenant != nil { - if loginIDField, ok := tenant.Config["loginIdField"].(string); ok && loginIDField != "" { - slog.Debug("[UpdateMe] Login ID sync active", "field", loginIDField) - // Search in Metadata (could be flat or namespaced) - var newLoginID string - if val, exists := req.Metadata[loginIDField]; exists { - if s, ok := val.(string); ok { - newLoginID = s - } - } else if namespaced, exists := req.Metadata[tenant.ID]; exists { - if subMeta, ok := namespaced.(map[string]any); ok { - if val, exists := subMeta[loginIDField]; exists { - if s, ok := val.(string); ok { - newLoginID = s - } - } - } - } - - if newLoginID != "" { - slog.Info("[UpdateMe] Syncing custom field to LoginID", "field", loginIDField, "value", newLoginID) - traits["id"] = newLoginID - } + // Perform sync AFTER metadata merge to ensure traits contains current values + syncCompCode := extractTraitString(traits, "companyCode") + if syncCompCode != "" && h.TenantService != nil { + if tenant, err := h.TenantService.GetTenantBySlug(c.Context(), syncCompCode); err == nil && tenant != nil { + if loginIdField, ok := tenant.Config["loginIdField"].(string); ok && loginIdField != "" { + syncLoginID(traits, req.Metadata, tenant.ID, loginIdField) } } } diff --git a/backend/internal/handler/user_handler.go b/backend/internal/handler/user_handler.go index 112b95bb..8191275f 100644 --- a/backend/internal/handler/user_handler.go +++ b/backend/internal/handler/user_handler.go @@ -334,12 +334,8 @@ func (h *UserHandler) CreateUser(c *fiber.Ctx) error { tenantID = tenant.ID // Sync custom field to LoginID if configured - if loginIDField, ok := tenant.Config["loginIdField"].(string); ok && loginIDField != "" { - if val, exists := req.Metadata[loginIDField]; exists { - if loginIDStr, ok := val.(string); ok && loginIDStr != "" { - attributes["id"] = loginIDStr - } - } + if loginIdField, ok := tenant.Config["loginIdField"].(string); ok && loginIdField != "" { + syncLoginID(attributes, req.Metadata, tenantID, loginIdField) } } } @@ -565,11 +561,7 @@ func (h *UserHandler) BulkCreateUsers(c *fiber.Ctx) error { // Sync LoginID from configured custom field (overrides explicit LoginID) if tItem.LoginIDField != "" { - if val, exists := item.Metadata[tItem.LoginIDField]; exists { - if loginIDStr, ok := val.(string); ok && loginIDStr != "" { - attributes["id"] = loginIDStr - } - } + syncLoginID(attributes, item.Metadata, tItem.ID, tItem.LoginIDField) } // Merge metadata @@ -1159,32 +1151,6 @@ func (h *UserHandler) UpdateUser(c *fiber.Ctx) error { traits["id"] = *req.LoginID } - // [LoginID Sync based on Tenant Settings] - schemaCompCode := extractTraitString(traits, "companyCode") - if req.CompanyCode != nil { - schemaCompCode = *req.CompanyCode - } - if schemaCompCode != "" && h.TenantService != nil { - if tenant, err := h.TenantService.GetTenantBySlug(c.Context(), schemaCompCode); err == nil && tenant != nil { - if loginIDField, ok := tenant.Config["loginIdField"].(string); ok && loginIDField != "" { - // Search in Metadata (could be flat or namespaced) - if val, exists := req.Metadata[loginIDField]; exists { - if loginIDStr, ok := val.(string); ok && loginIDStr != "" { - traits["id"] = loginIDStr - } - } else if namespaced, exists := req.Metadata[tenant.ID]; exists { - if subMeta, ok := namespaced.(map[string]any); ok { - if val, exists := subMeta[loginIDField]; exists { - if loginIDStr, ok := val.(string); ok && loginIDStr != "" { - traits["id"] = loginIDStr - } - } - } - } - } - } - } - // [Namespaced Metadata Sync] coreTraits := map[string]bool{ "email": true, "name": true, "phone_number": true, @@ -1212,6 +1178,17 @@ func (h *UserHandler) UpdateUser(c *fiber.Ctx) error { } } + // [LoginID Sync based on Tenant Settings] + // Perform sync AFTER metadata merge to ensure traits contains current values + syncCompCode := extractTraitString(traits, "companyCode") + if syncCompCode != "" && h.TenantService != nil { + if tenant, err := h.TenantService.GetTenantBySlug(c.Context(), syncCompCode); err == nil && tenant != nil { + if loginIdField, ok := tenant.Config["loginIdField"].(string); ok && loginIdField != "" { + syncLoginID(traits, req.Metadata, tenant.ID, loginIdField) + } + } + } + state := normalizeKratosState(req.Status) slog.Info("[UpdateUser] Calling Kratos UpdateIdentity", "userID", userID, "traits", traits, "state", state) @@ -1510,6 +1487,63 @@ func extractTraitString(traits map[string]interface{}, key string) string { return "" } +// syncLoginID ensures that the 'id' trait (used as Kratos identifier) is in sync with the configured custom field. +func syncLoginID(traits map[string]interface{}, metadata map[string]any, tenantID string, loginIDField string) { + if loginIDField == "" || loginIDField == "id" { + return + } + + var loginID string + + // 1. Check incoming metadata (flat) + if val, ok := metadata[loginIDField].(string); ok && val != "" { + loginID = val + } + + // 2. Check incoming metadata (namespaced by tenant ID) + if loginID == "" && tenantID != "" { + if namespaced, ok := metadata[tenantID].(map[string]any); ok { + if val, ok := namespaced[loginIDField].(string); ok && val != "" { + loginID = val + } + } else if namespaced, ok := metadata[tenantID].(map[string]interface{}); ok { + if val, ok := namespaced[loginIDField].(string); ok && val != "" { + loginID = val + } + } + } + + // 3. Check merged traits (which includes existing metadata) + if loginID == "" { + // Existing trait (flat) + if val, ok := traits[loginIDField].(string); ok && val != "" { + loginID = val + } else if tenantID != "" { + // Existing trait (namespaced) + if namespaced, ok := traits[tenantID].(map[string]interface{}); ok { + if val, ok := namespaced[loginIDField].(string); ok && val != "" { + loginID = val + } + } else if namespaced, ok := traits[tenantID].(map[string]any); ok { + if val, ok := namespaced[loginIDField].(string); ok && val != "" { + loginID = val + } + } + } + } + + if loginID != "" { + // Normalize if it looks like a phone number to be consistent with other identifiers + normalized := normalizePhoneNumber(loginID) + if normalized != "" { + loginID = normalized + } + + slog.Info("Syncing LoginID from custom field", "field", loginIDField, "value", loginID, "tenantID", tenantID) + traits["id"] = loginID + } +} + func formatTime(value time.Time) string { if value.IsZero() { return "" diff --git a/backend/internal/handler/user_handler_test.go b/backend/internal/handler/user_handler_test.go index 2319df0d..9e69063e 100644 --- a/backend/internal/handler/user_handler_test.go +++ b/backend/internal/handler/user_handler_test.go @@ -87,6 +87,14 @@ func (m *MockTenantServiceForUser) GetTenantBySlug(ctx context.Context, slug str return args.Get(0).(*domain.Tenant), args.Error(1) } +func (m *MockTenantServiceForUser) ListManageableTenants(ctx context.Context, userID string) ([]domain.Tenant, error) { + args := m.Called(ctx, userID) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]domain.Tenant), args.Error(1) +} + // --- Tests --- func TestUserHandler_BulkCreateUsers(t *testing.T) { @@ -353,3 +361,194 @@ func TestUserHandler_UpdateUser_AdminOnlyField(t *testing.T) { assert.Contains(t, result["error"].(string), "field salary is admin only") }) } + +func TestUserHandler_UpdateUser_LoginIDSync(t *testing.T) { + t.Run("Success - Sync LoginID from namespaced metadata", func(t *testing.T) { + app := fiber.New() + mockKratos := new(MockKratosAdmin) + mockTenant := new(MockTenantServiceForUser) + h := &UserHandler{ + KratosAdmin: mockKratos, + TenantService: mockTenant, + } + app.Put("/users/:id", func(c *fiber.Ctx) error { + c.Locals("user_profile", &domain.UserProfileResponse{Role: domain.RoleSuperAdmin}) + return h.UpdateUser(c) + }) + + tenantID := "t-123" + userID := "u-1" + mockKratos.On("GetIdentity", mock.Anything, userID).Return(&service.KratosIdentity{ + ID: userID, + Traits: map[string]interface{}{ + "email": "user@test.com", + "companyCode": "test-tenant", + "tenant_id": tenantID, + }, + }, nil).Once() + + mockTenant.On("GetTenantBySlug", mock.Anything, "test-tenant").Return(&domain.Tenant{ + ID: tenantID, + Slug: "test-tenant", + Config: domain.JSONMap{ + "loginIdField": "emp_no", + "userSchema": []interface{}{ + map[string]interface{}{"key": "emp_no", "label": "Employee No"}, + }, + }, + }, nil) // Allow multiple calls for validation and sync + + mockTenant.On("ListManageableTenants", mock.Anything, userID).Return([]domain.Tenant{}, nil).Once() + + // Expect traits to include 'id' synced from 'emp_no' + mockKratos.On("UpdateIdentity", mock.Anything, userID, mock.MatchedBy(func(traits map[string]interface{}) bool { + return traits["id"] == "E1001" + }), mock.Anything).Return(&service.KratosIdentity{ + ID: userID, + Traits: map[string]interface{}{ + "id": "E1001", + "email": "user@test.com", + }, + }, nil).Once() + + payload := map[string]interface{}{ + "metadata": map[string]interface{}{ + tenantID: map[string]interface{}{ + "emp_no": "E1001", + }, + }, + } + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/users/"+userID, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + + resp, _ := app.Test(req) + assert.Equal(t, 200, resp.StatusCode) + mockKratos.AssertExpectations(t) + }) + + t.Run("Success - Sync LoginID from existing traits when not in metadata", func(t *testing.T) { + app := fiber.New() + mockKratos := new(MockKratosAdmin) + mockTenant := new(MockTenantServiceForUser) + h := &UserHandler{ + KratosAdmin: mockKratos, + TenantService: mockTenant, + } + app.Put("/users/:id", func(c *fiber.Ctx) error { + c.Locals("user_profile", &domain.UserProfileResponse{Role: domain.RoleSuperAdmin}) + return h.UpdateUser(c) + }) + + tenantID := "t-123" + userID := "u-2" + mockKratos.On("GetIdentity", mock.Anything, userID).Return(&service.KratosIdentity{ + ID: userID, + Traits: map[string]interface{}{ + "email": "user2@test.com", + "companyCode": "test-tenant", + "tenant_id": tenantID, + "id": "old-id", + tenantID: map[string]interface{}{ + "emp_no": "E2002", + }, + }, + }, nil).Once() + + mockTenant.On("GetTenantBySlug", mock.Anything, "test-tenant").Return(&domain.Tenant{ + ID: tenantID, + Slug: "test-tenant", + Config: domain.JSONMap{ + "loginIdField": "emp_no", + }, + }, nil) + + mockTenant.On("ListManageableTenants", mock.Anything, userID).Return([]domain.Tenant{}, nil).Once() + + // Even if metadata is empty, it should sync from existing traits + mockKratos.On("UpdateIdentity", mock.Anything, userID, mock.MatchedBy(func(traits map[string]interface{}) bool { + return traits["id"] == "E2002" + }), mock.Anything).Return(&service.KratosIdentity{ + ID: userID, + Traits: map[string]interface{}{ + "id": "E2002", + }, + }, nil).Once() + + payload := map[string]interface{}{ + "name": "New Name", + } + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/users/"+userID, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + + resp, _ := app.Test(req) + assert.Equal(t, 200, resp.StatusCode) + mockKratos.AssertExpectations(t) + }) +} + +func TestUserHandler_CreateUser_LoginIDSync(t *testing.T) { + t.Run("Success - Sync LoginID from namespaced metadata", func(t *testing.T) { + app := fiber.New() + mockKratos := new(MockKratosAdmin) + mockOry := new(MockOryProvider) + mockTenant := new(MockTenantServiceForUser) + h := &UserHandler{ + KratosAdmin: mockKratos, + OryProvider: mockOry, + TenantService: mockTenant, + } + app.Post("/users", h.CreateUser) + + tenantID := "t-123" + mockTenant.On("GetTenantBySlug", mock.Anything, "test-tenant").Return(&domain.Tenant{ + ID: tenantID, + Slug: "test-tenant", + Config: domain.JSONMap{ + "loginIdField": "emp_no", + "userSchema": []interface{}{ + map[string]interface{}{"key": "emp_no", "label": "Employee No"}, + }, + }, + }, nil) + + mockOry.On("GetPasswordPolicy").Return(&domain.PasswordPolicy{MinLength: 8}, nil) + + // Expect OryProvider.CreateUser to be called with attributes["id"] synced from metadata + mockOry.On("CreateUser", mock.MatchedBy(func(user *domain.BrokerUser) bool { + return user.LoginID == "E1001" && user.Attributes["id"] == "E1001" + }), mock.Anything).Return("u-1", nil).Once() + + // Mock GetIdentity after creation + mockKratos.On("GetIdentity", mock.Anything, "u-1").Return(&service.KratosIdentity{ + ID: "u-1", + Traits: map[string]interface{}{ + "id": "E1001", + "email": "new@test.com", + "companyCode": "test-tenant", + }, + }, nil).Once() + + // Mock ListManageableTenants for mapIdentitySummary + mockTenant.On("ListManageableTenants", mock.Anything, "u-1").Return([]domain.Tenant{}, nil).Once() + + payload := map[string]interface{}{ + "email": "new@test.com", + "name": "New User", + "companyCode": "test-tenant", + "metadata": map[string]interface{}{ + tenantID: map[string]interface{}{ + "emp_no": "E1001", + }, + }, + } + body, _ := json.Marshal(payload) + req := httptest.NewRequest("POST", "/users", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + + resp, _ := app.Test(req) + assert.Equal(t, 201, resp.StatusCode) + mockOry.AssertExpectations(t) + }) +}