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

@@ -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`.

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 {

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)

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