feat(auhguest): upd renew quota limiting logic

services/auth-guest/README.md

@@ -16,7 +16,7 @@ rotated silently.
 The unauthenticated endpoints are protected against brute-force and abuse: the
 PIN is bound to a browser-held nonce, failed PIN attempts back off exponentially
 and invalidate the PIN after a maximum number of attempts, and `renew` is
-limited per share (cooldown and daily cap) when a session authorizes it. The
+limited per share (cooldown and daily cap) when no session authorizes it. The
 magic link is never affected by a PIN lockout.
 
 It is disabled by default. Set `OC_ENABLE_GUEST_LINKS=true` to enable the guest
@@ -31,7 +31,7 @@ links feature and start the service.
   authorized by a session, the PIN).
 - Exposes unauthenticated endpoints to redeem a token, renew it and verify a
   PIN; redeeming a token or verifying a PIN sets a session cookie. `renew`
-  (when authorized by a session) and `verify/pin` are rate-limited.
+  (when no session authorizes it) and `verify/pin` are rate-limited.
 - Stores only hashes of the token secret, of the PIN and of the nonce, and
   deletes the stored record when the share is removed or expires.
 
@@ -105,18 +105,19 @@ browser.
 Without a session the renewal is *silent*: the service validates the share,
 rotates the stored token secret and expiry, resets the redeemed flag so the new
 link can be redeemed, and publishes `GuestTokenRenewed` without a PIN; it
-answers `200`, sets no nonce cookie and does not count against the quota.
+answers `200` and sets no nonce cookie. Because such a request is
+unauthenticated, it is rate-limited per share.
 
 Both paths require an existing token record for the share and work whether the
 record is unused, expired or already redeemed (the guest already redeemed an
 earlier link); if publishing fails, the previous record is restored.
 
-When a session authorizes the renewal, `renew` is limited per share: a cooldown
+When no session authorizes the renewal, `renew` is limited per share: a cooldown
 between two renewals (`AUTH_GUEST_QUOTA_COOLDOWN`, default 1 minute) and a daily
 cap (`AUTH_GUEST_QUOTA_DAILY_CAP`, default 20 per `AUTH_GUEST_QUOTA_WINDOW`,
 default 24 hours). A request within the cooldown or over the cap is rejected
-with `429` and a `Retry-After` header. A silent renewal without a session is not
-rate-limited.
+with `429` and a `Retry-After` header. A renewal authorized by a valid session is
+not rate-limited.
 
 To turn the new credentials into a session the guest either redeems the link
 (`verify/token`) or enters the PIN (`verify/pin`); both return a session
@@ -142,7 +143,7 @@ sequenceDiagram
     Note over Web: Previous link used or expired
     Web->>+Renew: POST renew { permissionId } (+ session cookie)
     Note right of Renew: Validate session + share, rotate token
-    Note right of Renew: With a valid session: new PIN + nonce, rate-limited
+    Note right of Renew: With a valid session: new PIN + nonce; without one: silent + rate-limited
     Renew->>Bus: publish GuestTokenRenewed
     Note right of Bus: With a new PIN only when the session is valid
     Renew->>-Web: 200 OK, Set-Cookie (nonce) only when the session is valid
@@ -181,14 +182,14 @@ sequenceDiagram
    6-digit PIN (30 minutes by default), overwrites the stored token, PIN and
    nonce hashes (invalidating the previous link and PIN), resets the redeemed
    flag, publishes `GuestTokenRenewed` with the PIN and sets the nonce cookie;
-   these session renewals are limited per share (cooldown plus daily cap) and an
-   exceeded limit fails with `429`. Without a session the renewal is silent: the
-   service validates the share, rotates the stored token secret and expiry,
+   such a renewal is not rate-limited. Without a session the renewal is silent:
+   the service validates the share, rotates the stored token secret and expiry,
    resets the redeemed flag and publishes `GuestTokenRenewed` without a PIN; no
-   nonce cookie is set and the quota is not consumed. Both paths require an
-   existing record; otherwise the request fails with `tokenNotFound`. This works
-   whether the record is unused, expired or already redeemed. If publishing
-   fails, the previous record is restored.
+   nonce cookie is set. Because it is unauthenticated, it is limited per share
+   (cooldown plus daily cap) and an exceeded limit fails with `429`. Both paths
+   require an existing record; otherwise the request fails with `tokenNotFound`.
+   This works whether the record is unused, expired or already redeemed. If
+   publishing fails, the previous record is restored.
 4. **Verify PIN** — the guest posts the PIN, the `permissionId` and the nonce
    cookie to
    `POST /graph/v1beta1/extensions/org.libregraph/guestLinks/verify/pin`.
