diff --git a/pkg/lockdown/lockdown.go b/pkg/lockdown/lockdown.go index 8923c2f2..86250731 100644 --- a/pkg/lockdown/lockdown.go +++ b/pkg/lockdown/lockdown.go @@ -28,7 +28,6 @@ type RepoAccessCache struct { logger *slog.Logger trustedBotLogins map[string]struct{} identityDigest string - now func() time.Time viewerMu sync.Mutex viewerLogin string @@ -37,9 +36,6 @@ type RepoAccessCache struct { type repoAccessCacheEntry struct { isPrivate bool knownUsers map[string]bool // normalized login -> has push access - - // Preserved across entry updates, so age is bounded from the first fetch. - createdAt time.Time } // RepoAccessInfo captures repository metadata needed for lockdown decisions. @@ -56,9 +52,8 @@ const ( // RepoAccessOption configures RepoAccessCache at construction time. type RepoAccessOption func(*RepoAccessCache) -// WithTTL overrides the default maximum age applied to cache entries. A -// non-positive duration disables expiration. The age is absolute, measured -// from an entry's first fetch: repeated reads never extend it. +// WithTTL overrides the default TTL applied to cache entries. A non-positive +// duration disables expiration. func WithTTL(ttl time.Duration) RepoAccessOption { return func(c *RepoAccessCache) { c.ttl = ttl @@ -92,7 +87,7 @@ func WithCacheName(name string) RepoAccessOption { // no-op. // // Scoping lives in the entry key rather than the table so per-identity state -// stays bounded and is reclaimed by ordinary TTL cleanup. The identity is +// stays bounded and is reclaimed by ordinary idle-TTL cleanup. The identity is // hashed so it never appears verbatim in a key. func WithIdentity(identity string) RepoAccessOption { return func(c *RepoAccessCache) { @@ -217,44 +212,37 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner // so we publish a fresh entry with a cloned knownUsers map on every miss. if cacheItem, err := c.cache.Value(key); err == nil { entry := cacheItem.Data().(*repoAccessCacheEntry) - - if !c.entryExpired(entry) { - if cachedHasPush, known := entry.knownUsers[userKey]; known { - c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo)) - return RepoAccessInfo{ - IsPrivate: entry.isPrivate, - HasPushAccess: cachedHasPush, - }, nil - } - - c.logDebug(ctx, "known users cache miss, fetching permission") - - hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo) - if pushErr != nil { - return RepoAccessInfo{}, pushErr - } - - users := make(map[string]bool, len(entry.knownUsers)+1) - maps.Copy(users, entry.knownUsers) - users[userKey] = hasPush - // Preserve createdAt: a new author must not reset the entry's age. - c.cache.Add(key, c.ttl, &repoAccessCacheEntry{ - isPrivate: entry.isPrivate, - knownUsers: users, - createdAt: entry.createdAt, - }) - + if cachedHasPush, known := entry.knownUsers[userKey]; known { + c.logDebug(ctx, fmt.Sprintf("repo access cache hit for user %s to %s/%s", username, owner, repo)) return RepoAccessInfo{ IsPrivate: entry.isPrivate, - HasPushAccess: hasPush, + HasPushAccess: cachedHasPush, }, nil } - c.logDebug(ctx, fmt.Sprintf("repo access cache entry for %s/%s exceeded max age, refreshing", owner, repo)) - } else { - c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo)) + c.logDebug(ctx, "known users cache miss, fetching permission") + + hasPush, pushErr := c.checkPushAccess(ctx, username, owner, repo) + if pushErr != nil { + return RepoAccessInfo{}, pushErr + } + + users := make(map[string]bool, len(entry.knownUsers)+1) + maps.Copy(users, entry.knownUsers) + users[userKey] = hasPush + c.cache.Add(key, c.ttl, &repoAccessCacheEntry{ + isPrivate: entry.isPrivate, + knownUsers: users, + }) + + return RepoAccessInfo{ + IsPrivate: entry.isPrivate, + HasPushAccess: hasPush, + }, nil } + c.logDebug(ctx, fmt.Sprintf("repo access cache miss for user %s to %s/%s", username, owner, repo)) + isPrivate, viewerLogin, queryErr := c.queryRepoAccessInfo(ctx, owner, repo) if queryErr != nil { return RepoAccessInfo{}, queryErr @@ -269,7 +257,6 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner c.cache.Add(key, c.ttl, &repoAccessCacheEntry{ knownUsers: map[string]bool{userKey: hasPush}, isPrivate: isPrivate, - createdAt: c.clock(), }) return RepoAccessInfo{ @@ -278,23 +265,6 @@ func (c *RepoAccessCache) getRepoAccessInfo(ctx context.Context, username, owner }, nil } -// entryExpired reports whether entry has reached the cache's maximum age, -// measured from creation. cache2go's own expiry instead slides on every read, -// which would let repeated reads keep a stale decision alive indefinitely. -func (c *RepoAccessCache) entryExpired(entry *repoAccessCacheEntry) bool { - if c.ttl <= 0 { - return false - } - return c.clock().Sub(entry.createdAt) >= c.ttl -} - -func (c *RepoAccessCache) clock() time.Time { - if c.now != nil { - return c.now() - } - return time.Now() -} - // queryRepoAccessInfo fetches repository visibility and the viewer login in a single GraphQL round-trip. func (c *RepoAccessCache) queryRepoAccessInfo(ctx context.Context, owner, repo string) (bool, string, error) { if c.client == nil { diff --git a/pkg/lockdown/lockdown_test.go b/pkg/lockdown/lockdown_test.go index 9b1e7c4e..62545336 100644 --- a/pkg/lockdown/lockdown_test.go +++ b/pkg/lockdown/lockdown_test.go @@ -115,17 +115,14 @@ func newMockRepoAccessCache(t *testing.T, ttl time.Duration) (*RepoAccessCache, func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { ctx := t.Context() - cache, transport := newMockRepoAccessCache(t, time.Minute) - start := time.Now() - cache.now = func() time.Time { return start } - + cache, transport := newMockRepoAccessCache(t, 5*time.Millisecond) info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) require.NoError(t, err) require.False(t, info.IsPrivate) require.True(t, info.HasPushAccess) require.EqualValues(t, 1, transport.CallCount()) - cache.now = func() time.Time { return start.Add(2 * time.Minute) } + time.Sleep(20 * time.Millisecond) info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) require.NoError(t, err) @@ -134,64 +131,6 @@ func TestRepoAccessCacheEvictsAfterTTL(t *testing.T) { require.EqualValues(t, 2, transport.CallCount()) } -// Regression test for #3107: sliding expiry would let a frequently-read entry -// outlive revoked access, so age must be bounded from creation. -func TestRepoAccessCacheBoundedExpiryIgnoresRepeatedAccess(t *testing.T) { - ctx := t.Context() - - const ttl = 100 * time.Second - cache, transport := newMockRepoAccessCache(t, ttl) - current := time.Now() - cache.now = func() time.Time { return current } - - info, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) - require.NoError(t, err) - require.True(t, info.HasPushAccess) - require.EqualValues(t, 1, transport.CallCount()) - - // Each read lands well inside the TTL; only their sum exceeds it. - for range 4 { - current = current.Add(20 * time.Second) - _, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) - require.NoError(t, err) - } - require.EqualValues(t, 1, transport.CallCount(), "repeated access within the bounded window must still be served from cache") - - current = current.Add(30 * time.Second) - info, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) - require.NoError(t, err) - require.True(t, info.HasPushAccess) - require.EqualValues(t, 2, transport.CallCount(), "entry must refresh once its absolute age exceeds the TTL, regardless of access frequency") -} - -// A "known users" miss updates an existing entry, a second path that must not -// reset its age. -func TestRepoAccessCacheNewUserDoesNotResetEntryAge(t *testing.T) { - ctx := t.Context() - - const ttl = 100 * time.Second - gqlClient, transport := newMockGQLClient(testUser, false) - restClient := newMockRESTServer(t, "write") - cache := NewRepoAccessCache(gqlClient, restClient, WithTTL(ttl), WithCacheName(t.Name())) - - start := time.Now() - cache.now = func() time.Time { return start } - - _, err := cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) - require.NoError(t, err) - require.EqualValues(t, 1, transport.CallCount()) - - cache.now = func() time.Time { return start.Add(50 * time.Second) } - _, err = cache.getRepoAccessInfo(ctx, "someone-else", testOwner, testRepo) - require.NoError(t, err) - require.EqualValues(t, 1, transport.CallCount(), "checking a new user against a cached repo entry must not re-query repo metadata") - - cache.now = func() time.Time { return start.Add(120 * time.Second) } - _, err = cache.getRepoAccessInfo(ctx, testUser, testOwner, testRepo) - require.NoError(t, err) - require.EqualValues(t, 2, transport.CallCount(), "entry age must be bounded from its original creation, not reset by learning about a new user") -} - func TestRepoAccessCacheIsolatesViewerPerInstance(t *testing.T) { ctx := t.Context() @@ -272,8 +211,8 @@ func TestRepoAccessCacheIdentityScopingIsolatesWithinOneTable(t *testing.T) { require.EqualValues(t, 2, table.Count(), "a repeated request from a known identity must not add another entry") } -// Key-scoped entries stay bounded because ordinary TTL cleanup reclaims them; -// a table per identity could not shrink this way. +// Key-scoped entries stay bounded because ordinary idle-TTL cleanup reclaims +// them; a table per identity could not shrink this way. func TestRepoAccessCacheIdentityScopedEntriesAreReclaimed(t *testing.T) { ctx := t.Context() @@ -296,7 +235,7 @@ func TestRepoAccessCacheIdentityScopedEntriesAreReclaimed(t *testing.T) { require.EqualValues(t, len(identities), table.Count(), "each identity should hold exactly one entry in the shared table") require.Eventually(t, func() bool { return table.Count() == 0 }, 30*time.Second, 10*time.Millisecond, - "per-identity entries must be reclaimed by ordinary TTL cleanup so cache storage stays bounded") + "per-identity entries must be reclaimed by ordinary idle-TTL cleanup so cache storage stays bounded") } type flakyTransport struct {