refactor: extract OAuth provider identities into dedicated external_identities table by Copilot · Pull Request #25 · poly-workshop/identra

Copilot · 2026-04-15T14:08:14Z

…entities 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>

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>

Copilot

Pull request overview

Refactors OAuth identity persistence to a provider-agnostic (provider, provider_user_id) → user_id binding model by introducing a dedicated external_identities store/table/collection and updating OAuth login/bind flows to use it instead of per-provider columns on users.

Changes:

Introduces ExternalIdentityModel + ExternalIdentityStore, with MongoDB and GORM persistence implementations.
Removes GithubID and GetByGithubID from the user model/store and migrates service logic to resolve OAuth users via external identities.
Updates OAuth-related service tests to validate external identity linkage.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/infrastructure/persistence/mongo_user_store.go	Removes GitHub ID index and GitHub-ID lookup from the Mongo user store.
internal/infrastructure/persistence/mongo_external_identity_store.go	Adds Mongo external identity store with compound unique index.
internal/infrastructure/persistence/gorm_user_store.go	Removes GitHub-ID lookup from the GORM user store.
internal/infrastructure/persistence/gorm_external_identity_store.go	Adds GORM external identity store implementation.
internal/domain/user.go	Removes `GithubID` from `UserModel` and `GetByGithubID` from `UserStore`.
internal/domain/external_identity.go	Adds the new external identity domain model and store interface.
internal/application/identra/user_info_provider.go	Adds `Provider` to `UserInfo` and populates it for GitHub.
internal/application/identra/service_ensure_oauth_user_test.go	Updates tests to assert external identity creation/linking.
internal/application/identra/service.go	Updates OAuth bind/login flows to use `externalIdentityStore`; adds compensating delete.

Copilot · 2026-04-15T14:12:47Z

+				if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil {
+					return nil, status.Error(codes.Internal, "failed to link oauth account")
 				}


ensureOAuthUser maps externalIdentityStore.Create errors to codes.Internal even when the cause is domain.ErrAlreadyExists (identity already linked, possibly due to races or email-merge conflicts). Handle ErrAlreadyExists explicitly so the API returns a conflict-like code (AlreadyExists/FailedPrecondition) instead of Internal.

Copilot · 2026-04-15T14:12:47Z

+				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")
+				}


New compensating-delete behavior (deleting the newly created user when external identity creation fails) is important for correctness but currently has no test coverage. Add a test that forces externalIdentityStore.Create to fail and asserts the user is deleted (or that deletion is attempted) so regressions are caught.

Copilot · 2026-04-15T14:12:47Z

-		},
 	}

 	if _, err := r.coll.Indexes().CreateMany(ctx, models); err != nil {


Now that the github_id index model has been removed, note that CreateMany will not drop an already-existing idx_github_id_unique index in MongoDB. If the goal is to actually remove that index (per the PR description), add an explicit Indexes().DropOne(...) (or document a manual migration) so existing deployments don’t keep the stale index indefinitely.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

slhmy · 2026-04-16T07:06:16Z

@copilot apply changes based on the comments in this thread

…e 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>

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>

Copilot · 2026-04-16T07:15:55Z

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread (commits 493a97f–f2f68aa):

ErrAlreadyExists handling in ensureOAuthUser — all three externalIdentityStore.Create call sites now return codes.AlreadyExists instead of codes.Internal on duplicate identity errors. The error type is captured before the compensating delete runs so cleanup outcome doesn't affect the response code (addressed reviewer clarity concern).
Compensating-delete tests — added TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_NoEmail and TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_WithEmail, both force identity creation to fail and verify no orphaned users remain.
Stale MongoDB idx_github_id_unique index — mongoUserStore.ensureIndexes now attempts to drop the old index at startup (best-effort; "not found" errors are logged at debug level and ignored so fresh deployments are unaffected).

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Copilot · 2026-04-16T07:31:35Z

+	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:


BindUserByOAuth uses userInfo.ID as ProviderUserID without validating it. If a provider implementation ever returns an empty ID, this can create invalid external identity records and trigger uniqueness conflicts. Add a non-empty check for userInfo.ID before calling GetByProviderID/Create (similar to ensureOAuthUser).

…ore.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

slhmy · 2026-04-16T09:23:32Z

@copilot apply changes based on the comments in this thread

…il 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>

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>

Copilot · 2026-04-16T09:32:38Z

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread (commits dca0618–5c7be9b):

BindUserByOAuth empty-ID validation — added a userInfo.ID == "" guard (matching the pattern in ensureOAuthUser) that returns codes.Internal before any store operations are attempted.
TestEnsureOAuthUser_WithoutEmail updated — the test now asserts codes.FailedPrecondition is returned (no-email logins are rejected at the service layer) and verifies no user was created.
Removed TestEnsureOAuthUser_IdentityCreateFailure_OrphanedUserDeleted_NoEmail — the no-email path now returns early before any user creation, so the compensating-delete logic is never reached for that path; the test was covering the wrong behavior. The with-email variant still provides coverage for the actual compensating-delete path.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Copilot · 2026-04-16T09:41:27Z

@@ -53,10 +74,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"),


The comment above the MongoDB email index claims sparse+unique allows multiple NULL/empty emails and enables OAuth users without email. Sparse indexes only exclude documents where the field is missing; an empty string is still indexed, and UserModel’s bson tag always stores the field, so this comment is misleading (and conflicts with the new service behavior that rejects missing emails). Update the comment (and/or the model/index strategy) to reflect the actual semantics.

Copilot · 2026-04-16T09:41:27Z

+	if errors.As(err, &cmdErr) && cmdErr.Code == 27 {
+		return true
+	}
+
+	return strings.Contains(strings.ToLower(err.Error()), "index not found")


isIndexNotFoundError falls back to substring-matching the error text ("index not found"). That can accidentally treat unrelated operational failures as ignorable if they happen to include that phrase, masking real startup/migration issues. Prefer relying on MongoDB error codes (e.g., CommandError code 27 / IndexNotFound) and/or explicit structured error types instead of string matching.

Suggested change

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

Copilot · 2026-04-16T09:41:28Z

+				return nil, status.Error(codes.AlreadyExists, "oauth account already linked")
+			}
+			return nil, status.Error(codes.Internal, "failed to link oauth account")