@@ -226,7 +227,7 @@ Rate-limited responses (`429`) also carry a `retryAfter` field (seconds) and a
   new link/PIN is generated and no nonce cookie is set)
 - **Errors:** `400 invalidRequest`, `404 tokenNotFound`, `404 shareNotFound`,
   `409 conflict`, `410 shareExpired`, `429 tooManyRequests` (with `Retry-After`,
-  only when authorized by a session), `503 serviceUnavailable`,
+  only when no session authorizes the renewal), `503 serviceUnavailable`,
   `500 internalError`
 
 #### `POST .../guestLinks/verify/pin`
@@ -308,7 +309,8 @@ and nonce options are:
 - `AUTH_GUEST_QUOTA_DAILY_CAP` (default `20`),
   `AUTH_GUEST_QUOTA_COOLDOWN` (default `1m`),
   `AUTH_GUEST_QUOTA_WINDOW` (default `24h`) — maximum renewals per share per
-  window and the minimum time between two renewals.
+  window and the minimum time between two renewals (applies to `renew` without a
+  session).
 
 Relevant options:
 

services/auth-guest/pkg/service/authguest/service.go

@@ -140,7 +140,7 @@ func (s *AuthGuestService) VerifyToken(ctx context.Context, tokenString string)
 
 // Renew renews the guest link token for shareID. When the request is
 // authorized by a (possibly expired) session token, a new PIN and nonce are
