Repository: solid/community-server
Pull request #1153 (2022-02-10 14:49:18)

Fix: content-negotiation when fetching dataset from url
#### 📁 Related issues

<!-- 
Reference any relevant issues here. Closing keywords only have an effect when targeting the main branch. If there are no related issues, you must first create an issue through https://github.com/solid/community-server/issues/new/choose
-->
#1039 

#### ✍️ Description

<!-- Describe the relevant changes in this PR. Also add notes that might be relevant for code reviewers. -->
The reasoning behind this fix was to only use rdf-dereference when the `input` type is `string`. That is where the error seems to occur, other flows seem fine. (I might be wrong though)

### ✅ PR check list

Before this pull request can be merged, a core maintainer will check whether
  * [ ] this PR is labeled with the correct semver label
    - semver.patch: Backwards compatible bug fixes.
    - semver.minor: Backwards compatible feature additions.
    - semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
  * [ ] the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
  * [ ] the RELEASE_NOTES.md document in case of relevant feature or config changes.

<!-- Try to check these to the best of your abilities before opening the PR -->


Falx: I think it looks good now.
Falx: Force pushed the new changes
Falx: That should do it then

Patch:
diff --git a/config/identity/ownership/token.json b/config/identity/ownership/token.json
index fb3275c70..e5d9fdace 100644
--- a/config/identity/ownership/token.json
+++ b/config/identity/ownership/token.json
@@ -5,7 +5,6 @@
       "comment": "Determines WebID ownership by requesting a specific value to be added to the WebID document",
       "@id": "urn:solid-server:auth:password:OwnershipValidator",
       "@type": "TokenOwnershipValidator",
-      "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
       "storage": { "@id": "urn:solid-server:default:ExpiringTokenStorage" }
     },
 
