Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Broad provider, storage, and credential-persistence changes warrant final human review.
Review effort: Lite
Findings: None
What changed in this PR
Adds configurable asynchronous cache adapters and browser persistence across provider components.
Changes:
- Adds memory, IndexedDB, Web Storage, and TTL caches.
- Injects caches into issuer, discovery, client, and DPoP providers.
- Adds browser defaults, persistence tests, smoke tests, and CI coverage.
| File | Description |
|---|---|
test/providers.test.js |
Tests provider cache injection and behavior. |
test/dpop-cache.test.js |
Tests credential persistence and refresh rotation. |
test/cache.test.js |
Tests adapters, failures, namespaces, and TTLs. |
test/browser-cache.html |
Tests browser persistence and isolation. |
src/WebStorageCache.ts |
Implements codec-based Web Storage caching. |
src/mod.ts |
Exports cache APIs and adapters. |
src/MemoryCache.ts |
Implements in-memory caching. |
src/IndexedDbCache.ts |
Implements IndexedDB persistence. |
src/ExpiringCache.ts |
Adds absolute-TTL behavior. |
src/DPoPTokenProvider.ts |
Adds injectable credential caching and rotation handling. |
src/createBrowserCaches.ts |
Provides browser cache defaults. |
src/CachingIssuerProvider.ts |
Adds injectable issuer caching. |
src/CachingClientProvider.ts |
Adds injectable client caching. |
src/CachingAuthorizationServerProvider.ts |
Adds injectable discovery caching. |
src/Cache.ts |
Defines the asynchronous cache contract. |
README.md |
Documents cache configuration and browser storage policy. |
package.json |
Adds test tooling and dependencies. |
.github/workflows/ci.yml |
Runs the expanded test suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@langsamu note that I have taken a brief look at this PR. Overall I think it is good. I was curious as to whether there is a good library out there that we can use for the generic caching interface. The recommendation from Astra was to use idb-keyval to cleanup the The full response can be viewed here. EDIT: It turns out you can synthesise a library with exactly the functionality we want first shot with Astra. See it available here. Note the interface uses |
Provider caches on
mainare private Maps, so applications cannot choose persistence or manage their contents. This exposes a shared asyncCache<T>contract across all four existing caching components, preserving separate provider responsibilities and per-instance memory defaults.Closes #45. Related: #29 and #15. Storage-wide Solid/LWS cache keys and refresh-lock scope are issue-only in #46; request-URL/issuer keys remain unchanged here.
Changes
MemoryCache,IndexedDbCache, explicit-codecWebStorageCache, and a composable absolute-TTLExpiringCache, with get/set/delete/clear.DPoPTokenCacheEntry. Cache issuer href strings while retaining the provider's URL return type.Browser policy and limits
IndexedDB preserves non-extractable CryptoKeys by structured clone; Web Storage requires an explicit codec and is documented for non-secret values only. The Credential Management API is not a generic OAuth cache. Persisted credentials include the complete token/client/key record, not #15's refresh-only restore design. Non-extractability does not stop same-origin malicious JavaScript from using a key.
Use separate versioned namespaces for application/client configuration/account contexts. Cache clearing is not logout, revocation, or cancellation of in-flight work (#23). Storage errors reject without silently switching backends. Failed grants or writes can require reauthorization. Existing token-expiry behavior, including the missing-expiry TODO, remains unchanged. TTL expiration makes metadata a cache miss but does not automatically purge expired records from storage.
Superseded work
This is a fresh implementation on current
main, superseding #11's older per-issuer session/cache proposal. Its current diff has injectable session adapters, but they do not cover the separate issuer/discovery/client providers, and its IndexedDB writes resolve on request success rather than committed transactions. This PR follows the component architecture requested in #29 and already introduced by #30/#33/#35/#36.Also supersedes #12's older session-based refresh stack in conjunction with the refresh support already merged in #41/#42. The distinct bounded DPoP nonce retry and interactive
offline_accessconsent handling from #12 are not claimed as implemented here; they are called out in the #28 follow-up discussion. #14 (rejected-token retry), #23 (logout fencing), and #26 (WebID reporting) remain open for their separate behavior, as do unrelated PRs.Validation
npm test: TypeScript build and 23 tests pass, including adapter contracts, TTL boundaries, namespace isolation, failures/transaction aborts, restored signing keys, fresh request-specific proofs, refresh rotation, failed durable writes, and retry after cancelled authorization.npm audit --audit-level=moderate: zero vulnerabilities.test/browser-cache.htmlreports PASS after a full reload for IndexedDB key persistence/signing, localStorage/sessionStorage, preset policy, and namespace isolation.git diff --check: clean.Original user request (verbatim)
https://github.com/solid-contrib/reactive-authentication The link provided is to a GitHub repository for a library called Reactive Authentication. The design philosophy behind this library is to reactively upgrade fetch requests when an unauthorized response is returned, and it has been first built for Solid applications. The library, as it stands, is starting to implement caching so that, say issuer metadata, refresh tokens, and depop tokens can be cached and reused to upgrade requests. I would like you to help improve the caching functionality. Specifically, all of the caching at present is done in memory, and the caching is per URL rather than per storage in the context of Solid or LWS. I would like to open an issue, if there is not already one, and I would like you to check if there is already an issue, for both extending in-memory caching to other caching types, including in the browser, using appropriate storages. So the way that I want caches designed is to be configurable. So there's a generic cache interface, and then different components of this library can be instantiated with those caching interfaces. In particular, there's going to be a good browser default, which I imagine you can recommend based on your knowledge of the browser APIs, the credential APIs, the local storage APIs, and which of those APIs are going to be appropriate for caching different types of data that are cached in the library. And then I would like you to open an issue for improving the keying mechanism for caches so that it doesn't always cache just on the request URL, instead it caches to the appropriate context, i.e. storage for linked web storage and Solid. For that storage-wide context, open just an issue at the moment and not a PR. I would like you to look at the existing open PRs and see if they address these issues. Older PRs would have been opened by a less capable LLM to yourself, so you are going to need to start from scratch. But I would like you to reference any PRs that you're superseding and close them as you reference them. Finally, I would like you to include in a fold-out part of the PR description, so it's not there by default, the prompt that I have just given you, so the full context of this conversation is available to the maintainer of the library.