feat: expire FUP entries as soon as the limited periods allow

CHANGELOG.md

@@ -1,6 +1,17 @@
 Changelog
 ====================================
 
+v3.1.0
+------------
+
+### Added
+
+- `contract.FUPTTLCacheDriverInterface`, an optional addition to `contract.CacheDriverInterface` with a single method, `IncrementFUPEntryWithTTL(key string, ttl time.Duration) (*FUPCacheEntry, *AuthError)`. The FUP checkers use it when the configured driver implements it, deriving the TTL from the scope being enforced (`contract.FUPScope.GetEntryTTL`), so an entry is kept only as long as the longest period the scope limits by needs it - two days for a scope limiting per minute and per day, instead of the 35 the monthly period needs. That bounds the memory a FUP key with an unbounded key space (per IP, per cookie) can occupy: at the 490 requests/minute a single unauthenticated source was measured at, the difference is roughly 5 GB and 300 MB of Redis.
+
+  Nothing has to be implemented - this is not a breaking change. A driver that only implements `CacheDriverInterface` keeps being used through `IncrementFUPEntry`, with `constants.FUPEntryTTL` for every entry as before. **A custom driver that wraps a bundled one** (to add metrics, to inject failures) **has to override both increments**, though, or the embedded implementation stays in use for the TTL-aware one.
+
+- `constants.Period.GetEntryTTL` returns how long an entry has to survive inactivity to keep counting that period correctly, and `contract.FUPScope.GetEntryTTL(path)` the longest such TTL a given scope path needs.
+
 v3.0.0
 ------------
 

README.md

@@ -563,6 +563,8 @@ If you implement your own driver, `IncrementFUPEntry` is the method to be carefu
 - an increment that arrives **out of order** (its period was decided before another request that was stored first) must not reset a counter or move the stored timestamp backwards,
 - entries should **expire** after `constants.FUPEntryTTL` of inactivity, otherwise counters of one-off sources (per-IP, per-cookie) accumulate forever.
 
+Optionally, implement `contract.FUPTTLCacheDriverInterface` as well. Its `IncrementFUPEntryWithTTL` is the same increment with a caller-supplied expiration, derived from the scope being enforced, so entries live only as long as the longest period the scope limits by actually needs (a scope limiting per minute and per day keeps them for two days instead of 35). A driver that doesn't implement it keeps `constants.FUPEntryTTL` for everything, which is correct, just less frugal. If your driver *wraps* one of the built-in ones, override both increments - otherwise the embedded implementation stays in use for the TTL-aware one.
+
 ### With user registration:
 
 By default, user registration is disabled. If you don't already have registration process in place, you can enable built-in registration by setting `WithRegistration` to `true` in `User` configuration (see below).