diff --git a/config/ldp/authorization/readers/access-checkers/agent-group.json b/config/ldp/authorization/readers/access-checkers/agent-group.json
index fca974b8f..9cf614b1c 100644
--- a/config/ldp/authorization/readers/access-checkers/agent-group.json
+++ b/config/ldp/authorization/readers/access-checkers/agent-group.json
@@ -5,7 +5,6 @@
       "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" },
       "cache": {
         "@id": "urn:solid-server:default:ExpiringAclCache",
         "@type": "WrappedExpiringStorage",
diff --git a/package-lock.json b/package-lock.json
index 6d9cc1450..8da5e3526 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -51,6 +51,7 @@
         "oidc-provider": "^6.31.1",
         "pump": "^3.0.0",
         "punycode": "^2.1.1",
+        "rdf-dereference": "^1.9.0",
         "rdf-parse": "^1.9.1",
         "rdf-serialize": "^1.2.0",
         "rdf-terms": "^1.7.1",
diff --git a/package.json b/package.json
index a00ff2b86..181ac00a9 100644
--- a/package.json
+++ b/package.json
@@ -117,6 +117,7 @@
     "oidc-provider": "^6.31.1",
     "pump": "^3.0.0",
     "punycode": "^2.1.1",
+    "rdf-dereference": "^1.9.0",
     "rdf-parse": "^1.9.1",
     "rdf-serialize": "^1.2.0",
     "rdf-terms": "^1.7.1",
diff --git a/src/authorization/access/AgentGroupAccessChecker.ts b/src/authorization/access/AgentGroupAccessChecker.ts
index b56cc537d..64b458efc 100644
--- a/src/authorization/access/AgentGroupAccessChecker.ts
+++ b/src/authorization/access/AgentGroupAccessChecker.ts
@@ -1,6 +1,5 @@
 import type { Store, Term } from 'n3';
 import type { ResourceIdentifier } from '../../http/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';
@@ -19,14 +18,11 @@ import { AccessChecker } from './AccessChecker';
  * `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, cache: ExpiringStorage<string, Promise<Store>>,
-    expiration = 3600) {
+  public constructor(cache: ExpiringStorage<string, Promise<Store>>, expiration = 3600) {
     super();
-    this.converter = converter;
     this.cache = cache;
     this.expiration = expiration * 1000;
   }
@@ -65,7 +61,7 @@ export class AgentGroupAccessChecker extends AccessChecker {
     let result = await this.cache.get(url);
     if (!result) {
       const prom = (async(): Promise<Store> => {
-        const representation = await fetchDataset(url, this.converter);
+        const representation = await fetchDataset(url);
         return readableToQuads(representation.data);
       })();
       await this.cache.set(url, prom, this.expiration);
diff --git a/src/identity/ownership/TokenOwnershipValidator.ts b/src/identity/ownership/TokenOwnershipValidator.ts
index 4ba0ca501..3f0504d83 100644
--- a/src/identity/ownership/TokenOwnershipValidator.ts
+++ b/src/identity/ownership/TokenOwnershipValidator.ts
@@ -2,7 +2,6 @@ import type { Quad } from 'n3';
 import { DataFactory } from 'n3';
 import { v4 } from 'uuid';
 import { getLoggerFor } from '../../logging/LogUtil';
-import type { RepresentationConverter } from '../../storage/conversion/RepresentationConverter';
 import type { ExpiringStorage } from '../../storage/keyvalue/ExpiringStorage';
 import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError';
 import { fetchDataset } from '../../util/FetchUtil';
@@ -17,13 +16,11 @@ const { literal, namedNode, quad } = DataFactory;
 export class TokenOwnershipValidator extends OwnershipValidator {
   protected readonly logger = getLoggerFor(this);
 
-  private readonly converter: RepresentationConverter;
   private readonly storage: ExpiringStorage<string, string>;
   private readonly expiration: number;
 
-  public constructor(converter: RepresentationConverter, storage: ExpiringStorage<string, string>, expiration = 30) {
+  public constructor(storage: ExpiringStorage<string, string>, expiration = 30) {
     super();
-    this.converter = converter;
     this.storage = storage;
     // Convert minutes to milliseconds
     this.expiration = expiration * 60 * 1000;
@@ -66,7 +63,7 @@ export class TokenOwnershipValidator extends OwnershipValidator {
    * Fetches data from the WebID to determine if the token is present.
    */
   private async hasToken(webId: string, token: string): Promise<boolean> {
-    const representation = await fetchDataset(webId, this.converter);
+    const representation = await fetchDataset(webId);
     const expectedQuad = quad(namedNode(webId), SOLID.terms.oidcIssuerRegistrationToken, literal(token));
     for await (const data of representation.data) {
       const triple = data as Quad;
diff --git a/src/identity/storage/WebIdAdapterFactory.ts b/src/identity/storage/WebIdAdapterFactory.ts
index c29c334aa..734d0126e 100644
--- a/src/identity/storage/WebIdAdapterFactory.ts
+++ b/src/identity/storage/WebIdAdapterFactory.ts
@@ -5,7 +5,7 @@ import type { Adapter, AdapterPayload } from 'oidc-provider';
 import { getLoggerFor } from '../../logging/LogUtil';
 import type { RepresentationConverter } from '../../storage/conversion/RepresentationConverter';
 import { createErrorMessage } from '../../util/errors/ErrorUtil';
-import { fetchDataset } from '../../util/FetchUtil';
+import { responseToDataset } from '../../util/FetchUtil';
 import { OIDC } from '../../util/Vocabularies';
 import type { AdapterFactory } from './AdapterFactory';
 
@@ -91,7 +91,7 @@ export class WebIdAdapter implements Adapter {
    * @param response - Response object from the request.
    */
   private async parseRdfWebId(data: string, id: string, response: Response): Promise<AdapterPayload> {
-    const representation = await fetchDataset(response, this.converter, data);
+    const representation = await responseToDataset(response, this.converter, data);
 
     // Find the valid redirect URIs
     const redirectUris: string[] = [];
diff --git a/src/util/FetchUtil.ts b/src/util/FetchUtil.ts
index 9d1c317a3..5398dede5 100644
--- a/src/util/FetchUtil.ts
+++ b/src/util/FetchUtil.ts
@@ -1,5 +1,8 @@
+import type { Readable } from 'stream';
+import type { Quad } from '@rdfjs/types';
+import arrayifyStream from 'arrayify-stream';
 import type { Response } from 'cross-fetch';
-import { fetch } from 'cross-fetch';
+import rdfDereferencer from 'rdf-dereference';
 import { BasicRepresentation } from '../http/representation/BasicRepresentation';
 import type { Representation } from '../http/representation/Representation';
 import { getLoggerFor } from '../logging/LogUtil';
@@ -12,24 +15,32 @@ const logger = getLoggerFor('FetchUtil');
 
 /**
  * Fetches an RDF dataset from the given URL.
- * Input can also be a Response if the request was already made.
+ *
+ * Response will be a Representation with content-type internal/quads.
+ */
+export async function fetchDataset(url: string): Promise<Representation> {
+  // Try content negotiation to parse quads from the URL
+  return (async(): Promise<Representation> => {
+    try {
+      const quadStream = (await rdfDereferencer.dereference(url)).quads as Readable;
+      const quadArray = await arrayifyStream(quadStream) as Quad[];
+      return new BasicRepresentation(quadArray, { path: url }, INTERNAL_QUADS, false);
+    } catch {
+      throw new BadRequestHttpError(`Could not parse resource at URL (${url})!`);
+    }
+  })();
+}
+
+/**
+ * Converts a given Response (from a request that was already made) to  an RDF dataset.
  * In case the given Response object was already parsed its body can be passed along as a string.
  *
  * The converter will be used to convert the response body to RDF.
  *
  * Response will be a Representation with content-type internal/quads.
  */
-export async function fetchDataset(url: string, converter: RepresentationConverter): Promise<Representation>;
-export async function fetchDataset(response: Response, converter: RepresentationConverter, body?: string):
-Promise<Representation>;
-export async function fetchDataset(input: string | Response, converter: RepresentationConverter, body?: string):
+export async function responseToDataset(response: Response, converter: RepresentationConverter, body?: string):
 Promise<Representation> {
-  let response: Response;
-  if (typeof input === 'string') {
-    response = await fetch(input);
-  } else {
-    response = input;
-  }
   if (!body) {
     body = await response.text();
   }


Test patch:
diff --git a/test/unit/authorization/access/AgentGroupAccessChecker.test.ts b/test/unit/authorization/access/AgentGroupAccessChecker.test.ts
index ae86a9356..adc44727c 100644
--- a/test/unit/authorization/access/AgentGroupAccessChecker.test.ts
+++ b/test/unit/authorization/access/AgentGroupAccessChecker.test.ts
@@ -3,7 +3,6 @@ import type { AccessCheckerArgs } from '../../../../src/authorization/access/Acc
 import { AgentGroupAccessChecker } from '../../../../src/authorization/access/AgentGroupAccessChecker';
 import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation';
 import type { Representation } from '../../../../src/http/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';
@@ -18,7 +17,6 @@ describe('An AgentGroupAccessChecker', (): void => {
   acl.addQuad(namedNode('noMatch'), ACL.terms.agentGroup, namedNode('badGroup'));
   let fetchMock: jest.SpyInstance;
   let representation: Representation;
-  const converter: RepresentationConverter = {} as any;
   let cache: ExpiringStorage<string, Promise<Store>>;
   let checker: AgentGroupAccessChecker;
 
@@ -31,7 +29,7 @@ describe('An AgentGroupAccessChecker', (): void => {
 
     cache = new Map() as any;
 
-    checker = new AgentGroupAccessChecker(converter, cache);
+    checker = new AgentGroupAccessChecker(cache);
   });
 
   it('can handle all requests.', async(): Promise<void> => {
diff --git a/test/unit/identity/ownership/TokenOwnershipValidator.test.ts b/test/unit/identity/ownership/TokenOwnershipValidator.test.ts
index 78b327e15..46c58f0c3 100644
--- a/test/unit/identity/ownership/TokenOwnershipValidator.test.ts
+++ b/test/unit/identity/ownership/TokenOwnershipValidator.test.ts
@@ -1,15 +1,17 @@
-import { fetch } from 'cross-fetch';
+import { Readable } from 'stream';
 import { DataFactory } from 'n3';
 import type { Quad } from 'n3';
+import rdfDereferencer from 'rdf-dereference';
 import { v4 } from 'uuid';
 import { TokenOwnershipValidator } from '../../../../src/identity/ownership/TokenOwnershipValidator';
-import { RdfToQuadConverter } from '../../../../src/storage/conversion/RdfToQuadConverter';
 import type { ExpiringStorage } from '../../../../src/storage/keyvalue/ExpiringStorage';
 import { SOLID } from '../../../../src/util/Vocabularies';
 const { literal, namedNode, quad } = DataFactory;
 
-jest.mock('cross-fetch');
 jest.mock('uuid');
+jest.mock('rdf-dereference', (): any => ({
+  dereference: jest.fn(),
+}));
 
 function quadToString(qq: Quad): string {
   const subPred = `<${qq.subject.value}> <${qq.predicate.value}>`;
@@ -20,21 +22,19 @@ function quadToString(qq: Quad): string {
 }
 
 describe('A TokenOwnershipValidator', (): void => {
-  const fetchMock: jest.Mock = fetch as any;
+  const rdfDereferenceMock: jest.Mocked<typeof rdfDereferencer> = rdfDereferencer as any;
   const webId = 'http://alice.test.com/#me';
   const token = 'randomlyGeneratedToken';
   const tokenTriple = quad(namedNode(webId), SOLID.terms.oidcIssuerRegistrationToken, literal(token));
   const tokenString = `${quadToString(tokenTriple)}.`;
-  const converter = new RdfToQuadConverter();
   let storage: ExpiringStorage<string, string>;
   let validator: TokenOwnershipValidator;
 
-  function mockFetch(body: string): void {
-    fetchMock.mockImplementation((url: string): any => ({
-      text: (): any => body,
-      url,
-      status: 200,
-      headers: { get: (): any => 'text/turtle' },
+  function mockDereference(qq?: Quad): any {
+    rdfDereferenceMock.dereference.mockImplementation((uri: string): any => ({
+      uri,
+      quads: Readable.from(qq ? [ qq ] : []),
+      exists: true,
     }));
   }
 
@@ -50,32 +50,32 @@ describe('A TokenOwnershipValidator', (): void => {
       delete: jest.fn().mockImplementation((key: string): any => map.delete(key)),
     } as any;
 
-    mockFetch('');
+    mockDereference();
 
-    validator = new TokenOwnershipValidator(converter, storage);
+    validator = new TokenOwnershipValidator(storage);
   });
 
   it('errors if no token is stored in the storage.', async(): Promise<void> => {
     // Even if the token is in the WebId, it will error since it's not in the storage
-    mockFetch(tokenString);
+    mockDereference(tokenTriple);
     await expect(validator.handle({ webId })).rejects.toThrow(expect.objectContaining({
       message: expect.stringContaining(tokenString),
       details: { quad: tokenString },
     }));
-    expect(fetch).toHaveBeenCalledTimes(0);
+    expect(rdfDereferenceMock.dereference).toHaveBeenCalledTimes(0);
   });
 
   it('errors if the expected triple is missing.', async(): Promise<void> => {
     // First call will add the token to the storage
     await expect(validator.handle({ webId })).rejects.toThrow(tokenString);
-    expect(fetch).toHaveBeenCalledTimes(0);
+    expect(rdfDereferenceMock.dereference).toHaveBeenCalledTimes(0);
     // Second call will fetch the WebId
     await expect(validator.handle({ webId })).rejects.toThrow(tokenString);
-    expect(fetch).toHaveBeenCalledTimes(1);
+    expect(rdfDereferenceMock.dereference).toHaveBeenCalledTimes(1);
   });
 
   it('resolves if the WebId contains the verification triple.', async(): Promise<void> => {
-    mockFetch(tokenString);
+    mockDereference(tokenTriple);
     // First call will add the token to the storage
     await expect(validator.handle({ webId })).rejects.toThrow(tokenString);
     // Second call will succeed since it has the verification triple
@@ -84,7 +84,7 @@ describe('A TokenOwnershipValidator', (): void => {
 
   it('fails if the WebId contains the wrong verification triple.', async(): Promise<void> => {
     const wrongQuad = quad(namedNode(webId), SOLID.terms.oidcIssuerRegistrationToken, literal('wrongToken'));
-    mockFetch(`${quadToString(wrongQuad)} .`);
+    mockDereference(wrongQuad);
     // First call will add the token to the storage
     await expect(validator.handle({ webId })).rejects.toThrow(tokenString);
     // Second call will fail since it has the wrong verification triple
diff --git a/test/unit/util/FetchUtil.test.ts b/test/unit/util/FetchUtil.test.ts
index d08fa4c22..16e0d9782 100644
--- a/test/unit/util/FetchUtil.test.ts
+++ b/test/unit/util/FetchUtil.test.ts
@@ -1,55 +1,81 @@
+import { Readable } from 'stream';
+import type { Quad } from '@rdfjs/types';
 import arrayifyStream from 'arrayify-stream';
-import { fetch } from 'cross-fetch';
+import type { Response } from 'cross-fetch';
 import { DataFactory } from 'n3';
+import rdfDereferencer from 'rdf-dereference';
 import { RdfToQuadConverter } from '../../../src/storage/conversion/RdfToQuadConverter';
-import { fetchDataset } from '../../../src/util/FetchUtil';
+import { fetchDataset, responseToDataset } from '../../../src/util/FetchUtil';
 const { namedNode, quad } = DataFactory;
 
-jest.mock('cross-fetch');
+jest.mock('rdf-dereference', (): any => ({
+  dereference: jest.fn<string, any>(),
+}));
 
 describe('FetchUtil', (): void => {
+  const url = 'http://test.com/foo';
+
+  function mockResponse(body: string, contentType: string | null, status = 200): Response {
+    return ({
+      text: (): any => body,
+      url,
+      status,
+      headers: { get: (): any => contentType },
+    }) as any;
+  }
+
   describe('#fetchDataset', (): void => {
-    const fetchMock: jest.Mock = fetch as any;
-    const url = 'http://test.com/foo';
-    const converter = new RdfToQuadConverter();
+    const rdfDereferenceMock: jest.Mocked<typeof rdfDereferencer> = rdfDereferencer as any;
 
-    function mockFetch(body: string, status = 200): void {
-      fetchMock.mockImplementation((input: string): any => ({
-        text: (): any => body,
-        url: input,
-        status,
-        headers: { get: (): any => 'text/turtle' },
-      }));
+    function mockDereference(quads?: Quad[]): any {
+      rdfDereferenceMock.dereference.mockImplementation((uri: string): any => {
+        if (!quads) {
+          throw new Error('Throws error because url does not exist');
+        }
+        return {
+          uri,
+          quads: Readable.from(quads),
+          exists: true,
+        };
+      });
     }
 
-    it('errors if the status code is not 200.', async(): Promise<void> => {
-      mockFetch('Invalid URL!', 404);
-      await expect(fetchDataset(url, converter)).rejects.toThrow(`Unable to access data at ${url}`);
-      expect(fetchMock).toHaveBeenCalledWith(url);
-    });
-
-    it('errors if there is no content-type.', async(): Promise<void> => {
-      fetchMock.mockResolvedValueOnce({ url, text: (): any => '', status: 200, headers: { get: jest.fn() }});
-      await expect(fetchDataset(url, converter)).rejects.toThrow(`Unable to access data at ${url}`);
-      expect(fetchMock).toHaveBeenCalledWith(url);
+    it('errors if the URL does not exist.', async(): Promise<void> => {
+      mockDereference();
+      await expect(fetchDataset(url)).rejects.toThrow(`Could not parse resource at URL (${url})!`);
+      expect(rdfDereferenceMock.dereference).toHaveBeenCalledWith(url);
     });
 
     it('returns a Representation with quads.', async(): Promise<void> => {
-      mockFetch('<http://test.com/s> <http://test.com/p> <http://test.com/o>.');
-      const representation = await fetchDataset(url, converter);
+      const quads = [ quad(namedNode('http://test.com/s'), namedNode('http://test.com/p'), namedNode('http://test.com/o')) ];
+      mockDereference(quads);
+      const representation = await fetchDataset(url);
       await expect(arrayifyStream(representation.data)).resolves.toEqual([
         quad(namedNode('http://test.com/s'), namedNode('http://test.com/p'), namedNode('http://test.com/o')),
       ]);
     });
+  });
+
+  describe('#responseToDataset', (): void => {
+    const converter = new RdfToQuadConverter();
 
     it('accepts Response objects as input.', async(): Promise<void> => {
-      mockFetch('<http://test.com/s> <http://test.com/p> <http://test.com/o>.');
-      const response = await fetch(url);
+      const response = mockResponse('<http://test.com/s> <http://test.com/p> <http://test.com/o>.', 'text/turtle');
       const body = await response.text();
-      const representation = await fetchDataset(response, converter, body);
+      const representation = await responseToDataset(response, converter, body);
       await expect(arrayifyStream(representation.data)).resolves.toEqual([
         quad(namedNode('http://test.com/s'), namedNode('http://test.com/p'), namedNode('http://test.com/o')),
       ]);
     });
+
+    it('errors if the status code is not 200.', async(): Promise<void> => {
+      const response = mockResponse('Incorrect status!', null, 400);
+      await expect(responseToDataset(response, converter)).rejects.toThrow(`Unable to access data at ${url}`);
+    });
+
+    it('errors if there is no content-type.', async(): Promise<void> => {
+      const response = mockResponse('No content-type!', null);
+      await expect(responseToDataset(response, converter)).rejects.toThrow(`Unable to access data at ${url}`);
+    });
   });
 });