feat: Return correct status codes for invalid requests

Author: joachimvh

config/ldp/handler/components/operation-handler.json

@@ -28,6 +28,10 @@
         {
           "@type": "PatchOperationHandler",
           "store": { "@id": "urn:solid-server:default:ResourceStore" }
+        },
+        {
+          "@type": "StaticThrowHandler",
+          "error": { "@type": "MethodNotAllowedHttpError" }
         }
       ]
     }

config/ldp/modes/default.json

@@ -6,9 +6,34 @@
       "@id": "urn:solid-server:default:ModesExtractor",
       "@type": "WaterfallHandler",
       "handlers": [
-        { "@type": "MethodModesExtractor" },
-        { "@type": "SparqlPatchModesExtractor" }
+        {
+          "comment": "Extract access modes for PATCH requests based on the request body.",
+          "@id": "urn:solid-server:default:PatchModesExtractor"
+        },
+        {
+          "comment": "Extract access modes based on the HTTP method.",
+          "@type": "MethodModesExtractor"
+        },
+        {
+          "@type": "StaticThrowHandler",
+          "error": { "@type": "MethodNotAllowedHttpError" }
+        }
       ]
+    },
+    {
+      "@id": "urn:solid-server:default:PatchModesExtractor",
+      "@type": "MethodFilterHandler",
+      "methods": [ "PATCH" ],
+      "source": {
+        "@type": "WaterfallHandler",
+        "handlers": [
+          { "@type": "SparqlUpdateModesExtractor" },
+          {
+            "@type": "StaticThrowHandler",
+            "error": { "@type": "UnsupportedMediaTypeHttpError" }
+          }
+        ]
+      }
     }
   ]
 }

config/storage/middleware/stores/patching.json

@@ -22,6 +22,10 @@
               "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
               "intermediateType": "internal/quads",
               "defaultType": "text/turtle"
+            },
+            {
+              "@type": "StaticThrowHandler",
+              "error": { "@type": "UnsupportedMediaTypeHttpError" }
             }
           ]
         }

src/authorization/permissions/ModesExtractor.ts

@@ -2,4 +2,7 @@ import type { Operation } from '../../http/Operation';
 import { AsyncHandler } from '../../util/handlers/AsyncHandler';
 import type { AccessMode } from './Permissions';
 
+/**
+ * Extracts all {@link AccessMode}s that are necessary to execute the given {@link Operation}.
+ */
 export abstract class ModesExtractor extends AsyncHandler<Operation, Set<AccessMode>> {}

src/authorization/permissions/SparqlPatchModesExtractor.ts src/authorization/permissions/SparqlUpdateModesExtractor.ts

@@ -6,11 +6,13 @@ import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpErr
 import { ModesExtractor } from './ModesExtractor';
 import { AccessMode } from './Permissions';
 
