diff --git a/designs/TASKS.md b/designs/TASKS.md index 610162e..0932634 100644 --- a/designs/TASKS.md +++ b/designs/TASKS.md @@ -25,6 +25,14 @@ ## Implementation Tasks +- [x] 2026-04-24: Fixed social login duplicate principal creation when email slug already exists (TDD-first): + - Added integration regression coverage in `internal/social/identity_test.go`: + - `TestIdentityStore_FindOrCreate_ExistingEmailSlug_LinksPrincipal` + - Updated `internal/social/identity.go` `FindOrCreateWithPolicy` flow to: + - link social identities to an existing principal when `info.Email` matches an existing principal slug, + - only create a new principal when no social identity and no matching email principal exist. + - Updated slug-collision coverage in `internal/social/identity_test.go` to validate fallback collision handling for non-email slugs. + - [x] 2026-04-14: Fixed failing unit/integration tests after constructor signature changes (TDD green pass): - Updated `internal/codegraph/syncer_test.go` and `internal/api/rest/scopes_test.go` to use the current diff --git a/internal/social/identity.go b/internal/social/identity.go index baa881e..f0da25b 100644 --- a/internal/social/identity.go +++ b/internal/social/identity.go @@ -79,17 +79,7 @@ func (s *IdentityStore) FindOrCreateWithPolicy(ctx context.Context, provider str displayName = baseSlug } - if !policy.AutoCreateUsers { - if info.Email == "" { - return nil, ErrPrincipalNotProvisioned - } - principal, lookupErr := q.GetPrincipalBySlug(ctx, info.Email) - if lookupErr != nil { - if errors.Is(lookupErr, pgx.ErrNoRows) { - return nil, ErrPrincipalNotProvisioned - } - return nil, fmt.Errorf("lookup principal by email slug: %w", lookupErr) - } + linkPrincipal := func(principal *db.Principal) (*db.Principal, error) { if _, err := q.UpsertSocialIdentity(ctx, db.UpsertSocialIdentityParams{ PrincipalID: principal.ID, Provider: provider, @@ -107,6 +97,24 @@ func (s *IdentityStore) FindOrCreateWithPolicy(ctx context.Context, provider str return principal, nil } + emailSlug := strings.TrimSpace(info.Email) + if emailSlug != "" { + principal, lookupErr := q.GetPrincipalBySlug(ctx, emailSlug) + if lookupErr == nil { + if principal.Kind != "user" { + return nil, fmt.Errorf("email slug %q matched non-user principal kind %q", emailSlug, principal.Kind) + } + return linkPrincipal(principal) + } + if !errors.Is(lookupErr, pgx.ErrNoRows) { + return nil, fmt.Errorf("lookup principal by email slug: %w", lookupErr) + } + } + + if !policy.AutoCreateUsers { + return nil, ErrPrincipalNotProvisioned + } + slug := baseSlug if _, slugErr := q.GetPrincipalBySlug(ctx, baseSlug); slugErr == nil { slug = baseSlug + "-" + info.ProviderID @@ -124,22 +132,7 @@ func (s *IdentityStore) FindOrCreateWithPolicy(ctx context.Context, provider str return nil, err } - if _, err := q.UpsertSocialIdentity(ctx, db.UpsertSocialIdentityParams{ - PrincipalID: principal.ID, - Provider: provider, - ProviderID: info.ProviderID, - Email: optionalString(info.Email), - DisplayName: optionalString(info.DisplayName), - AvatarUrl: optionalString(info.AvatarURL), - RawProfile: info.RawProfile, - }); err != nil { - return nil, err - } - - if err := tx.Commit(ctx); err != nil { - return nil, err - } - return principal, nil + return linkPrincipal(principal) } func principalSlug(provider string, info *UserInfo) string { diff --git a/internal/social/identity_test.go b/internal/social/identity_test.go index f6f0e10..6db0557 100644 --- a/internal/social/identity_test.go +++ b/internal/social/identity_test.go @@ -100,7 +100,7 @@ func TestIdentityStore_FindOrCreate_SlugCollision_AppendsProviderID(t *testing.T q := db.New(pool) _, err := q.CreatePrincipal(ctx, db.CreatePrincipalParams{ Kind: "user", - Slug: "collision@example.com", + Slug: "github-gh-collision-1", DisplayName: "Existing User", Meta: []byte(`{}`), }) @@ -110,7 +110,6 @@ func TestIdentityStore_FindOrCreate_SlugCollision_AppendsProviderID(t *testing.T principal, err := store.FindOrCreate(ctx, "github", &UserInfo{ ProviderID: "gh-collision-1", - Email: "collision@example.com", DisplayName: "Colliding User", AvatarURL: "https://cdn.example/collision.png", RawProfile: []byte(`{"id":"gh-collision-1"}`), @@ -118,8 +117,8 @@ func TestIdentityStore_FindOrCreate_SlugCollision_AppendsProviderID(t *testing.T if err != nil { t.Fatalf("FindOrCreate collision: %v", err) } - if principal.Slug != "collision@example.com-gh-collision-1" { - t.Fatalf("principal slug = %q, want collision@example.com-gh-collision-1", principal.Slug) + if principal.Slug != "github-gh-collision-1-gh-collision-1" { + t.Fatalf("principal slug = %q, want github-gh-collision-1-gh-collision-1", principal.Slug) } var linkedPrincipalID string @@ -135,6 +134,60 @@ func TestIdentityStore_FindOrCreate_SlugCollision_AppendsProviderID(t *testing.T } } +func TestIdentityStore_FindOrCreate_ExistingEmailSlug_LinksPrincipal(t *testing.T) { + pool := testhelper.NewTestPool(t) + store := NewIdentityStore(pool) + ctx := context.Background() + + q := db.New(pool) + existing, err := q.CreatePrincipal(ctx, db.CreatePrincipalParams{ + Kind: "user", + Slug: "manohar@simplyblock.io", + DisplayName: "Manohar Reddy", + Meta: []byte(`{}`), + }) + if err != nil { + t.Fatalf("seed existing principal: %v", err) + } + + principal, err := store.FindOrCreate(ctx, "google", &UserInfo{ + ProviderID: "112743087521354650483", + Email: "manohar@simplyblock.io", + EmailVerified: true, + DisplayName: "Manohar Reddy", + RawProfile: []byte(`{"sub":"112743087521354650483"}`), + }) + if err != nil { + t.Fatalf("FindOrCreate: %v", err) + } + if principal.ID != existing.ID { + t.Fatalf("principal ID = %s, want %s", principal.ID, existing.ID) + } + if principal.Slug != existing.Slug { + t.Fatalf("principal slug = %q, want %q", principal.Slug, existing.Slug) + } + + var principalCount int + if err := pool.QueryRow(ctx, `SELECT COUNT(*) FROM principals`).Scan(&principalCount); err != nil { + t.Fatalf("count principals: %v", err) + } + if principalCount != 1 { + t.Fatalf("principal count = %d, want 1", principalCount) + } + + var linkedPrincipalID string + err = pool.QueryRow(ctx, ` + SELECT principal_id::text FROM social_identities + WHERE provider = 'google' AND provider_id = '112743087521354650483' + `).Scan(&linkedPrincipalID) + if err != nil { + t.Fatalf("query social identity: %v", err) + } + if linkedPrincipalID != existing.ID.String() { + t.Fatalf("linked principal_id = %s, want %s", linkedPrincipalID, existing.ID) + } +} + func TestIdentityStore_FindOrCreateWithPolicy_AutoCreateDisabled_PreprovisionedEmailLinksPrincipal(t *testing.T) { pool := testhelper.NewTestPool(t) store := NewIdentityStore(pool)