Require deleteMetadata to metadata manager (#419)

* Require deleteMetadata to metadata manager

* Update metadata-manager.ts

* Update src/settings-manager/metadata-manager.ts

Co-authored-by: Krzysztof Żuraw <9116238+krzysztofzuraw@users.noreply.github.com>

* fix lint

* fix build

* fix build

---------

Co-authored-by: Krzysztof Żuraw <9116238+krzysztofzuraw@users.noreply.github.com>

Author: lkostrowski

.changeset/tangy-rings-retire.md

@@ -0,0 +1,5 @@
+---
+"@saleor/app-sdk": major
+---
+
+MetadataManager now requires deleteMetadata to be defined

src/auth/verify-signature.test.ts

@@ -0,0 +1,24 @@
+import { describe, expect, it } from "vitest";
+
+import { verifySignatureWithJwks } from "@/auth/verify-signature";
+
+/**
+ * Actual signature copied from testing webhook payload.
+ */
+const testSignature =
+  "eyJhbGciOiJSUzI1NiIsImI2NCI6ZmFsc2UsImNyaXQiOlsiYjY0Il0sImlzcyI6Imh0dHBzOi8vaGFja2F0aG9uLXNoaXBwaW5nLmV1LnNhbGVvci5jbG91ZC9ncmFwaHFsLyIsImtpZCI6InlLdzlYeUVmOHgyYnd5SkdCWVlTckhzeXpHaHhKaktLdlpkb2tOZTlGSWMiLCJ0eXAiOiJKV1QifQ..ShPbfvYc_A5Aq3hiT6sisDclKikDkhxOvGXT2ZWgdsGRjZpg9ukiHRZym0kbfMfDqU5C3Pfo6n7am0ExwbnFWBfOil3pfe3uJOcOn_UGRj76Fy-59TB0JdS_WuTgNQcYM8Yjvlq2sNK4jdAfJVRTTx8FVgEpFrHBKmcMPfD7zuDozswIDMZOkklYqBcyQ76DJYIRVhl3QsktYPPrxDoqf-GJ--e9FuNqtqNDksP1weiDSraqXCF4-Ie7UWZsMIFxkPF8jdKjF_s1UmNS8Xel8soFQQ9L6Gps-NEv7xcHicGt5lgohH4mqhz1YIxCR7v_NTQgWImu_GQ6ELBiBSIZ2Q";
+
+const rawContent = "{\"__typename\": \"OrderCreated\"}";
+
+const jwks =
+  "{\"keys\": [{\"kty\": \"RSA\", \"key_ops\": [\"verify\"], \"n\": \"uDhbbpspufsQiqHsmC4kvmFQ5l2mGZsGcWhKVSQKQubSdXMedPpLnPD3Z3DsY76DILTm6WfOtSp5rr4KzF5wjurlOEhuFsB1HUfK9ZZB2nEDCQbweoIv3SOdclaNB__pYvQ0nmQHwsAeqH1QUuFUIvOL3t31rhjvzX6wvS49fGNb7rDlqQjufCvaX_n-ADJTgEAg6y1Mzn5NhgoTV1KTBeviyZqCdwvD6bk1ghN2XXWpNcARTzu3WHrmzIzkTwQeIMG8efwIddjfCaMGiOzAfzdQlqHlHPL1Xb5kV9AVX3kiSy-9shaQY23HdWwwiodrb4k2w34Z9ZZN-MHp8i6JdQ\", \"e\": \"AQAB\", \"use\": \"sig\", \"kid\": \"yKw9XyEf8x2bwyJGBYYSrHsyzGhxJjKKvZdokNe9FIc\"}]}";
+
+describe("verifySignatureWithJwks", () => {
+  it("Returns empty promise if signature is valid", () =>
+    expect(verifySignatureWithJwks(jwks, testSignature, rawContent)).resolves.not.toThrow());
+
+  it("Throws if signature is invalid", () =>
+    expect(
+      verifySignatureWithJwks(jwks, testSignature, "{\"forged\": \"payload\"}"),
+    ).rejects.toThrow());
+});

src/auth/verify-signature.ts

@@ -7,8 +7,6 @@ const debug = createDebug("verify-signature");
 /**
  * Verify the Webhook payload signature from provided JWKS string.
  * JWKS can be cached to avoid unnecessary calls.
- *
- * TODO: Add test
  */
 export const verifySignatureWithJwks = async (jwks: string, signature: string, rawBody: string) => {
   const [header, , jwsSignature] = signature.split(".");
@@ -22,14 +20,14 @@ export const verifySignatureWithJwks = async (jwks: string, signature: string, r
 
   try {
     const parsedJWKS = JSON.parse(jwks);
+
     localJwks = jose.createLocalJWKSet(parsedJWKS) as jose.FlattenedVerifyGetKey;
   } catch {
     debug("Could not create local JWKSSet from given data: %s", jwks);
+
     throw new Error("JWKS verification failed - could not parse given JWKS");
   }
 
-  debug("Created remote JWKS");
-
   try {
     await jose.flattenedVerify(jws, localJwks);
     debug("JWKS verified");

src/handlers/shared/saleor-webhook-validator.test.ts

@@ -230,7 +230,6 @@ describe("SaleorWebhookValidator", () => {
     });
   });
 
-  // TODO: This should be required
   it("Fallbacks to null if version is missing in payload", async () => {
     vi.spyOn(adapter, "getRawBody").mockResolvedValue(JSON.stringify({}));
     vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue(validHeaders);

src/settings-manager/encrypted-metadata-manager.test.ts

@@ -40,9 +40,9 @@ describe("settings-manager", () => {
               mutateMetadata: mutateMock,
               // @ts-expect-error
               encryptionKey: undefined,
-            })
+            }),
         ).toThrowError(
-          "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager"
+          "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager",
         );
       });
 
