Repository: solid/community-server (ts)

Issue:
Remove dependency between registration and pod templates
From https://github.com/solid/community-server/pull/843#discussion_r675550645:

@RubenVerborgh: What is passed, I found quite worrying though:

```javascript
{
  email: 'ruben@verborgh.org',
  password: 'j04ch1mr0cks',
  confirmPassword: 'j04ch1mr0cks',
  createWebId: 'on',
  register: 'on',
  webId: 'http://localhost:3000/ruben/profile/card#me',
  createPod: 'on',
  podName: 'ruben',
  submit: '',
  oidcIssuer: 'http://localhost:3000/'
}
```

1. Probably shouldn't be sending password around.
2. This is arbitrary JSON from registration leaking all the way into pod generation; we likely want to decouple those because this reliance seems quite fragile.
3. Worse, it's even a dependency on the the exact HTML structure ('on' rather than boolean, submit).

---

@joachimvh:

>   1. Probably shouldn't be sending password around.

I thought about this when making the registration handler. In the end I didn't remove them because the only time this would be an issue is if you have templates storing the password. So that would imply that you just can't trust the server in general. Someone would have to write new templates that contain the password fields and set up a server with those. So the only risk is the person setting up the server themselves.

>   3. Worse, it's even a dependency on the the exact HTML structure (`'on'` rather than boolean, `submit`).

The data actually gets cleaned in the registration handler: https://github.com/solid/community-server/blob/fe8d579c72bb5b146e638647644252f05e4bd353/src/identity/interaction/email-password/handler/RegistrationHandler.ts#L49-L62

But indeed, that cleaned data is not what we send to the templates, there the raw data is used. The reason behind that was to support someone setting up a custom registration page that accepts more fields than we require to fill in the templates. E.g., full name, address, etc. To support that we need to send along all the fields that we get as input.

One thing we could potentially do to clean this up is to update the raw object with all the values from the interface above, but keep the raw values for the other ones.

Pull request:
fix: Clean pod creation input parameters during registration
Closes #845.

This does completely remove the option for using different pod templates/template configs that require new variables without creating a new RegistrationHandler. But nobody probably knows that you can change the pod templates or even that we have dynamic pods with template configs 😉 

Patch:
diff --git a/config/default.json b/config/default.json
index cfe4bd369..6a213cd47 100644
--- a/config/default.json
+++ b/config/default.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/config/dynamic.json b/config/dynamic.json
index 21dbabb24..eb1429a15 100644
--- a/config/dynamic.json
+++ b/config/dynamic.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/dynamic.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/config/example-https-file.json b/config/example-https-file.json
index f5e510def..bb913f216 100644
--- a/config/example-https-file.json
+++ b/config/example-https-file.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/config/file.json b/config/file.json
index 262a8fd96..befd58868 100644
--- a/config/file.json
+++ b/config/file.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/config/identity/README.md b/config/identity/README.md
index 12ff67e96..80fd6d187 100644
--- a/config/identity/README.md
+++ b/config/identity/README.md
@@ -23,3 +23,8 @@ What to use for pod creation.
 * *dynamic*: Every created pod has its own Components.js config for its ResourceStore,
   which can differ from the others.
 * *static*: All pod data is stored in separate containers in the same ResourceStore.
+
+## Registration
+If users should be able to register on the server.
+* *enabled*: Enables registration.
+* *disabled*: Disables registration.
diff --git a/config/identity/handler/interaction/handler.json b/config/identity/handler/interaction/handler.json
index 94e2b9553..02de864c8 100644
--- a/config/identity/handler/interaction/handler.json
+++ b/config/identity/handler/interaction/handler.json
@@ -4,7 +4,6 @@
     "files-scs:config/identity/handler/interaction/handlers/forgot-password.json",
     "files-scs:config/identity/handler/interaction/handlers/initial.json",
     "files-scs:config/identity/handler/interaction/handlers/login.json",
-    "files-scs:config/identity/handler/interaction/handlers/registration.json",
     "files-scs:config/identity/handler/interaction/handlers/reset-password.json",
     "files-scs:config/identity/handler/interaction/handlers/session.json"
   ],