-export class SparqlPatchModesExtractor extends ModesExtractor {
-  public async canHandle({ method, body }: Operation): Promise<void> {
-    if (method !== 'PATCH') {
-      throw new NotImplementedHttpError(`Cannot determine permissions of ${method}, only PATCH.`);
-    }
+/**
+ * Generates permissions for a SPARQL DELETE/INSERT body.
+ * Updates with only an INSERT can be done with just append permissions,
+ * while DELETEs require write permissions as well.
+ */
+export class SparqlUpdateModesExtractor extends ModesExtractor {
+  public async canHandle({ body }: Operation): Promise<void> {
     if (!this.isSparql(body)) {
       throw new NotImplementedHttpError('Cannot determine permissions of non-SPARQL patches.');
     }

src/index.ts

@@ -18,7 +18,7 @@ export * from './authorization/access/AgentGroupAccessChecker';
 export * from './authorization/permissions/Permissions';
 export * from './authorization/permissions/ModesExtractor';
 export * from './authorization/permissions/MethodModesExtractor';
-export * from './authorization/permissions/SparqlPatchModesExtractor';
+export * from './authorization/permissions/SparqlUpdateModesExtractor';
 
 // Authorization
 export * from './authorization/AllStaticReader';
@@ -359,9 +359,12 @@ export * from './util/errors/UnsupportedMediaTypeHttpError';
 export * from './util/handlers/AsyncHandler';
 export * from './util/handlers/BooleanHandler';
 export * from './util/handlers/ConditionalHandler';
+export * from './util/handlers/HandlerUtil';
+export * from './util/handlers/MethodFilterHandler';
 export * from './util/handlers/ParallelHandler';
 export * from './util/handlers/SequenceHandler';
 export * from './util/handlers/StaticHandler';
+export * from './util/handlers/StaticThrowHandler';
 export * from './util/handlers/UnionHandler';
 export * from './util/handlers/UnsupportedAsyncHandler';
 export * from './util/handlers/WaterfallHandler';

src/util/handlers/MethodFilterHandler.ts

@@ -0,0 +1,30 @@
+import { NotImplementedHttpError } from '../errors/NotImplementedHttpError';
+import { AsyncHandler } from './AsyncHandler';
+
+/**
+ * Only accepts requests where the input has a `method` field that matches any one of the given methods.
+ * In case of a match, the input will be sent to the source handler.
+ */
+export class MethodFilterHandler<TIn extends { method: string }, TOut> extends AsyncHandler<TIn, TOut> {
+  private readonly methods: string[];
+  private readonly source: AsyncHandler<TIn, TOut>;
+
+  public constructor(methods: string[], source: AsyncHandler<TIn, TOut>) {
+    super();
+    this.methods = methods;
+    this.source = source;
+  }
+
+  public async canHandle(input: TIn): Promise<void> {
+    if (!this.methods.includes(input.method)) {
+      throw new NotImplementedHttpError(
+        `Cannot determine permissions of ${input.method}, only ${this.methods.join(',')}.`,
+      );
+    }
+    await this.source.canHandle(input);
+  }
+
+  public async handle(input: TIn): Promise<TOut> {
+    return this.source.handle(input);
+  }
+}

src/util/handlers/StaticThrowHandler.ts

@@ -0,0 +1,18 @@
+import type { HttpError } from '../errors/HttpError';
+import { AsyncHandler } from './AsyncHandler';
+
+/**
+ * Utility handler that can handle all input and always throws the given error.
+ */
+export class StaticThrowHandler extends AsyncHandler<any, never> {
+  private readonly error: HttpError;
+
+  public constructor(error: HttpError) {
+    super();
+    this.error = error;
+  }
+
+  public async handle(): Promise<never> {
+    throw this.error;
+  }
+}

test/integration/LdpHandlerWithoutAuth.test.ts

@@ -386,4 +386,14 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
     // DELETE
     expect(await deleteResource(documentUrl)).toBeUndefined();
   });
+
+  it('returns 405 for unsupported methods.', async(): Promise<void> => {
+    const response = await fetch(baseUrl, { method: 'TRACE' });
+    expect(response.status).toBe(405);
+  });
+
+  it('returns 415 for unsupported PATCH types.', async(): Promise<void> => {
+    const response = await fetch(baseUrl, { method: 'PATCH', headers: { 'content-type': 'text/plain' }, body: 'abc' });
+    expect(response.status).toBe(415);
+  });
 });

test/unit/authorization/permissions/SparqlPatchModesExtractor.test.ts test/unit/authorization/permissions/SparqlUpdateModesExtractor.test.ts

@@ -1,25 +1,21 @@
 import { Factory } from 'sparqlalgebrajs';
 import { AccessMode } from '../../../../src/authorization/permissions/Permissions';
-import { SparqlPatchModesExtractor } from '../../../../src/authorization/permissions/SparqlPatchModesExtractor';
+import { SparqlUpdateModesExtractor } from '../../../../src/authorization/permissions/SparqlUpdateModesExtractor';
 import type { Operation } from '../../../../src/http/Operation';
 import type { SparqlUpdatePatch } from '../../../../src/http/representation/SparqlUpdatePatch';
 import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
 