BindUserByOAuth has a TOCTOU race: if GetByProviderID returns not found but another concurrent bind creates the identity before Create runs, Create will return ErrAlreadyExists and this code returns codes.AlreadyExists. For an identity that is (now) linked to the same bindingUser, the bind flow should be idempotent success. Consider re-fetching the identity on ErrAlreadyExists and returning success if it is linked to bindingUser.ID, otherwise return AlreadyExists.

Suggested change

return nil, status.Error(codes.AlreadyExists, "oauth account already linked")

}

return nil, status.Error(codes.Internal, "failed to link oauth account")

providerIdentity, refetchErr := s.externalIdentityStore.GetByProviderID(ctx, stateData.Provider, userInfo.ID)

if refetchErr != nil {

return nil, status.Error(codes.Internal, "failed to verify oauth link")

}

if providerIdentity.UserID != bindingUser.ID {

return nil, status.Error(codes.AlreadyExists, "oauth account already linked to another user")

}

// Another concurrent bind linked this OAuth account to the same user.

// Treat the operation as idempotent success.

} else {

return nil, status.Error(codes.Internal, "failed to link oauth account")

}

Copilot · 2026-04-16T09:41:28Z

+					ProviderUserID: info.ID,
+				}
+				if createErr := s.externalIdentityStore.Create(ctx, identity); createErr != nil {
+					if errors.Is(createErr, domain.ErrAlreadyExists) {


ensureOAuthUser has the same TOCTOU issue when linking an existing user by email: if Create returns ErrAlreadyExists due to a concurrent request that just created the same external identity for this user, the login currently fails with codes.AlreadyExists. For OAuth login this should usually be treated as success by re-fetching the identity and proceeding when it points at the same user.

Suggested change

if errors.Is(createErr, domain.ErrAlreadyExists) {

if errors.Is(createErr, domain.ErrAlreadyExists) {

// A concurrent request may have created the same external identity

// after our initial lookup. Re-fetch and treat it as success when

// it is linked to the same user we intended to link.

existingIdentity, getErr := s.externalIdentityStore.GetByProviderAndProviderUserID(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

}

Copilot · 2026-04-16T09:41:28Z

-				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 {


When creating a new user in ensureOAuthUser (email not found case), userStore.Create errors are always mapped to codes.Internal. If the create fails due to a duplicate email (ErrAlreadyExists), the more accurate gRPC status is codes.AlreadyExists, consistent with other flows (e.g., RegisterByPassword). Handling ErrAlreadyExists here also helps with concurrent OAuth logins that race on user creation.

Suggested change

if createErr := s.userStore.Create(ctx, userModel); createErr != nil {

if createErr := s.userStore.Create(ctx, userModel); createErr != nil {

if errors.Is(createErr, domain.ErrAlreadyExists) {

return nil, status.Error(codes.AlreadyExists, "user already exists")

}

Copilot · 2026-04-16T09:41:29Z

+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+


This test exercises the orphan-cleanup path when externalIdentityStore.Create fails, but it doesn’t assert the returned gRPC status code (the implementation intends to return codes.AlreadyExists for ErrAlreadyExists). Adding an assertion on status.FromError(err).Code() would prevent regressions where cleanup logic accidentally changes the observed error code.

Suggested change

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())

}

slhmy · 2026-04-16T09:44:28Z

@copilot apply changes based on the comments in this thread

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>

Copilot · 2026-04-16T09:55:59Z

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread (commits 72ce88c–ebab67f):

isIndexNotFoundError — removed the string-match fallback; now relies solely on MongoDB CommandError code 27.
Misleading sparse-index comment — updated to accurately reflect that sparse indexes only exclude missing fields (empty strings are still indexed), so service-layer validation is what prevents blank emails.
BindUserByOAuth TOCTOU — on ErrAlreadyExists from Create, re-fetches the identity: returns idempotent success if it's linked to bindingUser, or codes.AlreadyExists ("already linked to another user") otherwise.
ensureOAuthUser email-merge TOCTOU — same pattern: re-fetches on ErrAlreadyExists and returns success if the existing identity points to byEmail.ID, otherwise codes.AlreadyExists ("already linked to another user").
userStore.Create duplicate email — mapped ErrAlreadyExists to codes.AlreadyExists ("user already exists").
Compensating-delete test — added status.FromError assertion verifying codes.AlreadyExists is returned when identity creation fails.

Copilot AI and others added 2 commits April 15, 2026 08:48

refactor: extract OAuth provider identities into separate external_id…

bec8276

…entities 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>

fix: compensate orphaned user if external identity creation fails

78ce8b7

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>

Copilot AI assigned Copilot and slhmy Apr 15, 2026

Copilot created this pull request from a session on behalf of slhmy April 15, 2026 14:08 View session

slhmy marked this pull request as ready for review April 15, 2026 14:08

slhmy requested a review from Copilot April 15, 2026 14:08

Copilot started reviewing on behalf of slhmy April 15, 2026 14:09 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

slhmy and others added 3 commits April 16, 2026 15:05

Update internal/domain/external_identity.go

1e5856d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/application/identra/service.go

504fbc8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/application/identra/service.go

ffb7fca

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot started work on behalf of slhmy April 16, 2026 07:06 View session

Copilot AI and others added 2 commits April 16, 2026 07:12

fix: handle ErrAlreadyExists in ensureOAuthUser, add tests, drop stal…

493a97f

…e 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>

fix: clarify error type check ordering in compensating delete paths

f2f68aa

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>

Copilot AI requested a review from slhmy April 16, 2026 07:16

Copilot finished work on behalf of slhmy April 16, 2026 07:16

slhmy requested a review from Copilot April 16, 2026 07:24

Copilot started reviewing on behalf of slhmy April 16, 2026 07:25 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

slhmy and others added 4 commits April 16, 2026 16:02

Update internal/infrastructure/persistence/mongo_external_identity_st…

78a4bdc

…ore.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/infrastructure/persistence/mongo_user_store.go

f542d33

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/application/identra/service.go

84a425d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update internal/application/identra/service.go

4571686

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot started work on behalf of slhmy April 16, 2026 09:23 View session

Copilot AI and others added 2 commits April 16, 2026 09:30

fix: validate userInfo.ID in BindUserByOAuth; update tests for no-ema…

dca0618

…il 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>

fix: use direct empty string check for userInfo.ID in BindUserByOAuth

5c7be9b

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>

Copilot finished work on behalf of slhmy April 16, 2026 09:34

slhmy requested a review from Copilot April 16, 2026 09:34

Copilot started reviewing on behalf of slhmy April 16, 2026 09:35 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Copilot started work on behalf of slhmy April 16, 2026 09:44 View session

Copilot AI and others added 2 commits April 16, 2026 09:53

fix: apply review comments from pullrequestreview-4119862184

72ce88c

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>

fix: clarify TOCTOU error message to 'linked to another user'

ebab67f

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>

Copilot finished work on behalf of slhmy April 16, 2026 09:59

slhmy merged commit c126e51 into main Apr 16, 2026
2 checks passed

		@@ -53,10 +74,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"),

-				return nil, status.Error(codes.AlreadyExists, "oauth account already linked")
-			}
-			return nil, status.Error(codes.Internal, "failed to link oauth account")
+				providerIdentity, refetchErr := s.externalIdentityStore.GetByProviderID(ctx, stateData.Provider, userInfo.ID)
+				if refetchErr != nil {
+					return nil, status.Error(codes.Internal, "failed to verify oauth link")
+				}
+				if providerIdentity.UserID != bindingUser.ID {
+					return nil, status.Error(codes.AlreadyExists, "oauth account already linked to another user")
+				}
+				// Another concurrent bind linked this OAuth account to the same user.
+				// Treat the operation as idempotent success.
+			} else {
+				return nil, status.Error(codes.Internal, "failed to link oauth account")
+			}

-					if errors.Is(createErr, domain.ErrAlreadyExists) {
+					if errors.Is(createErr, domain.ErrAlreadyExists) {
+						// A concurrent request may have created the same external identity
+						// after our initial lookup. Re-fetch and treat it as success when
+						// it is linked to the same user we intended to link.
+						existingIdentity, getErr := s.externalIdentityStore.GetByProviderAndProviderUserID(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
+						}

+	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())
+	}

Conversation

Copilot AI commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

slhmy commented Apr 16, 2026

Uh oh!

Copilot AI commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slhmy commented Apr 16, 2026

Uh oh!

Copilot AI commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

slhmy commented Apr 16, 2026

Uh oh!

Copilot AI commented Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Apr 15, 2026 •

edited

Loading