From 174161895ec926cce01f997b071a10c409ba1e84 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Tue, 20 Oct 2026 23:05:14 +0200 Subject: [PATCH] fix: atomic unlink with the PHP last-identity rule - OAuthIdentitiesDestroy locks the users row (SELECT ... FOR UPDATE) and runs the PHP guarded DELETE in one transaction: a self-set password or another identity allows the unlink; otherwise 409 with the unchanged body - the default allow-list adds microsoft, and an OAuthProviderSource's known names are used when the host passes no Providers - TestOAuthIdentitiesDestroyLastIdentityWithPassword (204), TestOAuthIdentitiesDestroyConcurrentSmoke; README - gofmt of the forgotPasswordBody line touched in the previous commit --- README.md | 6 +-- controllers/api_controller.go | 2 +- controllers/oauth_identities.go | 68 ++++++++++++++++++------ oauth_identities_test.go | 93 +++++++++++++++++++++++++++++++-- 4 files changed, 144 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index e5d2bc7..864d6a8 100644 --- a/README.md +++ b/README.md @@ -108,12 +108,12 @@ All routes are mounted under `/_user/api/v1` with the `throttle:user-api` middle ### Host-mounted OAuth identities -The host application mounts these constructors on a path it chooses. They are not registered under `/_user/api/v1`. `OAuthIdentitiesOptions.WriteNotFound` is the host's 404 writer so missing, foreign and unknown providers share one body. `OAuthIdentitiesOptions.Providers` defaults to `google`, `facebook` and `github` when empty. The 409 last-method phrase is `golem15.user::lang.oauth.last_method_blocked`. +The host application mounts these constructors on a path it chooses. They are not registered under `/_user/api/v1`. `OAuthIdentitiesOptions.WriteNotFound` is the host's 404 writer so missing, foreign and unknown providers share one body. `OAuthIdentitiesOptions.Providers` is the unlink allow-list; when empty it is read on every request from a published `classes.OAuthProviderSource` (its `KnownOAuthProviders`), else it is `google`, `facebook`, `github` and `microsoft`. Unlinking follows the PHP rule: it succeeds when the user has a self-set password (`has_self_set_password`) or another identity remains, and otherwise answers 409 with the phrase `golem15.user::lang.oauth.last_method_blocked` (body unchanged). The guarded delete runs in a transaction that first locks the user's `users` row, so two concurrent unlinks never remove the last sign-in method. | Constructor | Role | |-------------|------| | `controllers.OAuthIdentitiesIndex` | Lists the caller's linked providers as `{provider, linked_at}` only. | -| `controllers.OAuthIdentitiesDestroy` | Unlinks one provider for the caller. | +| `controllers.OAuthIdentitiesDestroy` | Unlinks one provider for the caller (204), or answers 409 when it is the last sign-in method of a user without a self-set password. | | `controllers.OAuthIdentitiesOptions` | Host injects `Providers` and `WriteNotFound`. | ### Admin @@ -176,7 +176,7 @@ Packages: - `PermissionManagePrivilegedGroups`, `PrivilegedGroupCodes(cfg)`, `IsPrivilegedCode(codes, code)`, `PrivilegedGroupIDs(ctx, db, codes)` and `IsPrivilegedMember(ctx, db, codes, userID)`, the privileged-group rule. - `ActivateUsers`, `DeactivateUsers`, `RestoreUsers`, `BanUsers`, `UnbanUsers`, `UnsuspendUser`, `ThrottleStates` and `ForceDeleteCleanup`, the user actions the admin runs. Each takes the handle of the caller's transaction. - `TouchLastSeen(ctx, db, userID)`. - - `OAuthProvider` (`Name`, `Label`) and `OAuthProviderSource` (`EnabledOAuthProviders(ctx)`, `KnownOAuthProviders()`), the seam a social-login plugin publishes with an explicit type argument, `app.Publish[classes.OAuthProviderSource](src)`, so the lookup finds it. `GET /oauth-providers` lists its enabled providers in order. + - `OAuthProvider` (`Name`, `Label`) and `OAuthProviderSource` (`EnabledOAuthProviders(ctx)`, `KnownOAuthProviders()`), the seam a social-login plugin publishes with an explicit type argument, `app.Publish[classes.OAuthProviderSource](src)`, so the lookup finds it. `GET /oauth-providers` lists its enabled providers in order; the unlink routes accept its known names. - `RequestUser(ctx, app, r)` and `EndRequestSession(ctx, app, w, r)`, for handlers that serve signed-in and anonymous browsers on one route (an OAuth provider's authorize page). `RequestUser` reads and checks the session JWT exactly as the `jwt` guard does (bearer header, then the `token` and `auth_token` cookies, the blacklist, `tokens_valid_after`) and returns the user, or `nil` for an anonymous, invalid, expired or blacklisted request and for a soft-deleted user; only an infrastructure failure is an error. `EndRequestSession` blacklists the request's JWT until it expires (as `POST /logout` does) and writes the forget cookie of `bouncer.LoadCookieConfig`, best effort: a blacklist failure is logged, never returned. - `AvatarURL(ctx, app, user, size)`, the display picture rule of the user payload at any size: the avatar thumbnail (size by size, mode auto) when the user has an avatar and a file bucket is published, the file path when the thumbnail fails, otherwise the gravatar URL. The URL may be protocol-relative or relative. The payload's `avatar_url` comes from it at 128. - `HandleGenerator`, `AssignHandle(ctx, db, user)`, `CreatedEvent` / `CreatedEventName`, `DeletingEvent` / `DeletingEventName`, `DeactivatingEvent` / `DeactivatingEventName`, `AuthorizeUser` and `RegisterUserAuthorizer`. diff --git a/controllers/api_controller.go b/controllers/api_controller.go index 9bd8626..de17314 100644 --- a/controllers/api_controller.go +++ b/controllers/api_controller.go @@ -241,7 +241,7 @@ func oauthProviderSource(app *backpack.App) (classes.OAuthProviderSource, bool) return src, ok && src != nil } -const forgotPasswordBody ="If that email exists, a reset link has been sent." +const forgotPasswordBody = "If that email exists, a reset link has been sent." // ForgotPassword always returns the same 200 body. A real account also gets a reset mail. func ForgotPassword(app *backpack.App) http.HandlerFunc { diff --git a/controllers/oauth_identities.go b/controllers/oauth_identities.go index 79f8e54..d3df0c9 100644 --- a/controllers/oauth_identities.go +++ b/controllers/oauth_identities.go @@ -13,13 +13,27 @@ import ( ) // OAuthIdentitiesOptions configures host-mounted list/unlink handlers. -// Providers defaults to google, facebook, github when nil or empty. +// Providers is the unlink allow-list. When nil or empty, the allow-list is +// read on every request: the KnownOAuthProviders of a published +// classes.OAuthProviderSource, else google, facebook, github and microsoft. // WriteNotFound is injected by the host so 404 bodies match that app. type OAuthIdentitiesOptions struct { Providers []string WriteNotFound func(http.ResponseWriter, *http.Request) } +// defaultOAuthProviders is the unlink allow-list without options and without +// a published provider source. +var defaultOAuthProviders = []string{"google", "facebook", "github", "microsoft"} + +// oauthGuardedDelete is the PHP unlink rule (User.php unlinkOAuthProvider): +// the identity goes only when the user has a self-set password or another +// identity remains. +const oauthGuardedDelete = `DELETE FROM golem15_user_oauth_identities +WHERE user_id = ? AND provider = ? + AND (EXISTS (SELECT 1 FROM users u WHERE u.id = golem15_user_oauth_identities.user_id AND u.has_self_set_password) + OR (SELECT COUNT(*) FROM golem15_user_oauth_identities oi WHERE oi.user_id = golem15_user_oauth_identities.user_id) > 1)` + // OAuthIdentitiesIndex lists the caller's linked OAuth providers. func OAuthIdentitiesIndex(app *backpack.App) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -50,19 +64,18 @@ func OAuthIdentitiesIndex(app *backpack.App) http.HandlerFunc { } } -// OAuthIdentitiesDestroy unlinks one provider for the caller. +// OAuthIdentitiesDestroy unlinks one provider for the caller, under the PHP +// rule: it succeeds (204) when the user has a self-set password or another +// identity remains, and answers 409 with the last_method_blocked phrase +// otherwise. An unknown provider or a missing identity goes to WriteNotFound. func OAuthIdentitiesDestroy(app *backpack.App, opts OAuthIdentitiesOptions) http.HandlerFunc { - providers := opts.Providers - if len(providers) == 0 { - providers = []string{"google", "facebook", "github"} - } return func(w http.ResponseWriter, r *http.Request) { db, user, ok := oauthIdentityDeps(w, r, app) if !ok { return } provider := r.PathValue("provider") - if !slices.Contains(providers, provider) { + if !slices.Contains(unlinkProviders(app, opts), provider) { writeOAuthIdentityNotFound(w, r, opts) return } @@ -78,28 +91,49 @@ func OAuthIdentitiesDestroy(app *backpack.App, opts OAuthIdentitiesOptions) http writeOpaque500(w) return } - var n int64 - if err := db.WithContext(r.Context()). - Model(&models.OAuthIdentity{}). - Where("user_id = ?", user.ID). - Count(&n).Error; err != nil { + // D-13: the guarded DELETE alone is not race-safe on Postgres. Under + // READ COMMITTED two unlinks of different identities each see the + // other's row in their COUNT and do not wait for each other, so both + // succeed and a password-less user loses every sign-in method. The + // lock on the users row serialises them: the second DELETE runs after + // the first commits and sees one identity left. + var deleted int64 + err = db.WithContext(r.Context()).Transaction(func(tx *gorm.DB) error { + if err := tx.Exec(`SELECT id FROM users WHERE id = ? FOR UPDATE`, user.ID).Error; err != nil { + return err + } + res := tx.Exec(oauthGuardedDelete, user.ID, provider) + if res.Error != nil { + return res.Error + } + deleted = res.RowsAffected + return nil + }) + if err != nil { writeOpaque500(w) return } - if n == 1 { + if deleted == 0 { writeJSON(w, http.StatusConflict, map[string]any{ "error": oauthLastMethodBlocked(r, app), }) return } - if err := db.WithContext(r.Context()).Delete(&row).Error; err != nil { - writeOpaque500(w) - return - } w.WriteHeader(http.StatusNoContent) } } +// unlinkProviders is the unlink allow-list for one request. +func unlinkProviders(app *backpack.App, opts OAuthIdentitiesOptions) []string { + if len(opts.Providers) > 0 { + return opts.Providers + } + if src, ok := oauthProviderSource(app); ok { + return src.KnownOAuthProviders() + } + return defaultOAuthProviders +} + func writeOAuthIdentityNotFound(w http.ResponseWriter, r *http.Request, opts OAuthIdentitiesOptions) { if opts.WriteNotFound != nil { opts.WriteNotFound(w, r) diff --git a/oauth_identities_test.go b/oauth_identities_test.go index 971dab7..9329017 100644 --- a/oauth_identities_test.go +++ b/oauth_identities_test.go @@ -109,6 +109,11 @@ func TestOAuthIdentitiesDestroyLastMethodBlockedENAndPL(t *testing.T) { } { t.Run(tc.locale, func(t *testing.T) { user := insertUser(t, db, tc.email, "correct-horse") + // The column defaults to TRUE; a user without a self-set + // password is the one the last identity is kept for. + if err := db.Model(user).Update("has_self_set_password", false).Error; err != nil { + t.Fatal(err) + } seedOAuthIdentity(t, db, user.ID, "facebook", "fb-only-"+tc.locale) rec := destroyOAuthIdentity(t, app, user.ID, "facebook", tc.locale, winter404OrFatal(t)) if rec.Code != http.StatusConflict { @@ -128,7 +133,9 @@ func TestOAuthIdentitiesDestroyLastMethodBlockedENAndPL(t *testing.T) { } } -func TestOAuthIdentitiesDestroyLastMethodBlockedWithPassword(t *testing.T) { +// TestOAuthIdentitiesDestroyLastIdentityWithPassword: under the PHP rule a +// user with a self-set password may unlink the last identity (D-13). +func TestOAuthIdentitiesDestroyLastIdentityWithPassword(t *testing.T) { app, db := sessionApp(t, "0s") activateOAuthIdentitiesLang(t, app) user := insertUser(t, db, "oauth-last-password@example.test", "correct-horse") @@ -137,6 +144,80 @@ func TestOAuthIdentitiesDestroyLastMethodBlockedWithPassword(t *testing.T) { } seedOAuthIdentity(t, db, user.ID, "google", "g-only") rec := destroyOAuthIdentity(t, app, user.ID, "google", "en", winter404OrFatal(t)) + if rec.Code != http.StatusNoContent { + t.Fatalf("status %d body %s", rec.Code, rec.Body.String()) + } + if rec.Body.Len() != 0 { + t.Fatalf("body = %q", rec.Body.String()) + } + if remaining := countOAuthIdentities(t, db, user.ID); remaining != 0 { + t.Fatalf("identities left = %d, want 0", remaining) + } +} + +// TestOAuthIdentitiesDestroyConcurrentSmoke: two unlinks on a password-less +// user with two identities never remove both (D-13, RESEARCH Pitfall 1). +// Transaction A holds the users row lock and has deleted google; the +// handler's unlink of facebook must wait for A, then see one identity left +// and answer 409. +func TestOAuthIdentitiesDestroyConcurrentSmoke(t *testing.T) { + app, db := sessionApp(t, "0s") + activateOAuthIdentitiesLang(t, app) + user := insertUser(t, db, "oauth-concurrent@example.test", "correct-horse") + if err := db.Model(user).Update("has_self_set_password", false).Error; err != nil { + t.Fatal(err) + } + seedOAuthIdentity(t, db, user.ID, "google", "g-concurrent") + seedOAuthIdentity(t, db, user.ID, "facebook", "fb-concurrent") + + txA := db.Begin() + if txA.Error != nil { + t.Fatal(txA.Error) + } + committed := false + t.Cleanup(func() { + if !committed { + txA.Rollback() + } + }) + if err := txA.Exec(`SELECT id FROM users WHERE id = ? FOR UPDATE`, user.ID).Error; err != nil { + t.Fatal(err) + } + res := txA.Exec(`DELETE FROM golem15_user_oauth_identities +WHERE user_id = ? AND provider = ? + AND (EXISTS (SELECT 1 FROM users u WHERE u.id = golem15_user_oauth_identities.user_id AND u.has_self_set_password) + OR (SELECT COUNT(*) FROM golem15_user_oauth_identities oi WHERE oi.user_id = golem15_user_oauth_identities.user_id) > 1)`, + user.ID, "google") + if res.Error != nil || res.RowsAffected != 1 { + t.Fatalf("transaction A delete: rows %d err %v", res.RowsAffected, res.Error) + } + + req := httptest.NewRequest(http.MethodDelete, "/_fonoteka/api/v1/oauth-identities/facebook", nil) + req.SetPathValue("provider", "facebook") + req = req.WithContext(towel.WithLocale(bouncer.WithUser(req.Context(), &bouncer.Principal{ID: user.ID}), "en")) + rec := httptest.NewRecorder() + h := controllers.OAuthIdentitiesDestroy(app, controllers.OAuthIdentitiesOptions{ + WriteNotFound: func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) }, + }) + done := make(chan struct{}) + go func() { + defer close(done) + h.ServeHTTP(rec, req) + }() + select { + case <-done: + t.Fatalf("the unlink answered %d while transaction A held the user row lock", rec.Code) + case <-time.After(300 * time.Millisecond): + } + if err := txA.Commit().Error; err != nil { + t.Fatal(err) + } + committed = true + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("the unlink did not answer after transaction A committed") + } if rec.Code != http.StatusConflict { t.Fatalf("status %d body %s", rec.Code, rec.Body.String()) } @@ -145,10 +226,14 @@ func TestOAuthIdentitiesDestroyLastMethodBlockedWithPassword(t *testing.T) { t.Fatal(err) } if body["error"] != lastMethodBlockedEN { - t.Fatalf("last_method_blocked with password = %#v", body["error"]) + t.Fatalf("409 body = %s", rec.Body.String()) } - if remaining := countOAuthIdentities(t, db, user.ID); remaining != 1 { - t.Fatalf("row still present = %d", remaining) + var left []models.OAuthIdentity + if err := db.Where("user_id = ?", user.ID).Find(&left).Error; err != nil { + t.Fatal(err) + } + if len(left) != 1 || left[0].Provider != "facebook" { + t.Fatalf("remaining = %#v, want facebook only", left) } }