FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: invalidate auth cache when rotating user API key (#883) · mkajnar/httpsms@888ddb3 · GitHub

/ httpsms Public
forked from NdoleStudio/httpsms

Commit 888ddb3

Browse files
AchoArnold
and
Copilot
authored
fix: invalidate auth cache when rotating user API key (NdoleStudio#883)
* fix: invalidate auth cache when rotating user API key (NdoleStudio#881) RotateAPIKey now loads the old API key before updating and calls cache.Del(oldKey) after the DB transaction succeeds. This ensures the old key immediately stops authenticating instead of remaining valid for up to 2 hours (the ristretto cache TTL). Follows the surgical approach (Option B) matching how gorm_phone_api_key_repository already handles cache invalidation. Adds an integration test that rotates the key and verifies the old key returns 401 while the new key returns 200. Closes NdoleStudio#881 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use dedicated test user for rotation test to avoid mutating shared credential The Greptile review correctly identified that the rotation test was mutating the shared userAPIKey, which would break all subsequent tests in the suite. This fix adds a dedicated 'rotate-test-user' in seed.sql and updates the test to use it instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: flush ristretto buffer before cache.Del to prevent async Set race Ristretto's Set operations are buffered asynchronously. If a prior request's SetWithTTL is still in the buffer when Del runs, Del finds nothing to remove, and the buffered Set then re-adds the entry — causing the old key to remain valid. Adding cache.Wait() before cache.Del() flushes all pending buffered operations first, ensuring the subsequent Del actually removes the cached entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: make UserRistrettoCache a singleton so all callers share one cache The root cause of the test failure: UserRistrettoCache() created a new ristretto cache on every call. The auth middleware and the RotateAPIKey handler received separate cache instances, so cache.Del() in the handler had no effect on the middleware's cache — the old key kept authenticating. Fix: store the cache as a field on the Container struct and return it on subsequent calls (lazy singleton pattern), matching how db, app, and eventDispatcher are already handled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: make PhoneRistrettoCache a singleton to prevent stale cache entries Same bug pattern as UserRistrettoCache — PhoneRistrettoCache() created a new ristretto cache on every call. PhoneRepository is used by PhoneService, PhoneAPIKeyService, and NotificationService, each getting a separate cache. Cache invalidations (Clear/Del) in one service had no effect on the others, leading to stale phone data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * revert: undo PhoneRistrettoCache singleton — it causes test interference The PhoneRistrettoCache stores phone metadata (not auth contexts). Making it a singleton caused cache.Clear() in one service to affect all other services, breaking TestReceiveSMS_Encrypted timing. Unlike the UserRistrettoCache (security-critical for auth), the phone cache only holds data with a 30-min TTL and doesn't have cross-service invalidation requirements. The non-singleton behavior is acceptable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent aa8bb70 commit 888ddb3

4 files changed

Lines changed: 116 additions & 2 deletions

File tree

‎api/pkg/di/container.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ type Container struct {
8686
eventDispatcher *services.EventDispatcher
8787
logger telemetry.Logger
8888
attachmentRepository repositories.AttachmentRepository
89+
userRistrettoCache *ristretto.Cache[string, entities.AuthContext]
8990
}
9091

9192
// NewLiteContainer creates a Container without any routes or listeners
@@ -1730,8 +1731,11 @@ func (container *Container) PhoneRistrettoCache() (cache *ristretto.Cache[string
17301731
}
17311732

17321733
// UserRistrettoCache creates an in-memory *ristretto.Cache[string, entities.AuthContext]
1733-
func (container *Container) UserRistrettoCache() (cache *ristretto.Cache[string, entities.AuthContext]) {
1734-
container.logger.Debug(fmt.Sprintf("creating %T", cache))
1734+
func (container *Container) UserRistrettoCache() *ristretto.Cache[string, entities.AuthContext] {
1735+
if container.userRistrettoCache != nil {
1736+
return container.userRistrettoCache
1737+
}
1738+
container.logger.Debug(fmt.Sprintf("creating %T", container.userRistrettoCache))
17351739
ristrettoCache, err := ristretto.NewCache[string, entities.AuthContext](&ristretto.Config[string, entities.AuthContext]{
17361740
MaxCost: 5000,
17371741
NumCounters: 5000 * 10,
@@ -1740,6 +1744,7 @@ func (container *Container) UserRistrettoCache() (cache *ristretto.Cache[string,
17401744
if err != nil {
17411745
container.logger.Fatal(stacktrace.Propagate(err, "cannot create user ristretto cache"))
17421746
}
1747+
container.userRistrettoCache = ristrettoCache
17431748
return ristrettoCache
17441749
}
17451750

‎api/pkg/repositories/gorm_user_repository.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,13 @@ func (repository *gormUserRepository) RotateAPIKey(ctx context.Context, userID e
6565
}
6666

6767
user := new(entities.User)
68+
var oldAPIKey string
6869
err = crdbgorm.ExecuteTx(ctx, repository.db, nil,
6970
func(tx *gorm.DB) error {
71+
if err := tx.WithContext(ctx).Where("id = ?", userID).First(user).Error; err != nil {
72+
return err
73+
}
74+
oldAPIKey = user.APIKey
7075
return tx.WithContext(ctx).Model(user).
7176
Clauses(clause.Returning{}).
7277
Where("id = ?", userID).
@@ -78,6 +83,13 @@ func (repository *gormUserRepository) RotateAPIKey(ctx context.Context, userID e
7883
return nil, repository.tracer.WrapErrorSpan(span, stacktrace.PropagateWithCode(err, ErrCodeNotFound, msg))
7984
}
8085

86+
if err == nil && oldAPIKey != "" {
87+
// Flush pending ristretto Set operations before Del to avoid a
88+
// buffered Set re-adding the entry after removal.
89+
repository.cache.Wait()
90+
repository.cache.Del(oldAPIKey)
91+
}
92+
8193
return user, nil
8294
}
8395

‎tests/integration_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"context"
66
"encoding/json"
7+
"fmt"
78
"io"
89
"net/http"
910
"strings"
@@ -213,6 +214,90 @@ func TestSendSMS_RateLimit(t *testing.T) {
213214
}
214215
}
215216

217+
func TestRotateAPIKey_InvalidatesCache(t *testing.T) {
218+
ctx := context.Background()
219+
220+
// Use a dedicated test user so we don't mutate the shared userAPIKey
221+
rotateUserAPIKey := "rotate-test-api-key"
222+
rotateUserID := "rotate-test-user-id"
223+
224+
// 1) Confirm the dedicated user's API key works and warm the cache
225+
meURL := apiBaseURL + "/v1/users/me"
226+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, meURL, nil)
227+
require.NoError(t, err)
228+
req.Header.Set("x-api-key", rotateUserAPIKey)
229+
230+
resp, err := http.DefaultClient.Do(req)
231+
require.NoError(t, err)
232+
defer resp.Body.Close()
233+
234+
body, err := io.ReadAll(resp.Body)
235+
require.NoError(t, err)
236+
require.Equal(t, http.StatusOK, resp.StatusCode, "initial auth failed: %s", string(body))
237+
238+
// Parse the current API key from the response
239+
var meResp struct {
240+
Data struct {
241+
ID string `json:"id"`
242+
APIKey string `json:"api_key"`
243+
} `json:"data"`
244+
}
245+
require.NoError(t, json.Unmarshal(body, &meResp))
246+
require.Equal(t, rotateUserID, meResp.Data.ID)
247+
oldAPIKey := meResp.Data.APIKey
248+
require.NotEmpty(t, oldAPIKey)
249+
t.Logf("user ID: %s, old API key prefix: %s...", rotateUserID, oldAPIKey[:10])
250+
251+
// 2) Rotate the API key
252+
rotateURL := fmt.Sprintf("%s/v1/users/%s/api-keys", apiBaseURL, rotateUserID)
253+
req, err = http.NewRequestWithContext(ctx, http.MethodDelete, rotateURL, nil)
254+
require.NoError(t, err)
255+
req.Header.Set("x-api-key", rotateUserAPIKey)
256+
257+
resp, err = http.DefaultClient.Do(req)
258+
require.NoError(t, err)
259+
defer resp.Body.Close()
260+
261+
body, err = io.ReadAll(resp.Body)
262+
require.NoError(t, err)
263+
require.Equal(t, http.StatusOK, resp.StatusCode, "rotate failed: %s", string(body))
264+
265+
// Parse new API key from rotate response
266+
var rotateResp struct {
267+
Data struct {
268+
APIKey string `json:"api_key"`
269+
} `json:"data"`
270+
}
271+
require.NoError(t, json.Unmarshal(body, &rotateResp))
272+
newAPIKey := rotateResp.Data.APIKey
273+
require.NotEmpty(t, newAPIKey)
274+
require.NotEqual(t, oldAPIKey, newAPIKey, "API key should have changed after rotation")
275+
t.Logf("new API key prefix: %s...", newAPIKey[:10])
276+
277+
// 3) Old API key should immediately fail (401) — this is the bug regression check
278+
req, err = http.NewRequestWithContext(ctx, http.MethodGet, meURL, nil)
279+
require.NoError(t, err)
280+
req.Header.Set("x-api-key", oldAPIKey)
281+
282+
resp, err = http.DefaultClient.Do(req)
283+
require.NoError(t, err)
284+
defer resp.Body.Close()
285+
assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "old API key should return 401 after rotation")
286+
287+
// 4) New API key should work
288+
req, err = http.NewRequestWithContext(ctx, http.MethodGet, meURL, nil)
289+
require.NoError(t, err)
290+
req.Header.Set("x-api-key", newAPIKey)
291+
292+
resp, err = http.DefaultClient.Do(req)
293+
require.NoError(t, err)
294+
defer resp.Body.Close()
295+
296+
body, err = io.ReadAll(resp.Body)
297+
require.NoError(t, err)
298+
assert.Equal(t, http.StatusOK, resp.StatusCode, "new API key should work: %s", string(body))
299+
}
300+
216301
func TestSendSMS_OutstandingFlow(t *testing.T) {
217302
ctx := context.Background()
218303
phone := setupPhone(ctx, t, 60)

‎tests/seed.sql‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,18 @@ VALUES (
1313
NOW()
1414
) ON CONFLICT (id) DO NOTHING;
1515

16+
-- Test user for API key rotation tests (isolated to avoid mutating the shared test user)
17+
INSERT INTO users (id, email, api_key, timezone, subscription_name, created_at, updated_at)
18+
VALUES (
19+
'rotate-test-user-id',
20+
'rotate-test@httpsms.com',
21+
'rotate-test-api-key',
22+
'UTC',
23+
'pro-monthly',
24+
NOW(),
25+
NOW()
26+
) ON CONFLICT (id) DO NOTHING;
27+
1628
-- System user (for event queue auth)
1729
INSERT INTO users (id, email, api_key, timezone, subscription_name, created_at, updated_at)
1830
VALUES (

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL