Do not cache auth probe denials (#31)

Author: joecorall

docs/authorization.md

@@ -158,16 +158,13 @@ repository permissions change rarely or when the mapping is used for content
 whose access is primarily stable. Shorter TTLs keep permission changes visible
 sooner at the cost of more probes to the upstream Drupal URL.
 
-Triplet caches successful probes, 401, 403, and 404 responses for the
-configured TTL. Other upstream errors are not cached.
-
-Negative auth-probe caching is conservative. For 401, 403, and 404 probe
-responses, Triplet checks the upstream `Last-Modified` header before caching the
-denial. If `Last-Modified` parses and is newer than
-5 minutes ago, the denial is not cached. This avoids holding a stale denial
-while repository access rules or file publication are still settling. If
-`Last-Modified` is absent, unparseable, or older than that minimum age window,
-the denial can be cached for the configured auth TTL.
+Triplet caches successful probes for the configured TTL. It does not cache
+401/403 authorization denials, because those commonly happen while Drupal is
+still publishing or recalculating access. Other upstream errors are not cached.
+
+It also does not cache 404 not-found responses. A 404 can happen while Drupal
+or Fedora is still publishing a just-created file, so Triplet rechecks upstream
+on the next request instead of holding a stale miss.
 
 The image cache invalidation route also clears matching auth-probe entries when
 the source backend supports it.

internal/storage/local_url.go

@@ -44,13 +44,12 @@ type LocalURLFallback struct {
 
 // LocalURLMapping maps a URL identifier prefix to a local file source.
 type LocalURLMapping struct {
-	Prefix               string
-	File                 *FileOpener
-	OCFL                 bool
-	AuthProbe            bool
-	AuthCacheTTL         time.Duration
-	AuthErrorCacheMinAge time.Duration
-	AuthCacheMaxEntries  int
+	Prefix              string
+	File                *FileOpener
+	OCFL                bool
+	AuthProbe           bool
+	AuthCacheTTL        time.Duration
+	AuthCacheMaxEntries int
 }
 
 type authCacheEntry struct {
@@ -65,10 +64,9 @@ var errAuthProbeHeadUnsupported = errors.New("auth probe head unsupported")
 type authCacheTier string
 
 const (
-	authCacheTierAnonymous      authCacheTier = "anonymous"
-	authCacheTierAuthenticated  authCacheTier = "authenticated"
-	defaultAuthErrorCacheMinAge               = 5 * time.Minute
-	defaultAuthCacheMaxEntries                = 4096
+	authCacheTierAnonymous     authCacheTier = "anonymous"
+	authCacheTierAuthenticated authCacheTier = "authenticated"
+	defaultAuthCacheMaxEntries               = 4096
 )
 
 // Open implements Opener.
@@ -315,7 +313,7 @@ func (l *LocalURLFallback) authorize(ctx context.Context, identifier string, map
 		}
 		return l.authorizeAuthenticated(ctx, identifier, mapping, headers)
 	}
-	anonErr := l.probeCached(ctx, anonKey, identifier, nil, mapping.anonymousAuthTTL(), mapping.authErrorCacheMinAge(), mapping.authCacheMaxEntries())
+	anonErr := l.probeCached(ctx, anonKey, identifier, nil, mapping.anonymousAuthTTL(), mapping.authCacheMaxEntries())
 	if anonErr == nil || !hasAuthHeaders(headers) {
 		return anonErr
 	}
@@ -330,10 +328,10 @@ func (l *LocalURLFallback) authorizeAuthenticated(ctx context.Context, identifie
 	if err, ok := l.cachedAuth(key); ok {
 		return err
 	}
-	return l.probeCached(ctx, key, identifier, headers, mapping.authenticatedAuthTTL(), mapping.authErrorCacheMinAge(), mapping.authCacheMaxEntries())
+	return l.probeCached(ctx, key, identifier, headers, mapping.authenticatedAuthTTL(), mapping.authCacheMaxEntries())
 }
 
-func (l *LocalURLFallback) probeCached(ctx context.Context, key, identifier string, headers http.Header, ttl, errorMinAge time.Duration, maxEntries int) error {
+func (l *LocalURLFallback) probeCached(ctx context.Context, key, identifier string, headers http.Header, ttl time.Duration, maxEntries int) error {
 	if wait, ok := l.beginAuthProbe(key); ok {
 		l.debug(ctx, "local url auth probe waiting on in-flight probe", identifier, "cache_key", redact.Hash(key))
 		select {
@@ -344,8 +342,8 @@ func (l *LocalURLFallback) probeCached(ctx context.Context, key, identifier stri
 			return ctx.Err()
 		}
 	}
-	respHeader, err := l.probe(ctx, identifier, headers)
-	cacheable := cacheableAuthProbeResult(err) && cacheableAuthProbeResponse(err, respHeader, errorMinAge, time.Now())
+	_, err := l.probe(ctx, identifier, headers)
+	cacheable := cacheableAuthProbeResult(err)
 	l.debug(ctx, "local url auth probe completed", identifier, "cache_key", redact.Hash(key), "cacheable", cacheable, "ttl", ttl.String(), "err", err)
 	if ttl > 0 && cacheable {
 		l.storeAuth(key, authCacheScope(key), identifier, err, ttl, maxEntries)
@@ -388,18 +386,7 @@ func hasAuthHeaders(headers http.Header) bool {
 }
 
 func cacheableAuthProbeResult(err error) bool {
-	return err == nil || errors.Is(err, ErrForbidden) || errors.Is(err, ErrNotFound)
-}
-
-func cacheableAuthProbeResponse(err error, header http.Header, errorMinAge time.Duration, now time.Time) bool {
-	if err == nil || (!errors.Is(err, ErrForbidden) && !errors.Is(err, ErrNotFound)) || errorMinAge <= 0 {
-		return true
-	}
-	modified, parseErr := http.ParseTime(header.Get("Last-Modified"))
-	if parseErr != nil {
-		return true
-	}
-	return !modified.After(now.Add(-errorMinAge))
+	return err == nil
 }
 
 func (m LocalURLMapping) anonymousAuthTTL() time.Duration {
@@ -410,13 +397,6 @@ func (m LocalURLMapping) authenticatedAuthTTL() time.Duration {
 	return m.AuthCacheTTL
 }
 
-func (m LocalURLMapping) authErrorCacheMinAge() time.Duration {
-	if m.AuthErrorCacheMinAge > 0 {
-		return m.AuthErrorCacheMinAge
-	}
-	return defaultAuthErrorCacheMinAge
-}
-
 func (m LocalURLMapping) authCacheMaxEntries() int {
 	if m.AuthCacheMaxEntries > 0 {
 		return m.AuthCacheMaxEntries

internal/storage/local_url_test.go

@@ -574,7 +574,7 @@ func TestLocalURLFallbackAuthenticatedAuthProbeTTL(t *testing.T) {
 		time.Sleep(ttl - time.Nanosecond)
 		synctest.Wait()
 		openAndClose(t, op, ctx, identifier)
-		if got := anonProbes.Load(); got != 1 {
+		if got := anonProbes.Load(); got != 2 {
 			t.Fatalf("anonymous probes before auth ttl = %d", got)
 		}
 		if got := authProbes.Load(); got != 1 {
@@ -584,7 +584,7 @@ func TestLocalURLFallbackAuthenticatedAuthProbeTTL(t *testing.T) {
 		time.Sleep(2 * time.Nanosecond)
 		synctest.Wait()
 		openAndClose(t, op, ctx, identifier)
-		if got := anonProbes.Load(); got != 2 {
+		if got := anonProbes.Load(); got != 3 {
 			t.Fatalf("anonymous probes after auth ttl = %d", got)
 		}
 		if got := authProbes.Load(); got != 2 {
@@ -636,7 +636,7 @@ func TestLocalURLFallbackAuthProbeAnonymousSucceedsAndCaches(t *testing.T) {
 	}
 }
 
-func TestLocalURLFallbackAuthProbeDoesNotCacheRecentError(t *testing.T) {
+func TestLocalURLFallbackAuthProbeDoesNotCacheForbidden(t *testing.T) {
 	root := t.TempDir()
 	if err := os.WriteFile(filepath.Join(root, "private.jp2"), []byte("private"), 0o600); err != nil {
 		t.Fatal(err)
@@ -654,11 +654,10 @@ func TestLocalURLFallbackAuthProbeDoesNotCacheRecentError(t *testing.T) {
 	}
 	op := &LocalURLFallback{
 		Mappings: []LocalURLMapping{{
-			Prefix:               srv.URL + "/system/files",
-			File:                 fileOp,
-			AuthProbe:            true,
-			AuthCacheTTL:         time.Minute,
-			AuthErrorCacheMinAge: time.Hour,
+			Prefix:       srv.URL + "/system/files",
+			File:         fileOp,
+			AuthProbe:    true,
+			AuthCacheTTL: time.Minute,
 		}},
 		Fallback:     errOpener{},
 		AuthFallback: testAuthHTTP(t, srv),
@@ -675,16 +674,15 @@ func TestLocalURLFallbackAuthProbeDoesNotCacheRecentError(t *testing.T) {
 	}
 }
 
-func TestLocalURLFallbackAuthProbeCachesOldError(t *testing.T) {
+func TestLocalURLFallbackAuthProbeDoesNotCacheNotFound(t *testing.T) {
 	root := t.TempDir()
 	if err := os.WriteFile(filepath.Join(root, "private.jp2"), []byte("private"), 0o600); err != nil {
 		t.Fatal(err)
 	}
 	var probes atomic.Int32
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		probes.Add(1)
-		w.Header().Set("Last-Modified", time.Now().Add(-2*time.Hour).UTC().Format(http.TimeFormat))
-		w.WriteHeader(http.StatusForbidden)
+		w.WriteHeader(http.StatusNotFound)
 	}))
 	defer srv.Close()
 	fileOp, err := NewFileOpener(root)
@@ -693,23 +691,22 @@ func TestLocalURLFallbackAuthProbeCachesOldError(t *testing.T) {
 	}
 	op := &LocalURLFallback{
 		Mappings: []LocalURLMapping{{
-			Prefix:               srv.URL + "/system/files",
-			File:                 fileOp,
-			AuthProbe:            true,
-			AuthCacheTTL:         time.Minute,
-			AuthErrorCacheMinAge: time.Hour,
+			Prefix:       srv.URL + "/system/files",
+			File:         fileOp,
+			AuthProbe:    true,
+			AuthCacheTTL: time.Minute,
 		}},
 		Fallback:     errOpener{},
 		AuthFallback: testAuthHTTP(t, srv),
 	}
 
 	for i := 0; i < 2; i++ {
 		_, _, err := op.Open(context.Background(), srv.URL+"/system/files/private.jp2")
-		if !errors.Is(err, ErrForbidden) {
+		if !errors.Is(err, ErrNotFound) {
 			t.Fatalf("err = %v", err)
 		}
 	}
-	if got := probes.Load(); got != 1 {
+	if got := probes.Load(); got != 2 {
 		t.Fatalf("probes = %d", got)
 	}
 }
@@ -753,7 +750,7 @@ func TestLocalURLFallbackAuthProbeFallsBackToCredentialedCache(t *testing.T) {
 		}
 		_ = rc.Close()
 	}
-	if got := probes.Load(); got != 2 {
+	if got := probes.Load(); got != 3 {
 		t.Fatalf("probes = %d", got)
 	}
 }