@@ -15,7 +14,6 @@
       "@type": "WaterfallHandler",
       "handlers": [
         { "@id": "urn:solid-server:auth:password:InitialInteractionHandler" },
-        { "@id": "urn:solid-server:auth:password:RegistrationInteractionHandler" },
         { "@id": "urn:solid-server:auth:password:LoginInteractionHandler" },
         { "@id": "urn:solid-server:auth:password:SessionInteractionHandler" },
         { "@id": "urn:solid-server:auth:password:ForgotPasswordInteractionHandler" },
diff --git a/config/identity/registration/disabled.json b/config/identity/registration/disabled.json
new file mode 100644
index 000000000..e4627517a
--- /dev/null
+++ b/config/identity/registration/disabled.json
@@ -0,0 +1,8 @@
+{
+  "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld",
+  "@graph": [
+    {
+      "comment": "Disable registration by not attaching a registration handler."
+    }
+  ]
+}
diff --git a/config/identity/registration/enabled.json b/config/identity/registration/enabled.json
new file mode 100644
index 000000000..8c5194b3c
--- /dev/null
+++ b/config/identity/registration/enabled.json
@@ -0,0 +1,15 @@
+{
+  "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld",
+  "import": [
+    "files-scs:config/identity/registration/handler/registration.json"
+  ],
+  "@graph": [
+    {
+      "comment": "Enable registration by adding a registration handler to the list of interaction handlers.",
+      "@id": "urn:solid-server:auth:password:InteractionHttpHandler",
+      "WaterfallHandler:_handlers": [
+        { "@id": "urn:solid-server:auth:password:RegistrationInteractionHandler" }
+      ]
+    }
+  ]
+}
diff --git a/config/identity/handler/interaction/handlers/registration.json b/config/identity/registration/handler/registration.json
similarity index 100%
rename from config/identity/handler/interaction/handlers/registration.json
rename to config/identity/registration/handler/registration.json
diff --git a/config/memory-subdomains.json b/config/memory-subdomains.json
index 0b167817a..2a495e03d 100644
--- a/config/memory-subdomains.json
+++ b/config/memory-subdomains.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/config/path-routing.json b/config/path-routing.json
index 231a5647f..67e3df917 100644
--- a/config/path-routing.json
+++ b/config/path-routing.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/src/identity/interaction/email-password/handler/RegistrationHandler.ts b/src/identity/interaction/email-password/handler/RegistrationHandler.ts
index c396f0fcc..0b125752d 100644
--- a/src/identity/interaction/email-password/handler/RegistrationHandler.ts
+++ b/src/identity/interaction/email-password/handler/RegistrationHandler.ts
@@ -4,6 +4,7 @@ import type { ResourceIdentifier } from '../../../../ldp/representation/Resource
 import { getLoggerFor } from '../../../../logging/LogUtil';
 import type { IdentifierGenerator } from '../../../../pods/generate/IdentifierGenerator';
 import type { PodManager } from '../../../../pods/PodManager';
+import type { PodSettings } from '../../../../pods/settings/PodSettings';
 import type { HttpHandlerInput } from '../../../../server/HttpHandler';
 import { HttpHandler } from '../../../../server/HttpHandler';
 import type { HttpRequest } from '../../../../server/HttpRequest';
@@ -48,17 +49,29 @@ export interface RegistrationHandlerArgs {
 
 /**
  * All the parameters that will be parsed from a request.
- * `data` contains all the raw values to potentially be used by pod templates.
  */
-interface ParseResult {
+interface ParsedInput {
   email: string;
+  webId?: string;
   password?: string;
   podName?: string;
+  template?: string;
+  createWebId: boolean;
+  register: boolean;
+  createPod: boolean;
+}
+
+/**
+ * The results that will be applied to the response template.
+ */
+interface RegistrationResponse {
+  email: string;
   webId?: string;
+  oidcIssuer?: string;
+  podBaseUrl?: string;
   createWebId: boolean;
   register: boolean;
   createPod: boolean;
-  data: NodeJS.Dict<string>;
 }
 
 /**
@@ -103,7 +116,9 @@ export class RegistrationHandler extends HttpHandler {
       const contents = await this.register(result);
       await this.responseHandler.handleSafe({ response, contents });
     } catch (error: unknown) {
-      throwIdpInteractionError(error, result.data as Record<string, string>);
+      // Don't expose the password field
+      delete result.password;
+      throwIdpInteractionError(error, result as Record<string, any>);
     }
   }
 
@@ -111,14 +126,16 @@ export class RegistrationHandler extends HttpHandler {
    * Does the full registration and pod creation process,
    * with the steps chosen by the values in the `ParseResult`.
    */
-  private async register(result: ParseResult): Promise<NodeJS.Dict<any>> {
+  private async register(result: ParsedInput): Promise<RegistrationResponse> {
     // This is only used when createWebId and/or createPod are true
     let podBaseUrl: ResourceIdentifier | undefined;
+    if (result.createWebId || result.createPod) {
+      podBaseUrl = this.identifierGenerator.generate(result.podName!);
+    }
 
     // Create or verify the WebID
     if (result.createWebId) {
-      podBaseUrl = this.identifierGenerator.generate(result.podName!);
-      result.webId = urljoin(podBaseUrl.path, this.webIdSuffix);
+      result.webId = urljoin(podBaseUrl!.path, this.webIdSuffix);
     } else {
       await this.ownershipValidator.handleSafe({ webId: result.webId! });
     }
@@ -126,20 +143,24 @@ export class RegistrationHandler extends HttpHandler {
     // Register the account
     if (result.register) {
       await this.accountStore.create(result.email, result.webId!, result.password!);
-
-      // Add relevant data for the templates
-      result.data.oidcIssuer = this.baseUrl;
     }
 
     // Create the pod
     if (result.createPod) {
-      podBaseUrl = podBaseUrl ?? this.identifierGenerator.generate(result.podName!);
+      const podSettings: PodSettings = {
+        email: result.email,
+        webId: result.webId!,
+        template: result.template,
+        podBaseUrl: podBaseUrl!.path,
+      };
+
+      // Set the OIDC issuer to our server when registering with the IDP
+      if (result.register) {
+        podSettings.oidcIssuer = this.baseUrl;
+      }
+
       try {
-        await this.podManager.createPod(podBaseUrl, {
-          ...result.data,
-          podBaseUrl: podBaseUrl.path,
-          webId: result.webId!,
-        });
+        await this.podManager.createPod(podBaseUrl!, podSettings);
       } catch (error: unknown) {
         // In case pod creation errors we don't want to keep the account
         if (result.register) {
@@ -170,7 +191,7 @@ export class RegistrationHandler extends HttpHandler {
   /**
    * Parses the input request into a `ParseResult`.
    */
-  private async parseInput(request: HttpRequest): Promise<ParseResult> {
+  private async parseInput(request: HttpRequest): Promise<ParsedInput> {
     const parsed = await getFormDataRequestBody(request);
     let prefilled: Record<string, string> = {};
     try {
@@ -190,18 +211,18 @@ export class RegistrationHandler extends HttpHandler {
    * Converts the raw input date into a `ParseResult`.
    * Verifies that all the data combinations make sense.
    */
-  private validateInput(parsed: NodeJS.Dict<string>): ParseResult {
-    const { email, password, confirmPassword, podName, webId, createWebId, register, createPod } = parsed;
+  private validateInput(parsed: NodeJS.Dict<string>): ParsedInput {
+    const { email, password, confirmPassword, podName, webId, template, createWebId, register, createPod } = parsed;
 
     assert(typeof email === 'string' && email.length > 0 && emailRegex.test(email),
       'A valid e-mail address is required');
 
-    const result: ParseResult = {
+    const result: ParsedInput = {
       email,
+      template,
       createWebId: Boolean(createWebId),
       register: Boolean(register),
       createPod: Boolean(createPod),
-      data: parsed,
     };
 
     const validWebId = typeof webId === 'string' && webId.length > 0;


Test patch:
diff --git a/test/integration/config/server-dynamic-unsafe.json b/test/integration/config/server-dynamic-unsafe.json
index 2d5064226..0e5cdef13 100644
--- a/test/integration/config/server-dynamic-unsafe.json
+++ b/test/integration/config/server-dynamic-unsafe.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/unsafe-no-check.json",
     "files-scs:config/identity/pod/dynamic.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/debug-auth-header.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/test/integration/config/server-memory.json b/test/integration/config/server-memory.json
index 4bc3bf1f6..122c004c6 100644
--- a/test/integration/config/server-memory.json
+++ b/test/integration/config/server-memory.json
@@ -10,6 +10,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/token.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/dpop-bearer.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/test/integration/config/server-subdomains-unsafe.json b/test/integration/config/server-subdomains-unsafe.json
index 0c972c83e..741180e25 100644
--- a/test/integration/config/server-subdomains-unsafe.json
+++ b/test/integration/config/server-subdomains-unsafe.json
@@ -11,6 +11,7 @@
     "files-scs:config/identity/handler/default.json",
     "files-scs:config/identity/ownership/unsafe-no-check.json",
     "files-scs:config/identity/pod/static.json",
+    "files-scs:config/identity/registration/enabled.json",
     "files-scs:config/ldp/authentication/debug-auth-header.json",
     "files-scs:config/ldp/authorization/webacl.json",
     "files-scs:config/ldp/handler/default.json",
diff --git a/test/unit/identity/interaction/email-password/handler/RegistrationHandler.test.ts b/test/unit/identity/interaction/email-password/handler/RegistrationHandler.test.ts
index d84f6be1c..278ca1a1f 100644
--- a/test/unit/identity/interaction/email-password/handler/RegistrationHandler.test.ts
+++ b/test/unit/identity/interaction/email-password/handler/RegistrationHandler.test.ts
@@ -8,6 +8,7 @@ import type { OwnershipValidator } from '../../../../../../src/identity/ownershi
 import type { ResourceIdentifier } from '../../../../../../src/ldp/representation/ResourceIdentifier';
 import type { IdentifierGenerator } from '../../../../../../src/pods/generate/IdentifierGenerator';
 import type { PodManager } from '../../../../../../src/pods/PodManager';
+import type { PodSettings } from '../../../../../../src/pods/settings/PodSettings';
 import type { HttpRequest } from '../../../../../../src/server/HttpRequest';
 import type { HttpResponse } from '../../../../../../src/server/HttpResponse';
 import type { TemplateHandler } from '../../../../../../src/server/util/TemplateHandler';
@@ -31,6 +32,7 @@ describe('A RegistrationHandler', (): void => {
 
   const baseUrl = 'http://test.com/';
   const webIdSuffix = '/profile/card';
+  let podSettings: PodSettings;
   let identifierGenerator: IdentifierGenerator;
   let ownershipValidator: OwnershipValidator;
   let accountStore: AccountStore;
@@ -39,6 +41,8 @@ describe('A RegistrationHandler', (): void => {
   let handler: RegistrationHandler;
 
   beforeEach(async(): Promise<void> => {
+    podSettings = { email, webId, podBaseUrl };
+
     identifierGenerator = {
       generate: jest.fn((name: string): ResourceIdentifier => ({ path: `${baseUrl}${name}/` })),
     };
@@ -179,9 +183,7 @@ describe('A RegistrationHandler', (): void => {
       expect(identifierGenerator.generate).toHaveBeenCalledTimes(1);
       expect(identifierGenerator.generate).toHaveBeenLastCalledWith(podName);
       expect(podManager.createPod).toHaveBeenCalledTimes(1);
-      expect(podManager.createPod).toHaveBeenLastCalledWith(
-        { path: `${baseUrl}${podName}/` }, { podBaseUrl, ...params },
-      );
+      expect(podManager.createPod).toHaveBeenLastCalledWith({ path: `${baseUrl}${podName}/` }, podSettings);
 
       expect(accountStore.create).toHaveBeenCalledTimes(0);
       expect(accountStore.verify).toHaveBeenCalledTimes(0);
@@ -190,6 +192,7 @@ describe('A RegistrationHandler', (): void => {
 
     it('adds an oidcIssuer to the data when doing both IDP registration and pod creation.', async(): Promise<void> => {
       const params = { email, webId, password, confirmPassword, podName, register, createPod };
+      podSettings.oidcIssuer = baseUrl;
       request = createPostFormRequest(params);
       await expect(handler.handle({ request, response })).resolves.toBeUndefined();
 
@@ -199,11 +202,8 @@ describe('A RegistrationHandler', (): void => {
       expect(accountStore.create).toHaveBeenLastCalledWith(email, webId, password);
       expect(identifierGenerator.generate).toHaveBeenCalledTimes(1);
       expect(identifierGenerator.generate).toHaveBeenLastCalledWith(podName);
-      (params as any).oidcIssuer = baseUrl;
       expect(podManager.createPod).toHaveBeenCalledTimes(1);
-      expect(podManager.createPod).toHaveBeenLastCalledWith(
-        { path: `${baseUrl}${podName}/` }, { podBaseUrl, ...params },
-      );
+      expect(podManager.createPod).toHaveBeenLastCalledWith({ path: `${baseUrl}${podName}/` }, podSettings);
       expect(accountStore.verify).toHaveBeenCalledTimes(1);
       expect(accountStore.verify).toHaveBeenLastCalledWith(email);
 
@@ -212,6 +212,7 @@ describe('A RegistrationHandler', (): void => {
 
     it('deletes the created account if pod generation fails.', async(): Promise<void> => {
       const params = { email, webId, password, confirmPassword, podName, register, createPod };
+      podSettings.oidcIssuer = baseUrl;
       request = createPostFormRequest(params);
       (podManager.createPod as jest.Mock).mockRejectedValueOnce(new Error('pod error'));
       await expect(handler.handle({ request, response })).rejects.toThrow('pod error');
@@ -222,11 +223,8 @@ describe('A RegistrationHandler', (): void => {
       expect(accountStore.create).toHaveBeenLastCalledWith(email, webId, password);
       expect(identifierGenerator.generate).toHaveBeenCalledTimes(1);
       expect(identifierGenerator.generate).toHaveBeenLastCalledWith(podName);
-      (params as any).oidcIssuer = baseUrl;
       expect(podManager.createPod).toHaveBeenCalledTimes(1);
-      expect(podManager.createPod).toHaveBeenLastCalledWith(
-        { path: `${baseUrl}${podName}/` }, { podBaseUrl, ...params },
-      );
+      expect(podManager.createPod).toHaveBeenLastCalledWith({ path: `${baseUrl}${podName}/` }, podSettings);
       expect(accountStore.deleteAccount).toHaveBeenCalledTimes(1);
       expect(accountStore.deleteAccount).toHaveBeenLastCalledWith(email);
 
@@ -235,10 +233,12 @@ describe('A RegistrationHandler', (): void => {
 
     it('can create a WebID with an account and pod.', async(): Promise<void> => {
       const params = { email, password, confirmPassword, podName, createWebId, register, createPod };
+      podSettings.oidcIssuer = baseUrl;
       request = createPostFormRequest(params);
       await expect(handler.handle({ request, response })).resolves.toBeUndefined();
 
       const generatedWebID = urljoin(baseUrl, podName, webIdSuffix);
+      podSettings.webId = generatedWebID;
 
       expect(identifierGenerator.generate).toHaveBeenCalledTimes(1);
       expect(identifierGenerator.generate).toHaveBeenLastCalledWith(podName);
@@ -247,9 +247,7 @@ describe('A RegistrationHandler', (): void => {
       expect(accountStore.verify).toHaveBeenCalledTimes(1);
       expect(accountStore.verify).toHaveBeenLastCalledWith(email);
       expect(podManager.createPod).toHaveBeenCalledTimes(1);
-      expect(podManager.createPod).toHaveBeenLastCalledWith(
-        { path: `${baseUrl}${podName}/` }, { ...params, podBaseUrl, oidcIssuer: baseUrl, webId: generatedWebID },
-      );
+      expect(podManager.createPod).toHaveBeenLastCalledWith({ path: `${baseUrl}${podName}/` }, podSettings);
 
       expect(ownershipValidator.handleSafe).toHaveBeenCalledTimes(0);
       expect(accountStore.deleteAccount).toHaveBeenCalledTimes(0);
@@ -262,7 +260,13 @@ describe('A RegistrationHandler', (): void => {
       const prom = handler.handle({ request, response });
       await expect(prom).rejects.toThrow('pod error');
       await expect(prom).rejects.toThrow(IdpInteractionError);
-      await expect(prom).rejects.toThrow(expect.objectContaining({ prefilled: params }));
+      // Using the cleaned input for prefilled
+      await expect(prom).rejects.toThrow(expect.objectContaining({ prefilled: {
+        ...params,
+        createWebId: false,
+        register: false,
+        createPod: true,
+      }}));
     });
   });
 });