fix: a device revoke cannot lose to a concurrent refresh

The revoke now reads the device's jti with UPDATE ... RETURNING under the
row lock, and the refresh's hand-over (classes.FollowDeviceJTI) reports a
session revoked meanwhile, so the refresh answers the blacklisted-token
401 without a token. A hand-over error fails the refresh closed instead
of leaving the session on a stale jti.

controllers/api_controller.go

@@ -300,29 +300,52 @@ func Refresh(app *backpack.App) http.HandlerFunc {
 			})
 			return
 		}
+		// A device session follows its token through refresh (D-12b), so a
+		// revoke blacklists the token the device holds now. Reading the old
+		// jti unverified is safe: bouncer.Refresh has just verified it. The
+		// hand-over and the revoke take the session's row lock, so whichever
+		// runs second sees the other: a device revoked after bouncer.Refresh
+		// checked the blacklist is refused here with the blacklisted-token
+		// answer and the new token is never sent. The hand-over fails
+		// closed: a database error answers the opaque 500 (the presented
+		// token is already rotated out), never a token the device row does
+		// not follow. With golem15.user.jwt.blacklist_grace above 0 two
+		// concurrent refreshes of one token can fork a device's lineage and
+		// only one fork is followed; a password change still ends both
+		// (tokens_valid_after). Log lines carry no token or jti.
+		if oldJTI := unverifiedJTI(raw); oldJTI != "" {
+			_, _, _, newJTI, err := bouncer.VerifyClaims(next, secret)
+			if err != nil {
+				pluginLog.Error("golem15.user: read the refreshed token", "user_id", userID, "err", err)
+				writeOpaque500(w)
+				return
+			}
+			revoked, err := classes.FollowDeviceJTI(r.Context(), gdb, oldJTI, newJTI)
+			if err != nil {
+				pluginLog.Error("golem15.user: hand a device session over on refresh", "user_id", userID, "err", err)
+				writeOpaque500(w)
+				return
+			}
+			if revoked {
+				writeJSON(w, http.StatusUnauthorized, map[string]any{
+					"error": "Could not refresh token",
+					"msg":   refreshBlacklistedMsg,
+				})
+				return
+			}
+		}
 		// A failed last_seen write is logged and never fails the refresh.
 		if err := classes.TouchLastSeen(r.Context(), gdb, userID); err != nil {
 			pluginLog.Warn("golem15.user: stamp last_seen on refresh", "user_id", userID, "err", err)
 		}
-		// A device session follows its token through refresh (D-12b), so a
-		// revoke blacklists the token the device holds now. Reading the old
-		// jti unverified is safe: bouncer.Refresh has just verified it. With
-		// golem15.user.jwt.blacklist_grace above 0 two concurrent refreshes of
-		// one token can fork a device's lineage and only one fork is
-		// followed; a password change still ends both (tokens_valid_after).
-		// A failure is logged without any token or jti and never fails the
-		// refresh.
-		if oldJTI := unverifiedJTI(raw); oldJTI != "" {
-			if _, _, _, newJTI, err := bouncer.VerifyClaims(next, secret); err == nil {
-				if err := classes.HandOverDeviceJTI(r.Context(), gdb, oldJTI, newJTI); err != nil {
-					pluginLog.Warn("golem15.user: hand a device session over on refresh", "user_id", userID, "err", err)
-				}
-			}
-		}
 		writeSession(w, app, rotate, next, map[string]any{})
 	}
 }
 
