From bec8276fae115d3444f597e37bb8c45e431c3d6a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:48:13 +0000 Subject: [PATCH 01/15] refactor: extract OAuth provider identities into separate external_identities table Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/42a73a29-3769-4a5e-891a-7ab32d085f24 Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 132 +++++++++------ .../identra/service_ensure_oauth_user_test.go | 133 +++++++++++---- .../application/identra/user_info_provider.go | 2 + internal/domain/external_identity.go | 46 ++++++ internal/domain/user.go | 8 +- .../gorm_external_identity_store.go | 70 ++++++++ .../persistence/gorm_user_store.go | 12 -- .../mongo_external_identity_store.go | 155 ++++++++++++++++++ .../persistence/mongo_user_store.go | 8 - 9 files changed, 461 insertions(+), 105 deletions(-) create mode 100644 internal/domain/external_identity.go create mode 100644 internal/infrastructure/persistence/gorm_external_identity_store.go create mode 100644 internal/infrastructure/persistence/mongo_external_identity_store.go diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index b159299..52d463d 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -44,6 +44,7 @@ type Service struct { emailCodeStore cache.EmailCodeStore oauthStateStore oauth.StateStore userStore domain.UserStore + externalIdentityStore domain.ExternalIdentityStore userStoreCleanup func(context.Context) error keyManager *security.KeyManager tokenCfg security.TokenConfig @@ -100,7 +101,7 @@ func NewService(ctx context.Context, cfg Config) (*Service, error) { stateTTL = DefaultOAuthStateExpiration } - userStore, cleanup, storeErr := buildUserStore(ctx, cfg) + userStore, externalIdentityStore, cleanup, storeErr := buildStores(ctx, cfg) if storeErr != nil { return nil, storeErr } @@ -164,6 +165,7 @@ func NewService(ctx context.Context, cfg Config) (*Service, error) { return &Service{ userStore: userStore, + externalIdentityStore: externalIdentityStore, keyManager: km, tokenCfg: tokenCfg, oauthStateStore: oauthStore, @@ -411,32 +413,26 @@ func (s *Service) BindUserByOAuth( } s.maybeFillOAuthEmail(ctx, userProvider, token.AccessToken, &userInfo) - providerUser, err := s.userStore.GetByGithubID(ctx, userInfo.ID) + providerIdentity, err := s.externalIdentityStore.GetByProviderID(ctx, stateData.Provider, userInfo.ID) switch { - case err == nil && providerUser.ID != bindingUser.ID: + case err == nil && providerIdentity.UserID != bindingUser.ID: return nil, status.Error(codes.AlreadyExists, "oauth account already linked to another user") case err == nil: - // Already linked to this user; continue to refresh token pair and email if needed. + // Already linked to this user; continue to refresh token pair. case errors.Is(err, domain.ErrNotFound): - // Not linked yet. + // Not linked yet; create the external identity. + identity := &domain.ExternalIdentityModel{ + UserID: bindingUser.ID, + Provider: stateData.Provider, + ProviderUserID: userInfo.ID, + } + if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + return nil, status.Error(codes.Internal, "failed to link oauth account") + } default: return nil, status.Error(codes.Internal, "failed to check existing oauth link") } - if bindingUser.GithubID == nil || *bindingUser.GithubID == "" { - bindingUser.GithubID = &userInfo.ID - } else if *bindingUser.GithubID != userInfo.ID { - return nil, status.Error(codes.FailedPrecondition, "user already linked to another oauth account") - } - - if _, err := s.updateEmailIfNeeded(ctx, bindingUser, userInfo.Email); err != nil { - return nil, err - } - - if err := s.userStore.Update(ctx, bindingUser); err != nil { - return nil, status.Error(codes.Internal, "failed to link oauth account") - } - s.recordLogin(ctx, bindingUser) tokenPair, err := security.NewTokenPair(bindingUser.ID, s.tokenCfg) if err != nil { @@ -789,12 +785,19 @@ func (s *Service) GetCurrentUserLoginInfo( PasswordEnabled: usr.HashedPassword != nil && strings.TrimSpace(*usr.HashedPassword) != "", } - if usr.GithubID != nil && strings.TrimSpace(*usr.GithubID) != "" { - resp.GithubId = usr.GithubID - resp.OauthConnections = append(resp.OauthConnections, &identra_v1_pb.OAuthConnection{ - Provider: "github", - ProviderUserId: *usr.GithubID, - }) + identities, err := s.externalIdentityStore.GetByUserID(ctx, usr.ID) + if err != nil { + slog.WarnContext(ctx, "failed to fetch external identities", "error", err, "user_id", usr.ID) + } else { + for _, identity := range identities { + resp.OauthConnections = append(resp.OauthConnections, &identra_v1_pb.OAuthConnection{ + Provider: identity.Provider, + ProviderUserId: identity.ProviderUserID, + }) + if identity.Provider == "github" { + resp.GithubId = &identity.ProviderUserID + } + } } return resp, nil @@ -804,40 +807,67 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U if info.ID == "" { return nil, status.Error(codes.Internal, "provider user id is empty") } + if info.Provider == "" { + return nil, status.Error(codes.Internal, "provider name is empty") + } - existing, err := s.userStore.GetByGithubID(ctx, info.ID) + existing, err := s.externalIdentityStore.GetByProviderID(ctx, info.Provider, info.ID) switch { case err == nil: - return s.updateEmailIfNeeded(ctx, existing, info.Email) + // External identity already exists; fetch and return the linked user. + usr, userErr := s.userStore.GetByID(ctx, existing.UserID) + if userErr != nil { + return nil, status.Error(codes.Internal, "failed to fetch user linked to oauth identity") + } + return s.updateEmailIfNeeded(ctx, usr, info.Email) case errors.Is(err, domain.ErrNotFound): - // If user is not linked by provider id, try to link by email if available. - // (Email may be missing for some OAuth users, depending on provider settings / privacy.) + // No existing external identity; try to link by email if available. if strings.TrimSpace(info.Email) != "" { byEmail, emailErr := s.userStore.GetByEmail(ctx, info.Email) switch { case emailErr == nil: - byEmail.GithubID = &info.ID - if updateErr := s.userStore.Update(ctx, byEmail); updateErr != nil { - return nil, status.Error(codes.Internal, "failed to link github account") + // Merge: link the external identity to the existing user. + identity := &domain.ExternalIdentityModel{ + UserID: byEmail.ID, + Provider: info.Provider, + ProviderUserID: info.ID, + } + if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + return nil, status.Error(codes.Internal, "failed to link oauth account") } return byEmail, nil case errors.Is(emailErr, domain.ErrNotFound): - // Email is provided but no existing user, create a new one - userModel := &domain.UserModel{Email: info.Email, GithubID: &info.ID} + // Create new user and link external identity. + userModel := &domain.UserModel{Email: info.Email} if createErr := s.userStore.Create(ctx, userModel); createErr != nil { return nil, status.Error(codes.Internal, "failed to create user") } + identity := &domain.ExternalIdentityModel{ + UserID: userModel.ID, + Provider: info.Provider, + ProviderUserID: info.ID, + } + if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + return nil, status.Error(codes.Internal, "failed to create oauth identity") + } return userModel, nil default: return nil, status.Error(codes.Internal, "failed to fetch user by email") } } - // No email provided from OAuth provider, create user with GitHub ID only. - // Email is intentionally left as empty string (default value). - userModel := &domain.UserModel{GithubID: &info.ID} + // No email provided; create user with external identity only. + userModel := &domain.UserModel{} if createErr := s.userStore.Create(ctx, userModel); createErr != nil { return nil, status.Error(codes.Internal, "failed to create user") } + identity := &domain.ExternalIdentityModel{ + UserID: userModel.ID, + Provider: info.Provider, + ProviderUserID: info.ID, + } + if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + return nil, status.Error(codes.Internal, "failed to create oauth identity") + } return userModel, nil default: return nil, status.Error(codes.Internal, "failed to fetch user by provider id") @@ -884,41 +914,49 @@ func (s *Service) maybeFillOAuthEmail(ctx context.Context, provider UserInfoProv info.Email = strings.TrimSpace(email) } -func buildUserStore(ctx context.Context, cfg Config) (domain.UserStore, func(context.Context) error, error) { +func buildStores(ctx context.Context, cfg Config) (domain.UserStore, domain.ExternalIdentityStore, func(context.Context) error, error) { repoType := strings.ToLower(strings.TrimSpace(cfg.PersistenceType)) switch repoType { case "mongo", "mongodb": mongoCfg := cfg.MongoClient if strings.TrimSpace(mongoCfg.URI) == "" { - return nil, nil, fmt.Errorf("mongo uri is required when using mongo user repository") + return nil, nil, nil, fmt.Errorf("mongo uri is required when using mongo user repository") } if strings.TrimSpace(mongoCfg.Database) == "" { - return nil, nil, fmt.Errorf("mongo database is required when using mongo user repository") + return nil, nil, nil, fmt.Errorf("mongo database is required when using mongo user repository") } client, err := mongo.Connect(options.Client().ApplyURI(mongoCfg.URI)) if err != nil { - return nil, nil, fmt.Errorf("failed to connect to mongo: %w", err) + return nil, nil, nil, fmt.Errorf("failed to connect to mongo: %w", err) } - repo, repoErr := persistence.NewMongoUserStore(ctx, client, mongoCfg.Database, "users") + userStore, repoErr := persistence.NewMongoUserStore(ctx, client, mongoCfg.Database, "users") if repoErr != nil { _ = client.Disconnect(ctx) - return nil, nil, repoErr + return nil, nil, nil, repoErr + } + + extStore, extErr := persistence.NewMongoExternalIdentityStore(ctx, client, mongoCfg.Database, "external_identities") + if extErr != nil { + _ = client.Disconnect(ctx) + return nil, nil, nil, extErr } cleanup := func(cleanupCtx context.Context) error { return client.Disconnect(cleanupCtx) } - return repo, cleanup, nil + return userStore, extStore, cleanup, nil case "", "gorm", "postgres", "mysql", "sqlite": db := gorm.NewDB(*cfg.GORMClient) - if err := db.AutoMigrate(&domain.UserModel{}); err != nil { + if err := db.AutoMigrate(&domain.UserModel{}, &domain.ExternalIdentityModel{}); err != nil { slog.Error("failed to migrate database", "error", err) } - return persistence.NewGormUserStore(db), func(context.Context) error { return nil }, nil + userStore := persistence.NewGormUserStore(db) + extStore := persistence.NewGormExternalIdentityStore(db) + return userStore, extStore, func(context.Context) error { return nil }, nil default: - return nil, nil, fmt.Errorf("unsupported user repository type: %s", cfg.PersistenceType) + return nil, nil, nil, fmt.Errorf("unsupported user repository type: %s", cfg.PersistenceType) } } diff --git a/internal/application/identra/service_ensure_oauth_user_test.go b/internal/application/identra/service_ensure_oauth_user_test.go index 5181387..dc18b8a 100644 --- a/internal/application/identra/service_ensure_oauth_user_test.go +++ b/internal/application/identra/service_ensure_oauth_user_test.go @@ -46,15 +46,6 @@ func (m *mockUserStore) GetByEmail(ctx context.Context, email string) (*domain.U return nil, domain.ErrNotFound } -func (m *mockUserStore) GetByGithubID(ctx context.Context, githubID string) (*domain.UserModel, error) { - for _, user := range m.users { - if user.GithubID != nil && *user.GithubID == githubID { - return user, nil - } - } - return nil, domain.ErrNotFound -} - func (m *mockUserStore) Update(ctx context.Context, user *domain.UserModel) error { if _, ok := m.users[user.ID]; !ok { return domain.ErrNotFound @@ -83,13 +74,61 @@ func (m *mockUserStore) Count(ctx context.Context) (int64, error) { return int64(len(m.users)), nil } +// mockExternalIdentityStore is a simple in-memory external identity store for testing. +type mockExternalIdentityStore struct { + identities []*domain.ExternalIdentityModel + forceCreateErr error +} + +func newMockExternalIdentityStore() *mockExternalIdentityStore { + return &mockExternalIdentityStore{} +} + +func (m *mockExternalIdentityStore) Create(_ context.Context, identity *domain.ExternalIdentityModel) error { + if m.forceCreateErr != nil { + return m.forceCreateErr + } + if identity.ID == "" { + identity.ID = "test-identity-id" + } + m.identities = append(m.identities, identity) + return nil +} + +func (m *mockExternalIdentityStore) GetByProviderID( + _ context.Context, + provider, providerUserID string, +) (*domain.ExternalIdentityModel, error) { + for _, id := range m.identities { + if id.Provider == provider && id.ProviderUserID == providerUserID { + return id, nil + } + } + return nil, domain.ErrNotFound +} + +func (m *mockExternalIdentityStore) GetByUserID( + _ context.Context, + userID string, +) ([]*domain.ExternalIdentityModel, error) { + var result []*domain.ExternalIdentityModel + for _, id := range m.identities { + if id.UserID == userID { + result = append(result, id) + } + } + return result, nil +} + func TestEnsureOAuthUser_WithEmail(t *testing.T) { store := newMockUserStore() - svc := &Service{userStore: store} + extStore := newMockExternalIdentityStore() + svc := &Service{userStore: store, externalIdentityStore: extStore} info := UserInfo{ - ID: "github123", - Email: "user@example.com", + Provider: "github", + ID: "github123", + Email: "user@example.com", } user, err := svc.ensureOAuthUser(context.Background(), info) @@ -100,18 +139,26 @@ func TestEnsureOAuthUser_WithEmail(t *testing.T) { if user.Email != "user@example.com" { t.Errorf("expected email 'user@example.com', got %q", user.Email) } - if user.GithubID == nil || *user.GithubID != "github123" { - t.Errorf("expected GithubID 'github123', got %v", user.GithubID) + + // Verify external identity was created with correct fields. + id, lookupErr := extStore.GetByProviderID(context.Background(), "github", "github123") + if lookupErr != nil { + t.Fatalf("expected external identity to exist, got %v", lookupErr) + } + if id.UserID != user.ID { + t.Errorf("expected external identity user_id %q, got %q", user.ID, id.UserID) } } func TestEnsureOAuthUser_WithoutEmail(t *testing.T) { store := newMockUserStore() - svc := &Service{userStore: store} + extStore := newMockExternalIdentityStore() + svc := &Service{userStore: store, externalIdentityStore: extStore} info := UserInfo{ - ID: "github456", - Email: "", + Provider: "github", + ID: "github456", + Email: "", } user, err := svc.ensureOAuthUser(context.Background(), info) @@ -122,26 +169,37 @@ func TestEnsureOAuthUser_WithoutEmail(t *testing.T) { if user.Email != "" { t.Errorf("expected empty email, got %q", user.Email) } - if user.GithubID == nil || *user.GithubID != "github456" { - t.Errorf("expected GithubID 'github456', got %v", user.GithubID) + + id, lookupErr := extStore.GetByProviderID(context.Background(), "github", "github456") + if lookupErr != nil { + t.Fatalf("expected external identity to exist, got %v", lookupErr) + } + if id.UserID != user.ID { + t.Errorf("expected external identity user_id %q, got %q", user.ID, id.UserID) } } -func TestEnsureOAuthUser_ExistingUserByGithubID(t *testing.T) { +func TestEnsureOAuthUser_ExistingUserByProviderID(t *testing.T) { store := newMockUserStore() - svc := &Service{userStore: store} + extStore := newMockExternalIdentityStore() + svc := &Service{userStore: store, externalIdentityStore: extStore} - githubID := "github789" existingUser := &domain.UserModel{ - ID: "existing-user-id", - Email: "existing@example.com", - GithubID: &githubID, + ID: "existing-user-id", + Email: "existing@example.com", } _ = store.Create(context.Background(), existingUser) + _ = extStore.Create(context.Background(), &domain.ExternalIdentityModel{ + ID: "eid1", + UserID: "existing-user-id", + Provider: "github", + ProviderUserID: "github789", + }) info := UserInfo{ - ID: "github789", - Email: "different@example.com", + Provider: "github", + ID: "github789", + Email: "different@example.com", } user, err := svc.ensureOAuthUser(context.Background(), info) @@ -152,7 +210,7 @@ func TestEnsureOAuthUser_ExistingUserByGithubID(t *testing.T) { if user.ID != existingUser.ID { t.Errorf("expected existing user ID, got %q", user.ID) } - // Email should be updated + // Email should be updated on the user. if user.Email != "different@example.com" { t.Errorf("expected email to be updated to 'different@example.com', got %q", user.Email) } @@ -160,7 +218,8 @@ func TestEnsureOAuthUser_ExistingUserByGithubID(t *testing.T) { func TestEnsureOAuthUser_LinkExistingUserByEmail(t *testing.T) { store := newMockUserStore() - svc := &Service{userStore: store} + extStore := newMockExternalIdentityStore() + svc := &Service{userStore: store, externalIdentityStore: extStore} existingUser := &domain.UserModel{ ID: "existing-user-id", @@ -169,8 +228,9 @@ func TestEnsureOAuthUser_LinkExistingUserByEmail(t *testing.T) { _ = store.Create(context.Background(), existingUser) info := UserInfo{ - ID: "github999", - Email: "existing@example.com", + Provider: "github", + ID: "github999", + Email: "existing@example.com", } user, err := svc.ensureOAuthUser(context.Background(), info) @@ -181,7 +241,14 @@ func TestEnsureOAuthUser_LinkExistingUserByEmail(t *testing.T) { if user.ID != existingUser.ID { t.Errorf("expected existing user ID, got %q", user.ID) } - if user.GithubID == nil || *user.GithubID != "github999" { - t.Errorf("expected GithubID to be linked to 'github999', got %v", user.GithubID) + + // Verify external identity was linked to the existing user. + id, lookupErr := extStore.GetByProviderID(context.Background(), "github", "github999") + if lookupErr != nil { + t.Fatalf("expected external identity to exist, got %v", lookupErr) + } + if id.UserID != existingUser.ID { + t.Errorf("expected external identity user_id %q, got %q", existingUser.ID, id.UserID) } } + diff --git a/internal/application/identra/user_info_provider.go b/internal/application/identra/user_info_provider.go index 361c10f..5457443 100644 --- a/internal/application/identra/user_info_provider.go +++ b/internal/application/identra/user_info_provider.go @@ -8,6 +8,7 @@ import ( ) type UserInfo struct { + Provider string ID string Email string Username string @@ -40,6 +41,7 @@ func (g *GitHubUserInfoProvider) GetUserInfo(ctx context.Context, token string) return UserInfo{}, err } return UserInfo{ + Provider: "github", ID: fmt.Sprintf("%d", user.GetID()), Email: user.GetEmail(), Username: user.GetLogin(), diff --git a/internal/domain/external_identity.go b/internal/domain/external_identity.go new file mode 100644 index 0000000..d573415 --- /dev/null +++ b/internal/domain/external_identity.go @@ -0,0 +1,46 @@ +package domain + +import ( + "context" + "time" + + "github.com/google/uuid" + "gorm.io/gorm" +) + +// ExternalIdentityModel represents an OAuth provider identity linked to a user. +// Each row represents the binding (provider, provider_user_id) -> user_id. +type ExternalIdentityModel struct { + ID string `gorm:"type:varchar(36);primaryKey" bson:"_id,omitempty" json:"id"` + UserID string `gorm:"type:varchar(36);index" bson:"user_id" json:"user_id"` + Provider string `gorm:"type:varchar(50)" bson:"provider" json:"provider"` + ProviderUserID string `gorm:"type:varchar(255)" bson:"provider_user_id" json:"provider_user_id"` + CreatedAt time.Time `bson:"created_at,omitempty" json:"created_at"` + UpdatedAt time.Time `bson:"updated_at,omitempty" json:"updated_at"` + DeletedAt gorm.DeletedAt `gorm:"index" bson:"-" json:"-"` +} + +// TableName returns the database table name for ExternalIdentityModel. +func (ExternalIdentityModel) TableName() string { + return "external_identities" +} + +// BeforeCreate generates a UUID for the external identity before creating. +func (e *ExternalIdentityModel) BeforeCreate(_ *gorm.DB) error { + if e.ID == "" { + e.ID = uuid.New().String() + } + return nil +} + +// ExternalIdentityStore defines the interface for external identity persistence operations. +type ExternalIdentityStore interface { + // Create persists a new external identity. Returns ErrAlreadyExists if the + // (provider, provider_user_id) pair is already linked to a user. + Create(ctx context.Context, identity *ExternalIdentityModel) error + // GetByProviderID looks up a single external identity by (provider, provider_user_id). + // Returns ErrNotFound if no match exists. + GetByProviderID(ctx context.Context, provider, providerUserID string) (*ExternalIdentityModel, error) + // GetByUserID returns all external identities linked to the given user. + GetByUserID(ctx context.Context, userID string) ([]*ExternalIdentityModel, error) +} diff --git a/internal/domain/user.go b/internal/domain/user.go index 436de87..ff4c0ba 100644 --- a/internal/domain/user.go +++ b/internal/domain/user.go @@ -22,10 +22,9 @@ type UserModel struct { UpdatedAt time.Time `bson:"updated_at,omitempty" json:"updated_at"` DeletedAt gorm.DeletedAt `gorm:"index" bson:"-" json:"-"` Email string `gorm:"type:varchar(255);uniqueIndex" bson:"email" json:"email"` - HashedPassword *string `gorm:"column:hashed_password" bson:"hashed_password,omitempty" json:"-"` - VerificationHash *string `gorm:"column:hash" bson:"hash,omitempty" json:"-"` - GithubID *string `gorm:"column:github_id;unique" bson:"github_id,omitempty" json:"github_id"` - LastLoginAt *time.Time ` bson:"last_login_at,omitempty" json:"last_login_at"` + HashedPassword *string `gorm:"column:hashed_password" bson:"hashed_password,omitempty" json:"-"` + VerificationHash *string `gorm:"column:hash" bson:"hash,omitempty" json:"-"` + LastLoginAt *time.Time ` bson:"last_login_at,omitempty" json:"last_login_at"` } // TableName returns the database table name for UserModel. @@ -46,7 +45,6 @@ type UserStore interface { Create(ctx context.Context, user *UserModel) error GetByID(ctx context.Context, id string) (*UserModel, error) GetByEmail(ctx context.Context, email string) (*UserModel, error) - GetByGithubID(ctx context.Context, githubID string) (*UserModel, error) Update(ctx context.Context, user *UserModel) error Delete(ctx context.Context, id string) error List(ctx context.Context, offset, limit int) ([]*UserModel, error) diff --git a/internal/infrastructure/persistence/gorm_external_identity_store.go b/internal/infrastructure/persistence/gorm_external_identity_store.go new file mode 100644 index 0000000..8d3ecfb --- /dev/null +++ b/internal/infrastructure/persistence/gorm_external_identity_store.go @@ -0,0 +1,70 @@ +package persistence + +import ( + "context" + "log/slog" + + "github.com/poly-workshop/identra/internal/domain" + "gorm.io/gorm" +) + +type gormExternalIdentityStore struct { + db *gorm.DB +} + +// NewGormExternalIdentityStore creates a new GORM-based external identity store. +func NewGormExternalIdentityStore(db *gorm.DB) domain.ExternalIdentityStore { + return &gormExternalIdentityStore{db: db} +} + +func (r *gormExternalIdentityStore) Create( + ctx context.Context, + identity *domain.ExternalIdentityModel, +) error { + err := r.db.WithContext(ctx).Create(identity).Error + if err != nil { + slog.ErrorContext( + ctx, + "failed to create external identity", + "error", err, + "provider", identity.Provider, + "provider_user_id", identity.ProviderUserID, + ) + return wrapGormError(err) + } + slog.InfoContext( + ctx, + "external identity created successfully", + "id", identity.ID, + "user_id", identity.UserID, + "provider", identity.Provider, + ) + return nil +} + +func (r *gormExternalIdentityStore) GetByProviderID( + ctx context.Context, + provider, providerUserID string, +) (*domain.ExternalIdentityModel, error) { + var identity domain.ExternalIdentityModel + err := r.db.WithContext(ctx). + Where("provider = ? AND provider_user_id = ?", provider, providerUserID). + First(&identity).Error + if err != nil { + return nil, wrapGormError(err) + } + return &identity, nil +} + +func (r *gormExternalIdentityStore) GetByUserID( + ctx context.Context, + userID string, +) ([]*domain.ExternalIdentityModel, error) { + var identities []*domain.ExternalIdentityModel + err := r.db.WithContext(ctx).Where("user_id = ?", userID).Find(&identities).Error + if err != nil { + slog.ErrorContext(ctx, "failed to list external identities", "error", err, "user_id", userID) + return nil, err + } + return identities, nil +} diff --git a/internal/infrastructure/persistence/gorm_user_store.go b/internal/infrastructure/persistence/gorm_user_store.go index e34e0c1..1876441 100644 --- a/internal/infrastructure/persistence/gorm_user_store.go +++ b/internal/infrastructure/persistence/gorm_user_store.go @@ -69,18 +69,6 @@ func (r *gormUserStore) GetByEmail(ctx context.Context, email string) (*domain.U return &user, nil } -func (r *gormUserStore) GetByGithubID( - ctx context.Context, - githubID string, -) (*domain.UserModel, error) { - var user domain.UserModel - err := r.db.WithContext(ctx).Where("github_id = ?", githubID).First(&user).Error - if err != nil { - return nil, wrapGormError(err) - } - return &user, nil -} - func (r *gormUserStore) Update(ctx context.Context, user *domain.UserModel) error { err := r.db.WithContext(ctx).Save(user).Error if err != nil { diff --git a/internal/infrastructure/persistence/mongo_external_identity_store.go b/internal/infrastructure/persistence/mongo_external_identity_store.go new file mode 100644 index 0000000..e2b8d93 --- /dev/null +++ b/internal/infrastructure/persistence/mongo_external_identity_store.go @@ -0,0 +1,155 @@ +package persistence + +import ( + "context" + "errors" + "log/slog" + "strings" + "time" + + "github.com/google/uuid" + "github.com/poly-workshop/identra/internal/domain" + "go.mongodb.org/mongo-driver/v2/bson" + "go.mongodb.org/mongo-driver/v2/mongo" + "go.mongodb.org/mongo-driver/v2/mongo/options" +) + +type mongoExternalIdentityStore struct { + coll *mongo.Collection +} + +// NewMongoExternalIdentityStore builds a MongoDB-backed external identity store and ensures indexes. +func NewMongoExternalIdentityStore( + ctx context.Context, + client *mongo.Client, + databaseName string, + collectionName string, +) (domain.ExternalIdentityStore, error) { + if client == nil { + return nil, errors.New("mongo client is required") + } + if strings.TrimSpace(databaseName) == "" { + return nil, errors.New("mongo database name is required") + } + if strings.TrimSpace(collectionName) == "" { + collectionName = "external_identities" + } + + store := &mongoExternalIdentityStore{ + coll: client.Database(databaseName).Collection(collectionName), + } + if err := store.ensureIndexes(ctx); err != nil { + return nil, err + } + return store, nil +} + +func (r *mongoExternalIdentityStore) ensureIndexes(ctx context.Context) error { + models := []mongo.IndexModel{ + { + Keys: bson.D{ + {Key: "provider", Value: 1}, + {Key: "provider_user_id", Value: 1}, + }, + Options: options.Index().SetUnique(true).SetName("idx_provider_user_id_unique"), + }, + { + Keys: bson.D{{Key: "user_id", Value: 1}}, + Options: options.Index().SetName("idx_user_id"), + }, + } + + if _, err := r.coll.Indexes().CreateMany(ctx, models); err != nil { + slog.WarnContext(ctx, "failed to ensure external identity indexes", "error", err) + return err + } + return nil +} + +func (r *mongoExternalIdentityStore) Create( + ctx context.Context, + identity *domain.ExternalIdentityModel, +) error { + now := time.Now().UTC() + if strings.TrimSpace(identity.ID) == "" { + identity.ID = uuid.New().String() + } + if identity.CreatedAt.IsZero() { + identity.CreatedAt = now + } + identity.UpdatedAt = now + + if _, err := r.coll.InsertOne(ctx, identity); err != nil { + slog.ErrorContext( + ctx, + "failed to create external identity (mongo)", + "error", err, + "provider", identity.Provider, + "provider_user_id", identity.ProviderUserID, + ) + if mongo.IsDuplicateKeyError(err) { + return domain.ErrAlreadyExists + } + return err + } + slog.InfoContext( + ctx, + "external identity created successfully (mongo)", + "id", identity.ID, + "user_id", identity.UserID, + "provider", identity.Provider, + ) + return nil +} + +func (r *mongoExternalIdentityStore) GetByProviderID( + ctx context.Context, + provider, providerUserID string, +) (*domain.ExternalIdentityModel, error) { + var identity domain.ExternalIdentityModel + err := r.coll.FindOne(ctx, bson.M{ + "provider": provider, + "provider_user_id": providerUserID, + }).Decode(&identity) + if err != nil { + if errors.Is(err, mongo.ErrNoDocuments) { + return nil, domain.ErrNotFound + } + slog.ErrorContext( + ctx, + "failed to fetch external identity (mongo)", + "error", err, + "provider", provider, + "provider_user_id", providerUserID, + ) + return nil, err + } + return &identity, nil +} + +func (r *mongoExternalIdentityStore) GetByUserID( + ctx context.Context, + userID string, +) ([]*domain.ExternalIdentityModel, error) { + cursor, err := r.coll.Find(ctx, bson.M{"user_id": userID}) + if err != nil { + slog.ErrorContext(ctx, "failed to list external identities (mongo)", "error", err, "user_id", userID) + return nil, err + } + defer func() { _ = cursor.Close(ctx) }() + + var identities []*domain.ExternalIdentityModel + for cursor.Next(ctx) { + var identity domain.ExternalIdentityModel + if decodeErr := cursor.Decode(&identity); decodeErr != nil { + _ = cursor.Close(ctx) + return nil, decodeErr + } + identities = append(identities, &identity) + } + + if err := cursor.Err(); err != nil { + return nil, err + } + return identities, nil +} diff --git a/internal/infrastructure/persistence/mongo_user_store.go b/internal/infrastructure/persistence/mongo_user_store.go index be90ef5..fd7a468 100644 --- a/internal/infrastructure/persistence/mongo_user_store.go +++ b/internal/infrastructure/persistence/mongo_user_store.go @@ -53,10 +53,6 @@ func (r *mongoUserStore) ensureIndexes(ctx context.Context) error { // This enables OAuth users without email to be created. Options: options.Index().SetUnique(true).SetSparse(true).SetName("idx_email_unique"), }, - { - Keys: bson.D{{Key: "github_id", Value: 1}}, - Options: options.Index().SetUnique(true).SetSparse(true).SetName("idx_github_id_unique"), - }, } if _, err := r.coll.Indexes().CreateMany(ctx, models); err != nil { @@ -88,10 +84,6 @@ func (r *mongoUserStore) GetByEmail(ctx context.Context, email string) (*domain. return r.findOne(ctx, bson.M{"email": email}, "email", email) } -func (r *mongoUserStore) GetByGithubID(ctx context.Context, githubID string) (*domain.UserModel, error) { - return r.findOne(ctx, bson.M{"github_id": githubID}, "github_id", githubID) -} - func (r *mongoUserStore) Update(ctx context.Context, user *domain.UserModel) error { if strings.TrimSpace(user.ID) == "" { return errors.New("user id is required for update") From 78ce8b7645905bbec9b9792caa4ae843058e10ff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:51:08 +0000 Subject: [PATCH 02/15] fix: compensate orphaned user if external identity creation fails Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/42a73a29-3769-4a5e-891a-7ab32d085f24 Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 52d463d..2d51452 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -848,6 +848,11 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U ProviderUserID: info.ID, } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + // Compensate: remove the newly created user to avoid orphaned records. + if deleteErr := s.userStore.Delete(ctx, userModel.ID); deleteErr != nil { + slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", + "error", deleteErr, "user_id", userModel.ID) + } return nil, status.Error(codes.Internal, "failed to create oauth identity") } return userModel, nil @@ -866,6 +871,11 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U ProviderUserID: info.ID, } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + // Compensate: remove the newly created user to avoid orphaned records. + if deleteErr := s.userStore.Delete(ctx, userModel.ID); deleteErr != nil { + slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", + "error", deleteErr, "user_id", userModel.ID) + } return nil, status.Error(codes.Internal, "failed to create oauth identity") } return userModel, nil From 1e5856dde7541e598b36df0ca9ddcee8b3d1141a Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 15:05:35 +0800 Subject: [PATCH 03/15] Update internal/domain/external_identity.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/domain/external_identity.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/domain/external_identity.go b/internal/domain/external_identity.go index d573415..cb29d6d 100644 --- a/internal/domain/external_identity.go +++ b/internal/domain/external_identity.go @@ -13,8 +13,8 @@ import ( type ExternalIdentityModel struct { ID string `gorm:"type:varchar(36);primaryKey" bson:"_id,omitempty" json:"id"` UserID string `gorm:"type:varchar(36);index" bson:"user_id" json:"user_id"` - Provider string `gorm:"type:varchar(50)" bson:"provider" json:"provider"` - ProviderUserID string `gorm:"type:varchar(255)" bson:"provider_user_id" json:"provider_user_id"` + Provider string `gorm:"type:varchar(50);uniqueIndex:idx_external_identities_provider_provider_user_id" bson:"provider" json:"provider"` + ProviderUserID string `gorm:"type:varchar(255);uniqueIndex:idx_external_identities_provider_provider_user_id" bson:"provider_user_id" json:"provider_user_id"` CreatedAt time.Time `bson:"created_at,omitempty" json:"created_at"` UpdatedAt time.Time `bson:"updated_at,omitempty" json:"updated_at"` DeletedAt gorm.DeletedAt `gorm:"index" bson:"-" json:"-"` From 504fbc8b5ad8e8891952165f6c6891982efd6694 Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 15:05:46 +0800 Subject: [PATCH 04/15] Update internal/application/identra/service.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/application/identra/service.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 2d51452..9a6b7e8 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -411,7 +411,6 @@ func (s *Service) BindUserByOAuth( slog.ErrorContext(ctx, "failed to fetch user info (bind)", "error", err) return nil, status.Error(codes.Unauthenticated, "failed to fetch user info") } - s.maybeFillOAuthEmail(ctx, userProvider, token.AccessToken, &userInfo) providerIdentity, err := s.externalIdentityStore.GetByProviderID(ctx, stateData.Provider, userInfo.ID) switch { From ffb7fca854529ea09283414a3bc01bc829d2367b Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 15:05:57 +0800 Subject: [PATCH 05/15] Update internal/application/identra/service.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/application/identra/service.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 9a6b7e8..0a51539 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -426,6 +426,9 @@ func (s *Service) BindUserByOAuth( ProviderUserID: userInfo.ID, } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + if errors.Is(createErr, domain.ErrAlreadyExists) { + return nil, status.Error(codes.AlreadyExists, "oauth account already linked") + } return nil, status.Error(codes.Internal, "failed to link oauth account") } default: From 493a97f551384eaf8d541bb5c78bbf96d8634118 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 07:12:43 +0000 Subject: [PATCH 06/15] fix: handle ErrAlreadyExists in ensureOAuthUser, add tests, drop stale mongo index Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/08bbc906-0d17-4698-83b9-bb727335733f Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 9 ++++ .../identra/service_ensure_oauth_user_test.go | 47 +++++++++++++++++++ .../persistence/mongo_user_store.go | 7 +++ 3 files changed, 63 insertions(+) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 0a51539..ae864cb 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -835,6 +835,9 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U ProviderUserID: info.ID, } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + if errors.Is(createErr, domain.ErrAlreadyExists) { + return nil, status.Error(codes.AlreadyExists, "oauth account already linked") + } return nil, status.Error(codes.Internal, "failed to link oauth account") } return byEmail, nil @@ -855,6 +858,9 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", "error", deleteErr, "user_id", userModel.ID) } + if errors.Is(createErr, domain.ErrAlreadyExists) { + return nil, status.Error(codes.AlreadyExists, "oauth account already linked") + } return nil, status.Error(codes.Internal, "failed to create oauth identity") } return userModel, nil @@ -878,6 +884,9 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", "error", deleteErr, "user_id", userModel.ID) } + if errors.Is(createErr, domain.ErrAlreadyExists) { + return nil, status.Error(codes.AlreadyExists, "oauth account already linked") + } return nil, status.Error(codes.Internal, "failed to create oauth identity") } return userModel, nil diff --git a/internal/application/identra/service_ensure_oauth_user_test.go b/internal/application/identra/service_ensure_oauth_user_test.go index dc18b8a..5faa757 100644 --- a/internal/application/identra/service_ensure_oauth_user_test.go +++ b/internal/application/identra/service_ensure_oauth_user_test.go @@ -252,3 +252,50 @@ func TestEnsureOAuthUser_LinkExistingUserByEmail(t *testing.T) { } } +func TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_NoEmail(t *testing.T) { + store := newMockUserStore() + extStore := newMockExternalIdentityStore() + extStore.forceCreateErr = domain.ErrAlreadyExists + svc := &Service{userStore: store, externalIdentityStore: extStore} + + info := UserInfo{ + Provider: "github", + ID: "github-fail", + Email: "", + } + + _, err := svc.ensureOAuthUser(context.Background(), info) + if err == nil { + t.Fatal("expected error, got nil") + } + + // Verify the orphaned user was cleaned up. + count, _ := store.Count(context.Background()) + if count != 0 { + t.Errorf("expected no users after identity create failure, got %d", count) + } +} + +func TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_WithEmail(t *testing.T) { + store := newMockUserStore() + extStore := newMockExternalIdentityStore() + extStore.forceCreateErr = domain.ErrAlreadyExists + svc := &Service{userStore: store, externalIdentityStore: extStore} + + info := UserInfo{ + Provider: "github", + ID: "github-fail", + Email: "new@example.com", + } + + _, err := svc.ensureOAuthUser(context.Background(), info) + if err == nil { + t.Fatal("expected error, got nil") + } + + // Verify the orphaned user was cleaned up. + count, _ := store.Count(context.Background()) + if count != 0 { + t.Errorf("expected no users after identity create failure, got %d", count) + } +} diff --git a/internal/infrastructure/persistence/mongo_user_store.go b/internal/infrastructure/persistence/mongo_user_store.go index fd7a468..80790be 100644 --- a/internal/infrastructure/persistence/mongo_user_store.go +++ b/internal/infrastructure/persistence/mongo_user_store.go @@ -45,6 +45,13 @@ func NewMongoUserStore( } func (r *mongoUserStore) ensureIndexes(ctx context.Context) error { + // Drop the stale github_id index left over from the old schema, if present. + // This is a best-effort migration step; the error is intentionally ignored so + // that both fresh deployments (index never existed) and upgraded ones work. + if err := r.coll.Indexes().DropOne(ctx, "idx_github_id_unique"); err != nil { + slog.DebugContext(ctx, "could not drop stale github_id index (may not exist)", "error", err) + } + models := []mongo.IndexModel{ { Keys: bson.D{{Key: "email", Value: 1}}, From f2f68aa4240f27c0d5ebd58e7a795244419996fc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 07:15:39 +0000 Subject: [PATCH 07/15] fix: clarify error type check ordering in compensating delete paths Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/08bbc906-0d17-4698-83b9-bb727335733f Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index ae864cb..1658a44 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -853,12 +853,15 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U ProviderUserID: info.ID, } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + // Determine the response code before attempting cleanup so that the + // cleanup outcome does not affect the error returned to the caller. + isConflict := errors.Is(createErr, domain.ErrAlreadyExists) // Compensate: remove the newly created user to avoid orphaned records. if deleteErr := s.userStore.Delete(ctx, userModel.ID); deleteErr != nil { slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", "error", deleteErr, "user_id", userModel.ID) } - if errors.Is(createErr, domain.ErrAlreadyExists) { + if isConflict { return nil, status.Error(codes.AlreadyExists, "oauth account already linked") } return nil, status.Error(codes.Internal, "failed to create oauth identity") @@ -879,12 +882,15 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U ProviderUserID: info.ID, } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { + // Determine the response code before attempting cleanup so that the + // cleanup outcome does not affect the error returned to the caller. + isConflict := errors.Is(createErr, domain.ErrAlreadyExists) // Compensate: remove the newly created user to avoid orphaned records. if deleteErr := s.userStore.Delete(ctx, userModel.ID); deleteErr != nil { slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", "error", deleteErr, "user_id", userModel.ID) } - if errors.Is(createErr, domain.ErrAlreadyExists) { + if isConflict { return nil, status.Error(codes.AlreadyExists, "oauth account already linked") } return nil, status.Error(codes.Internal, "failed to create oauth identity") From 78a4bdc120bb40cc4abd09d3ea593347e4d5a93f Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 16:02:04 +0800 Subject: [PATCH 08/15] Update internal/infrastructure/persistence/mongo_external_identity_store.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../persistence/mongo_external_identity_store.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/infrastructure/persistence/mongo_external_identity_store.go b/internal/infrastructure/persistence/mongo_external_identity_store.go index e2b8d93..fbb28d5 100644 --- a/internal/infrastructure/persistence/mongo_external_identity_store.go +++ b/internal/infrastructure/persistence/mongo_external_identity_store.go @@ -53,6 +53,13 @@ func (r *mongoExternalIdentityStore) ensureIndexes(ctx context.Context) error { }, Options: options.Index().SetUnique(true).SetName("idx_provider_user_id_unique"), }, + { + Keys: bson.D{ + {Key: "provider", Value: 1}, + {Key: "user_id", Value: 1}, + }, + Options: options.Index().SetUnique(true).SetName("idx_provider_user_id_per_user_unique"), + }, { Keys: bson.D{{Key: "user_id", Value: 1}}, Options: options.Index().SetName("idx_user_id"), From f542d330387e680e5bd8318cfe2a10f9a6aa9f88 Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 16:02:23 +0800 Subject: [PATCH 09/15] Update internal/infrastructure/persistence/mongo_user_store.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../persistence/mongo_user_store.go | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/infrastructure/persistence/mongo_user_store.go b/internal/infrastructure/persistence/mongo_user_store.go index 80790be..6dab9db 100644 --- a/internal/infrastructure/persistence/mongo_user_store.go +++ b/internal/infrastructure/persistence/mongo_user_store.go @@ -44,12 +44,26 @@ func NewMongoUserStore( return repo, nil } +func isIndexNotFoundError(err error) bool { + var cmdErr mongo.CommandError + if errors.As(err, &cmdErr) && cmdErr.Code == 27 { + return true + } + + return strings.Contains(strings.ToLower(err.Error()), "index not found") +} + func (r *mongoUserStore) ensureIndexes(ctx context.Context) error { // Drop the stale github_id index left over from the old schema, if present. - // This is a best-effort migration step; the error is intentionally ignored so - // that both fresh deployments (index never existed) and upgraded ones work. + // Ignore only the expected "index not found" case so fresh deployments and + // upgraded ones both work, but fail fast for operational problems. if err := r.coll.Indexes().DropOne(ctx, "idx_github_id_unique"); err != nil { - slog.DebugContext(ctx, "could not drop stale github_id index (may not exist)", "error", err) + if isIndexNotFoundError(err) { + slog.DebugContext(ctx, "stale github_id index not present", "error", err) + } else { + slog.WarnContext(ctx, "failed to drop stale github_id index", "error", err) + return err + } } models := []mongo.IndexModel{ From 84a425d286aca72acdb60ce3ed8c26279bf0694c Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 16:02:42 +0800 Subject: [PATCH 10/15] Update internal/application/identra/service.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/application/identra/service.go | 30 +++++-------------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 1658a44..cd6bbe3 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -871,31 +871,11 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U return nil, status.Error(codes.Internal, "failed to fetch user by email") } } - // No email provided; create user with external identity only. - userModel := &domain.UserModel{} - if createErr := s.userStore.Create(ctx, userModel); createErr != nil { - return nil, status.Error(codes.Internal, "failed to create user") - } - identity := &domain.ExternalIdentityModel{ - UserID: userModel.ID, - Provider: info.Provider, - ProviderUserID: info.ID, - } - if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { - // Determine the response code before attempting cleanup so that the - // cleanup outcome does not affect the error returned to the caller. - isConflict := errors.Is(createErr, domain.ErrAlreadyExists) - // Compensate: remove the newly created user to avoid orphaned records. - if deleteErr := s.userStore.Delete(ctx, userModel.ID); deleteErr != nil { - slog.ErrorContext(ctx, "failed to clean up orphaned user after identity create failure", - "error", deleteErr, "user_id", userModel.ID) - } - if isConflict { - return nil, status.Error(codes.AlreadyExists, "oauth account already linked") - } - return nil, status.Error(codes.Internal, "failed to create oauth identity") - } - return userModel, nil + // The current persistence layer enforces unique email values, so creating + // a user without an email address can lead to duplicate empty-email + // records and subsequent signup failures. Reject this flow until missing + // emails are represented distinctly at the model/index level. + return nil, status.Error(codes.FailedPrecondition, "oauth provider did not supply an email address") default: return nil, status.Error(codes.Internal, "failed to fetch user by provider id") } From 4571686d15cb28f6ebf3c89c28bdadb442c3fb51 Mon Sep 17 00:00:00 2001 From: slhmy Date: Thu, 16 Apr 2026 16:02:56 +0800 Subject: [PATCH 11/15] Update internal/application/identra/service.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/application/identra/service.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index cd6bbe3..ffade32 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -791,15 +791,19 @@ func (s *Service) GetCurrentUserLoginInfo( if err != nil { slog.WarnContext(ctx, "failed to fetch external identities", "error", err, "user_id", usr.ID) } else { + var githubID string for _, identity := range identities { resp.OauthConnections = append(resp.OauthConnections, &identra_v1_pb.OAuthConnection{ Provider: identity.Provider, ProviderUserId: identity.ProviderUserID, }) - if identity.Provider == "github" { - resp.GithubId = &identity.ProviderUserID + if identity.Provider == "github" && (githubID == "" || identity.ProviderUserID < githubID) { + githubID = identity.ProviderUserID } } + if githubID != "" { + resp.GithubId = &githubID + } } return resp, nil From dca061802f210d356daa8860bb07bb81ec744f42 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 09:30:42 +0000 Subject: [PATCH 12/15] fix: validate userInfo.ID in BindUserByOAuth; update tests for no-email rejection Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/9730e222-ed3f-49c8-b632-3686f678b549 Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 4 ++ .../identra/service_ensure_oauth_user_test.go | 47 +++++-------------- 2 files changed, 15 insertions(+), 36 deletions(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index ffade32..78d2619 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -411,6 +411,10 @@ func (s *Service) BindUserByOAuth( slog.ErrorContext(ctx, "failed to fetch user info (bind)", "error", err) return nil, status.Error(codes.Unauthenticated, "failed to fetch user info") } + if strings.TrimSpace(userInfo.ID) == "" { + slog.ErrorContext(ctx, "provider returned empty user id (bind)", "provider", stateData.Provider) + return nil, status.Error(codes.Internal, "provider returned empty user id") + } providerIdentity, err := s.externalIdentityStore.GetByProviderID(ctx, stateData.Provider, userInfo.ID) switch { diff --git a/internal/application/identra/service_ensure_oauth_user_test.go b/internal/application/identra/service_ensure_oauth_user_test.go index 5faa757..dbaafde 100644 --- a/internal/application/identra/service_ensure_oauth_user_test.go +++ b/internal/application/identra/service_ensure_oauth_user_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/poly-workshop/identra/internal/domain" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // mockUserStore is a simple in-memory user store for testing. @@ -161,21 +163,18 @@ func TestEnsureOAuthUser_WithoutEmail(t *testing.T) { Email: "", } - user, err := svc.ensureOAuthUser(context.Background(), info) - if err != nil { - t.Fatalf("expected no error, got %v", err) + _, err := svc.ensureOAuthUser(context.Background(), info) + if err == nil { + t.Fatal("expected error for missing email, got nil") } - - if user.Email != "" { - t.Errorf("expected empty email, got %q", user.Email) + if st, ok := status.FromError(err); !ok || st.Code() != codes.FailedPrecondition { + t.Errorf("expected FailedPrecondition, got %v", err) } - id, lookupErr := extStore.GetByProviderID(context.Background(), "github", "github456") - if lookupErr != nil { - t.Fatalf("expected external identity to exist, got %v", lookupErr) - } - if id.UserID != user.ID { - t.Errorf("expected external identity user_id %q, got %q", user.ID, id.UserID) + // No user should have been created. + count, _ := store.Count(context.Background()) + if count != 0 { + t.Errorf("expected no users after no-email rejection, got %d", count) } } @@ -252,30 +251,6 @@ func TestEnsureOAuthUser_LinkExistingUserByEmail(t *testing.T) { } } -func TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_NoEmail(t *testing.T) { - store := newMockUserStore() - extStore := newMockExternalIdentityStore() - extStore.forceCreateErr = domain.ErrAlreadyExists - svc := &Service{userStore: store, externalIdentityStore: extStore} - - info := UserInfo{ - Provider: "github", - ID: "github-fail", - Email: "", - } - - _, err := svc.ensureOAuthUser(context.Background(), info) - if err == nil { - t.Fatal("expected error, got nil") - } - - // Verify the orphaned user was cleaned up. - count, _ := store.Count(context.Background()) - if count != 0 { - t.Errorf("expected no users after identity create failure, got %d", count) - } -} - func TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_WithEmail(t *testing.T) { store := newMockUserStore() extStore := newMockExternalIdentityStore() From 5c7be9bf2e4f7400405d7e0f7e355410418b333a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 09:32:24 +0000 Subject: [PATCH 13/15] fix: use direct empty string check for userInfo.ID in BindUserByOAuth Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/9730e222-ed3f-49c8-b632-3686f678b549 Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 78d2619..3e6c07a 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -411,7 +411,7 @@ func (s *Service) BindUserByOAuth( slog.ErrorContext(ctx, "failed to fetch user info (bind)", "error", err) return nil, status.Error(codes.Unauthenticated, "failed to fetch user info") } - if strings.TrimSpace(userInfo.ID) == "" { + if userInfo.ID == "" { slog.ErrorContext(ctx, "provider returned empty user id (bind)", "provider", stateData.Provider) return nil, status.Error(codes.Internal, "provider returned empty user id") } From 72ce88cc8967b7723314b03e487ab03b5d542d1e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 09:53:11 +0000 Subject: [PATCH 14/15] fix: apply review comments from pullrequestreview-4119862184 Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/2fe3c333-609f-4187-9841-46ebe7516ff7 Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 26 +++++++++++++++++-- .../identra/service_ensure_oauth_user_test.go | 8 ++++++ .../persistence/mongo_user_store.go | 12 +++------ 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index 3e6c07a..aeb45a2 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -431,9 +431,19 @@ func (s *Service) BindUserByOAuth( } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { if errors.Is(createErr, domain.ErrAlreadyExists) { - return nil, status.Error(codes.AlreadyExists, "oauth account already linked") + // A concurrent request may have created the same identity. Re-fetch + // and treat as success if it is linked to this user. + existing, refetchErr := s.externalIdentityStore.GetByProviderID(ctx, stateData.Provider, userInfo.ID) + if refetchErr != nil { + return nil, status.Error(codes.Internal, "failed to verify oauth link") + } + if existing.UserID != bindingUser.ID { + return nil, status.Error(codes.AlreadyExists, "oauth account already linked to another user") + } + // Idempotent: identity exists and belongs to this user. + } else { + return nil, status.Error(codes.Internal, "failed to link oauth account") } - return nil, status.Error(codes.Internal, "failed to link oauth account") } default: return nil, status.Error(codes.Internal, "failed to check existing oauth link") @@ -844,6 +854,15 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U } if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil { if errors.Is(createErr, domain.ErrAlreadyExists) { + // A concurrent request may have created the same identity for + // this user. Re-fetch and proceed if it belongs to the same user. + existingIdentity, getErr := s.externalIdentityStore.GetByProviderID(ctx, info.Provider, info.ID) + if getErr != nil { + return nil, status.Error(codes.Internal, "failed to verify oauth account link") + } + if existingIdentity.UserID == byEmail.ID { + return byEmail, nil + } return nil, status.Error(codes.AlreadyExists, "oauth account already linked") } return nil, status.Error(codes.Internal, "failed to link oauth account") @@ -853,6 +872,9 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U // Create new user and link external identity. userModel := &domain.UserModel{Email: info.Email} if createErr := s.userStore.Create(ctx, userModel); createErr != nil { + if errors.Is(createErr, domain.ErrAlreadyExists) { + return nil, status.Error(codes.AlreadyExists, "user already exists") + } return nil, status.Error(codes.Internal, "failed to create user") } identity := &domain.ExternalIdentityModel{ diff --git a/internal/application/identra/service_ensure_oauth_user_test.go b/internal/application/identra/service_ensure_oauth_user_test.go index dbaafde..ebd8cf3 100644 --- a/internal/application/identra/service_ensure_oauth_user_test.go +++ b/internal/application/identra/service_ensure_oauth_user_test.go @@ -268,6 +268,14 @@ func TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_WithEmail(t * t.Fatal("expected error, got nil") } + st, ok := status.FromError(err) + if !ok { + t.Fatalf("expected gRPC status error, got %v", err) + } + if st.Code() != codes.AlreadyExists { + t.Fatalf("expected gRPC code %v, got %v", codes.AlreadyExists, st.Code()) + } + // Verify the orphaned user was cleaned up. count, _ := store.Count(context.Background()) if count != 0 { diff --git a/internal/infrastructure/persistence/mongo_user_store.go b/internal/infrastructure/persistence/mongo_user_store.go index 6dab9db..c6770b0 100644 --- a/internal/infrastructure/persistence/mongo_user_store.go +++ b/internal/infrastructure/persistence/mongo_user_store.go @@ -46,11 +46,7 @@ func NewMongoUserStore( func isIndexNotFoundError(err error) bool { var cmdErr mongo.CommandError - if errors.As(err, &cmdErr) && cmdErr.Code == 27 { - return true - } - - return strings.Contains(strings.ToLower(err.Error()), "index not found") + return errors.As(err, &cmdErr) && cmdErr.Code == 27 } func (r *mongoUserStore) ensureIndexes(ctx context.Context) error { @@ -69,9 +65,9 @@ func (r *mongoUserStore) ensureIndexes(ctx context.Context) error { models := []mongo.IndexModel{ { Keys: bson.D{{Key: "email", Value: 1}}, - // Sparse index allows multiple documents with NULL/empty email values - // while maintaining uniqueness constraint for non-empty values. - // This enables OAuth users without email to be created. + // Sparse index excludes documents where the email field is absent. + // An empty string is still indexed, so service-layer validation must + // reject blank emails before writes reach this store. Options: options.Index().SetUnique(true).SetSparse(true).SetName("idx_email_unique"), }, } From ebab67f444317353c0298d43eccc070632c6e135 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Apr 2026 09:55:31 +0000 Subject: [PATCH 15/15] fix: clarify TOCTOU error message to 'linked to another user' Agent-Logs-Url: https://github.com/poly-workshop/identra/sessions/2fe3c333-609f-4187-9841-46ebe7516ff7 Co-authored-by: slhmy <31381093+slhmy@users.noreply.github.com> --- internal/application/identra/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/application/identra/service.go b/internal/application/identra/service.go index aeb45a2..8474a5c 100644 --- a/internal/application/identra/service.go +++ b/internal/application/identra/service.go @@ -863,7 +863,7 @@ func (s *Service) ensureOAuthUser(ctx context.Context, info UserInfo) (*domain.U if existingIdentity.UserID == byEmail.ID { return byEmail, nil } - return nil, status.Error(codes.AlreadyExists, "oauth account already linked") + return nil, status.Error(codes.AlreadyExists, "oauth account already linked to another user") } return nil, status.Error(codes.Internal, "failed to link oauth account") }