@@ -824,7 +826,7 @@ If no limit is reached, each response to a request that has limits configured wi
 {"hourly":{"limit":200,"used":3},"minutely":{"limit":10,"used":1},"weekly":{"limit":100,"used":46}}
 ```
 
-FUP counters are stored in the cache under a key derived from the FUP key (client id, user login, `anonymous`) and the value the checker limits by (path, IP, cookie). They are incremented atomically, and they expire after 35 days of inactivity (slightly more than the longest supported interval).
+FUP counters are stored in the cache under a key derived from the FUP key (client id, user login, `anonymous`) and the value the checker limits by (path, IP, cookie). They are incremented atomically, and they expire after the longest period the scope limits by needs them - two days for a scope limiting per minute and per day, 35 days if it limits per month (slightly more than the longest supported interval). The expiration is refreshed on every increment, so an entry only has to outlive the gap between two requests sharing a period. This matters for keys with an unbounded key space: a per-IP scope of `{"minutely": 60, "daily": 5000}` holds a key for two days per source IP, not 35.
 
 ### With anonymous FUP limits:
 

auth/cache/memory.go

@@ -152,10 +152,10 @@ func (d *MemoryCacheDriver) getFUPEntry(entryKey string) *contract.FUPCacheEntry
 	}
 }
 
-func (d *MemoryCacheDriver) setFUPEntry(entryKey string, entry *contract.FUPCacheEntry) {
+func (d *MemoryCacheDriver) setFUPEntry(entryKey string, entry *contract.FUPCacheEntry, ttl time.Duration) {
 	d.fupMemory[entryKey] = MemoryCacheEntry[contract.FUPCacheEntry]{
 		Value:    *detachFUPEntry(entry),
-		ExpireAt: time.Now().Add(constants.FUPEntryTTL),
+		ExpireAt: time.Now().Add(ttl),
 	}
 }
 
@@ -168,17 +168,21 @@ func (d *MemoryCacheDriver) GetFUPEntry(key string) (*contract.FUPCacheEntry, *c
 func (d *MemoryCacheDriver) SetFUPEntry(key string, entry *contract.FUPCacheEntry) *contract.AuthError {
 	d.fupLock.Lock()
 	defer d.fupLock.Unlock()
-	d.setFUPEntry(d.getPrefix(GroupTypeFUP)+key, entry)
+	d.setFUPEntry(d.getPrefix(GroupTypeFUP)+key, entry, constants.FUPEntryTTL)
 	return nil
 }
 
 func (d *MemoryCacheDriver) IncrementFUPEntry(key string) (*contract.FUPCacheEntry, *contract.AuthError) {
+	return d.IncrementFUPEntryWithTTL(key, constants.FUPEntryTTL)
+}
+
+func (d *MemoryCacheDriver) IncrementFUPEntryWithTTL(key string, ttl time.Duration) (*contract.FUPCacheEntry, *contract.AuthError) {
 	d.fupLock.Lock()
 	defer d.fupLock.Unlock()
 	entryKey := d.getPrefix(GroupTypeFUP) + key
 	entry := d.getFUPEntry(entryKey)
 	entry.Increment()
-	d.setFUPEntry(entryKey, entry)
+	d.setFUPEntry(entryKey, entry, ttl)
 	return entry, nil
 }
 

auth/cache/memory_test.go

@@ -130,3 +130,30 @@ func TestMemoryCacheDriver_FUPEntryExpiration(t *testing.T) {
 		t.Error("the expired entry is still in memory")
 	}
 }
+
+// TestMemoryCacheDriver_IncrementFUPEntryWithTTL covers that the memory driver honours the
+// caller-supplied expiration as well, so both bundled drivers behave the same
+func TestMemoryCacheDriver_IncrementFUPEntryWithTTL(t *testing.T) {
+	driver := newTestMemoryDriver(t)
+	ttl := constants.PeriodDaily.GetEntryTTL()
+
+	if _, err := driver.IncrementFUPEntryWithTTL("key", ttl); nil != err {
+		t.Fatalf("IncrementFUPEntryWithTTL() error = %v", err)
+	}
+	stored, ok := driver.fupMemory["test:fup_key"]
+	if !ok {
+		t.Fatalf("fupMemory has no entry for the incremented key")
+	}
+	if want, got := time.Now().Add(ttl), stored.ExpireAt; got.Sub(want) > time.Minute || want.Sub(got) > time.Minute {
+		t.Errorf("ExpireAt = %v, want about %v", got, want)
+	}
+
+	// the default is unchanged for callers going through the interface method
+	if _, err := driver.IncrementFUPEntry("other"); nil != err {
+		t.Fatalf("IncrementFUPEntry() error = %v", err)
+	}
+	stored = driver.fupMemory["test:fup_other"]
+	if want, got := time.Now().Add(constants.FUPEntryTTL), stored.ExpireAt; got.Sub(want) > time.Minute || want.Sub(got) > time.Minute {
+		t.Errorf("ExpireAt = %v, want about %v", got, want)
+	}
+}

auth/cache/redis.go

@@ -285,10 +285,14 @@ func (d *RedisCacheDriver) SetFUPEntry(key string, entry *contract.FUPCacheEntry
 }
 
 func (d *RedisCacheDriver) IncrementFUPEntry(key string) (*contract.FUPCacheEntry, *contract.AuthError) {
+	return d.IncrementFUPEntryWithTTL(key, constants.FUPEntryTTL)
+}
+
+func (d *RedisCacheDriver) IncrementFUPEntryWithTTL(key string, ttl time.Duration) (*contract.FUPCacheEntry, *contract.AuthError) {
 	entryKey := d.getPrefix(GroupTypeFUP) + key
 	updatedAt := time.Now()
 	args := make([]any, 0, 2+len(constants.FUPScopePeriods)*3)
-	args = append(args, updatedAt.Format(time.RFC3339Nano), int(constants.FUPEntryTTL.Seconds()))
+	args = append(args, updatedAt.Format(time.RFC3339Nano), int(ttl.Seconds()))
 	for _, period := range constants.FUPScopePeriods {
 		from, to := period.GetTimestampBounds(updatedAt)
 		args = append(args, string(period), from, to)

auth/cache/redis_test.go

@@ -286,3 +286,37 @@ func TestRedisCacheDriver_IncrementFUPEntry_Concurrent(t *testing.T) {
 		t.Errorf("GetFUPEntry() daily = %d, want %d", got, requests)
 	}
 }
+
+// TestRedisCacheDriver_IncrementFUPEntryWithTTL_TTL covers the expiration the FUP checkers derive
+// from the scope - a scope limiting nothing longer than a day must not keep per-IP keys for 35 days
+func TestRedisCacheDriver_IncrementFUPEntryWithTTL_TTL(t *testing.T) {
+	driver, server := newTestRedisDriver(t)
+	ttl := constants.PeriodDaily.GetEntryTTL()
+
+	entry, err := driver.IncrementFUPEntryWithTTL("key", ttl)
+	if nil != err {
+		t.Fatalf("IncrementFUPEntryWithTTL() error = %v", err)
+	}
+	if got := entry.GetUsed(constants.PeriodDaily); 1 != got {
+		t.Errorf("IncrementFUPEntryWithTTL() daily = %d, want 1", got)
+	}
+	if got := server.TTL("test:fup_key"); ttl != got {
+		t.Errorf("TTL() = %v, want %v", got, ttl)
+	}
+
+	// the expiration is refreshed on every increment, so the entry only has to outlive the gap
+	// between two requests
+	server.FastForward(ttl / 2)
+	if _, err := driver.IncrementFUPEntryWithTTL("key", ttl); nil != err {
+		t.Fatalf("IncrementFUPEntryWithTTL() error = %v", err)
+	}
+	if got := server.TTL("test:fup_key"); ttl != got {
+		t.Errorf("TTL() = %v, want %v after a second increment", got, ttl)
+	}
+
+	// while a gap longer than the TTL releases the key instead of keeping it around
+	server.FastForward(ttl + time.Minute)
+	if server.Exists("test:fup_key") {
+		t.Errorf("Exists() = true, want the entry to be released after %v of inactivity", ttl)
+	}
+}

auth/constants/main.go

@@ -36,11 +36,34 @@ const (
 	ApiUser   = "api-user"
 )
 
-// FUPEntryTTL is the expiration of FUP cache entries. It is slightly longer than the longest
-// FUP period (monthly), so that counters that are no longer used (e.g. per-IP counters of
-// one-off visitors) are released instead of growing unbounded.
+// FUPEntryTTL is the expiration of FUP cache entries whose limited periods are not known to the
+// caller. It is slightly longer than the longest FUP period (monthly), so that counters that are
+// no longer used (e.g. per-IP counters of one-off visitors) are released instead of growing
+// unbounded.
 const FUPEntryTTL = time.Hour * 24 * 35
 
+// GetEntryTTL returns how long a FUP cache entry has to survive inactivity to keep counting this
+// period correctly - the period itself plus a margin, since the expiration is refreshed on every
+// increment and the entry only has to outlive the gap between two requests that share a period.
+// A scope that limits by nothing longer than a day therefore keeps its entries for two days
+// instead of the 35 the monthly period needs, which matters for keys with an unbounded key space
+// (per IP, per cookie).
+func (p Period) GetEntryTTL() time.Duration {
+	switch p {
+	case PeriodMinutely:
+		return time.Hour
+	case PeriodHourly:
+		return time.Hour * 25
+	case PeriodDaily:
+		return time.Hour * 24 * 2
+	case PeriodWeekly:
+		return time.Hour * 24 * 8
+	case PeriodMonthly:
+		return FUPEntryTTL
+	}
+	return FUPEntryTTL
+}
+
 var ScopeAccessibilityOptions = []ScopeAccessibility{
 	ScopeAccessibilityAccessible,
 	ScopeAccessibilityForbidden,

auth/constants/main_test.go

@@ -77,3 +77,33 @@ func TestPeriod_GetTimestampBounds_WeekBoundaries(t *testing.T) {
 		})
 	}
 }
+
+// TestPeriod_GetEntryTTL covers that every period keeps a counter alive for longer than the period
+// it counts - an entry expiring within its own period would reset the counter early and let more
+// requests through than the limit allows
+func TestPeriod_GetEntryTTL(t *testing.T) {
+	tests := []struct {
+		period  Period
+		want    time.Duration
+		atLeast time.Duration
+	}{
+		{period: PeriodMinutely, want: time.Hour, atLeast: time.Minute},
+		{period: PeriodHourly, want: time.Hour * 25, atLeast: time.Hour},
+		{period: PeriodDaily, want: time.Hour * 24 * 2, atLeast: time.Hour * 24},
+		{period: PeriodWeekly, want: time.Hour * 24 * 8, atLeast: time.Hour * 24 * 7},
+		{period: PeriodMonthly, want: FUPEntryTTL, atLeast: time.Hour * 24 * 31},
+		// an unknown period must not shorten the expiration
+		{period: Period("yearly"), want: FUPEntryTTL, atLeast: time.Hour * 24 * 31},
+	}
+	for _, tt := range tests {
+		t.Run(string(tt.period), func(t *testing.T) {
+			got := tt.period.GetEntryTTL()
+			if tt.want != got {
+				t.Errorf("GetEntryTTL() = %v, want %v", got, tt.want)
+			}
+			if got <= tt.atLeast {
+				t.Errorf("GetEntryTTL() = %v, want longer than the period itself (%v)", got, tt.atLeast)
+			}
+		})
+	}
+}

auth/contract/cache.go

@@ -25,9 +25,27 @@ type CacheDriverInterface interface {
 	//     arrives out of order, i.e. one that decided its period before an increment that was
 	//     stored first - it counts towards the newer period instead,
 	//   - expire entries after constants.FUPEntryTTL of inactivity, otherwise the counters of
-	//     one-off sources (per IP, per cookie) accumulate forever,
+	//     one-off sources (per IP, per cookie) accumulate forever - implement
+	//     FUPTTLCacheDriverInterface as well to expire them as soon as the limited periods allow,
 	//   - return an entry that shares no state with what is stored, since the caller reads it
 	//     while other requests keep incrementing.
 	IncrementFUPEntry(key string) (*FUPCacheEntry, *AuthError)
 	InvalidateToken(token string) *AuthError
 }
+
+// FUPTTLCacheDriverInterface is an optional addition to CacheDriverInterface for drivers that can
+// expire a FUP entry after a caller-supplied TTL. The FUP checkers derive it from the scope being
+// enforced (FUPScope.GetEntryTTL), so an entry is kept only as long as the longest period the
+// scope limits by needs it - a scope limiting per minute and per day keeps its entries for two
+// days rather than the 35 the monthly period would need. That bounds the memory a FUP key with an
+// unbounded key space (per IP, per cookie) can occupy.
+//
+// A driver that doesn't implement it is used through IncrementFUPEntry and keeps every entry for
+// constants.FUPEntryTTL, which is correct, just less frugal. The bundled memory and Redis drivers
+// implement it - a driver that wraps one of them (to add metrics, to inject failures) therefore
+// has to override both increments, or the embedded implementation stays in use for this one.
+type FUPTTLCacheDriverInterface interface {
+	// IncrementFUPEntryWithTTL behaves exactly like CacheDriverInterface.IncrementFUPEntry, except
+	// that the entry expires after the given TTL of inactivity instead of constants.FUPEntryTTL.
+	IncrementFUPEntryWithTTL(key string, ttl time.Duration) (*FUPCacheEntry, *AuthError)
+}

auth/contract/entity.go

@@ -202,6 +202,28 @@ func (s FUPScope) HasLimit(key string) bool {
 	return false
 }
 
+// GetEntryTTL returns how long the cache entry of the given FUP path (e.g. constants.FUPIPKey)
+// has to survive inactivity for this scope to be enforced - the longest period the scope actually
+// limits by, plus a margin. A path the scope doesn't limit at all falls back to
+// constants.FUPEntryTTL, which covers every period.
+func (s FUPScope) GetEntryTTL(path string) time.Duration {
+	var ttl time.Duration
+	for _, period := range constants.FUPScopePeriods {
+		limit := s.GetLimit(path + "." + string(period))
+		if nil == limit || *limit < 0 {
+			// no limitation, so nothing has to be counted for this period
+			continue
+		}
+		if periodTTL := period.GetEntryTTL(); periodTTL > ttl {
+			ttl = periodTTL
+		}
+	}
+	if 0 == ttl {
+		return constants.FUPEntryTTL
+	}
+	return ttl
+}
+
 type OneOffToken struct {
 	Value   string    `json:"token"`
 	Expires time.Time `json:"expires"`

auth/contract/entity_test.go

@@ -5,6 +5,7 @@ import (
 	"regexp"
 	"sync"
 	"testing"
+	"time"
 )
 
 func TestAccessScope_GetAccessibility(t *testing.T) {
@@ -677,3 +678,72 @@ func TestFUPScope_GetLimit_HasLimit(t *testing.T) {
 		})
 	}
 }
+
+// TestFUPScope_GetEntryTTL covers that an entry is kept exactly as long as the periods the scope
+// limits by need it - the point of the whole thing is that a scope limiting nothing longer than a
+// day doesn't keep per-IP keys for 35 days
+func TestFUPScope_GetEntryTTL(t *testing.T) {
+	tests := []struct {
+		name  string
+		scope FUPScope
+		path  string
+		want  time.Duration
+	}{
+		{
+			// the live anonymous scope
+			name:  "minutely and daily",
+			scope: FUPScope{constants.FUPIPKey: map[string]any{"minutely": 60, "daily": 5000}},
+			path:  constants.FUPIPKey,
+			want:  constants.PeriodDaily.GetEntryTTL(),
+		},
+		{
+			name:  "the longest period wins",
+			scope: FUPScope{constants.FUPIPKey: map[string]any{"minutely": 60, "monthly": 100000}},
+			path:  constants.FUPIPKey,
+			want:  constants.FUPEntryTTL,
+		},
+		{
+			name:  "minutely only",
+			scope: FUPScope{constants.FUPIPKey: map[string]any{"minutely": 60}},
+			path:  constants.FUPIPKey,
+			want:  constants.PeriodMinutely.GetEntryTTL(),
+		},
+		{
+			// a negative limit means no limitation, so nothing has to be counted for that period
+			name:  "negative limits are not counted",
+			scope: FUPScope{constants.FUPIPKey: map[string]any{"daily": 500, "monthly": -1}},
+			path:  constants.FUPIPKey,
+			want:  constants.PeriodDaily.GetEntryTTL(),
+		},
+		{
+			// other paths must not extend this one
+			name: "another path is not counted",
+			scope: FUPScope{
+				constants.FUPIPKey:     map[string]any{"minutely": 60},
+				constants.FUPCookieKey: map[string]any{"monthly": 1000},
+			},
+			path: constants.FUPIPKey,
+			want: constants.PeriodMinutely.GetEntryTTL(),
+		},
+		{
+			// nothing is limited, so fall back to the expiration that covers every period
+			name:  "path not limited",
+			scope: FUPScope{constants.FUPCookieKey: map[string]any{"daily": 500}},
+			path:  constants.FUPIPKey,
+			want:  constants.FUPEntryTTL,
+		},
+		{
+			name:  "empty scope",
+			scope: FUPScope{},
+			path:  constants.FUPIPKey,
+			want:  constants.FUPEntryTTL,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.scope.GetEntryTTL(tt.path); tt.want != got {
+				t.Errorf("GetEntryTTL() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

auth/fup/main.go

@@ -6,12 +6,23 @@ import (
 	"github.com/wernerdweight/api-auth-go/v3/auth/constants"
 	"github.com/wernerdweight/api-auth-go/v3/auth/contract"
 	"strings"
+	"time"
 )
 
+// incrementFUPEntry increments the entry, expiring it as soon as the enforced periods allow if the
+// driver supports it (see contract.FUPTTLCacheDriverInterface); a driver that doesn't keeps every
+// entry for constants.FUPEntryTTL.
+func incrementFUPEntry(cacheDriver contract.CacheDriverInterface, cacheKey string, ttl time.Duration) (*contract.FUPCacheEntry, *contract.AuthError) {
+	if ttlDriver, ok := cacheDriver.(contract.FUPTTLCacheDriverInterface); ok {
+		return ttlDriver.IncrementFUPEntryWithTTL(cacheKey, ttl)
+	}
+	return cacheDriver.IncrementFUPEntry(cacheKey)
+}
+
 func checkLimits(scope *contract.FUPScope, key string, cacheId string, path string, cacheDriver contract.CacheDriverInterface) (map[constants.Period]contract.FUPLimits, *contract.FUPScopeLimits) {
 	limits := make(map[constants.Period]contract.FUPLimits)
 	cacheKey := fmt.Sprintf("%s_%s", key, strings.Replace(cacheId, "/", "-", -1))
-	cacheEntry, err := cacheDriver.IncrementFUPEntry(cacheKey)
+	cacheEntry, err := incrementFUPEntry(cacheDriver, cacheKey, scope.GetEntryTTL(path))
 	if nil != err {
 		return nil, &contract.FUPScopeLimits{
 			Error: err,