-describe('A SparqlPatchModesExtractor', (): void => {
-  const extractor = new SparqlPatchModesExtractor();
+describe('A SparqlUpdateModesExtractor', (): void => {
+  const extractor = new SparqlUpdateModesExtractor();
   const factory = new Factory();
 
-  it('can only handle (composite) SPARQL DELETE/INSERT PATCH operations.', async(): Promise<void> => {
+  it('can only handle (composite) SPARQL DELETE/INSERT operations.', async(): Promise<void> => {
     const operation = { method: 'PATCH', body: { algebra: factory.createDeleteInsert() }} as unknown as Operation;
     await expect(extractor.canHandle(operation)).resolves.toBeUndefined();
     (operation.body as SparqlUpdatePatch).algebra = factory.createCompositeUpdate([ factory.createDeleteInsert() ]);
     await expect(extractor.canHandle(operation)).resolves.toBeUndefined();
 
-    let result = extractor.canHandle({ ...operation, method: 'GET' });
-    await expect(result).rejects.toThrow(NotImplementedHttpError);
-    await expect(result).rejects.toThrow('Cannot determine permissions of GET, only PATCH.');
-
-    result = extractor.canHandle({ ...operation, body: {} as SparqlUpdatePatch });
+    let result = extractor.canHandle({ ...operation, body: {} as SparqlUpdatePatch });
     await expect(result).rejects.toThrow(NotImplementedHttpError);
     await expect(result).rejects.toThrow('Cannot determine permissions of non-SPARQL patches.');
 

test/unit/util/handlers/MethodFilterHandler.test.ts

@@ -0,0 +1,52 @@
+import type { Operation } from '../../../../src/http/Operation';
+import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation';
+import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
+import type { AsyncHandler } from '../../../../src/util/handlers/AsyncHandler';
+import {
+  MethodFilterHandler,
+} from '../../../../src/util/handlers/MethodFilterHandler';
+
+describe('A MethodFilterHandler', (): void => {
+  const modes = [ 'PATCH', 'POST' ];
+  const result = 'RESULT';
+  let operation: Operation;
+  let source: jest.Mocked<AsyncHandler<Operation, string>>;
+  let handler: MethodFilterHandler<Operation, string>;
+
+  beforeEach(async(): Promise<void> => {
+    operation = {
+      method: 'PATCH',
+      preferences: {},
+      permissionSet: {},
+      target: { path: 'http://example.com/foo' },
+      body: new BasicRepresentation(),
+    };
+
+    source = {
+      canHandle: jest.fn(),
+      handle: jest.fn().mockResolvedValue(result),
+    } as any;
+
+    handler = new MethodFilterHandler(modes, source);
+  });
+
+  it('rejects unknown methods.', async(): Promise<void> => {
+    operation.method = 'GET';
+    await expect(handler.canHandle(operation)).rejects.toThrow(NotImplementedHttpError);
+  });
+
+  it('checks if the source handle supports the request.', async(): Promise<void> => {
+    operation.method = 'PATCH';
+    await expect(handler.canHandle(operation)).resolves.toBeUndefined();
+    operation.method = 'POST';
+    await expect(handler.canHandle(operation)).resolves.toBeUndefined();
+    source.canHandle.mockRejectedValueOnce(new Error('not supported'));
+    await expect(handler.canHandle(operation)).rejects.toThrow('not supported');
+    expect(source.canHandle).toHaveBeenLastCalledWith(operation);
+  });
+
+  it('calls the source extractor.', async(): Promise<void> => {
+    await expect(handler.handle(operation)).resolves.toBe(result);
+    expect(source.handle).toHaveBeenLastCalledWith(operation);
+  });
+});

test/unit/util/handlers/StaticThrowHandler.test.ts

@@ -0,0 +1,15 @@
+import { BadRequestHttpError } from '../../../../src/util/errors/BadRequestHttpError';
+import { StaticThrowHandler } from '../../../../src/util/handlers/StaticThrowHandler';
+
+describe('A StaticThrowHandler', (): void => {
+  const error = new BadRequestHttpError();
+  const handler = new StaticThrowHandler(error);
+
+  it('can handle all requests.', async(): Promise<void> => {
+    await expect(handler.canHandle({})).resolves.toBeUndefined();
+  });
+
+  it('always throws the given error.', async(): Promise<void> => {
+    await expect(handler.handle()).rejects.toThrow(error);
+  });
+});