-// also issued (rate limited); otherwise the token is only rotated and no
+// also issued; otherwise the token is only rotated (rate limited) and no
 // PIN or nonce is issued.
 func (s *AuthGuestService) Renew(ctx context.Context, shareID, sessionToken string) (string, error) {
 	if s.publisher == nil {
@@ -161,7 +161,7 @@ func (s *AuthGuestService) Renew(ctx context.Context, shareID, sessionToken stri
 		validSession = err == nil && sessionShareID == shareID
 	}
 
-	if validSession {
+	if !validSession {
 		// Reject an active cooldown before doing the (remote) share validation.
 		if wait := s.guards.quotaLimiter.RetryAfter(&current.QuotaLimiterState, now); wait > 0 {
 			return "", &GuestError{ErrorType: ErrTooManyAttempts, ShareID: shareID, RetryAfter: wait}
@@ -278,8 +278,9 @@ func (s *AuthGuestService) CleanupShare(shareID string) error {
 }
 
 // rotateGuestToken replaces the guest token's secret and expiry under the
-// record lock. When sessionValid, the PIN, nonce and quota state are replaced
-// as well. When the record is modified concurrently it re-reads
+// record lock. When sessionValid, the PIN and nonce are replaced as well;
+// otherwise the quota limiter state is recorded. When the record is modified
+// concurrently it re-reads
 // and re-applies the rotation, up to renewMaxAttempts. It returns the issued
 // nonce, the record state seen before the successful update (for rollback) and
 // the updated record.
@@ -292,18 +293,18 @@ func (s *AuthGuestService) rotateGuestToken(hash string, tok *token.Token, pinHa
 		}
 
 		updated, err := s.store.UpdateFrom(*rec, func(record *storage.Record) error {
-			if sessionValid {
+			if !sessionValid {
 				if wait := s.guards.quotaLimiter.RetryAfter(&record.QuotaLimiterState, now); wait > 0 {
 					return &GuestError{ErrorType: ErrTooManyAttempts, ShareID: record.ShareID, RetryAfter: wait}
 				}
-
+				s.guards.quotaLimiter.Record(&record.QuotaLimiterState, now)
+			} else {
 				n, err := s.guards.nonce.Issue(&record.NonceState, now)
 				if err != nil {
 					return err
 				}
 				nonce = n
 				record.BackoffLimiterState = guard.BackoffLimiterState{}
-				s.guards.quotaLimiter.Record(&record.QuotaLimiterState, now)
 
 				record.PinHash = pinHash
 				record.PinExpiry = now.Add(s.pinAndNonceTTL)

services/auth-guest/pkg/service/authguest/service_test.go

@@ -465,6 +465,7 @@ func TestRenewPersistsRecordAndPublishesEvent(t *testing.T) {
 	assert.Equal(t, token.Hash(nonce), replaced.NonceState.Hash)
 	assert.NoError(t, guard.NewNonce(time.Hour).Verify(&replaced.NonceState, nonce, time.Now()))
 	assert.False(t, replaced.Redeemed)
+	assert.Zero(t, replaced.QuotaLimiterState.Count)
 
 	require.Len(t, publisher.published, 1)
 	ev, ok := publisher.published[0].(ocEvents.GuestTokenRenewed)
@@ -511,7 +512,7 @@ func TestRenewWithExpiredSessionToken(t *testing.T) {
 	assert.NotEmpty(t, ev.Pin)
 }
 
-func TestRenewValidSessionBlockedDuringCooldown(t *testing.T) {
+func TestRenewValidSessionBypassesCooldown(t *testing.T) {
 	store := storage.NewFileManager(t.TempDir())
 	rec := existingRecord()
 	rec.QuotaLimiterState = guard.QuotaLimiterState{
@@ -528,15 +529,14 @@ func TestRenewValidSessionBlockedDuringCooldown(t *testing.T) {
 		DailyCap: 20,
 	})
 
-	_, err := s.Renew(context.Background(), testShareID, validSessionToken(t))
-	require.Error(t, err)
+	nonce, err := s.Renew(context.Background(), testShareID, validSessionToken(t))
+	require.NoError(t, err)
+	assert.NotEmpty(t, nonce)
+	gwc.AssertCalled(t, "GetShare", mock.Anything, mock.Anything)
 
-	var ge *GuestError
-	require.ErrorAs(t, err, &ge)
-	assert.ErrorIs(t, ge.ErrorType, ErrTooManyAttempts)
-	assert.Greater(t, ge.RetryAfter, time.Duration(0))
-
-	gwc.AssertNotCalled(t, "GetShare", mock.Anything, mock.Anything)
+	got, err := store.Get(token.Hash(testShareID))
+	require.NoError(t, err)
+	assert.Equal(t, 1, got.QuotaLimiterState.Count)
 }
 
 func TestRenewWithoutSessionRotatesSilently(t *testing.T) {
@@ -555,7 +555,7 @@ func TestRenewWithoutSessionRotatesSilently(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotEmpty(t, got.SecretHash)
 	assert.Empty(t, got.PinHash)
-	assert.Zero(t, got.QuotaLimiterState.Count)
+	assert.Equal(t, 1, got.QuotaLimiterState.Count)
 
 	require.Len(t, publisher.published, 1)
 	ev, ok := publisher.published[0].(ocEvents.GuestTokenRenewed)
@@ -563,7 +563,7 @@ func TestRenewWithoutSessionRotatesSilently(t *testing.T) {
 	assert.Empty(t, ev.Pin)
 }
 
-func TestRenewWithoutSessionBypassesCooldown(t *testing.T) {
+func TestRenewWithoutSessionBlockedDuringCooldown(t *testing.T) {
 	store := storage.NewFileManager(t.TempDir())
 	rec := existingRecord()
 	rec.QuotaLimiterState = guard.QuotaLimiterState{
@@ -581,8 +581,14 @@ func TestRenewWithoutSessionBypassesCooldown(t *testing.T) {
 	})
 
 	_, err := s.Renew(context.Background(), testShareID, "")
-	require.NoError(t, err)
-	gwc.AssertCalled(t, "GetShare", mock.Anything, mock.Anything)
+	require.Error(t, err)
+
+	var ge *GuestError
+	require.ErrorAs(t, err, &ge)
+	assert.ErrorIs(t, ge.ErrorType, ErrTooManyAttempts)
+	assert.Greater(t, ge.RetryAfter, time.Duration(0))
+
+	gwc.AssertNotCalled(t, "GetShare", mock.Anything, mock.Anything)
 }
 
 func TestRenewIgnoresRedeemedAndExpiry(t *testing.T) {
@@ -777,7 +783,7 @@ func TestRenewRecordsCooldown(t *testing.T) {
 		DailyCap: 20,
 	})
 
-	_, err := s.Renew(context.Background(), testShareID, validSessionToken(t))
+	_, err := s.Renew(context.Background(), testShareID, "")
 	require.NoError(t, err)
 
 	got, err := store.Get(token.Hash(testShareID))
@@ -804,7 +810,7 @@ func TestRenewBlockedDuringCooldown(t *testing.T) {
 		DailyCap: 20,
 	})
 
-	_, err := s.Renew(context.Background(), testShareID, validSessionToken(t))
+	_, err := s.Renew(context.Background(), testShareID, "")
 	require.Error(t, err)
 
 	var ge *GuestError
@@ -832,7 +838,7 @@ func TestRenewDailyCap(t *testing.T) {
 		DailyCap: 20,
 	})
 
-	_, err := s.Renew(context.Background(), testShareID, validSessionToken(t))
+	_, err := s.Renew(context.Background(), testShareID, "")
 	require.Error(t, err)
 
 	var ge *GuestError
@@ -862,7 +868,7 @@ func TestRotateGuestTokenBlockedReturnsRetryAfter(t *testing.T) {
 	tok, err := s.tokenSvc.Parse(tokStr)
 	require.NoError(t, err)
 
-	_, _, _, err = s.rotateGuestToken(token.Hash(testShareID), tok, "pin-hash", time.Now(), true)
+	_, _, _, err = s.rotateGuestToken(token.Hash(testShareID), tok, "pin-hash", time.Now(), false)
 	require.Error(t, err)
 
 	var ge *GuestError