+// refreshBlacklistedMsg is bouncer.Refresh's message for a blacklisted
+// token, which POST /refresh also answers for a revoked device session.
+const refreshBlacklistedMsg = "The token has been blacklisted"
+
 // unverifiedJTI returns the jti claim of a token without verifying it, ""
 // when there is none. Only call it on a token that was verified already.
 func unverifiedJTI(raw string) string {

controllers/devices.go

@@ -458,12 +458,15 @@ func DeviceIndex(app *backpack.App) http.HandlerFunc {
 // device sessions. In one transaction the row is marked revoked and the jti
 // of the session token the device holds (followed through refresh) is
 // added to the JWT blacklist for the token's whole refresh window, so the
-// device is signed out at once and cannot refresh. A blacklist failure
-// answers the opaque 500 and leaves the row as it was. Success answers
-// PHP's body with success true and the message Device revoked
-// successfully, also for a row already revoked; a row of another user, or
-// none, answers 404 with the error Device not found; no session answers
-// 401 {"error":"Unauthorized"}.
+// device is signed out at once and cannot refresh. The jti is the one the
+// revoking UPDATE returns, read under the row lock a concurrent refresh's
+// hand-over also takes (classes.FollowDeviceJTI), so a refresh that moved
+// the session first is blacklisted and one that comes after is refused. A
+// blacklist failure answers the opaque 500 and leaves the row as it was.
+// Success answers PHP's body with success true and the message Device
+// revoked successfully, also for a row already revoked; a row of another
+// user, or none, answers 404 with the error Device not found; no session
+// answers 401 {"error":"Unauthorized"}.
 func DeviceDestroy(app *backpack.App) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		gdb, secret, ok := sessionDeps(w, app)
@@ -480,22 +483,24 @@ func DeviceDestroy(app *backpack.App) http.HandlerFunc {
 			notFound()
 			return
 		}
-		var row models.DeviceAuthSession
-		res := gdb.WithContext(r.Context()).Where("id = ? AND user_id = ?", id, user.ID).Limit(1).Find(&row)
-		if res.Error != nil {
-			writeOpaque500(w)
-			return
-		}
-		if res.RowsAffected == 0 {
-			notFound()
-			return
-		}
+		found := false
 		err = lagoon.Transaction(r.Context(), gdb, func(ctx context.Context, tx *gorm.DB) error {
-			if err := tx.WithContext(ctx).Exec(`UPDATE `+deviceTable+` SET status = 'revoked', updated_at = NOW()
-	WHERE id = ? AND user_id = ?`, row.ID, user.ID).Error; err != nil {
+			// The jti current at the revoke, under the row lock: never one
+			// read before a concurrent refresh moved the session on.
+			var revoked []struct {
+				JWTJTI *string `gorm:"column:jwt_jti"`
+			}
+			if err := tx.WithContext(ctx).Raw(`UPDATE `+deviceTable+` SET status = 'revoked', updated_at = NOW()
+	WHERE id = ? AND user_id = ?
+	RETURNING jwt_jti`, id, user.ID).Scan(&revoked).Error; err != nil {
 				return err
 			}
-			if row.JWTJTI == nil || *row.JWTJTI == "" {
+			if len(revoked) == 0 {
+				return nil
+			}
+			found = true
+			jti := revoked[0].JWTJTI
+			if jti == nil || *jti == "" {
 				return nil
 			}
 			bl, ok := app.Lookup[bouncer.BlacklistStore]()
@@ -503,13 +508,17 @@ func DeviceDestroy(app *backpack.App) http.HandlerFunc {
 				return errors.New("golem15.user: no JWT blacklist store is published")
 			}
 			now := time.Now()
-			return bl.Add(ctx, *row.JWTJTI, now.Add(refreshTTL(app)+sessionTTL(app)+time.Minute), now)
+			return bl.Add(ctx, *jti, now.Add(refreshTTL(app)+sessionTTL(app)+time.Minute), now)
 		})
 		if err != nil {
 			pluginLog.Error("golem15.user: revoke device session", "user_id", user.ID, "err", err)
 			writeOpaque500(w)
 			return
 		}
+		if !found {
+			notFound()
+			return
+		}
 		writeJSON(w, http.StatusOK, map[string]any{"success": true, "message": "Device revoked successfully"})
 	}
 }