feat: Add a cache to the AgentGroupAccessChecker

config/ldp/authorization/authorizers/access-checkers/agent-group.json

@@ -5,7 +5,17 @@
       "comment": "Checks if the agent belongs to a group that has access.",
       "@id": "urn:solid-server:default:AgentGroupAccessChecker",
       "@type": "AgentGroupAccessChecker",
-      "converter": { "@id": "urn:solid-server:default:RepresentationConverter" }
+      "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
+      "cache": {
+        "@id": "urn:solid-server:default:ExpiringAclCache",
+        "@type": "WrappedExpiringStorage",
+        "source": { "@type": "MemoryMapStorage" }
+      }
+    },
+    {
+      "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.",
+      "@id": "urn:solid-server:default:Finalizer",
+      "ParallelFinalizer:_finalizers": [ { "@id": "urn:solid-server:default:ExpiringAclCache" } ]
     }
   ]
 }

src/authorization/access-checkers/AgentGroupAccessChecker.ts

@@ -1,6 +1,8 @@
-import type { Term } from 'n3';
+import type { Store, Term } from 'n3';
+
 import type { ResourceIdentifier } from '../../ldp/representation/ResourceIdentifier';
 import type { RepresentationConverter } from '../../storage/conversion/RepresentationConverter';
+import type { ExpiringStorage } from '../../storage/keyvalue/ExpiringStorage';
 import { fetchDataset } from '../../util/FetchUtil';
 import { promiseSome } from '../../util/PromiseUtil';
 import { readableToQuads } from '../../util/StreamUtil';