@@ -70,6 +70,7 @@ describe("settings-manager", () => {
         fetchMetadata: fetchMock,
         mutateMetadata: mutateMock,
         encryptionKey: "key",
+        deleteMetadata: deleteMetadataMock,
       });
 
       beforeEach(() => {
@@ -112,6 +113,7 @@ describe("settings-manager", () => {
           encryptionKey: "key",
           encryptionMethod: customEncrypt,
           decryptionMethod: customDecrypt,
+          deleteMetadata: deleteMetadataMock,
         });
 
         await customManager.set(newEntry);

src/settings-manager/encrypted-metadata-manager.ts

@@ -80,11 +80,11 @@ export class EncryptedMetadataManager implements SettingsManager {
       if (process.env.NODE_ENV === "production") {
         console.error("Can't start the application without the secret key.");
         throw new Error(
-          "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager"
+          "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager",
         );
       }
       console.warn(
-        "WARNING: Encrypted Metadata Manager encryption key has not been set. For production deployments, it need's to be set"
+        "WARNING: Encrypted Metadata Manager encryption key has not been set. For production deployments, it need's to be set",
       );
       console.warn("Using placeholder value for the development.");
       this.encryptionKey = "CHANGE_ME";

src/settings-manager/metadata-manager.test.ts

@@ -21,7 +21,11 @@ describe("settings-manager", () => {
     });
 
     it("Get method - using cached values", async () => {
-      const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock });
+      const manager = new MetadataManager({
+        fetchMetadata: fetchMock,
+        mutateMetadata: mutateMock,
+        deleteMetadata: deleteMetadataMock,
+      });
       expect(fetchMock).toBeCalledTimes(0);
 
       // Fetch should be called just after getting a first value
@@ -38,15 +42,23 @@ describe("settings-manager", () => {
     });
 
     it("Get method - return values for chosen domain", async () => {
-      const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock });
+      const manager = new MetadataManager({
+        fetchMetadata: fetchMock,
+        mutateMetadata: mutateMock,
+        deleteMetadata: deleteMetadataMock,
+      });
 
       expect(await manager.get("a", "x.com")).toBe(entryForDomainX.value);
       expect(await manager.get("a", "y.com")).toBe(entryForDomainY.value);
       expect(await manager.get("a", "unknown.com")).toBe(undefined);
     });
 
     it("Set method - return values for chosen domain", async () => {
-      const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock });
+      const manager = new MetadataManager({
+        fetchMetadata: fetchMock,
+        mutateMetadata: mutateMock,
+        deleteMetadata: deleteMetadataMock,
+      });
       const newEntry = { key: "new", value: "new value" };
 
       await manager.set(newEntry);
@@ -64,22 +76,12 @@ describe("settings-manager", () => {
         const manager = new MetadataManager({
           fetchMetadata: fetchMock,
           mutateMetadata: mutateMock,
+          deleteMetadata: deleteMetadataMock,
         });
 
         expect(manager).toBeDefined();
       });
 
-      it("Throws if \"delete\" method is called, but deleteMetadata was not passed to constructor", async () => {
-        const manager = new MetadataManager({
-          fetchMetadata: fetchMock,
-          mutateMetadata: mutateMock,
-        });
-
-        await expect(manager.delete("test")).rejects.toThrowErrorMatchingInlineSnapshot(
-          "[Error: Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor]"
-        );
-      });
-
       it("Calls deleteMetadata constructor param when \"delete\" method called", async () => {
         const manager = new MetadataManager({
           fetchMetadata: fetchMock,

src/settings-manager/metadata-manager.ts

@@ -41,11 +41,7 @@ const serializeSettingsToMetadata = ({ key, value, domain }: SettingsValue): Met
 export interface MetadataManagerConfig {
   fetchMetadata: FetchMetadataCallback;
   mutateMetadata: MutateMetadataCallback;
-  /**
-   * Keep it optional, to avoid breaking changes. If not provided, delete will throw.
-   * TODO: Make it required in next major version
-   */
-  deleteMetadata?: DeleteMetadataCallback;
+  deleteMetadata: DeleteMetadataCallback;
 }
 
 /**
@@ -62,7 +58,7 @@ export class MetadataManager implements SettingsManager {
 
   private mutateMetadata: MutateMetadataCallback;
 
-  private deleteMetadata?: DeleteMetadataCallback;
+  private deleteMetadata: DeleteMetadataCallback;
 
   constructor({ fetchMetadata, mutateMetadata, deleteMetadata }: MetadataManagerConfig) {
     this.fetchMetadata = fetchMetadata;
@@ -98,7 +94,7 @@ export class MetadataManager implements SettingsManager {
   async delete(args: DeleteSettingsValue | DeleteSettingsValue[] | string | string[]) {
     if (!this.deleteMetadata) {
       throw new Error(
-        "Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor"
+        "Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor",
       );
     }