From 5acddcb5b204fe91c0ed2f4b24c0241e851afd3a Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 1 Feb 2023 15:34:53 +0100 Subject: [PATCH 01/20] docs: Add Zenodo badge in README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index fdbdaa7b6..5154d32e5 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ [![Node.js version](https://img.shields.io/node/v/@solid/community-server)](https://www.npmjs.com/package/@solid/community-server) [![Build Status](https://github.com/CommunitySolidServer/CommunitySolidServer/workflows/CI/badge.svg)](https://github.com/CommunitySolidServer/CommunitySolidServer/actions) [![Coverage Status](https://coveralls.io/repos/github/CommunitySolidServer/CommunitySolidServer/badge.svg)](https://coveralls.io/github/CommunitySolidServer/CommunitySolidServer) +[![DOI](https://zenodo.org/badge/265197208.svg)](https://zenodo.org/badge/latestdoi/265197208) [![GitHub discussions](https://img.shields.io/github/discussions/CommunitySolidServer/CommunitySolidServer)](https://github.com/CommunitySolidServer/CommunitySolidServer/discussions) [![Chat on Gitter](https://badges.gitter.im/CommunitySolidServer/community.svg)](https://gitter.im/CommunitySolidServer/community) From 2efc141baa11f4f2a0063c189843583a05cc4012 Mon Sep 17 00:00:00 2001 From: Pasini Luca Date: Wed, 1 Feb 2023 18:50:33 +0100 Subject: [PATCH 02/20] feat: SeededPodInitializer log exceptions as warning instead of crashing --- src/init/SeededPodInitializer.ts | 12 +++++++++--- test/unit/init/SeededPodInitializer.test.ts | 7 +++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/init/SeededPodInitializer.ts b/src/init/SeededPodInitializer.ts index ec9f11940..1e76fb292 100644 --- a/src/init/SeededPodInitializer.ts +++ b/src/init/SeededPodInitializer.ts @@ -1,6 +1,7 @@ import { readJson } from 'fs-extra'; import type { RegistrationManager } from '../identity/interaction/email-password/util/RegistrationManager'; import { getLoggerFor } from '../logging/LogUtil'; +import { createErrorMessage } from '../util/errors/ErrorUtil'; import { Initializer } from './Initializer'; /** @@ -42,10 +43,15 @@ export class SeededPodInitializer extends Initializer { this.logger.debug(`Validated input: ${JSON.stringify(validated)}`); // Register and/or create a pod as requested. Potentially does nothing if all booleans are false. - await this.registrationManager.register(validated, true); - this.logger.info(`Initialized seeded pod and account for "${input.podName}".`); - count += 1; + try { + await this.registrationManager.register(validated, true); + this.logger.info(`Initialized seeded pod and account for "${input.podName}".`); + count += 1; + } catch (error: unknown) { + this.logger.warn(`Error while initializing seeded pod: ${createErrorMessage(error)})}`); + } } + this.logger.info(`Initialized ${count} seeded pods.`); } } diff --git a/test/unit/init/SeededPodInitializer.test.ts b/test/unit/init/SeededPodInitializer.test.ts index d4b8647d4..142f88655 100644 --- a/test/unit/init/SeededPodInitializer.test.ts +++ b/test/unit/init/SeededPodInitializer.test.ts @@ -45,4 +45,11 @@ describe('A SeededPodInitializer', (): void => { expect(registrationManager.validateInput).toHaveBeenCalledTimes(2); expect(registrationManager.register).toHaveBeenCalledTimes(2); }); + + it('does not throw exceptions when a seeded pod already exists.', async(): Promise => { + registrationManager.register = jest.fn().mockRejectedValueOnce(new Error('Pod already exists')); + await new SeededPodInitializer(registrationManager, configFilePath).handle(); + expect(registrationManager.validateInput).toHaveBeenCalledTimes(2); + expect(registrationManager.register).toHaveBeenCalledTimes(2); + }); }); From 3bb8001c72fa5b0ce58e8f42552a37a825e92a66 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Fri, 3 Feb 2023 11:32:07 +0100 Subject: [PATCH 03/20] docs: Reference correct configuration entry --- config/app/README.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/config/app/README.md b/config/app/README.md index 20da89aa8..e3978a41c 100644 --- a/config/app/README.md +++ b/config/app/README.md @@ -2,13 +2,6 @@ Options related to the server startup. -## Base - -This is the entry point to the main server setup. - -* *default*: The main application. This should only be changed/replaced - if you want to start from a different kind of class. - ## Init Contains a list of initializer that need to be run when starting the server. @@ -18,6 +11,13 @@ Contains a list of initializer that need to be run when starting the server. This is only relevant if setup is disabled but root container access is still required. * *initialize-prefilled-root*: Similar to `initialize-root` but adds some introductory resources to the root container. +## Main + +This is the entry point to the main server setup. + +* *default*: The main application. This should only be changed/replaced + if you want to start from a different kind of class. + ## Setup Handles the setup page the first time the server is started. From 948dad7bcd2990c128da4ce45f51018f8eb0b977 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 3 Feb 2023 10:46:11 +0000 Subject: [PATCH 04/20] chore(deps): bump http-cache-semantics from 4.1.0 to 4.1.1 Bumps [http-cache-semantics](https://github.com/kornelski/http-cache-semantics) from 4.1.0 to 4.1.1. - [Release notes](https://github.com/kornelski/http-cache-semantics/releases) - [Commits](https://github.com/kornelski/http-cache-semantics/compare/v4.1.0...v4.1.1) --- updated-dependencies: - dependency-name: http-cache-semantics dependency-type: indirect ... Signed-off-by: dependabot[bot] --- package-lock.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index ef38c3c67..690b9d33f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9394,9 +9394,9 @@ } }, "node_modules/http-cache-semantics": { - "version": "4.1.0", - "resolved": "https://registry.npmjs.org/http-cache-semantics/-/http-cache-semantics-4.1.0.tgz", - "integrity": "sha512-carPklcUh7ROWRK7Cv27RPtdhYhUsela/ue5/jKzjegVvXDqM2ILE9Q2BGn9JZJh1g87cp56su/FgQSzcWS8cQ==" + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/http-cache-semantics/-/http-cache-semantics-4.1.1.tgz", + "integrity": "sha512-er295DKPVsV82j5kw1Gjt+ADA/XYHsajl82cGNQG2eyoPkvgUhX+nDIyelzhIWbbsXP39EHcI6l5tYs2FYqYXQ==" }, "node_modules/http-errors": { "version": "1.8.1", @@ -22783,9 +22783,9 @@ } }, "http-cache-semantics": { - "version": "4.1.0", - "resolved": "https://registry.npmjs.org/http-cache-semantics/-/http-cache-semantics-4.1.0.tgz", - "integrity": "sha512-carPklcUh7ROWRK7Cv27RPtdhYhUsela/ue5/jKzjegVvXDqM2ILE9Q2BGn9JZJh1g87cp56su/FgQSzcWS8cQ==" + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/http-cache-semantics/-/http-cache-semantics-4.1.1.tgz", + "integrity": "sha512-er295DKPVsV82j5kw1Gjt+ADA/XYHsajl82cGNQG2eyoPkvgUhX+nDIyelzhIWbbsXP39EHcI6l5tYs2FYqYXQ==" }, "http-errors": { "version": "1.8.1", From c332412074d3969420348687e8323fffaec2f882 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Fri, 10 Feb 2023 10:13:53 +0100 Subject: [PATCH 05/20] feat: Provide clear error message for unknown clients * feat: Provide clear error message for unknown clients * docs: Rephrase error message. * docs: Update error message to reference local storage --------- Co-authored-by: Ruben Verborgh --- .../configuration/IdentityProviderFactory.ts | 17 +++++++++++ templates/error/descriptions/E0003.md.hbs | 26 ++++++++++++++++ .../IdentityProviderFactory.test.ts | 30 +++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100644 templates/error/descriptions/E0003.md.hbs diff --git a/src/identity/configuration/IdentityProviderFactory.ts b/src/identity/configuration/IdentityProviderFactory.ts index 10bf782ad..ea442ab21 100644 --- a/src/identity/configuration/IdentityProviderFactory.ts +++ b/src/identity/configuration/IdentityProviderFactory.ts @@ -19,6 +19,7 @@ import type { ResponseWriter } from '../../http/output/ResponseWriter'; import { BasicRepresentation } from '../../http/representation/BasicRepresentation'; import { getLoggerFor } from '../../logging/LogUtil'; import type { KeyValueStorage } from '../../storage/keyvalue/KeyValueStorage'; +import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError'; import { InternalServerError } from '../../util/errors/InternalServerError'; import { RedirectHttpError } from '../../util/errors/RedirectHttpError'; import { guardStream } from '../../util/GuardedStream'; @@ -398,6 +399,22 @@ export class IdentityProviderFactory implements ProviderFactory { } } + // A client not being found is quite often the result of cookies being stored by the authn client, + // so we want to provide a more detailed error message explaining what to do. + if (oidcError.error_description === 'client is invalid' && oidcError.error_detail === 'client not found') { + const unknownClientError = new BadRequestHttpError( + 'Unknown client, you might need to clear the local storage on the client.', { + errorCode: 'E0003', + details: { + client_id: ctx.request.query.client_id, + redirect_uri: ctx.request.query.redirect_uri, + }, + }, + ); + unknownClientError.stack = error.stack; + error = unknownClientError; + } + const result = await this.errorHandler.handleSafe({ error, request: guardStream(ctx.req) }); await this.responseWriter.handleSafe({ response: ctx.res, result }); }; diff --git a/templates/error/descriptions/E0003.md.hbs b/templates/error/descriptions/E0003.md.hbs new file mode 100644 index 000000000..1bd8513e3 --- /dev/null +++ b/templates/error/descriptions/E0003.md.hbs @@ -0,0 +1,26 @@ +# Authenticating with unknown client +You are trying to log in to an application, +but we can't proceed +because the app is using invalid settings. + +To force the app to send us the right details, +delete the local storage in your browser for the site that sent you here. +Based on the data the app sent us, +this is probably `{{ redirect_uri }}`. + +## Detailed error information +We received a request from a client with ID `{{ client_id }}`, +but this client is not registered with the server. + +Probably, +this client was registered with the server in the past, +but it is no longer recognized +because some internal server data was removed. +Your data is still safe; +we just don't recognize the app's previous authentication anymore. + +Because your browser still has the old authentication settings stored, +it tries to use them instead of setting up new ones. +By clearing those settings, +the application should automatically create a new client, +allowing you to log in again. diff --git a/test/unit/identity/configuration/IdentityProviderFactory.test.ts b/test/unit/identity/configuration/IdentityProviderFactory.test.ts index 05701709d..7adf696e7 100644 --- a/test/unit/identity/configuration/IdentityProviderFactory.test.ts +++ b/test/unit/identity/configuration/IdentityProviderFactory.test.ts @@ -59,6 +59,10 @@ describe('An IdentityProviderFactory', (): void => { res: {}, request: { href: 'http://example.com/idp/', + query: { + client_id: 'CLIENT_ID', + redirect_uri: 'REDIRECT_URI', + }, }, accepts: jest.fn().mockReturnValue('type'), } as any; @@ -235,6 +239,32 @@ describe('An IdentityProviderFactory', (): void => { expect(error.stack).toContain('Error: bad data - more info - more details'); }); + it('throws a specific error for unknown clients.', async(): Promise => { + const provider = await factory.getProvider() as any; + const { config } = provider as { config: Configuration }; + + const error = new Error('invalid_client') as errors.OIDCProviderError; + error.error_description = 'client is invalid'; + error.error_detail = 'client not found'; + + await expect((config.renderError as any)(ctx, {}, error)).resolves.toBeUndefined(); + expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1); + expect(errorHandler.handleSafe) + .toHaveBeenLastCalledWith({ error: expect.objectContaining({ + statusCode: 400, + name: 'BadRequestHttpError', + message: 'Unknown client, you might need to clear the local storage on the client.', + errorCode: 'E0003', + details: { + client_id: 'CLIENT_ID', + redirect_uri: 'REDIRECT_URI', + }, + }), + request: ctx.req }); + expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1); + expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response: ctx.res, result: { statusCode: 500 }}); + }); + it('adds middleware to make the OIDC provider think the request wants HTML.', async(): Promise => { const provider = await factory.getProvider() as any; const use = provider.use as jest.Mock, Parameters>; From bb7427842c0f360852e33863972f14bc72d7502a Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Thu, 9 Feb 2023 16:39:46 +0100 Subject: [PATCH 06/20] fix: Do not add port 80 to default base URL --- src/init/variables/extractors/BaseUrlExtractor.ts | 4 +++- test/unit/init/variables/extractors/BaseUrlExtractor.test.ts | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/init/variables/extractors/BaseUrlExtractor.ts b/src/init/variables/extractors/BaseUrlExtractor.ts index 939e0514b..665938df5 100644 --- a/src/init/variables/extractors/BaseUrlExtractor.ts +++ b/src/init/variables/extractors/BaseUrlExtractor.ts @@ -22,6 +22,8 @@ export class BaseUrlExtractor extends ShorthandExtractor { throw new Error('BaseUrl argument should be provided when using Unix Domain Sockets.'); } const port = args.port ?? this.defaultPort; - return `http://localhost:${port}/`; + const url = new URL('http://localhost/'); + url.port = `${port}`; + return url.href; } } diff --git a/test/unit/init/variables/extractors/BaseUrlExtractor.test.ts b/test/unit/init/variables/extractors/BaseUrlExtractor.test.ts index 55e4281fc..8e31120aa 100644 --- a/test/unit/init/variables/extractors/BaseUrlExtractor.test.ts +++ b/test/unit/init/variables/extractors/BaseUrlExtractor.test.ts @@ -24,4 +24,8 @@ describe('A BaseUrlExtractor', (): void => { it('defaults to port 3000.', async(): Promise => { await expect(computer.handle({})).resolves.toBe('http://localhost:3000/'); }); + + it('does not add the port if it is 80.', async(): Promise => { + await expect(computer.handle({ port: 80 })).resolves.toBe('http://localhost/'); + }); }); From 7eb938044d30290d90dcfc9dcd78149989f6aa2c Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Mon, 27 Feb 2023 11:13:30 +0100 Subject: [PATCH 07/20] chore: Update caniuse-lite dependency --- package-lock.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 690b9d33f..5a9b6ad60 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5548,9 +5548,9 @@ } }, "node_modules/caniuse-lite": { - "version": "1.0.30001374", - "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001374.tgz", - "integrity": "sha512-mWvzatRx3w+j5wx/mpFN5v5twlPrabG8NqX2c6e45LCpymdoGqNvRkRutFUqpRTXKFQFNQJasvK0YT7suW6/Hw==", + "version": "1.0.30001458", + "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001458.tgz", + "integrity": "sha512-lQ1VlUUq5q9ro9X+5gOEyH7i3vm+AYVT1WDCVB69XOZ17KZRhnZ9J0Sqz7wTHQaLBJccNCHq8/Ww5LlOIZbB0w==", "dev": true, "funding": [ { @@ -19857,9 +19857,9 @@ } }, "caniuse-lite": { - "version": "1.0.30001374", - "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001374.tgz", - "integrity": "sha512-mWvzatRx3w+j5wx/mpFN5v5twlPrabG8NqX2c6e45LCpymdoGqNvRkRutFUqpRTXKFQFNQJasvK0YT7suW6/Hw==", + "version": "1.0.30001458", + "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001458.tgz", + "integrity": "sha512-lQ1VlUUq5q9ro9X+5gOEyH7i3vm+AYVT1WDCVB69XOZ17KZRhnZ9J0Sqz7wTHQaLBJccNCHq8/Ww5LlOIZbB0w==", "dev": true }, "canonicalize": { From 63fd062f16d2e88e86c1aa44733c3daddef2bb23 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Mon, 6 Mar 2023 16:01:07 +0100 Subject: [PATCH 08/20] fix: Output required OAuth error fields --- .../configuration/IdentityProviderFactory.ts | 27 +++++++++----- src/index.ts | 1 + .../conversion/ErrorToJsonConverter.ts | 6 ++++ src/util/errors/OAuthHttpError.ts | 36 +++++++++++++++++++ test/integration/Identity.test.ts | 1 + .../IdentityProviderFactory.test.ts | 9 +++-- .../conversion/ErrorToJsonConverter.test.ts | 31 ++++++++++++++++ test/unit/util/errors/OAuthHttpError.test.ts | 24 +++++++++++++ 8 files changed, 124 insertions(+), 11 deletions(-) create mode 100644 src/util/errors/OAuthHttpError.ts create mode 100644 test/unit/util/errors/OAuthHttpError.test.ts diff --git a/src/identity/configuration/IdentityProviderFactory.ts b/src/identity/configuration/IdentityProviderFactory.ts index ea442ab21..9395f94ea 100644 --- a/src/identity/configuration/IdentityProviderFactory.ts +++ b/src/identity/configuration/IdentityProviderFactory.ts @@ -20,7 +20,9 @@ import { BasicRepresentation } from '../../http/representation/BasicRepresentati import { getLoggerFor } from '../../logging/LogUtil'; import type { KeyValueStorage } from '../../storage/keyvalue/KeyValueStorage'; import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError'; +import type { HttpError } from '../../util/errors/HttpError'; import { InternalServerError } from '../../util/errors/InternalServerError'; +import { OAuthHttpError } from '../../util/errors/OAuthHttpError'; import { RedirectHttpError } from '../../util/errors/RedirectHttpError'; import { guardStream } from '../../util/GuardedStream'; import { joinUrl } from '../../util/PathUtil'; @@ -379,7 +381,8 @@ export class IdentityProviderFactory implements ProviderFactory { // Doesn't really matter which type it is since all relevant fields are optional const oidcError = error as errors.OIDCProviderError; - let detailedError = error.message; + // Create a more detailed error message for logging and to show is `showStackTrace` is enabled. + let detailedError = oidcError.message; if (oidcError.error_description) { detailedError += ` - ${oidcError.error_description}`; } @@ -389,13 +392,21 @@ export class IdentityProviderFactory implements ProviderFactory { this.logger.warn(`OIDC request failed: ${detailedError}`); - // OIDC library hides extra details in these fields + // Convert to our own error object. + // This ensures serializing the error object will generate the correct output later on. + // We specifically copy the fields instead of passing the object to contain the `oidc-provider` dependency + // to the current file. + let resultingError: HttpError = new OAuthHttpError(out, oidcError.name, oidcError.statusCode, oidcError.message); + // Keep the original stack to make debugging easier + resultingError.stack = oidcError.stack; + if (this.showStackTrace) { - error.message = detailedError; + // Expose more information if `showStackTrace` is enabled + resultingError.message = detailedError; // Also change the error message in the stack trace - if (error.stack) { - error.stack = error.stack.replace(/.*/u, `${error.name}: ${error.message}`); + if (resultingError.stack) { + resultingError.stack = resultingError.stack.replace(/.*/u, `${oidcError.name}: ${oidcError.message}`); } } @@ -411,11 +422,11 @@ export class IdentityProviderFactory implements ProviderFactory { }, }, ); - unknownClientError.stack = error.stack; - error = unknownClientError; + unknownClientError.stack = oidcError.stack; + resultingError = unknownClientError; } - const result = await this.errorHandler.handleSafe({ error, request: guardStream(ctx.req) }); + const result = await this.errorHandler.handleSafe({ error: resultingError, request: guardStream(ctx.req) }); await this.responseWriter.handleSafe({ response: ctx.res, result }); }; } diff --git a/src/index.ts b/src/index.ts index d38f5c906..2cc35642d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -406,6 +406,7 @@ export * from './util/errors/MethodNotAllowedHttpError'; export * from './util/errors/MovedPermanentlyHttpError'; export * from './util/errors/NotFoundHttpError'; export * from './util/errors/NotImplementedHttpError'; +export * from './util/errors/OAuthHttpError'; export * from './util/errors/PreconditionFailedHttpError'; export * from './util/errors/RedirectHttpError'; export * from './util/errors/SystemError'; diff --git a/src/storage/conversion/ErrorToJsonConverter.ts b/src/storage/conversion/ErrorToJsonConverter.ts index 2f33c44f8..986a13888 100644 --- a/src/storage/conversion/ErrorToJsonConverter.ts +++ b/src/storage/conversion/ErrorToJsonConverter.ts @@ -2,6 +2,7 @@ import { BasicRepresentation } from '../../http/representation/BasicRepresentati import type { Representation } from '../../http/representation/Representation'; import { APPLICATION_JSON, INTERNAL_ERROR } from '../../util/ContentTypes'; import { HttpError } from '../../util/errors/HttpError'; +import { OAuthHttpError } from '../../util/errors/OAuthHttpError'; import { getSingleItem } from '../../util/StreamUtil'; import { BaseTypedRepresentationConverter } from './BaseTypedRepresentationConverter'; import type { RepresentationConverterArgs } from './RepresentationConverter'; @@ -22,6 +23,11 @@ export class ErrorToJsonConverter extends BaseTypedRepresentationConverter { message: error.message, }; + // OAuth errors responses require additional fields + if (OAuthHttpError.isInstance(error)) { + Object.assign(result, error.mandatoryFields); + } + if (HttpError.isInstance(error)) { result.statusCode = error.statusCode; result.errorCode = error.errorCode; diff --git a/src/util/errors/OAuthHttpError.ts b/src/util/errors/OAuthHttpError.ts new file mode 100644 index 000000000..715f1af3e --- /dev/null +++ b/src/util/errors/OAuthHttpError.ts @@ -0,0 +1,36 @@ +import type { HttpErrorOptions } from './HttpError'; +import { HttpError } from './HttpError'; + +/** + * These are the fields that can occur in an OAuth error response as described in RFC 6749, §4.1.2.1. + * https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + * + * This interface is identical to the ErrorOut interface of the `oidc-provider` library, + * but having our own version reduces the part of the codebase that is dependent on that library. + */ +export interface OAuthErrorFields { + error: string; + // eslint-disable-next-line @typescript-eslint/naming-convention + error_description?: string | undefined; + scope?: string | undefined; + state?: string | undefined; +} + +/** + * Represents on OAuth error that is being thrown. + * OAuth error responses have additional fields that need to be present in the JSON response, + * as described in RFC 6749, §4.1.2.1. + */ +export class OAuthHttpError extends HttpError { + public readonly mandatoryFields: OAuthErrorFields; + + public constructor(mandatoryFields: OAuthErrorFields, name?: string, statusCode?: number, message?: string, + options?: HttpErrorOptions) { + super(statusCode ?? 500, name ?? 'OAuthHttpError', message, options); + this.mandatoryFields = mandatoryFields; + } + + public static isInstance(error: unknown): error is OAuthHttpError { + return HttpError.isInstance(error) && Boolean((error as OAuthHttpError).mandatoryFields); + } +} diff --git a/test/integration/Identity.test.ts b/test/integration/Identity.test.ts index d1c3ee3c8..c8302de76 100644 --- a/test/integration/Identity.test.ts +++ b/test/integration/Identity.test.ts @@ -628,6 +628,7 @@ describe('A Solid server with IDP', (): void => { expect(json.message).toBe(`invalid_request - unrecognized route or not allowed method (GET on /.oidc/foo)`); expect(json.statusCode).toBe(404); expect(json.stack).toBeDefined(); + expect(json.error).toBe('invalid_request'); }); }); }); diff --git a/test/unit/identity/configuration/IdentityProviderFactory.test.ts b/test/unit/identity/configuration/IdentityProviderFactory.test.ts index 7adf696e7..9e0292a9b 100644 --- a/test/unit/identity/configuration/IdentityProviderFactory.test.ts +++ b/test/unit/identity/configuration/IdentityProviderFactory.test.ts @@ -12,6 +12,7 @@ import type { Interaction, InteractionHandler } from '../../../../src/identity/i import type { AdapterFactory } from '../../../../src/identity/storage/AdapterFactory'; import type { KeyValueStorage } from '../../../../src/storage/keyvalue/KeyValueStorage'; import { FoundHttpError } from '../../../../src/util/errors/FoundHttpError'; +import { OAuthHttpError } from '../../../../src/util/errors/OAuthHttpError'; /* eslint-disable @typescript-eslint/naming-convention */ jest.mock('oidc-provider', (): any => ({ @@ -229,14 +230,16 @@ describe('An IdentityProviderFactory', (): void => { error.error_description = 'more info'; error.error_detail = 'more details'; + const oAuthError = new OAuthHttpError(error, error.name, 500, 'bad data - more info - more details'); + await expect((config.renderError as any)(ctx, {}, error)).resolves.toBeUndefined(); expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1); expect(errorHandler.handleSafe) - .toHaveBeenLastCalledWith({ error, request: ctx.req }); + .toHaveBeenLastCalledWith({ error: oAuthError, request: ctx.req }); expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1); expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response: ctx.res, result: { statusCode: 500 }}); - expect(error.message).toBe('bad data - more info - more details'); - expect(error.stack).toContain('Error: bad data - more info - more details'); + expect(oAuthError.message).toBe('bad data - more info - more details'); + expect(oAuthError.stack).toContain('Error: bad data - more info - more details'); }); it('throws a specific error for unknown clients.', async(): Promise => { diff --git a/test/unit/storage/conversion/ErrorToJsonConverter.test.ts b/test/unit/storage/conversion/ErrorToJsonConverter.test.ts index 1b9ca1f01..018d0398d 100644 --- a/test/unit/storage/conversion/ErrorToJsonConverter.test.ts +++ b/test/unit/storage/conversion/ErrorToJsonConverter.test.ts @@ -1,6 +1,8 @@ import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation'; import { ErrorToJsonConverter } from '../../../../src/storage/conversion/ErrorToJsonConverter'; import { BadRequestHttpError } from '../../../../src/util/errors/BadRequestHttpError'; +import type { OAuthErrorFields } from '../../../../src/util/errors/OAuthHttpError'; +import { OAuthHttpError } from '../../../../src/util/errors/OAuthHttpError'; import { readJsonStream } from '../../../../src/util/StreamUtil'; describe('An ErrorToJsonConverter', (): void => { @@ -47,6 +49,35 @@ describe('An ErrorToJsonConverter', (): void => { }); }); + it('adds OAuth fields if present.', async(): Promise => { + const out: OAuthErrorFields = { + error: 'error', + // eslint-disable-next-line @typescript-eslint/naming-convention + error_description: 'error_description', + scope: 'scope', + state: 'state', + }; + const error = new OAuthHttpError(out, 'InvalidRequest', 400, 'error text'); + const representation = new BasicRepresentation([ error ], 'internal/error', false); + const prom = converter.handle({ identifier, representation, preferences }); + await expect(prom).resolves.toBeDefined(); + const result = await prom; + expect(result.binary).toBe(true); + expect(result.metadata.contentType).toBe('application/json'); + await expect(readJsonStream(result.data)).resolves.toEqual({ + name: 'InvalidRequest', + message: 'error text', + statusCode: 400, + errorCode: 'H400', + stack: error.stack, + error: 'error', + // eslint-disable-next-line @typescript-eslint/naming-convention + error_description: 'error_description', + scope: 'scope', + state: 'state', + }); + }); + it('does not copy the details if they are not serializable.', async(): Promise => { const error = new BadRequestHttpError('error text', { details: { object: BigInt(1) }}); const representation = new BasicRepresentation([ error ], 'internal/error', false); diff --git a/test/unit/util/errors/OAuthHttpError.test.ts b/test/unit/util/errors/OAuthHttpError.test.ts new file mode 100644 index 000000000..629cdb0e1 --- /dev/null +++ b/test/unit/util/errors/OAuthHttpError.test.ts @@ -0,0 +1,24 @@ +import { OAuthHttpError } from '../../../../src/util/errors/OAuthHttpError'; + +describe('An OAuthHttpError', (): void => { + it('contains relevant information.', async(): Promise => { + const error = new OAuthHttpError({ error: 'error!' }, 'InvalidRequest', 400, 'message!'); + expect(error.mandatoryFields.error).toBe('error!'); + expect(error.name).toBe('InvalidRequest'); + expect(error.statusCode).toBe(400); + expect(error.message).toBe('message!'); + }); + + it('has optional fields.', async(): Promise => { + const error = new OAuthHttpError({ error: 'error!' }); + expect(error.mandatoryFields.error).toBe('error!'); + expect(error.name).toBe('OAuthHttpError'); + expect(error.statusCode).toBe(500); + }); + + it('can identify OAuth errors.', async(): Promise => { + const error = new OAuthHttpError({ error: 'error!' }); + expect(OAuthHttpError.isInstance('apple')).toBe(false); + expect(OAuthHttpError.isInstance(error)).toBe(true); + }); +}); From b6faed0db3cb2b6282acdafb64e1225c89e66842 Mon Sep 17 00:00:00 2001 From: zg009 <103973669+zg009@users.noreply.github.com> Date: Mon, 13 Mar 2023 02:30:42 -0500 Subject: [PATCH 09/20] fix: Updated WrappedExpiringStorage to use timer.unref * fix: updated WrappedExpiringStorage tests and timer.unref calls * fix: removed finalizable configs and inheritors that only used timer * fix: updated test function to test setSafeInterval and timer.unref --- .../handler/account-store/default.json | 11 ------ config/identity/ownership/token.json | 11 ------ .../readers/access-checkers/agent-group.json | 11 ------ .../keyvalue/WrappedExpiringStorage.ts | 11 ++---- .../keyvalue/WrappedExpiringStorage.test.ts | 36 ++++++------------- 5 files changed, 12 insertions(+), 68 deletions(-) diff --git a/config/identity/handler/account-store/default.json b/config/identity/handler/account-store/default.json index c547e117e..68ddc0db2 100644 --- a/config/identity/handler/account-store/default.json +++ b/config/identity/handler/account-store/default.json @@ -24,17 +24,6 @@ "relativePath": "/forgot-password/", "source": { "@id": "urn:solid-server:default:KeyValueStorage" } } - }, - { - "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelHandler", - "handlers": [ - { - "@type": "FinalizableHandler", - "finalizable": { "@id": "urn:solid-server:default:ExpiringForgotPasswordStorage" } - } - ] } ] } diff --git a/config/identity/ownership/token.json b/config/identity/ownership/token.json index 551d33467..b24921687 100644 --- a/config/identity/ownership/token.json +++ b/config/identity/ownership/token.json @@ -17,17 +17,6 @@ "relativePath": "/idp/tokens/", "source": { "@id": "urn:solid-server:default:KeyValueStorage" } } - }, - { - "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelHandler", - "handlers": [ - { - "@type": "FinalizableHandler", - "finalizable": { "@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 994bd8e92..a5ee84fcf 100644 --- a/config/ldp/authorization/readers/access-checkers/agent-group.json +++ b/config/ldp/authorization/readers/access-checkers/agent-group.json @@ -10,17 +10,6 @@ "@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", - "@type": "ParallelHandler", - "handlers": [ - { - "@type": "FinalizableHandler", - "finalizable": { "@id": "urn:solid-server:default:ExpiringAclCache" } - } - ] } ] } diff --git a/src/storage/keyvalue/WrappedExpiringStorage.ts b/src/storage/keyvalue/WrappedExpiringStorage.ts index d94196a2f..32a9f062c 100644 --- a/src/storage/keyvalue/WrappedExpiringStorage.ts +++ b/src/storage/keyvalue/WrappedExpiringStorage.ts @@ -1,4 +1,3 @@ -import type { Finalizable } from '../../init/final/Finalizable'; import { getLoggerFor } from '../../logging/LogUtil'; import { InternalServerError } from '../../util/errors/InternalServerError'; import { setSafeInterval } from '../../util/TimerUtil'; @@ -13,7 +12,7 @@ export type Expires = { expires?: string; payload: T }; * Will delete expired entries when trying to get their value. * Has a timer that will delete all expired data every hour (default value). */ -export class WrappedExpiringStorage implements ExpiringStorage, Finalizable { +export class WrappedExpiringStorage implements ExpiringStorage { protected readonly logger = getLoggerFor(this); private readonly source: KeyValueStorage>; private readonly timer: NodeJS.Timeout; @@ -28,6 +27,7 @@ export class WrappedExpiringStorage implements ExpiringStorage { @@ -121,11 +121,4 @@ export class WrappedExpiringStorage implements ExpiringStorage { - clearInterval(this.timer); - } } diff --git a/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts b/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts index a1af5ef88..9cad6b134 100644 --- a/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts +++ b/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts @@ -121,7 +121,12 @@ describe('A WrappedExpiringStorage', (): void => { // Disable interval function and simply check it was called with the correct parameters // Otherwise it gets quite difficult to verify the async interval function gets executed const mockInterval = jest.spyOn(global, 'setInterval'); - mockInterval.mockImplementation(jest.fn()); + + // We only need to call the timer.unref() once when the object is created + const mockTimer = { unref: jest.fn() }; + const mockFn = jest.fn().mockReturnValueOnce(mockTimer); + mockInterval.mockImplementationOnce(mockFn); + // Timeout of 1 minute storage = new WrappedExpiringStorage(source, 1); const data = [ @@ -141,33 +146,12 @@ describe('A WrappedExpiringStorage', (): void => { // Await the function that should have been executed by the interval await (mockInterval.mock.calls[0][0] as () => Promise)(); + // Make sure timer.unref() is called on initialization + expect(mockTimer.unref).toHaveBeenCalledTimes(1); + // Make sure setSafeInterval has been called once as well + expect(mockFn).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenLastCalledWith('key2'); mockInterval.mockRestore(); }); - - it('can stop the timer.', async(): Promise => { - const mockInterval = jest.spyOn(global, 'setInterval'); - const mockClear = jest.spyOn(global, 'clearInterval'); - // Timeout of 1 minute - storage = new WrappedExpiringStorage(source, 1); - const data = [ - [ 'key1', createExpires('data1', tomorrow) ], - [ 'key2', createExpires('data2', yesterday) ], - [ 'key3', createExpires('data3') ], - ]; - source.entries.mockImplementationOnce(function* (): any { - yield* data; - }); - - await expect(storage.finalize()).resolves.toBeUndefined(); - - // Make sure clearInterval was called with the interval timer - expect(mockClear.mock.calls).toHaveLength(1); - expect(mockClear.mock.calls[0]).toHaveLength(1); - expect(mockClear.mock.calls[0][0]).toBe(mockInterval.mock.results[0].value); - - mockInterval.mockRestore(); - mockClear.mockRestore(); - }); }); From d5c8d9e2ffbf2973a097999987eff34f7002eed8 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 16 Mar 2023 02:00:01 +0000 Subject: [PATCH 10/20] chore(deps): bump actions/checkout from 3.3.0 to 3.4.0 Bumps [actions/checkout](https://github.com/actions/checkout) from 3.3.0 to 3.4.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3.3.0...v3.4.0) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/cth-test.yml | 2 +- .github/workflows/docker.yml | 4 ++-- .github/workflows/mkdocs.yml | 6 +++--- .github/workflows/npm-test.yml | 10 +++++----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/cth-test.yml b/.github/workflows/cth-test.yml index aa2486609..907a389b8 100644 --- a/.github/workflows/cth-test.yml +++ b/.github/workflows/cth-test.yml @@ -42,7 +42,7 @@ jobs: with: node-version: 16.x - name: Check out the project - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 with: ref: ${{ inputs.branch || github.ref }} - name: Install dependencies and run build scripts diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 0eb3bcf42..ec44e3277 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -21,7 +21,7 @@ jobs: tags: ${{ steps.meta-main.outputs.tags || steps.meta-version.outputs.tags }} steps: - name: Checkout - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 - if: startsWith(github.ref, 'refs/tags/v') || (github.ref == 'refs/heads/main') name: Docker meta edge and version tag id: meta-main @@ -55,7 +55,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 - name: Set up QEMU uses: docker/setup-qemu-action@v2 - name: Set up Docker Buildx diff --git a/.github/workflows/mkdocs.yml b/.github/workflows/mkdocs.yml index 446b59476..8f7e8c672 100644 --- a/.github/workflows/mkdocs.yml +++ b/.github/workflows/mkdocs.yml @@ -21,7 +21,7 @@ jobs: outputs: major: ${{ steps.tagged_version.outputs.major || steps.current_version.outputs.major }} steps: - - uses: actions/checkout@v3.3.0 + - uses: actions/checkout@v3.4.0 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -43,7 +43,7 @@ jobs: runs-on: ubuntu-latest needs: mkdocs-prep steps: - - uses: actions/checkout@v3.3.0 + - uses: actions/checkout@v3.4.0 - uses: actions/setup-python@v4 with: python-version: 3.x @@ -63,7 +63,7 @@ jobs: needs: [mkdocs-prep, mkdocs] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.3.0 + - uses: actions/checkout@v3.4.0 - uses: actions/setup-node@v3 with: node-version: '16.x' diff --git a/.github/workflows/npm-test.yml b/.github/workflows/npm-test.yml index 7881588c2..303f9e65b 100644 --- a/.github/workflows/npm-test.yml +++ b/.github/workflows/npm-test.yml @@ -7,7 +7,7 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.3.0 + - uses: actions/checkout@v3.4.0 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -38,7 +38,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 - name: Install dependencies and run build scripts run: npm ci - name: Type-check tests @@ -81,7 +81,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Check out repository - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -105,7 +105,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -127,7 +127,7 @@ jobs: with: node-version: '16.x' - name: Check out repository - uses: actions/checkout@v3.3.0 + uses: actions/checkout@v3.4.0 - name: Install dependencies and run build scripts run: npm ci - name: Run deploy tests From b5106ceaeeb3034c24936a16276d819ff40f088b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 24 Mar 2023 02:00:21 +0000 Subject: [PATCH 11/20] chore(deps): bump actions/stale from 7 to 8 Bumps [actions/stale](https://github.com/actions/stale) from 7 to 8. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/stale/compare/v7...v8) --- updated-dependencies: - dependency-name: actions/stale dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/stale.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index ed396bcd0..c369edbbb 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -10,7 +10,7 @@ jobs: stale: runs-on: ubuntu-latest steps: - - uses: actions/stale@v7 + - uses: actions/stale@v8 with: debug-only: true stale-issue-label: 🏚️ abandoned From 50bb8cf923cf6f7ea8e10e305df4959f7221275e Mon Sep 17 00:00:00 2001 From: Pieter Heyvaert Date: Fri, 24 Mar 2023 13:45:49 +0100 Subject: [PATCH 12/20] docs: add responses when client credentials are incorrect * docs: add responses when client credentials are incorrect * fix: comment Joachim * Revert "fix: comment Joachim" This reverts commit aac6c738318e874101be044523a7679d53b74617. * fix: my own damn mess --- documentation/markdown/usage/client-credentials.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/documentation/markdown/usage/client-credentials.md b/documentation/markdown/usage/client-credentials.md index faf9f08a7..7889e85fc 100644 --- a/documentation/markdown/usage/client-credentials.md +++ b/documentation/markdown/usage/client-credentials.md @@ -40,6 +40,11 @@ const response = await fetch('http://localhost:3000/idp/credentials/', { const { id, secret } = await response.json(); ``` +If there is something wrong with your input the response code will be 500. +If no account is linked to the email, +the message will be "Account does not exist" and +if the password is wrong it will be "Incorrect password". + ## Requesting an Access token The ID and secret combination generated above can be used to request an Access Token from the server. From 2780e88acf38d3aad880dcf5350fa3a6d2785878 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 27 Mar 2023 01:38:52 +0000 Subject: [PATCH 13/20] chore(deps): bump actions/checkout from 3.4.0 to 3.5.0 Bumps [actions/checkout](https://github.com/actions/checkout) from 3.4.0 to 3.5.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3.4.0...v3.5.0) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/cth-test.yml | 2 +- .github/workflows/docker.yml | 4 ++-- .github/workflows/mkdocs.yml | 6 +++--- .github/workflows/npm-test.yml | 10 +++++----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/cth-test.yml b/.github/workflows/cth-test.yml index 907a389b8..8303471df 100644 --- a/.github/workflows/cth-test.yml +++ b/.github/workflows/cth-test.yml @@ -42,7 +42,7 @@ jobs: with: node-version: 16.x - name: Check out the project - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 with: ref: ${{ inputs.branch || github.ref }} - name: Install dependencies and run build scripts diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index ec44e3277..8739cb848 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -21,7 +21,7 @@ jobs: tags: ${{ steps.meta-main.outputs.tags || steps.meta-version.outputs.tags }} steps: - name: Checkout - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 - if: startsWith(github.ref, 'refs/tags/v') || (github.ref == 'refs/heads/main') name: Docker meta edge and version tag id: meta-main @@ -55,7 +55,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 - name: Set up QEMU uses: docker/setup-qemu-action@v2 - name: Set up Docker Buildx diff --git a/.github/workflows/mkdocs.yml b/.github/workflows/mkdocs.yml index 8f7e8c672..07475e0fa 100644 --- a/.github/workflows/mkdocs.yml +++ b/.github/workflows/mkdocs.yml @@ -21,7 +21,7 @@ jobs: outputs: major: ${{ steps.tagged_version.outputs.major || steps.current_version.outputs.major }} steps: - - uses: actions/checkout@v3.4.0 + - uses: actions/checkout@v3.5.0 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -43,7 +43,7 @@ jobs: runs-on: ubuntu-latest needs: mkdocs-prep steps: - - uses: actions/checkout@v3.4.0 + - uses: actions/checkout@v3.5.0 - uses: actions/setup-python@v4 with: python-version: 3.x @@ -63,7 +63,7 @@ jobs: needs: [mkdocs-prep, mkdocs] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.4.0 + - uses: actions/checkout@v3.5.0 - uses: actions/setup-node@v3 with: node-version: '16.x' diff --git a/.github/workflows/npm-test.yml b/.github/workflows/npm-test.yml index 303f9e65b..607809042 100644 --- a/.github/workflows/npm-test.yml +++ b/.github/workflows/npm-test.yml @@ -7,7 +7,7 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.4.0 + - uses: actions/checkout@v3.5.0 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -38,7 +38,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 - name: Install dependencies and run build scripts run: npm ci - name: Type-check tests @@ -81,7 +81,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Check out repository - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -105,7 +105,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -127,7 +127,7 @@ jobs: with: node-version: '16.x' - name: Check out repository - uses: actions/checkout@v3.4.0 + uses: actions/checkout@v3.5.0 - name: Install dependencies and run build scripts run: npm ci - name: Run deploy tests From f0596c2eb8b1f62b31fdcd26070ea9fbf8468a74 Mon Sep 17 00:00:00 2001 From: zg009 <103973669+zg009@users.noreply.github.com> Date: Tue, 28 Mar 2023 02:24:15 -0500 Subject: [PATCH 14/20] feat: Support conditions for GET/HEAD requests * fix: updated WrappedExpiringStorage tests and timer.unref calls * fix: removed finalizable configs and inheritors that only used timer * fix: updated test function to test setSafeInterval and timer.unref * fix: added NotModifiedHttpError class * fix: added 304 error test to HttpError test file * fix: 304 errors when making read request with matching ETag * Update src/util/errors/NotModifiedHttpError.ts Co-authored-by: Ted Thibodeau Jr * fix: updated tests * fix: try notMatchesEtag in test * fix: DataAccessorBasedStore test passes * fix: removed conditions check and added extra test --------- Co-authored-by: Ted Thibodeau Jr --- package-lock.json | 14 ++++++------- package.json | 2 +- src/storage/DataAccessorBasedStore.ts | 20 ++++++++++++------ src/util/errors/NotModifiedHttpError.ts | 14 +++++++++++++ test/integration/Conditions.test.ts | 21 +++++++++++++++++++ .../storage/DataAccessorBasedStore.test.ts | 14 +++++++++++++ test/unit/util/errors/HttpError.test.ts | 3 +++ 7 files changed, 74 insertions(+), 14 deletions(-) create mode 100644 src/util/errors/NotModifiedHttpError.ts diff --git a/package-lock.json b/package-lock.json index 5a9b6ad60..be67c5461 100644 --- a/package-lock.json +++ b/package-lock.json @@ -34,7 +34,7 @@ "@types/uuid": "^8.3.4", "@types/ws": "^8.5.3", "@types/yargs": "^17.0.10", - "arrayify-stream": "^2.0.0", + "arrayify-stream": "^2.0.1", "async-lock": "^1.3.2", "bcryptjs": "^2.4.3", "componentsjs": "^5.3.2", @@ -5150,9 +5150,9 @@ } }, "node_modules/arrayify-stream": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/arrayify-stream/-/arrayify-stream-2.0.0.tgz", - "integrity": "sha512-Z2NRtxpWQIz3NRA2bEZOziIungBH+fpsFFEolc5u8uVRheYitvsDNvejlfyh/hjZ9VyS9Ba62oY0zc5oa6Wu7g==" + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/arrayify-stream/-/arrayify-stream-2.0.1.tgz", + "integrity": "sha512-z8fB6PtmnewQpFB53piS2d1KlUi3BPMICH2h7leCOUXpQcwvZ4GbHHSpdKoUrgLMR6b4Qan/uDe1St3Ao3yIHg==" }, "node_modules/arrify": { "version": "1.0.1", @@ -19566,9 +19566,9 @@ } }, "arrayify-stream": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/arrayify-stream/-/arrayify-stream-2.0.0.tgz", - "integrity": "sha512-Z2NRtxpWQIz3NRA2bEZOziIungBH+fpsFFEolc5u8uVRheYitvsDNvejlfyh/hjZ9VyS9Ba62oY0zc5oa6Wu7g==" + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/arrayify-stream/-/arrayify-stream-2.0.1.tgz", + "integrity": "sha512-z8fB6PtmnewQpFB53piS2d1KlUi3BPMICH2h7leCOUXpQcwvZ4GbHHSpdKoUrgLMR6b4Qan/uDe1St3Ao3yIHg==" }, "arrify": { "version": "1.0.1", diff --git a/package.json b/package.json index fdfcf9ad8..f9a58083c 100644 --- a/package.json +++ b/package.json @@ -124,7 +124,7 @@ "@types/uuid": "^8.3.4", "@types/ws": "^8.5.3", "@types/yargs": "^17.0.10", - "arrayify-stream": "^2.0.0", + "arrayify-stream": "^2.0.1", "async-lock": "^1.3.2", "bcryptjs": "^2.4.3", "componentsjs": "^5.3.2", diff --git a/src/storage/DataAccessorBasedStore.ts b/src/storage/DataAccessorBasedStore.ts index 15452a848..bc9860da5 100644 --- a/src/storage/DataAccessorBasedStore.ts +++ b/src/storage/DataAccessorBasedStore.ts @@ -7,6 +7,7 @@ import { BasicRepresentation } from '../http/representation/BasicRepresentation' import type { Patch } from '../http/representation/Patch'; import type { Representation } from '../http/representation/Representation'; import { RepresentationMetadata } from '../http/representation/RepresentationMetadata'; +import type { RepresentationPreferences } from '../http/representation/RepresentationPreferences'; import type { ResourceIdentifier } from '../http/representation/ResourceIdentifier'; import { getLoggerFor } from '../logging/LogUtil'; import { INTERNAL_QUADS } from '../util/ContentTypes'; @@ -17,6 +18,7 @@ import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError'; import { MethodNotAllowedHttpError } from '../util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; +import { NotModifiedHttpError } from '../util/errors/NotModifiedHttpError'; import { PreconditionFailedHttpError } from '../util/errors/PreconditionFailedHttpError'; import type { IdentifierStrategy } from '../util/identifiers/IdentifierStrategy'; import { concat } from '../util/IterableUtil'; @@ -103,7 +105,8 @@ export class DataAccessorBasedStore implements ResourceStore { } } - public async getRepresentation(identifier: ResourceIdentifier): Promise { + public async getRepresentation(identifier: ResourceIdentifier, + preferences?: RepresentationPreferences, conditions?: Conditions): Promise { this.validateIdentifier(identifier); let isMetadata = false; @@ -116,6 +119,8 @@ export class DataAccessorBasedStore implements ResourceStore { let metadata = await this.accessor.getMetadata(identifier); let representation: Representation; + this.validateConditions(true, conditions, metadata); + // Potentially add auxiliary related metadata // Solid, §4.3: "Clients can discover auxiliary resources associated with a subject resource by making an HTTP HEAD // or GET request on the target URL, and checking the HTTP Link header with the rel parameter" @@ -181,7 +186,7 @@ export class DataAccessorBasedStore implements ResourceStore { throw new MethodNotAllowedHttpError([ 'POST' ], 'The given path is not a container.'); } - this.validateConditions(conditions, parentMetadata); + this.validateConditions(false, conditions, parentMetadata); // Solid, §5.1: "Servers MAY allow clients to suggest the URI of a resource created through POST, // using the HTTP Slug header as defined in [RFC5023]. @@ -246,7 +251,7 @@ export class DataAccessorBasedStore implements ResourceStore { await this.accessor.canHandle(representation); } - this.validateConditions(conditions, oldMetadata); + this.validateConditions(false, conditions, oldMetadata); if (this.metadataStrategy.isAuxiliaryIdentifier(identifier)) { return await this.writeMetadata(identifier, representation); @@ -268,7 +273,7 @@ export class DataAccessorBasedStore implements ResourceStore { } } - this.validateConditions(conditions, metadata); + this.validateConditions(false, conditions, metadata); } throw new NotImplementedHttpError('Patches are not supported by the default store.'); @@ -309,7 +314,7 @@ export class DataAccessorBasedStore implements ResourceStore { throw new ConflictHttpError('Can only delete empty containers.'); } - this.validateConditions(conditions, metadata); + this.validateConditions(false, conditions, metadata); // Solid, §5.4: "When a contained resource is deleted, // the server MUST also delete the associated auxiliary resources" @@ -347,10 +352,13 @@ export class DataAccessorBasedStore implements ResourceStore { /** * Verify if the given metadata matches the conditions. */ - protected validateConditions(conditions?: Conditions, metadata?: RepresentationMetadata): void { + protected validateConditions(read: boolean, conditions?: Conditions, metadata?: RepresentationMetadata): void { // The 412 (Precondition Failed) status code indicates // that one or more conditions given in the request header fields evaluated to false when tested on the server. if (conditions && !conditions.matchesMetadata(metadata)) { + if (read) { + throw new NotModifiedHttpError(); + } throw new PreconditionFailedHttpError(); } } diff --git a/src/util/errors/NotModifiedHttpError.ts b/src/util/errors/NotModifiedHttpError.ts new file mode 100644 index 000000000..9504de3b7 --- /dev/null +++ b/src/util/errors/NotModifiedHttpError.ts @@ -0,0 +1,14 @@ +import type { HttpErrorOptions } from './HttpError'; +import { generateHttpErrorClass } from './HttpError'; + +// eslint-disable-next-line @typescript-eslint/naming-convention +const BaseHttpError = generateHttpErrorClass(304, 'NotModifiedHttpError'); + +/** + * An error is thrown when a request conflicts with the current state of the server. + */ +export class NotModifiedHttpError extends BaseHttpError { + public constructor(message?: string, options?: HttpErrorOptions) { + super(message, options); + } +} diff --git a/test/integration/Conditions.test.ts b/test/integration/Conditions.test.ts index 4980a7480..a57cf83ee 100644 --- a/test/integration/Conditions.test.ts +++ b/test/integration/Conditions.test.ts @@ -170,6 +170,27 @@ describe.each(stores)('A server supporting conditions with %s', (name, { storeCo expect(await deleteResource(documentUrl!)).toBeUndefined(); }); + it('throws 304 error if "if-none-match" header matches and request type is GET or HEAD.', async(): Promise => { + // GET root ETag + let response = await getResource(baseUrl); + const eTag = response.headers.get('ETag'); + expect(typeof eTag).toBe('string'); + + // GET fails because of header + response = await fetch(baseUrl, { + method: 'GET', + headers: { 'if-none-match': eTag! }, + }); + expect(response.status).toBe(304); + + // HEAD fails because of header + response = await fetch(baseUrl, { + method: 'HEAD', + headers: { 'if-none-match': eTag! }, + }); + expect(response.status).toBe(304); + }); + it('prevents operations if the "if-unmodified-since" header is before the modified date.', async(): Promise => { const documentUrl = `${baseUrl}document3.txt`; // PUT diff --git a/test/unit/storage/DataAccessorBasedStore.test.ts b/test/unit/storage/DataAccessorBasedStore.test.ts index 5e1ecd2de..8d9f3d543 100644 --- a/test/unit/storage/DataAccessorBasedStore.test.ts +++ b/test/unit/storage/DataAccessorBasedStore.test.ts @@ -19,6 +19,7 @@ import { ForbiddenHttpError } from '../../../src/util/errors/ForbiddenHttpError' import { MethodNotAllowedHttpError } from '../../../src/util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../../../src/util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; +import { NotModifiedHttpError } from '../../../src/util/errors/NotModifiedHttpError'; import { PreconditionFailedHttpError } from '../../../src/util/errors/PreconditionFailedHttpError'; import type { Guarded } from '../../../src/util/GuardedStream'; import { SingleRootIdentifierStrategy } from '../../../src/util/identifiers/SingleRootIdentifierStrategy'; @@ -26,6 +27,7 @@ import { trimTrailingSlashes } from '../../../src/util/PathUtil'; import { guardedStreamFrom } from '../../../src/util/StreamUtil'; import { CONTENT_TYPE, SOLID_HTTP, LDP, PIM, RDF, SOLID_META, DC, SOLID_AS, AS } from '../../../src/util/Vocabularies'; import { SimpleSuffixStrategy } from '../../util/SimpleSuffixStrategy'; + const { namedNode, quad, literal } = DataFactory; const GENERATED_PREDICATE = namedNode('generated'); @@ -216,6 +218,18 @@ describe('A DataAccessorBasedStore', (): void => { ); expect(result.metadata.contentType).toBe(INTERNAL_QUADS); }); + + it('throws a 304 if the request is a read type error.', async(): Promise => { + const resourceID = { path: root }; + const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); + await expect(store.getRepresentation(resourceID, undefined, conditions)).rejects.toThrow(NotModifiedHttpError); + }); + + it('has conditions but throws no error.', async(): Promise => { + const resourceID = { path: root }; + const conditions = new BasicConditions({ matchesETag: [ '*' ]}); + await expect(store.getRepresentation(resourceID, undefined, conditions)).resolves.toBeTruthy(); + }); }); describe('adding a Resource', (): void => { diff --git a/test/unit/util/errors/HttpError.test.ts b/test/unit/util/errors/HttpError.test.ts index 46d9a8773..4b2726dd2 100644 --- a/test/unit/util/errors/HttpError.test.ts +++ b/test/unit/util/errors/HttpError.test.ts @@ -9,16 +9,19 @@ import { InternalServerError } from '../../../../src/util/errors/InternalServerE import { MethodNotAllowedHttpError } from '../../../../src/util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError'; +import { NotModifiedHttpError } from '../../../../src/util/errors/NotModifiedHttpError'; import { PayloadHttpError } from '../../../../src/util/errors/PayloadHttpError'; import { PreconditionFailedHttpError } from '../../../../src/util/errors/PreconditionFailedHttpError'; import { UnauthorizedHttpError } from '../../../../src/util/errors/UnauthorizedHttpError'; import { UnprocessableEntityHttpError } from '../../../../src/util/errors/UnprocessableEntityHttpError'; import { UnsupportedMediaTypeHttpError } from '../../../../src/util/errors/UnsupportedMediaTypeHttpError'; import { SOLID_ERROR } from '../../../../src/util/Vocabularies'; + const { literal, namedNode, quad } = DataFactory; describe('HttpError', (): void => { const errors: [string, number, HttpErrorClass][] = [ + [ 'NotModifiedHttpError', 304, NotModifiedHttpError ], [ 'BadRequestHttpError', 400, BadRequestHttpError ], [ 'UnauthorizedHttpError', 401, UnauthorizedHttpError ], [ 'ForbiddenHttpError', 403, ForbiddenHttpError ], From f149e3d8843c589b8c466b14959585baf0e341c6 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Tue, 4 Apr 2023 11:46:02 +0200 Subject: [PATCH 15/20] chore: Update date in templates --- LICENSE.md | 2 +- templates/main.html.ejs | 2 +- templates/root/prefilled/index.html | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/LICENSE.md b/LICENSE.md index dfb243b82..0c863f6c0 100644 --- a/LICENSE.md +++ b/LICENSE.md @@ -1,6 +1,6 @@ MIT License -Copyright © 2019–2022 Inrupt Inc. and imec +Copyright © 2019–2023 Inrupt Inc. and imec Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal diff --git a/templates/main.html.ejs b/templates/main.html.ejs index f0e7f20a4..7d664f5ea 100644 --- a/templates/main.html.ejs +++ b/templates/main.html.ejs @@ -17,7 +17,7 @@ diff --git a/templates/root/prefilled/index.html b/templates/root/prefilled/index.html index 90b43b336..0c6e0376c 100644 --- a/templates/root/prefilled/index.html +++ b/templates/root/prefilled/index.html @@ -58,7 +58,7 @@ From 4b3301738e3884a3b4324982c324463a1beb990c Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 5 Apr 2023 10:07:23 +0200 Subject: [PATCH 16/20] docs: Add references to the configuration generator --- README.md | 6 ++++-- documentation/markdown/README.md | 1 + templates/root/prefilled/index.html | 11 +++++++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5154d32e5..abdeb98ad 100644 --- a/README.md +++ b/README.md @@ -167,7 +167,9 @@ The Community Solid Server uses [Components.js](https://componentsjs.readthedocs to specify how modules and components need to be wired together at runtime. Examples and guidance on configurations -are available in the [`config` folder](https://github.com/CommunitySolidServer/CommunitySolidServer/tree/main/config). +are available in the [`config` folder](https://github.com/CommunitySolidServer/CommunitySolidServer/tree/main/config), +and the [configurations tutorial](https://github.com/CommunitySolidServer/tutorials/blob/main/custom-configurations.md). +There is also a [configuration generator](https://communitysolidserver.github.io/configuration-generator/). Recipes for configuring the server can be found at [CommunitySolidServer/recipes](https://github.com/CommunitySolidServer/recipes). @@ -176,7 +178,7 @@ Recipes for configuring the server can be found at [CommunitySolidServer/recipes The server allows writing and plugging in custom modules without altering its base source code. -The [📗 API documentation](https://communitysolidserver.github.io/CommunitySolidServer/latest/5.x/docs) and +The [📗 API documentation](https://communitysolidserver.github.io/CommunitySolidServer/5.x/docs) and the [📓 user documentation](https://communitysolidserver.github.io/CommunitySolidServer/) can help you find your way. There is also a repository of [📚 comprehensive tutorials](https://github.com/CommunitySolidServer/tutorials/) diff --git a/documentation/markdown/README.md b/documentation/markdown/README.md index e2843ded4..66b59a84d 100644 --- a/documentation/markdown/README.md +++ b/documentation/markdown/README.md @@ -44,6 +44,7 @@ the [changelog](https://github.com/CommunitySolidServer/CommunitySolidServer/blo ## Comprehensive guides and tutorials * [The CSS tutorial repository](https://github.com/CommunitySolidServer/tutorials/) +* [CSS configuration generator](https://communitysolidserver.github.io/configuration-generator/) ## Making changes diff --git a/templates/root/prefilled/index.html b/templates/root/prefilled/index.html index 0c6e0376c..20c0e0702 100644 --- a/templates/root/prefilled/index.html +++ b/templates/root/prefilled/index.html @@ -30,13 +30,20 @@ If you want to keep data permanently, choose a configuration that saves data to disk instead.

+

+ To learn more about how this server can be used, + have a look at the + getting started tutorial. +

Getting started as a developer

- Run the setup to configure your server. -
The default configuration includes the ready-to-use root Pod you're currently looking at. +
+ Besides the provided configurations, + you can also fine-tune your own custom configuration using the + configuration generator.

You can easily choose any folder on your disk From 9c55c2d05d8c4a68e7576a6f3347807f89826c5e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 13 Apr 2023 01:00:26 +0000 Subject: [PATCH 17/20] chore(deps): bump actions/checkout from 3.5.0 to 3.5.1 Bumps [actions/checkout](https://github.com/actions/checkout) from 3.5.0 to 3.5.1. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3.5.0...v3.5.1) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/cth-test.yml | 2 +- .github/workflows/docker.yml | 4 ++-- .github/workflows/mkdocs.yml | 6 +++--- .github/workflows/npm-test.yml | 10 +++++----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/cth-test.yml b/.github/workflows/cth-test.yml index 8303471df..2a332e9a3 100644 --- a/.github/workflows/cth-test.yml +++ b/.github/workflows/cth-test.yml @@ -42,7 +42,7 @@ jobs: with: node-version: 16.x - name: Check out the project - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 with: ref: ${{ inputs.branch || github.ref }} - name: Install dependencies and run build scripts diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 8739cb848..12a706e1a 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -21,7 +21,7 @@ jobs: tags: ${{ steps.meta-main.outputs.tags || steps.meta-version.outputs.tags }} steps: - name: Checkout - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 - if: startsWith(github.ref, 'refs/tags/v') || (github.ref == 'refs/heads/main') name: Docker meta edge and version tag id: meta-main @@ -55,7 +55,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 - name: Set up QEMU uses: docker/setup-qemu-action@v2 - name: Set up Docker Buildx diff --git a/.github/workflows/mkdocs.yml b/.github/workflows/mkdocs.yml index 07475e0fa..f3c049595 100644 --- a/.github/workflows/mkdocs.yml +++ b/.github/workflows/mkdocs.yml @@ -21,7 +21,7 @@ jobs: outputs: major: ${{ steps.tagged_version.outputs.major || steps.current_version.outputs.major }} steps: - - uses: actions/checkout@v3.5.0 + - uses: actions/checkout@v3.5.1 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -43,7 +43,7 @@ jobs: runs-on: ubuntu-latest needs: mkdocs-prep steps: - - uses: actions/checkout@v3.5.0 + - uses: actions/checkout@v3.5.1 - uses: actions/setup-python@v4 with: python-version: 3.x @@ -63,7 +63,7 @@ jobs: needs: [mkdocs-prep, mkdocs] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.5.0 + - uses: actions/checkout@v3.5.1 - uses: actions/setup-node@v3 with: node-version: '16.x' diff --git a/.github/workflows/npm-test.yml b/.github/workflows/npm-test.yml index 607809042..2fa2f37d9 100644 --- a/.github/workflows/npm-test.yml +++ b/.github/workflows/npm-test.yml @@ -7,7 +7,7 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.5.0 + - uses: actions/checkout@v3.5.1 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -38,7 +38,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 - name: Install dependencies and run build scripts run: npm ci - name: Type-check tests @@ -81,7 +81,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Check out repository - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -105,7 +105,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -127,7 +127,7 @@ jobs: with: node-version: '16.x' - name: Check out repository - uses: actions/checkout@v3.5.0 + uses: actions/checkout@v3.5.1 - name: Install dependencies and run build scripts run: npm ci - name: Run deploy tests From 46ceb6d7eba32a31cc737a38ca3f96511a99b2c3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 14 Apr 2023 01:00:44 +0000 Subject: [PATCH 18/20] chore(deps): bump actions/checkout from 3.5.1 to 3.5.2 Bumps [actions/checkout](https://github.com/actions/checkout) from 3.5.1 to 3.5.2. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3.5.1...v3.5.2) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/cth-test.yml | 2 +- .github/workflows/docker.yml | 4 ++-- .github/workflows/mkdocs.yml | 6 +++--- .github/workflows/npm-test.yml | 10 +++++----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/cth-test.yml b/.github/workflows/cth-test.yml index 2a332e9a3..b58380e85 100644 --- a/.github/workflows/cth-test.yml +++ b/.github/workflows/cth-test.yml @@ -42,7 +42,7 @@ jobs: with: node-version: 16.x - name: Check out the project - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 with: ref: ${{ inputs.branch || github.ref }} - name: Install dependencies and run build scripts diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 12a706e1a..a59a58b02 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -21,7 +21,7 @@ jobs: tags: ${{ steps.meta-main.outputs.tags || steps.meta-version.outputs.tags }} steps: - name: Checkout - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 - if: startsWith(github.ref, 'refs/tags/v') || (github.ref == 'refs/heads/main') name: Docker meta edge and version tag id: meta-main @@ -55,7 +55,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 - name: Set up QEMU uses: docker/setup-qemu-action@v2 - name: Set up Docker Buildx diff --git a/.github/workflows/mkdocs.yml b/.github/workflows/mkdocs.yml index f3c049595..1a7d8918e 100644 --- a/.github/workflows/mkdocs.yml +++ b/.github/workflows/mkdocs.yml @@ -21,7 +21,7 @@ jobs: outputs: major: ${{ steps.tagged_version.outputs.major || steps.current_version.outputs.major }} steps: - - uses: actions/checkout@v3.5.1 + - uses: actions/checkout@v3.5.2 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -43,7 +43,7 @@ jobs: runs-on: ubuntu-latest needs: mkdocs-prep steps: - - uses: actions/checkout@v3.5.1 + - uses: actions/checkout@v3.5.2 - uses: actions/setup-python@v4 with: python-version: 3.x @@ -63,7 +63,7 @@ jobs: needs: [mkdocs-prep, mkdocs] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.5.1 + - uses: actions/checkout@v3.5.2 - uses: actions/setup-node@v3 with: node-version: '16.x' diff --git a/.github/workflows/npm-test.yml b/.github/workflows/npm-test.yml index 2fa2f37d9..0cda80acf 100644 --- a/.github/workflows/npm-test.yml +++ b/.github/workflows/npm-test.yml @@ -7,7 +7,7 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3.5.1 + - uses: actions/checkout@v3.5.2 - uses: actions/setup-node@v3 with: node-version: '16.x' @@ -38,7 +38,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 - name: Install dependencies and run build scripts run: npm ci - name: Type-check tests @@ -81,7 +81,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Check out repository - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -105,7 +105,7 @@ jobs: - name: Ensure line endings are consistent run: git config --global core.autocrlf input - name: Check out repository - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 - name: Install dependencies and run build scripts run: npm ci - name: Run integration tests @@ -127,7 +127,7 @@ jobs: with: node-version: '16.x' - name: Check out repository - uses: actions/checkout@v3.5.1 + uses: actions/checkout@v3.5.2 - name: Install dependencies and run build scripts run: npm ci - name: Run deploy tests From c3f48ddb97c842dc3a2bbfd06230cb6caa10917d Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Thu, 30 Mar 2023 13:23:04 +0200 Subject: [PATCH 19/20] fix: Ensure the ETag is representation specific --- src/http/ldp/GetOperationHandler.ts | 4 ++ src/http/ldp/HeadOperationHandler.ts | 5 ++ src/storage/BasicConditions.ts | 41 ++++++----- src/storage/Conditions.ts | 42 ++++++++--- src/storage/DataAccessorBasedStore.ts | 20 ++---- src/util/ResourceUtil.ts | 24 +++++++ test/integration/Conditions.test.ts | 25 +++++++ .../unit/http/ldp/GetOperationHandler.test.ts | 20 +++++- .../http/ldp/HeadOperationHandler.test.ts | 15 +++- .../metadata/ModifiedMetadataWriter.test.ts | 7 +- test/unit/storage/BasicConditions.test.ts | 71 +++++++++++-------- test/unit/storage/Conditions.test.ts | 48 +++++++++++-- .../storage/DataAccessorBasedStore.test.ts | 13 ---- test/unit/util/ResourceUtil.test.ts | 43 ++++++++++- 14 files changed, 276 insertions(+), 102 deletions(-) diff --git a/src/http/ldp/GetOperationHandler.ts b/src/http/ldp/GetOperationHandler.ts index 6525016d1..5f7de96ba 100644 --- a/src/http/ldp/GetOperationHandler.ts +++ b/src/http/ldp/GetOperationHandler.ts @@ -1,5 +1,6 @@ import type { ResourceStore } from '../../storage/ResourceStore'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; +import { assertReadConditions } from '../../util/ResourceUtil'; import { OkResponseDescription } from '../output/response/OkResponseDescription'; import type { ResponseDescription } from '../output/response/ResponseDescription'; import type { OperationHandlerInput } from './OperationHandler'; @@ -26,6 +27,9 @@ export class GetOperationHandler extends OperationHandler { public async handle({ operation }: OperationHandlerInput): Promise { const body = await this.store.getRepresentation(operation.target, operation.preferences, operation.conditions); + // Check whether the cached representation is still valid or it is necessary to send a new representation + assertReadConditions(body, operation.conditions); + return new OkResponseDescription(body.metadata, body.data); } } diff --git a/src/http/ldp/HeadOperationHandler.ts b/src/http/ldp/HeadOperationHandler.ts index 6bb7676da..276c57229 100644 --- a/src/http/ldp/HeadOperationHandler.ts +++ b/src/http/ldp/HeadOperationHandler.ts @@ -1,5 +1,6 @@ import type { ResourceStore } from '../../storage/ResourceStore'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; +import { assertReadConditions } from '../../util/ResourceUtil'; import { OkResponseDescription } from '../output/response/OkResponseDescription'; import type { ResponseDescription } from '../output/response/ResponseDescription'; import type { OperationHandlerInput } from './OperationHandler'; @@ -29,6 +30,10 @@ export class HeadOperationHandler extends OperationHandler { // Close the Readable as we will not return it. body.data.destroy(); + // Check whether the cached representation is still valid or it is necessary to send a new representation. + // Generally it doesn't make much sense to use condition headers with a HEAD request, but it should be supported. + assertReadConditions(body, operation.conditions); + return new OkResponseDescription(body.metadata); } } diff --git a/src/storage/BasicConditions.ts b/src/storage/BasicConditions.ts index e87acaf99..8affe5736 100644 --- a/src/storage/BasicConditions.ts +++ b/src/storage/BasicConditions.ts @@ -1,6 +1,6 @@ import type { RepresentationMetadata } from '../http/representation/RepresentationMetadata'; import { DC } from '../util/Vocabularies'; -import { getETag } from './Conditions'; +import { getETag, isCurrentETag } from './Conditions'; import type { Conditions } from './Conditions'; export interface BasicConditionsOptions { @@ -26,40 +26,43 @@ export class BasicConditions implements Conditions { this.unmodifiedSince = options.unmodifiedSince; } - public matchesMetadata(metadata?: RepresentationMetadata): boolean { + public matchesMetadata(metadata?: RepresentationMetadata, strict?: boolean): boolean { if (!metadata) { // RFC7232: ...If-Match... If the field-value is "*", the condition is false if the origin server // does not have a current representation for the target resource. return !this.matchesETag?.includes('*'); } - const modified = metadata.get(DC.terms.modified); - const modifiedDate = modified ? new Date(modified.value) : undefined; - const etag = getETag(metadata); - return this.matches(etag, modifiedDate); - } - - public matches(eTag?: string, lastModified?: Date): boolean { // RFC7232: ...If-None-Match... If the field-value is "*", the condition is false if the origin server // has a current representation for the target resource. if (this.notMatchesETag?.includes('*')) { return false; } - if (eTag) { - if (this.matchesETag && !this.matchesETag.includes(eTag) && !this.matchesETag.includes('*')) { - return false; - } - if (this.notMatchesETag?.includes(eTag)) { - return false; - } + // Helper function to see if an ETag matches the provided metadata + // eslint-disable-next-line func-style + let eTagMatches = (tag: string): boolean => isCurrentETag(tag, metadata); + if (strict) { + const eTag = getETag(metadata); + eTagMatches = (tag: string): boolean => tag === eTag; } - if (lastModified) { - if (this.modifiedSince && lastModified < this.modifiedSince) { + if (this.matchesETag && !this.matchesETag.includes('*') && !this.matchesETag.some(eTagMatches)) { + return false; + } + if (this.notMatchesETag?.some(eTagMatches)) { + return false; + } + + // In practice, this will only be undefined on a backend + // that doesn't store the modified date. + const modified = metadata.get(DC.terms.modified); + if (modified) { + const modifiedDate = new Date(modified.value); + if (this.modifiedSince && modifiedDate < this.modifiedSince) { return false; } - if (this.unmodifiedSince && lastModified > this.unmodifiedSince) { + if (this.unmodifiedSince && modifiedDate > this.unmodifiedSince) { return false; } } diff --git a/src/storage/Conditions.ts b/src/storage/Conditions.ts index c31f8d247..586252de3 100644 --- a/src/storage/Conditions.ts +++ b/src/storage/Conditions.ts @@ -25,16 +25,12 @@ export interface Conditions { /** * Checks validity based on the given metadata. * @param metadata - Metadata of the representation. Undefined if the resource does not exist. + * @param strict - How to compare the ETag related headers. + * If true, exact string matching will be used to compare with the ETag for the given metadata. + * If false, it will take into account that content negotiation might still happen + * which can change the ETag. */ - matchesMetadata: (metadata?: RepresentationMetadata) => boolean; - /** - * Checks validity based on the given ETag and/or date. - * This function assumes the resource being checked exists. - * If not, the `matchesMetadata` function should be used. - * @param eTag - Condition based on ETag. - * @param lastModified - Condition based on last modified date. - */ - matches: (eTag?: string, lastModified?: Date) => boolean; + matchesMetadata: (metadata?: RepresentationMetadata, strict?: boolean) => boolean; } /** @@ -45,8 +41,32 @@ export interface Conditions { */ export function getETag(metadata: RepresentationMetadata): string | undefined { const modified = metadata.get(DC.terms.modified); - if (modified) { + const { contentType } = metadata; + if (modified && contentType) { const date = new Date(modified.value); - return `"${date.getTime()}"`; + return `"${date.getTime()}-${contentType}"`; } } + +/** + * Validates whether a given ETag corresponds to the current state of the resource, + * independent of the representation the ETag corresponds to. + * Assumes ETags are made with the {@link getETag} function. + * Since we base the ETag on the last modified date, + * we know the ETag still matches as long as that didn't change. + * + * @param eTag - ETag to validate. + * @param metadata - Metadata of the resource. + * + * @returns `true` if the ETag represents the current state of the resource. + */ +export function isCurrentETag(eTag: string, metadata: RepresentationMetadata): boolean { + const modified = metadata.get(DC.terms.modified); + if (!modified) { + return false; + } + const time = eTag.split('-', 1)[0]; + const date = new Date(modified.value); + // `time` will still have the initial`"` of the ETag string + return time === `"${date.getTime()}`; +} diff --git a/src/storage/DataAccessorBasedStore.ts b/src/storage/DataAccessorBasedStore.ts index bc9860da5..15452a848 100644 --- a/src/storage/DataAccessorBasedStore.ts +++ b/src/storage/DataAccessorBasedStore.ts @@ -7,7 +7,6 @@ import { BasicRepresentation } from '../http/representation/BasicRepresentation' import type { Patch } from '../http/representation/Patch'; import type { Representation } from '../http/representation/Representation'; import { RepresentationMetadata } from '../http/representation/RepresentationMetadata'; -import type { RepresentationPreferences } from '../http/representation/RepresentationPreferences'; import type { ResourceIdentifier } from '../http/representation/ResourceIdentifier'; import { getLoggerFor } from '../logging/LogUtil'; import { INTERNAL_QUADS } from '../util/ContentTypes'; @@ -18,7 +17,6 @@ import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError'; import { MethodNotAllowedHttpError } from '../util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; -import { NotModifiedHttpError } from '../util/errors/NotModifiedHttpError'; import { PreconditionFailedHttpError } from '../util/errors/PreconditionFailedHttpError'; import type { IdentifierStrategy } from '../util/identifiers/IdentifierStrategy'; import { concat } from '../util/IterableUtil'; @@ -105,8 +103,7 @@ export class DataAccessorBasedStore implements ResourceStore { } } - public async getRepresentation(identifier: ResourceIdentifier, - preferences?: RepresentationPreferences, conditions?: Conditions): Promise { + public async getRepresentation(identifier: ResourceIdentifier): Promise { this.validateIdentifier(identifier); let isMetadata = false; @@ -119,8 +116,6 @@ export class DataAccessorBasedStore implements ResourceStore { let metadata = await this.accessor.getMetadata(identifier); let representation: Representation; - this.validateConditions(true, conditions, metadata); - // Potentially add auxiliary related metadata // Solid, §4.3: "Clients can discover auxiliary resources associated with a subject resource by making an HTTP HEAD // or GET request on the target URL, and checking the HTTP Link header with the rel parameter" @@ -186,7 +181,7 @@ export class DataAccessorBasedStore implements ResourceStore { throw new MethodNotAllowedHttpError([ 'POST' ], 'The given path is not a container.'); } - this.validateConditions(false, conditions, parentMetadata); + this.validateConditions(conditions, parentMetadata); // Solid, §5.1: "Servers MAY allow clients to suggest the URI of a resource created through POST, // using the HTTP Slug header as defined in [RFC5023]. @@ -251,7 +246,7 @@ export class DataAccessorBasedStore implements ResourceStore { await this.accessor.canHandle(representation); } - this.validateConditions(false, conditions, oldMetadata); + this.validateConditions(conditions, oldMetadata); if (this.metadataStrategy.isAuxiliaryIdentifier(identifier)) { return await this.writeMetadata(identifier, representation); @@ -273,7 +268,7 @@ export class DataAccessorBasedStore implements ResourceStore { } } - this.validateConditions(false, conditions, metadata); + this.validateConditions(conditions, metadata); } throw new NotImplementedHttpError('Patches are not supported by the default store.'); @@ -314,7 +309,7 @@ export class DataAccessorBasedStore implements ResourceStore { throw new ConflictHttpError('Can only delete empty containers.'); } - this.validateConditions(false, conditions, metadata); + this.validateConditions(conditions, metadata); // Solid, §5.4: "When a contained resource is deleted, // the server MUST also delete the associated auxiliary resources" @@ -352,13 +347,10 @@ export class DataAccessorBasedStore implements ResourceStore { /** * Verify if the given metadata matches the conditions. */ - protected validateConditions(read: boolean, conditions?: Conditions, metadata?: RepresentationMetadata): void { + protected validateConditions(conditions?: Conditions, metadata?: RepresentationMetadata): void { // The 412 (Precondition Failed) status code indicates // that one or more conditions given in the request header fields evaluated to false when tested on the server. if (conditions && !conditions.matchesMetadata(metadata)) { - if (read) { - throw new NotModifiedHttpError(); - } throw new PreconditionFailedHttpError(); } } diff --git a/src/util/ResourceUtil.ts b/src/util/ResourceUtil.ts index f06e593e1..43de0b121 100644 --- a/src/util/ResourceUtil.ts +++ b/src/util/ResourceUtil.ts @@ -3,6 +3,8 @@ import { DataFactory } from 'n3'; import { BasicRepresentation } from '../http/representation/BasicRepresentation'; import type { Representation } from '../http/representation/Representation'; import { RepresentationMetadata } from '../http/representation/RepresentationMetadata'; +import type { Conditions } from '../storage/Conditions'; +import { NotModifiedHttpError } from './errors/NotModifiedHttpError'; import { guardedStreamFrom } from './StreamUtil'; import { toLiteral } from './TermUtil'; import { CONTENT_TYPE_TERM, DC, LDP, RDF, SOLID_META, XSD } from './Vocabularies'; @@ -65,3 +67,25 @@ export async function cloneRepresentation(representation: Representation): Promi representation.data = guardedStreamFrom(data); return result; } + +/** + * Verify whether the given {@link Representation} matches the given conditions. + * If not, destroy the data stream and throw a {@link NotModifiedHttpError}. + * If `conditions` is not defined, nothing will happen. + * + * This uses the strict conditions check which takes the content type into account; + * therefore, this should only be called after content negotiation, when it is certain what the output will be. + * + * Note that browsers only keep track of one ETag, and the Vary header has no impact on this, + * meaning the browser could send us the ETag for a Turtle resource even though it is requesting JSON-LD; + * this is why we have to check ETags after content negotiation. + * + * @param body - The representation to compare the conditions against. + * @param conditions - The conditions to assert. + */ +export function assertReadConditions(body: Representation, conditions?: Conditions): void { + if (conditions && !conditions.matchesMetadata(body.metadata, true)) { + body.data.destroy(); + throw new NotModifiedHttpError(); + } +} diff --git a/test/integration/Conditions.test.ts b/test/integration/Conditions.test.ts index a57cf83ee..574681e95 100644 --- a/test/integration/Conditions.test.ts +++ b/test/integration/Conditions.test.ts @@ -189,6 +189,13 @@ describe.each(stores)('A server supporting conditions with %s', (name, { storeCo headers: { 'if-none-match': eTag! }, }); expect(response.status).toBe(304); + + // GET succeeds if the ETag header doesn't match + response = await fetch(baseUrl, { + method: 'GET', + headers: { 'if-none-match': '"123456"' }, + }); + expect(response.status).toBe(200); }); it('prevents operations if the "if-unmodified-since" header is before the modified date.', async(): Promise => { @@ -218,4 +225,22 @@ describe.each(stores)('A server supporting conditions with %s', (name, { storeCo }); expect(response.status).toBe(205); }); + + it('returns different ETags for different content-types.', async(): Promise => { + let response = await getResource(baseUrl, { accept: 'text/turtle' }, { contentType: 'text/turtle' }); + const eTagTurtle = response.headers.get('ETag'); + response = await getResource(baseUrl, { accept: 'application/ld+json' }, { contentType: 'application/ld+json' }); + const eTagJson = response.headers.get('ETag'); + expect(eTagTurtle).not.toEqual(eTagJson); + + // Both ETags can be used on the same resource + response = await fetch(baseUrl, { headers: { 'if-none-match': eTagTurtle!, accept: 'text/turtle' }}); + expect(response.status).toBe(304); + response = await fetch(baseUrl, { headers: { 'if-none-match': eTagJson!, accept: 'application/ld+json' }}); + expect(response.status).toBe(304); + + // But not for the other representation + response = await fetch(baseUrl, { headers: { 'if-none-match': eTagTurtle!, accept: 'application/ld+json' }}); + expect(response.status).toBe(200); + }); }); diff --git a/test/unit/http/ldp/GetOperationHandler.test.ts b/test/unit/http/ldp/GetOperationHandler.test.ts index b667c70fc..20044c6e1 100644 --- a/test/unit/http/ldp/GetOperationHandler.test.ts +++ b/test/unit/http/ldp/GetOperationHandler.test.ts @@ -1,10 +1,13 @@ +import type { Readable } from 'stream'; import { GetOperationHandler } from '../../../../src/http/ldp/GetOperationHandler'; import type { Operation } from '../../../../src/http/Operation'; import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation'; import type { Representation } from '../../../../src/http/representation/Representation'; +import { RepresentationMetadata } from '../../../../src/http/representation/RepresentationMetadata'; import { BasicConditions } from '../../../../src/storage/BasicConditions'; import type { ResourceStore } from '../../../../src/storage/ResourceStore'; import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError'; +import { NotModifiedHttpError } from '../../../../src/util/errors/NotModifiedHttpError'; describe('A GetOperationHandler', (): void => { let operation: Operation; @@ -13,12 +16,15 @@ describe('A GetOperationHandler', (): void => { const body = new BasicRepresentation(); let store: ResourceStore; let handler: GetOperationHandler; + let data: Readable; + const metadata = new RepresentationMetadata(); beforeEach(async(): Promise => { operation = { method: 'GET', target: { path: 'http://test.com/foo' }, preferences, conditions, body }; + data = { destroy: jest.fn() } as any; store = { getRepresentation: jest.fn(async(): Promise => - ({ binary: false, data: 'data', metadata: 'metadata' } as any)), + ({ binary: false, data, metadata } as any)), } as unknown as ResourceStore; handler = new GetOperationHandler(store); @@ -33,9 +39,17 @@ describe('A GetOperationHandler', (): void => { it('returns the representation from the store with the correct response.', async(): Promise => { const result = await handler.handle({ operation }); expect(result.statusCode).toBe(200); - expect(result.metadata).toBe('metadata'); - expect(result.data).toBe('data'); + expect(result.metadata).toBe(metadata); + expect(result.data).toBe(data); expect(store.getRepresentation).toHaveBeenCalledTimes(1); expect(store.getRepresentation).toHaveBeenLastCalledWith(operation.target, preferences, conditions); }); + + it('returns a 304 if the conditions do not match.', async(): Promise => { + operation.conditions = { + matchesMetadata: (): boolean => false, + }; + await expect(handler.handle({ operation })).rejects.toThrow(NotModifiedHttpError); + expect(data.destroy).toHaveBeenCalledTimes(1); + }); }); diff --git a/test/unit/http/ldp/HeadOperationHandler.test.ts b/test/unit/http/ldp/HeadOperationHandler.test.ts index 141c87715..140d98fca 100644 --- a/test/unit/http/ldp/HeadOperationHandler.test.ts +++ b/test/unit/http/ldp/HeadOperationHandler.test.ts @@ -3,9 +3,11 @@ import { HeadOperationHandler } from '../../../../src/http/ldp/HeadOperationHand import type { Operation } from '../../../../src/http/Operation'; import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation'; import type { Representation } from '../../../../src/http/representation/Representation'; +import { RepresentationMetadata } from '../../../../src/http/representation/RepresentationMetadata'; import { BasicConditions } from '../../../../src/storage/BasicConditions'; import type { ResourceStore } from '../../../../src/storage/ResourceStore'; import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError'; +import { NotModifiedHttpError } from '../../../../src/util/errors/NotModifiedHttpError'; describe('A HeadOperationHandler', (): void => { let operation: Operation; @@ -15,13 +17,14 @@ describe('A HeadOperationHandler', (): void => { let store: ResourceStore; let handler: HeadOperationHandler; let data: Readable; + const metadata = new RepresentationMetadata(); beforeEach(async(): Promise => { operation = { method: 'HEAD', target: { path: 'http://test.com/foo' }, preferences, conditions, body }; data = { destroy: jest.fn() } as any; store = { getRepresentation: jest.fn(async(): Promise => - ({ binary: false, data, metadata: 'metadata' } as any)), + ({ binary: false, data, metadata } as any)), } as any; handler = new HeadOperationHandler(store); @@ -38,10 +41,18 @@ describe('A HeadOperationHandler', (): void => { it('returns the representation from the store with the correct response.', async(): Promise => { const result = await handler.handle({ operation }); expect(result.statusCode).toBe(200); - expect(result.metadata).toBe('metadata'); + expect(result.metadata).toBe(metadata); expect(result.data).toBeUndefined(); expect(data.destroy).toHaveBeenCalledTimes(1); expect(store.getRepresentation).toHaveBeenCalledTimes(1); expect(store.getRepresentation).toHaveBeenLastCalledWith(operation.target, preferences, conditions); }); + + it('returns a 304 if the conditions do not match.', async(): Promise => { + operation.conditions = { + matchesMetadata: (): boolean => false, + }; + await expect(handler.handle({ operation })).rejects.toThrow(NotModifiedHttpError); + expect(data.destroy).toHaveBeenCalledTimes(2); + }); }); diff --git a/test/unit/http/output/metadata/ModifiedMetadataWriter.test.ts b/test/unit/http/output/metadata/ModifiedMetadataWriter.test.ts index 824fd2b1f..d0ad42bbd 100644 --- a/test/unit/http/output/metadata/ModifiedMetadataWriter.test.ts +++ b/test/unit/http/output/metadata/ModifiedMetadataWriter.test.ts @@ -2,21 +2,22 @@ import { createResponse } from 'node-mocks-http'; import { ModifiedMetadataWriter } from '../../../../../src/http/output/metadata/ModifiedMetadataWriter'; import { RepresentationMetadata } from '../../../../../src/http/representation/RepresentationMetadata'; import type { HttpResponse } from '../../../../../src/server/HttpResponse'; +import { getETag } from '../../../../../src/storage/Conditions'; import { updateModifiedDate } from '../../../../../src/util/ResourceUtil'; -import { DC } from '../../../../../src/util/Vocabularies'; +import { CONTENT_TYPE, DC } from '../../../../../src/util/Vocabularies'; describe('A ModifiedMetadataWriter', (): void => { const writer = new ModifiedMetadataWriter(); it('adds the Last-Modified and ETag header if there is dc:modified metadata.', async(): Promise => { const response = createResponse() as HttpResponse; - const metadata = new RepresentationMetadata(); + const metadata = new RepresentationMetadata({ [CONTENT_TYPE]: 'text/turtle' }); updateModifiedDate(metadata); const dateTime = metadata.get(DC.terms.modified)!.value; await expect(writer.handle({ response, metadata })).resolves.toBeUndefined(); expect(response.getHeaders()).toEqual({ 'last-modified': new Date(dateTime).toUTCString(), - etag: `"${new Date(dateTime).getTime()}"`, + etag: getETag(metadata), }); }); diff --git a/test/unit/storage/BasicConditions.test.ts b/test/unit/storage/BasicConditions.test.ts index 27bca8710..fa79c6db6 100644 --- a/test/unit/storage/BasicConditions.test.ts +++ b/test/unit/storage/BasicConditions.test.ts @@ -1,71 +1,82 @@ import { RepresentationMetadata } from '../../../src/http/representation/RepresentationMetadata'; import { BasicConditions } from '../../../src/storage/BasicConditions'; import { getETag } from '../../../src/storage/Conditions'; -import { DC } from '../../../src/util/Vocabularies'; +import { CONTENT_TYPE, DC } from '../../../src/util/Vocabularies'; + +function getMetadata(modified: Date, type = 'application/ld+json'): RepresentationMetadata { + return new RepresentationMetadata({ + [DC.modified]: `${modified.toISOString()}`, + [CONTENT_TYPE]: type, + }); +} describe('A BasicConditions', (): void => { const now = new Date(2020, 10, 20); const tomorrow = new Date(2020, 10, 21); const yesterday = new Date(2020, 10, 19); - const eTags = [ '123456', 'abcdefg' ]; + const turtleTag = getETag(getMetadata(now, 'text/turtle'))!; + const jsonLdTag = getETag(getMetadata(now))!; it('copies the input parameters.', async(): Promise => { + const eTags = [ '123456', 'abcdefg' ]; const options = { matchesETag: eTags, notMatchesETag: eTags, modifiedSince: now, unmodifiedSince: now }; expect(new BasicConditions(options)).toMatchObject(options); }); it('always returns false if notMatchesETag contains *.', async(): Promise => { const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); - expect(conditions.matches()).toBe(false); + expect(conditions.matchesMetadata(new RepresentationMetadata())).toBe(false); }); - it('requires matchesETag to contain the provided ETag.', async(): Promise => { - const conditions = new BasicConditions({ matchesETag: [ '1234' ]}); - expect(conditions.matches('abcd')).toBe(false); - expect(conditions.matches('1234')).toBe(true); + it('requires matchesETag to match the provided ETag timestamp.', async(): Promise => { + const conditions = new BasicConditions({ matchesETag: [ turtleTag ]}); + expect(conditions.matchesMetadata(getMetadata(yesterday))).toBe(false); + expect(conditions.matchesMetadata(getMetadata(now))).toBe(true); + }); + + it('requires matchesETag to match the exact provided ETag in strict mode.', async(): Promise => { + const turtleConditions = new BasicConditions({ matchesETag: [ turtleTag ]}); + const jsonLdConditions = new BasicConditions({ matchesETag: [ jsonLdTag ]}); + expect(turtleConditions.matchesMetadata(getMetadata(now), true)).toBe(false); + expect(jsonLdConditions.matchesMetadata(getMetadata(now), true)).toBe(true); }); it('supports all ETags if matchesETag contains *.', async(): Promise => { const conditions = new BasicConditions({ matchesETag: [ '*' ]}); - expect(conditions.matches('abcd')).toBe(true); - expect(conditions.matches('1234')).toBe(true); + expect(conditions.matchesMetadata(getMetadata(yesterday))).toBe(true); + expect(conditions.matchesMetadata(getMetadata(now))).toBe(true); }); - it('requires notMatchesETag to not contain the provided ETag.', async(): Promise => { - const conditions = new BasicConditions({ notMatchesETag: [ '1234' ]}); - expect(conditions.matches('1234')).toBe(false); - expect(conditions.matches('abcd')).toBe(true); + it('requires notMatchesETag to not match the provided ETag timestamp.', async(): Promise => { + const conditions = new BasicConditions({ notMatchesETag: [ turtleTag ]}); + expect(conditions.matchesMetadata(getMetadata(yesterday))).toBe(true); + expect(conditions.matchesMetadata(getMetadata(now))).toBe(false); + }); + + it('requires notMatchesETag to not match the exact provided ETag in strict mode.', async(): Promise => { + const turtleConditions = new BasicConditions({ notMatchesETag: [ turtleTag ]}); + const jsonLdConditions = new BasicConditions({ notMatchesETag: [ jsonLdTag ]}); + expect(turtleConditions.matchesMetadata(getMetadata(now), true)).toBe(true); + expect(jsonLdConditions.matchesMetadata(getMetadata(now), true)).toBe(false); }); it('requires lastModified to be after modifiedSince.', async(): Promise => { const conditions = new BasicConditions({ modifiedSince: now }); - expect(conditions.matches(undefined, yesterday)).toBe(false); - expect(conditions.matches(undefined, tomorrow)).toBe(true); + expect(conditions.matchesMetadata(getMetadata(yesterday))).toBe(false); + expect(conditions.matchesMetadata(getMetadata(tomorrow))).toBe(true); }); it('requires lastModified to be before unmodifiedSince.', async(): Promise => { const conditions = new BasicConditions({ unmodifiedSince: now }); - expect(conditions.matches(undefined, tomorrow)).toBe(false); - expect(conditions.matches(undefined, yesterday)).toBe(true); - }); - - it('can match based on the last modified date in the metadata.', async(): Promise => { - const metadata = new RepresentationMetadata({ [DC.modified]: now.toISOString() }); - const conditions = new BasicConditions({ - modifiedSince: yesterday, - unmodifiedSince: tomorrow, - matchesETag: [ getETag(metadata)! ], - notMatchesETag: [ '123456' ], - }); - expect(conditions.matchesMetadata(metadata)).toBe(true); + expect(conditions.matchesMetadata(getMetadata(yesterday))).toBe(true); + expect(conditions.matchesMetadata(getMetadata(tomorrow))).toBe(false); }); it('matches if no date is found in the metadata.', async(): Promise => { - const metadata = new RepresentationMetadata(); + const metadata = new RepresentationMetadata({ [CONTENT_TYPE]: 'text/turtle' }); const conditions = new BasicConditions({ modifiedSince: yesterday, unmodifiedSince: tomorrow, - matchesETag: [ getETag(metadata)! ], notMatchesETag: [ '123456' ], }); expect(conditions.matchesMetadata(metadata)).toBe(true); diff --git a/test/unit/storage/Conditions.test.ts b/test/unit/storage/Conditions.test.ts index 98a44e589..9a9eee469 100644 --- a/test/unit/storage/Conditions.test.ts +++ b/test/unit/storage/Conditions.test.ts @@ -1,17 +1,53 @@ import { RepresentationMetadata } from '../../../src/http/representation/RepresentationMetadata'; -import { getETag } from '../../../src/storage/Conditions'; -import { DC } from '../../../src/util/Vocabularies'; +import { getETag, isCurrentETag } from '../../../src/storage/Conditions'; +import { CONTENT_TYPE, DC } from '../../../src/util/Vocabularies'; describe('Conditions', (): void => { describe('#getETag', (): void => { - it('creates an ETag based on the date last modified.', async(): Promise => { + it('creates an ETag based on the date last modified and content-type.', async(): Promise => { const now = new Date(); - const metadata = new RepresentationMetadata({ [DC.modified]: now.toISOString() }); - expect(getETag(metadata)).toBe(`"${now.getTime()}"`); + const metadata = new RepresentationMetadata({ + [DC.modified]: now.toISOString(), + [CONTENT_TYPE]: 'text/turtle', + }); + expect(getETag(metadata)).toBe(`"${now.getTime()}-text/turtle"`); }); - it('returns undefined if no date was found.', async(): Promise => { + it('returns undefined if no date or content-type was found.', async(): Promise => { + const now = new Date(); expect(getETag(new RepresentationMetadata())).toBeUndefined(); + expect(getETag(new RepresentationMetadata({ [DC.modified]: now.toISOString() }))).toBeUndefined(); + expect(getETag(new RepresentationMetadata({ [CONTENT_TYPE]: 'text/turtle' }))).toBeUndefined(); + }); + }); + + describe('#isCurrentETag', (): void => { + const now = new Date(); + + it('compares an ETag with the current resource state.', async(): Promise => { + const metadata = new RepresentationMetadata({ + [DC.modified]: now.toISOString(), + [CONTENT_TYPE]: 'text/turtle', + }); + const eTag = getETag(metadata)!; + expect(isCurrentETag(eTag, metadata)).toBe(true); + expect(isCurrentETag('"ETag"', metadata)).toBe(false); + }); + + it('ignores the content-type.', async(): Promise => { + const metadata = new RepresentationMetadata({ + [DC.modified]: now.toISOString(), + [CONTENT_TYPE]: 'text/turtle', + }); + const eTag = getETag(metadata)!; + metadata.contentType = 'application/ld+json'; + expect(isCurrentETag(eTag, metadata)).toBe(true); + expect(isCurrentETag('"ETag"', metadata)).toBe(false); + }); + + it('returns false if the metadata has no last modified date.', async(): Promise => { + const metadata = new RepresentationMetadata(); + expect(isCurrentETag('"ETag"', metadata)).toBe(false); }); }); }); diff --git a/test/unit/storage/DataAccessorBasedStore.test.ts b/test/unit/storage/DataAccessorBasedStore.test.ts index 8d9f3d543..2ccda135b 100644 --- a/test/unit/storage/DataAccessorBasedStore.test.ts +++ b/test/unit/storage/DataAccessorBasedStore.test.ts @@ -19,7 +19,6 @@ import { ForbiddenHttpError } from '../../../src/util/errors/ForbiddenHttpError' import { MethodNotAllowedHttpError } from '../../../src/util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../../../src/util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; -import { NotModifiedHttpError } from '../../../src/util/errors/NotModifiedHttpError'; import { PreconditionFailedHttpError } from '../../../src/util/errors/PreconditionFailedHttpError'; import type { Guarded } from '../../../src/util/GuardedStream'; import { SingleRootIdentifierStrategy } from '../../../src/util/identifiers/SingleRootIdentifierStrategy'; @@ -218,18 +217,6 @@ describe('A DataAccessorBasedStore', (): void => { ); expect(result.metadata.contentType).toBe(INTERNAL_QUADS); }); - - it('throws a 304 if the request is a read type error.', async(): Promise => { - const resourceID = { path: root }; - const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); - await expect(store.getRepresentation(resourceID, undefined, conditions)).rejects.toThrow(NotModifiedHttpError); - }); - - it('has conditions but throws no error.', async(): Promise => { - const resourceID = { path: root }; - const conditions = new BasicConditions({ matchesETag: [ '*' ]}); - await expect(store.getRepresentation(resourceID, undefined, conditions)).resolves.toBeTruthy(); - }); }); describe('adding a Resource', (): void => { diff --git a/test/unit/util/ResourceUtil.test.ts b/test/unit/util/ResourceUtil.test.ts index f177f9658..d70b9187b 100644 --- a/test/unit/util/ResourceUtil.test.ts +++ b/test/unit/util/ResourceUtil.test.ts @@ -1,9 +1,18 @@ import 'jest-rdf'; +import type { Readable } from 'stream'; import type { NamedNode, Literal } from 'n3'; import { BasicRepresentation } from '../../../src/http/representation/BasicRepresentation'; import type { Representation } from '../../../src/http/representation/Representation'; import { RepresentationMetadata } from '../../../src/http/representation/RepresentationMetadata'; -import { addTemplateMetadata, cloneRepresentation, updateModifiedDate } from '../../../src/util/ResourceUtil'; +import type { Conditions } from '../../../src/storage/Conditions'; +import { NotModifiedHttpError } from '../../../src/util/errors/NotModifiedHttpError'; +import type { Guarded } from '../../../src/util/GuardedStream'; +import { + addTemplateMetadata, + assertReadConditions, + cloneRepresentation, + updateModifiedDate, +} from '../../../src/util/ResourceUtil'; import { CONTENT_TYPE_TERM, DC, SOLID_META, XSD } from '../../../src/util/Vocabularies'; describe('ResourceUtil', (): void => { @@ -59,4 +68,36 @@ describe('ResourceUtil', (): void => { expect(representation.metadata.contentType).not.toBe(res.metadata.contentType); }); }); + + describe('#assertReadConditions', (): void => { + let data: jest.Mocked>; + + beforeEach(async(): Promise => { + data = { + destroy: jest.fn(), + } as any; + representation.data = data; + }); + + it('does nothing if the conditions are undefined.', async(): Promise => { + expect((): any => assertReadConditions(representation)).not.toThrow(); + expect(data.destroy).toHaveBeenCalledTimes(0); + }); + + it('does nothing if the conditions match.', async(): Promise => { + const conditions: Conditions = { + matchesMetadata: (): boolean => true, + }; + expect((): any => assertReadConditions(representation, conditions)).not.toThrow(); + expect(data.destroy).toHaveBeenCalledTimes(0); + }); + + it('throws a NotModifiedHttpError if the conditions do not match.', async(): Promise => { + const conditions: Conditions = { + matchesMetadata: (): boolean => false, + }; + expect((): any => assertReadConditions(representation, conditions)).toThrow(NotModifiedHttpError); + expect(data.destroy).toHaveBeenCalledTimes(1); + }); + }); }); From 0a30be55ef433400965ff76581213b23f0e59a4e Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Tue, 18 Apr 2023 15:57:55 +0200 Subject: [PATCH 20/20] fix: Use EqualReadWriteLocker with file locker to prevent deadlocks --- config/util/resource-locker/file.json | 10 ++-------- src/util/locking/FileSystemResourceLocker.ts | 3 +++ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/config/util/resource-locker/file.json b/config/util/resource-locker/file.json index 9c067c155..a75ee80d7 100644 --- a/config/util/resource-locker/file.json +++ b/config/util/resource-locker/file.json @@ -6,18 +6,12 @@ "@id": "urn:solid-server:default:ResourceLocker", "@type": "WrappedExpiringReadWriteLocker", "locker": { - "@type": "GreedyReadWriteLocker", + "@type": "EqualReadWriteLocker", "locker": { "@id": "urn:solid-server:default:FileSystemResourceLocker", "@type": "FileSystemResourceLocker", "args_rootFilePath": { "@id": "urn:solid-server:default:variable:rootFilePath" } - }, - "storage": { - "@id": "urn:solid-server:default:LockStorage" - }, - "suffixes_count": "count", - "suffixes_read": "read", - "suffixes_write": "write" + } }, "expiration": 6000 }, diff --git a/src/util/locking/FileSystemResourceLocker.ts b/src/util/locking/FileSystemResourceLocker.ts index 57ed70a19..6ad8018cd 100644 --- a/src/util/locking/FileSystemResourceLocker.ts +++ b/src/util/locking/FileSystemResourceLocker.ts @@ -53,6 +53,9 @@ function isCodedError(err: unknown): err is { code: string } & Error { /** * A resource locker making use of the [proper-lockfile](https://www.npmjs.com/package/proper-lockfile) library. * Note that no locks are kept in memory, thus this is considered thread- and process-safe. + * While it stores the actual locks on disk, it also tracks them in memory for when they need to be released. + * This means only the worker thread that acquired a lock can release it again, + * making this implementation unusable in combination with a wrapping read/write lock implementation. * * This **proper-lockfile** library has its own retry mechanism for the operations, since a lock/unlock call will * either resolve successfully or reject immediately with the causing error. The retry function of the library