@@ -11,13 +13,23 @@ import { AccessChecker } from './AccessChecker';
 /**
  * Checks if the given WebID belongs to a group that has access.
  * Implements the behaviour of groups from the WAC specification.
+ *
+ * Fetched results will be stored in an ExpiringStorage.
+ *
+ * Requires a storage that can store JS objects.
+ * `expiration` parameter is how long entries in the cache should be stored in seconds, defaults to 3600.
  */
 export class AgentGroupAccessChecker extends AccessChecker {
   private readonly converter: RepresentationConverter;
+  private readonly cache: ExpiringStorage<string, Promise<Store>>;
+  private readonly expiration: number;
 
-  public constructor(converter: RepresentationConverter) {
+  public constructor(converter: RepresentationConverter, cache: ExpiringStorage<string, Promise<Store>>,
+    expiration = 3600) {
     super();
     this.converter = converter;
+    this.cache = cache;
+    this.expiration = expiration * 1000;
   }
 
   public async handle({ acl, rule, credentials }: AccessCheckerArgs): Promise<boolean> {
@@ -42,9 +54,24 @@ export class AgentGroupAccessChecker extends AccessChecker {
     const groupDocument: ResourceIdentifier = { path: /^[^#]*/u.exec(group.value)![0] };
 
     // Fetch the required vCard group file
-    const dataset = await fetchDataset(groupDocument.path, this.converter);
+    const quads = await this.fetchCachedQuads(groupDocument.path);
-
-    const quads = await readableToQuads(dataset.data);
     return quads.countQuads(group, VCARD.terms.hasMember, webId, null) !== 0;
   }
+
+  /**
+   * Fetches quads from the given URL.
+   * Will cache the values for later re-use.
+   */
+  private async fetchCachedQuads(url: string): Promise<Store> {
+    let result = await this.cache.get(url);
+    if (!result) {
+      const prom = (async(): Promise<Store> => {
+        const representation = await fetchDataset(url, this.converter);
+        return readableToQuads(representation.data);
+      })();
+      await this.cache.set(url, prom, this.expiration);
+      result = await prom;
+    }
+    return result;
+  }
 }

src/identity/ownership/TokenOwnershipValidator.ts

@@ -36,7 +36,7 @@ export class TokenOwnershipValidator extends OwnershipValidator {
     // No reason to fetch the WebId if we don't have a token yet
     if (!token) {
       token = this.generateToken();
-      await this.storage.set(key, token, new Date(Date.now() + this.expiration));
+      await this.storage.set(key, token, this.expiration);
       this.throwError(webId, token);
     }
 

src/identity/storage/ExpiringAdapterFactory.ts

@@ -43,13 +43,13 @@ export class ExpiringAdapter implements Adapter {
 
   public async upsert(id: string, payload: AdapterPayload, expiresIn?: number): Promise<void> {
     // Despite what the typings say, `expiresIn` can be undefined
-    const expires = expiresIn ? new Date(Date.now() + (expiresIn * 1000)) : undefined;
+    const expiration = expiresIn ? expiresIn * 1000 : undefined;
     const key = this.keyFor(id);
 
     this.logger.debug(`Storing payload data for ${id}`);
 
     const storagePromises: Promise<unknown>[] = [
-      this.storage.set(key, payload, expires),
+      this.storage.set(key, payload, expiration),
     ];
     if (payload.grantId) {
       storagePromises.push(
@@ -57,15 +57,15 @@ export class ExpiringAdapter implements Adapter {
           const grantKey = this.grantKeyFor(payload.grantId!);
           const grants = (await this.storage.get(grantKey) || []) as string[];
           grants.push(key);
-          await this.storage.set(grantKey, grants, expires);
+          await this.storage.set(grantKey, grants, expiration);
         })(),
       );
     }
     if (payload.userCode) {
-      storagePromises.push(this.storage.set(this.userCodeKeyFor(payload.userCode), id, expires));
+      storagePromises.push(this.storage.set(this.userCodeKeyFor(payload.userCode), id, expiration));
     }
     if (payload.uid) {
-      storagePromises.push(this.storage.set(this.uidKeyFor(payload.uid), id, expires));
+      storagePromises.push(this.storage.set(this.uidKeyFor(payload.uid), id, expiration));
     }
     await Promise.all(storagePromises);
   }

src/storage/keyvalue/ExpiringStorage.ts

@@ -1,10 +1,23 @@
 import type { KeyValueStorage } from './KeyValueStorage';
 
+/* eslint-disable @typescript-eslint/method-signature-style */
 /**
  * A KeyValueStorage in which the values can expire.
  * Entries with no expiration date never expire.
  */
 export interface ExpiringStorage<TKey, TValue> extends KeyValueStorage<TKey, TValue> {
+  /**
+   * Sets the value for the given key.
+   * Should error if the data is already expired.
+   *
+   * @param key - Key to set/update.
+   * @param value - Value to store.
+   * @param expiration - How long this data should stay valid in milliseconds.
+   *
+   * @returns The storage.
+   */
+  set(key: TKey, value: TValue, expiration?: number): Promise<this>;
+
   /**
    * Sets the value for the given key.
    * Should error if the data is already expired.
@@ -15,5 +28,5 @@ export interface ExpiringStorage<TKey, TValue> extends KeyValueStorage<TKey, TVa
    *
    * @returns The storage.
    */
-  set: (key: TKey, value: TValue, expires?: Date) => Promise<this>;
+  set(key: TKey, value: TValue, expires?: Date): Promise<this>;
 }

src/storage/keyvalue/WrappedExpiringStorage.ts

@@ -34,7 +34,10 @@ export class WrappedExpiringStorage<TKey, TValue> implements ExpiringStorage<TKe
     return Boolean(await this.getUnexpired(key));
   }
 
-  public async set(key: TKey, value: TValue, expires?: Date): Promise<this> {
+  public async set(key: TKey, value: TValue, expiration?: number): Promise<this>;
+  public async set(key: TKey, value: TValue, expires?: Date): Promise<this>;
+  public async set(key: TKey, value: TValue, expireValue?: number | Date): Promise<this> {
+    const expires = typeof expireValue === 'number' ? new Date(Date.now() + expireValue) : expireValue;
     if (this.isExpired(expires)) {
       throw new InternalServerError('Value is already expired');
     }

test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts

@@ -4,6 +4,7 @@ import { AgentGroupAccessChecker } from '../../../../src/authorization/access-ch
 import { BasicRepresentation } from '../../../../src/ldp/representation/BasicRepresentation';
 import type { Representation } from '../../../../src/ldp/representation/Representation';
 import type { RepresentationConverter } from '../../../../src/storage/conversion/RepresentationConverter';
+import type { ExpiringStorage } from '../../../../src/storage/keyvalue/ExpiringStorage';
 import { INTERNAL_QUADS } from '../../../../src/util/ContentTypes';
 import * as fetchUtil from '../../../../src/util/FetchUtil';
 import { ACL, VCARD } from '../../../../src/util/Vocabularies';
@@ -18,6 +19,7 @@ describe('An AgentGroupAccessChecker', (): void => {
   let fetchMock: jest.SpyInstance;
   let representation: Representation;
   const converter: RepresentationConverter = {} as any;
+  let cache: ExpiringStorage<string, Promise<Store>>;
   let checker: AgentGroupAccessChecker;
 
   beforeEach(async(): Promise<void> => {
@@ -25,8 +27,11 @@ describe('An AgentGroupAccessChecker', (): void => {
     representation = new BasicRepresentation(groupQuads, INTERNAL_QUADS, false);
     fetchMock = jest.spyOn(fetchUtil, 'fetchDataset');
     fetchMock.mockResolvedValue(representation);
+    fetchMock.mockClear();
 
-    checker = new AgentGroupAccessChecker(converter);
+    cache = new Map() as any;
+
+    checker = new AgentGroupAccessChecker(converter, cache);
   });
 
   it('can handle all requests.', async(): Promise<void> => {
@@ -47,4 +52,11 @@ describe('An AgentGroupAccessChecker', (): void => {
     const input: AccessCheckerArgs = { acl, rule: namedNode('groupMatch'), credentials: {}};
     await expect(checker.handle(input)).resolves.toBe(false);
   });
+
+  it('caches fetched results.', async(): Promise<void> => {
+    const input: AccessCheckerArgs = { acl, rule: namedNode('groupMatch'), credentials: { webId }};
+    await expect(checker.handle(input)).resolves.toBe(true);
+    await expect(checker.handle(input)).resolves.toBe(true);
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+  });
 });

test/unit/identity/storage/ExpiringAdapterFactory.test.ts

@@ -15,8 +15,7 @@ describe('An ExpiringAdapterFactory', (): void => {
   let storage: ExpiringStorage<string, unknown>;
   let adapter: ExpiringAdapter;
   let factory: ExpiringAdapterFactory;
-  const expiresIn = 333;
+  const expiresIn = 333 * 1000;
-  const expireDate = new Date(Date.now() + (expiresIn * 1000));
 
   beforeEach(async(): Promise<void> => {
     payload = { data: 'data!' };
@@ -35,7 +34,7 @@ describe('An ExpiringAdapterFactory', (): void => {
   it('can find payload by id.', async(): Promise<void> => {
     await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined();
     expect(storage.set).toHaveBeenCalledTimes(1);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn);
     await expect(adapter.find(id)).resolves.toBe(payload);
   });
 
@@ -50,8 +49,8 @@ describe('An ExpiringAdapterFactory', (): void => {
     payload.userCode = userCode;
     await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined();
     expect(storage.set).toHaveBeenCalledTimes(2);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expiresIn);
     await expect(adapter.findByUserCode(userCode)).resolves.toBe(payload);
   });
 
@@ -60,8 +59,8 @@ describe('An ExpiringAdapterFactory', (): void => {
     payload.uid = uid;
     await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined();
     expect(storage.set).toHaveBeenCalledTimes(2);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expiresIn);
     await expect(adapter.findByUid(uid)).resolves.toBe(payload);
   });
 
@@ -69,8 +68,8 @@ describe('An ExpiringAdapterFactory', (): void => {
     payload.grantId = grantId;
     await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined();
     expect(storage.set).toHaveBeenCalledTimes(2);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn);
-    expect(storage.set).toHaveBeenCalledWith(expect.anything(), [ expect.anything() ], expireDate);
+    expect(storage.set).toHaveBeenCalledWith(expect.anything(), [ expect.anything() ], expiresIn);
     await expect(adapter.find(id)).resolves.toBe(payload);
     await expect(adapter.revokeByGrantId(grantId)).resolves.toBeUndefined();
     expect(storage.delete).toHaveBeenCalledTimes(2);

test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts

@@ -17,7 +17,7 @@ describe('A WrappedExpiringStorage', (): void => {
   tomorrow.setDate(tomorrow.getDate() + 1);
   const yesterday = new Date();
   yesterday.setDate(yesterday.getDate() - 1);
-  let source: KeyValueStorage<string, Internal>;
+  let source: jest.Mocked<KeyValueStorage<string, Internal>>;
   let storage: WrappedExpiringStorage<string, string>;
 
   beforeEach(async(): Promise<void> => {
@@ -42,12 +42,12 @@ describe('A WrappedExpiringStorage', (): void => {
   });
 
   it('returns data if it has not expired.', async(): Promise<void> => {
-    (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', tomorrow));
+    source.get.mockResolvedValueOnce(createExpires('data!', tomorrow));
     await expect(storage.get('key')).resolves.toEqual('data!');
   });
 
   it('deletes expired data when trying to get it.', async(): Promise<void> => {
-    (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', yesterday));
+    source.get.mockResolvedValueOnce(createExpires('data!', yesterday));
     await expect(storage.get('key')).resolves.toBeUndefined();
     expect(source.delete).toHaveBeenCalledTimes(1);
     expect(source.delete).toHaveBeenLastCalledWith('key');
@@ -60,12 +60,12 @@ describe('A WrappedExpiringStorage', (): void => {
   });
 
   it('true on `has` checks if there is non-expired data.', async(): Promise<void> => {
-    (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', tomorrow));
+    source.get.mockResolvedValueOnce(createExpires('data!', tomorrow));
     await expect(storage.has('key')).resolves.toBe(true);
   });
 
   it('deletes expired data when checking if it exists.', async(): Promise<void> => {
-    (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', yesterday));
+    source.get.mockResolvedValueOnce(createExpires('data!', yesterday));
     await expect(storage.has('key')).resolves.toBe(false);
     expect(source.delete).toHaveBeenCalledTimes(1);
     expect(source.delete).toHaveBeenLastCalledWith('key');
@@ -77,6 +77,12 @@ describe('A WrappedExpiringStorage', (): void => {
     expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!', tomorrow));
   });
 
+  it('can store data with an expiration duration.', async(): Promise<void> => {
+    await storage.set('key', 'data!', tomorrow.getTime() - Date.now());
+    expect(source.set).toHaveBeenCalledTimes(1);
+    expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!', tomorrow));
+  });
+
   it('can store data without expiry date.', async(): Promise<void> => {
     await storage.set('key', 'data!');
     expect(source.set).toHaveBeenCalledTimes(1);
@@ -99,7 +105,7 @@ describe('A WrappedExpiringStorage', (): void => {
       [ 'key2', createExpires('data2', yesterday) ],
       [ 'key3', createExpires('data3') ],
     ];
-    (source.entries as jest.Mock).mockImplementationOnce(function* (): any {
+    source.entries.mockImplementationOnce(function* (): any {
       yield* data;
     });
     const it = storage.entries();
@@ -123,7 +129,7 @@ describe('A WrappedExpiringStorage', (): void => {
       [ 'key2', createExpires('data2', yesterday) ],
       [ 'key3', createExpires('data3') ],
     ];
-    (source.entries as jest.Mock).mockImplementationOnce(function* (): any {
+    source.entries.mockImplementationOnce(function* (): any {
       yield* data;
     });
 
@@ -150,7 +156,7 @@ describe('A WrappedExpiringStorage', (): void => {
       [ 'key2', createExpires('data2', yesterday) ],
       [ 'key3', createExpires('data3') ],
     ];
-    (source.entries as jest.Mock).mockImplementationOnce(function* (): any {
+    source.entries.mockImplementationOnce(function* (): any {
       yield* data;
     });