fix: /login throttles failed logins per client address behind trusted proxies (surf.ClientIP), like /register

Behind a reverse proxy every visitor had the proxy's RemoteAddr, so one client's failed logins locked the account for all. The throttle key is now surf.ClientIP with http.trusted_proxies, the expression /register uses; X-Forwarded-For from an untrusted peer is still ignored. RemoteIP is unchanged.

README.md

@@ -20,7 +20,7 @@ It owns the `users` table extensions, `user_throttle`, `jwt_blacklist`, `user_ap
 - **JWT sessions.** `POST /login`, `GET /fetch`, `POST /refresh`, `POST /logout`. Boot registers the `jwt` guard with the framework's `bouncer.Registry` and exposes it as the `jwt.auth` middleware for other plugins. The guard reads the session cookies `classes.SessionCookieNames()` lists and resolves the published `bouncer.BlacklistStore` on every request. Boot installs the plugin's GORM hooks (`classes.RegisterHooks`), publishes the Postgres `jwt_blacklist` store (`bouncer.NewPostgresBlacklist`) and starts its background sweep, which prunes the blacklist, through `lagoon.OnDatabase`. Under `summer serve`, which publishes the database after Boot, they take effect when `lagoon.Publish` runs, before the router is built; when the database is already published they take effect at once. While no store is published, no token counts as blacklisted. `POST /logout` ends the session however the token travels: the bearer header, or only the `token` or `auth_token` cookie the guard also reads. Every session token the request carries goes to the Postgres `jwt_blacklist` until its refresh window closes (the later of `exp` and `iat` plus `refresh_ttl`, plus a minute), so the guard refuses it on the next request and `POST /refresh` cannot revive it after the access token expired; a token whose access lifetime passed is revoked too while it could still be refreshed. The answer, a 401 included, expires the `auth.cookie.*` token cookie (the one `golem15.sociallogin`'s `token_cookie` sets), and any other session cookie the request carries, with that cookie's path, domain, SameSite and Secure. `POST /refresh` loads the token's user and refuses (401, `Could not refresh token`, `User not found`) a deactivated or banned user and a token issued before the user's `tokens_valid_after`; a suspension alone does not stop a refresh. `GET /fetch`, `POST /refresh`, `POST /update` and `POST /change-password`, like the plugin's other signed-in routes, read the session token as the `jwt` guard does: the bearer header first, then the `token` and `auth_token` cookies, so a cookie-only session works on them and a bearer header wins when both are sent. A cookie-carried `POST /refresh` answers the rotated token in the body and writes no cookie; the old token is blacklisted after `golem15.user.jwt.blacklist_grace`, so the caller that wrote the cookie stores the new token in it. A cookie-carried session relies on the cookie's SameSite attribute against cross-site requests (the `auth.cookie` default is `lax`), as on the `jwt.auth` routes.
 - **Password reset and change.** `POST /forgot-password` answers the same body for known and unknown addresses; `POST /reset-password` consumes a code from `IssueResetCode`. `POST /change-password` requires the current password.
 - **Must-change-password lock.** The `user:require-password-change` console command flags an account. The flag rides on the authenticated principal (`bouncer.Principal.MustChangePassword`) for guards and other plugins to act on, and a successful `POST /change-password` clears it.
-- **Login throttling.** Failed logins are counted per user and IP in `user_throttle` (`CheckAndRecordLogin`, `RejectIfThrottled`) and suspend the account for a configured time.
+- **Login throttling.** Failed logins are counted per user and client address in `user_throttle` (`CheckAndRecordLogin`, `RejectIfThrottled`) and suspend the account for a configured time. The client address is `surf.ClientIP` with the framework's `http.trusted_proxies`, as for the `/register` IP limit and the `user-api` rate limit: behind a listed reverse proxy it is the `X-Forwarded-For` client, otherwise the connection address, and an `X-Forwarded-For` header from any other peer is ignored. A deployment behind a proxy lists the proxy's address in `http.trusted_proxies`; otherwise every visitor shares the proxy's counter.
 - **Rate limiting.** Every route runs behind the `user-api` bucket: 120 requests a minute per authenticated user, or per client IP for anonymous calls.
 - **Profile.** `POST /update`, `POST /marketing-consent`, `POST /avatar` and `POST /avatar/remove` (the avatar is stored through the framework's attachment storage).
 - **Deactivation and bans.** Deactivation is self-service and reversible: a password login or a `golem15.sociallogin` sign-in restores a deactivated account and sends the reactivate mail, and an administrator can restore it too. It ends every earlier session JWT and personal API token of the user (`RevokeUserAccess` stamps `tokens_valid_after` and revokes the `user_api_tokens` rows), so the user signs in again and gets a fresh token, while the old API tokens stay revoked. A ban is the lock: password login, social sign-in and `POST /refresh` refuse a banned user, and banning revokes the user's sessions and API tokens. Neither unban nor restore revives a token. Activation gates login, deactivation does not: while `require_activation` is on, an account that is both deactivated and not activated is refused and stays deactivated, with no reactivate mail. Upgrade note: accounts deactivated or banned before this version keep their API tokens until they are deactivated or banned again (both actions are idempotent) or `RevokeUserAccess` runs for them.
@@ -180,7 +180,7 @@ Packages:
   - `APIArray(ctx, app, user)` builds the user payload, extended by `GetApiArrayEvent` listeners.
   - `FinishLogin(ctx, app, db, user, ip)` runs the steps `POST /login` takes after authentication, for a caller that authenticated the user another way (a social sign-in): it restores a soft-deleted account and sends the reactivate mail, records `ip` as `last_ip_address` and touches `last_seen` (a failure there is only logged). It checks no password, throttle, activation or ban and fires no event; the caller does those checks.
   - `RegistrationSettings(app)` returns `allow_registration`, `use_register_throttle`, `activate_mode` and `require_activation` with their defaults (`true`, `true`, `auto`, `true`), and `RegistrationsFromIP(ctx, db, ip)` counts the accounts created from an IP in the last 60 minutes (`/register` refuses at 3 with the throttle on).
-  - `RefreshTTL(app)` is the refresh window, `golem15.user.jwt.refresh_ttl` minutes (default 20160), and `RemoteIP(r)` the host part of `r.RemoteAddr` that `/login` uses for its throttle.
+  - `RefreshTTL(app)` is the refresh window, `golem15.user.jwt.refresh_ttl` minutes (default 20160), and `RemoteIP(r)` the host part of `r.RemoteAddr`, with no proxy handling (`/login` and `/register` use `surf.ClientIP` with `http.trusted_proxies`).
 - `classes`: `JWTSecret`, `GormUsers` (the `bouncer.UserProvider`), `LookupByEmail`, `UserPermissionGrants`, `UserGroupCodes`, `HasGroupCode`, `APITokenPrefix`, `MintAPIToken`, `HashAPIToken` and `RevokeAPIToken`, plus the activation/reset, throttle, mail locale and event helpers. `RegisterEvent` carries `User` and `Payload map[string]any`, a read-only copy of the register input without the password keys. For the admin and for other plugins it also has:
   - `MergedPermissions(ctx, db, userID)`, `HasPermission(merged, code)` and `UserHasPermission(ctx, db, userID, code)`, the frontend permission resolver, and `FrontendPermissionOptions(ctx, db)`, the catalogue by tab and code.
   - `PermissionManagePrivilegedGroups`, `PrivilegedGroupCodes(cfg)`, `IsPrivilegedCode(codes, code)`, `PrivilegedGroupIDs(ctx, db, codes)` and `IsPrivilegedMember(ctx, db, codes, userID)`, the privileged-group rule.
@@ -267,6 +267,8 @@ go test ./...           # full run, starts Postgres with testcontainers (needs D
 
 `TestLoginRequiresActivation` pins the activation gate of `POST /login`: under the default and an explicit `require_activation: true` a not-activated account's correct password gets the byte-identical 401 (status, body, `Content-Type` and `Cache-Control`) of a wrong password, stamps no `last_seen`, adds one throttle attempt and suspends the account after `attempt_limit` refusals; `require_activation: false` and activated accounts sign in; a deactivated activated account is restored with the reactivate mail, and a deactivated not-activated account is refused and stays deactivated unless the key is off.
 
+`TestLoginThrottleClientIP` sends failed logins through one trusted proxy (`http.trusted_proxies`). It checks that the throttle row carries the `X-Forwarded-For` client, and that this client is suspended while another client behind the same proxy still signs in. It also checks that without trusted proxies a changing `X-Forwarded-For` neither moves the counter nor escapes the suspension.
+
 `TestAdminOrganisationMembershipClearsRole` checks that the members manager's unlink and link and a Users form save that changes the organisation clear `organisation_role`, that a save keeping the organisation keeps it, and that deleting an organisation clears both columns.
 
 The admin tests (`TestAdmin*`) boot the plugin with the admin API mounted on a database of their own and sign in backend administrators with a chosen permission set; the helpers are in `admin_harness_test.go`.

controllers/api_controller.go

@@ -54,7 +54,9 @@ const invalidLoginBody = "Invalid email or password"
 // as a wrong password and counts as a failed attempt for the throttle; an
 // account that is also deactivated is refused before the restore, so it
 // stays deactivated and gets no mail. With the key off such an account signs
-// in as before. Activation gates login, deactivation does not.
+// in as before. Activation gates login, deactivation does not. The throttle
+// counts failed attempts per user and client address, which is
+// surf.ClientIP with http.trusted_proxies, as for /register.
 func Login(app *backpack.App) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		gdb, secret, ok := sessionDeps(w, app)
@@ -74,7 +76,7 @@ func Login(app *backpack.App) http.HandlerFunc {
 			writeOpaque500(w)
 			return
 		}
-		ip := remoteIP(r)
+		ip := surf.ClientIP(r, surf.TrustedProxies(appConfig(app)))
 		if user != nil {
 			if err := classes.RejectIfThrottled(ctx, gdb, user, ip); err != nil {
 				if classes.IsLoginRejected(err) {
@@ -1159,9 +1161,10 @@ func requestURL(r *http.Request) string {
 	return scheme + "://" + r.Host + r.URL.Path
 }
 
-// RemoteIP returns the host part of r.RemoteAddr, the address /login uses
-// for its throttle (r.RemoteAddr itself when it has no port, "" when it is
-// empty). /register records surf.ClientIP with the trusted proxies instead.
+// RemoteIP returns the host part of r.RemoteAddr (r.RemoteAddr itself when
+// it has no port, "" when it is empty), with no proxy handling. /login and
+// /register key their throttles on surf.ClientIP with the trusted proxies;
+// RemoteIP stays for callers that want the connection address.
 func RemoteIP(r *http.Request) string {
 	return remoteIP(r)
 }

login_client_ip_test.go

@@ -0,0 +1,106 @@
+package user
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"git.golem15.com/golem15/sm-user-plugin/models"
+	"git.golem15.com/golem15/summercms/modules/compass"
+)
+
+// TestLoginThrottleClientIP pins the /login throttle key: the client address
+// surf.ClientIP derives with http.trusted_proxies, as /register and the
+// user-api rate limit use, and never an X-Forwarded-For header from an
+// untrusted peer.
+func TestLoginThrottleClientIP(t *testing.T) {
+	t.Run("behind a trusted proxy", func(t *testing.T) {
+		e := newAdminEnv(t, func(cfg *compass.Config) {
+			if err := cfg.Set("http.trusted_proxies", []string{"10.0.0.0/8"}); err != nil {
+				t.Fatal(err)
+			}
+		})
+		user := e.seedUser(t, "throttle-proxy", nil)
+		const proxy = "10.0.0.2:5000"
+		for i := range 5 {
+			rec := e.loginFrom(t, proxy, "203.0.113.10", user.Email, "wrong-password")
+			if rec.Code != http.StatusUnauthorized || rec.Body.String() != invalidLoginBody {
+				t.Fatalf("failed login %d = %d %s", i+1, rec.Code, rec.Body.String())
+			}
+		}
+		assertThrottleRow(t, e, user, "203.0.113.10")
+		if rec := e.loginFrom(t, proxy, "203.0.113.10", user.Email, "correct-horse"); rec.Code != http.StatusUnauthorized || rec.Body.String() != invalidLoginBody {
+			t.Fatalf("suspended client login = %d %s", rec.Code, rec.Body.String())
+		}
+		rec := e.loginFrom(t, proxy, "203.0.113.20", user.Email, "correct-horse")
+		var body struct {
+			Token string `json:"token"`
+		}
+		if err := json.Unmarshal(rec.Body.Bytes(), &body); rec.Code != http.StatusOK || err != nil || body.Token == "" {
+			t.Fatalf("other client behind the proxy = %d %s", rec.Code, rec.Body.String())
+		}
+	})
+
+	t.Run("untrusted X-Forwarded-For is ignored", func(t *testing.T) {
+		e := newAdminEnv(t)
+		user := e.seedUser(t, "throttle-direct", nil)
+		const peer = "192.0.2.50:4000"
+		for _, forged := range []string{"203.0.113.1", "203.0.113.2", "203.0.113.3", "203.0.113.4", "203.0.113.5"} {
+			if rec := e.loginFrom(t, peer, forged, user.Email, "wrong-password"); rec.Code != http.StatusUnauthorized {
+				t.Fatalf("failed login with X-Forwarded-For %s = %d %s", forged, rec.Code, rec.Body.String())
+			}
+		}
+		assertThrottleRow(t, e, user, "192.0.2.50")
+		if rec := e.loginFrom(t, peer, "203.0.113.99", user.Email, "correct-horse"); rec.Code != http.StatusUnauthorized || rec.Body.String() != invalidLoginBody {
+			t.Fatalf("forged X-Forwarded-For escaped the suspension: %d %s", rec.Code, rec.Body.String())
+		}
+		if rec := e.loginFrom(t, "192.0.2.51:4000", "", user.Email, "correct-horse"); rec.Code != http.StatusOK {
+			t.Fatalf("login from another address = %d %s", rec.Code, rec.Body.String())
+		}
+	})
+}
+
+// assertThrottleRow: the account has exactly one user_throttle row, keyed on
+// ip, with five attempts and suspended.
+func assertThrottleRow(t *testing.T, e *adminEnv, user *models.User, ip string) {
+	t.Helper()
+	var rows []models.Throttle
+	if err := e.gdb.Where("user_id = ?", user.ID).Find(&rows).Error; err != nil {
+		t.Fatal(err)
+	}
+	if len(rows) != 1 {
+		t.Fatalf("throttle rows = %d, want 1: %+v", len(rows), rows)
+	}
+	row := rows[0]
+	got := "<nil>"
+	if row.IPAddress != nil {
+		got = *row.IPAddress
+	}
+	if got != ip || row.Attempts != 5 || !row.IsSuspended {
+		t.Fatalf("throttle row ip=%s attempts=%d suspended=%v, want ip=%s attempts=5 suspended", got, row.Attempts, row.IsSuspended, ip)
+	}
+}
+
+// loginFrom posts a login from a fixed connection address, with an
+// X-Forwarded-For header when forwardedFor is set. send cannot model a
+// reverse proxy: it gives every request its own address.
+func (e *adminEnv) loginFrom(t *testing.T, remoteAddr, forwardedFor, email, password string) *httptest.ResponseRecorder {
+	t.Helper()
+	raw, err := json.Marshal(map[string]string{"email": email, "password": password})
+	if err != nil {
+		t.Fatal(err)
+	}
+	req := httptest.NewRequest(http.MethodPost, "/_user/api/v1/login", bytes.NewReader(raw))
+	req.Host = "example.com"
+	req.RemoteAddr = remoteAddr
+	req.Header.Set("Accept-Language", "en")
+	req.Header.Set("Content-Type", "application/json")
+	if forwardedFor != "" {
+		req.Header.Set("X-Forwarded-For", forwardedFor)
+	}
+	rec := httptest.NewRecorder()
+	e.h.ServeHTTP(rec, req)
+	return rec
+}