SDK-3189: Fix normalization of denied request (#758)

Access Grants were not normalized on denial, while they were on approval. This fix aligns the JSON-LD frame returned when approving and denying an Access Grant. --------- Co-authored-by: Jesse Wright <[email protected]>
inrupt · Sep 18, 2023 · 320a4d6 · 320a4d6 · vercel · Sep 18, 2023
1 parent a22ab74
commit 320a4d6
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 83 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,13 @@ The following changes are pending, and will be applied on the next major release
 
 ## Unreleased
 
+### Bugfixes
+
+- `denyAccessRequest` didn't normalize the returned denied Access Grant, resulting in it having a
+  JSON-LD frame different from the value returned by `approveAccessRequest`. The value is now normalized,
+  and both functions return a similarly shaped object. This also fixes the return type of `denyAccessRequest`,
+  which now returns the more strict `AccessGrant` type rather than the `VerifiableCredential` type.
+
 ## [2.6.0](https://github.com/inrupt/solid-client-access-grants-js/releases/tag/v2.6.0) - 2023-09-18
 
 ### New feature

diff --git a/e2e/node/e2e.test.ts b/e2e/node/e2e.test.ts
@@ -48,6 +48,7 @@ import {
   createContainerInContainer,
   denyAccessRequest,
   getAccessApiEndpoint,
+  getAccessGrant,
   getAccessGrantAll,
   getFile,
   getResources,
@@ -527,14 +528,10 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
 
   describe("access request, deny flow", () => {
     it("can issue an access grant denying an access request", async () => {
-      const grant = await denyAccessRequest(
-        resourceOwnerSession.info.webId as string,
-        sharedRequest,
-        {
-          fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
-          accessEndpoint: vcProvider,
-        },
-      );
+      const grant = await denyAccessRequest(sharedRequest, {
+        fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
+        accessEndpoint: vcProvider,
+      });
 
       await expect(
         isValidAccessGrant(grant, {
@@ -564,6 +561,12 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
         TEST_USER_AGENT,
       )(sharedFileIri);
       expect(fileResponse.status).toBe(403);
+
+      // Retrieving the grant should still be possible:
+      const retrievedGrant = await getAccessGrant(grant.id, {
+        fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
+      });
+      expect(retrievedGrant).toStrictEqual(grant);
     });
   });
 
@@ -624,8 +627,10 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
 
     it("can filter VCs held by the service based on requestor", async () => {
       const allGrants = getAccessGrantAll(
-        sharedFilterTestIri,
-        { requestor: requestorSession.info.webId as string },
+        {
+          requestor: requestorSession.info.webId as string,
+          resource: sharedFilterTestIri,
+        },
         {
           fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
           accessEndpoint: vcProvider,
@@ -647,8 +652,10 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
 
       await expect(
         getAccessGrantAll(
-          sharedFilterTestIri,
-          { requestor: "https://some.unknown.requestor" },
+          {
+            requestor: "https://some.unknown.requestor",
+            resource: sharedFilterTestIri,
+          },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
@@ -675,46 +682,47 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
         ),
       ).resolves.toHaveLength(1);
       await expect(
-        getAccessGrantAll("https://some.unkown.resource", undefined, {
-          fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
-          accessEndpoint: vcProvider,
-        }),
+        getAccessGrantAll(
+          { resource: "https://some.unkown.resource" },
+          {
+            fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
+            accessEndpoint: vcProvider,
+          },
+        ),
       ).resolves.toHaveLength(0);
     });
 
     it("can filter VCs held by the service based on status", async () => {
       const [granted, denied, both, unspecified] = await Promise.all([
         getAccessGrantAll(
-          sharedFilterTestIri,
           {
             status: "granted",
+            resource: sharedFilterTestIri,
           },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
           },
         ),
         getAccessGrantAll(
-          sharedFilterTestIri,
           {
             status: "denied",
+            resource: sharedFilterTestIri,
           },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
           },
         ),
         getAccessGrantAll(
-          sharedFilterTestIri,
-          { status: "all" },
+          { status: "all", resource: sharedFilterTestIri },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
           },
         ),
         getAccessGrantAll(
-          sharedFilterTestIri,
-          {},
+          { resource: sharedFilterTestIri },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
@@ -746,10 +754,9 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
             ...denyGrant.credentialSubject,
             providedConsent: {
               ...(denyGrant.credentialSubject.providedConsent as any),
-              forPersonalData: [
-                (denyGrant.credentialSubject.providedConsent as any)
-                  .forPersonalData,
-              ],
+              forPersonalData: (
+                denyGrant.credentialSubject.providedConsent as any
+              ).forPersonalData,
             },
           },
         },
@@ -764,42 +771,51 @@ describe(`End-to-end access grant tests for environment [${environment}]`, () =>
         bothPurposeFilter,
         unknownPurposeFilter,
       ] = await Promise.all([
-        getAccessGrantAll(sharedFilterTestIri, undefined, {
-          fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
-          accessEndpoint: vcProvider,
-        }),
         getAccessGrantAll(
-          sharedFilterTestIri,
-          { purpose: ["https://some.purpose/not-a-nefarious-one/i-promise"] },
+          { resource: sharedFilterTestIri },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
           },
         ),
         getAccessGrantAll(
-          sharedFilterTestIri,
-          { purpose: ["https://some.other.purpose/"] },
+          {
+            purpose: ["https://some.purpose/not-a-nefarious-one/i-promise"],
+            resource: sharedFilterTestIri,
+          },
+          {
+            fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
+            accessEndpoint: vcProvider,
+          },
+        ),
+        getAccessGrantAll(
+          {
+            purpose: ["https://some.other.purpose/"],
+            resource: sharedFilterTestIri,
+          },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
           },
         ),
         getAccessGrantAll(
-          sharedFilterTestIri,
           {
             purpose: [
               "https://some.purpose/not-a-nefarious-one/i-promise",
               "https://some.other.purpose/",
             ],
+            resource: sharedFilterTestIri,
           },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,
           },
         ),
         getAccessGrantAll(
-          sharedFilterTestIri,
-          { purpose: ["https://some.unknown.purpose/"] },
+          {
+            purpose: ["https://some.unknown.purpose/"],
+            resource: sharedFilterTestIri,
+          },
           {
             fetch: addUserAgent(resourceOwnerSession.fetch, TEST_USER_AGENT),
             accessEndpoint: vcProvider,

diff --git a/src/gConsent/manage/denyAccessRequest.test.ts b/src/gConsent/manage/denyAccessRequest.test.ts
@@ -24,11 +24,12 @@ import { Response } from "@inrupt/universal-fetch";
 import type * as CrossFetch from "@inrupt/universal-fetch";
 
 import { denyAccessRequest } from "./denyAccessRequest";
-import { mockAccessRequestVc } from "../util/access.mock";
+import { mockAccessGrantVc, mockAccessRequestVc } from "../util/access.mock";
 import {
   mockAccessApiEndpoint,
   MOCKED_ACCESS_ISSUER,
 } from "../request/request.mock";
+import type { AccessGrant } from "../type/AccessGrant";
 
 jest.mock("@inrupt/solid-client", () => {
   const solidClientModule = jest.requireActual("@inrupt/solid-client") as any;
@@ -72,13 +73,14 @@ describe("denyAccessRequest", () => {
   });
 
   it("uses the provided access endpoint, if any", async () => {
+    mockAccessApiEndpoint();
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
+
     await denyAccessRequest(mockAccessRequestVc(), {
       accessEndpoint: "https://some.access-endpoint.override/",
       fetch: jest.fn<typeof fetch>(),
@@ -95,19 +97,15 @@ describe("denyAccessRequest", () => {
     mockAccessApiEndpoint();
     const mockedFetch = jest.fn(global.fetch);
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
-    await denyAccessRequest(
-      "https://some.resource/owner",
-      mockAccessRequestVc(),
-      {
-        fetch: mockedFetch,
-      },
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
+
+    await denyAccessRequest(mockAccessRequestVc(), {
+      fetch: mockedFetch,
+    });
     expect(spiedIssueRequest).toHaveBeenCalledWith(
       expect.anything(),
       expect.anything(),
@@ -119,12 +117,11 @@ describe("denyAccessRequest", () => {
   it("issues a proper denied access VC", async () => {
     mockAccessApiEndpoint();
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
     const accessRequestWithPurpose = mockAccessRequestVc({
       purpose: ["https://example.org/some-purpose"],
     });
@@ -158,12 +155,11 @@ describe("denyAccessRequest", () => {
   it("issues a proper denied access VC from a given access request VC IRI", async () => {
     mockAccessApiEndpoint();
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
     const mockedFetch = jest
       .fn(global.fetch)
       .mockResolvedValueOnce(
@@ -194,12 +190,12 @@ describe("denyAccessRequest", () => {
   it("can take a URL as VC IRI parameter", async () => {
     mockAccessApiEndpoint();
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
+
     const mockedFetch = jest
       .fn(global.fetch)
       .mockResolvedValueOnce(
@@ -231,12 +227,13 @@ describe("denyAccessRequest", () => {
   it("issues a proper denied access VC using the deprecated signature and VC value", async () => {
     mockAccessApiEndpoint();
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
+
+    // This explicitly tests the deprecated signature.
     await denyAccessRequest(
       "https://some.resource.owner",
       mockAccessRequestVc(),
@@ -268,12 +265,12 @@ describe("denyAccessRequest", () => {
   it("issues a proper denied access VC using the deprecated signature and VC IRI", async () => {
     mockAccessApiEndpoint();
     const mockedVcModule = jest.requireMock("@inrupt/solid-client-vc") as {
-      issueVerifiableCredential: () => unknown;
+      issueVerifiableCredential: () => Promise<AccessGrant>;
     };
-    const spiedIssueRequest = jest.spyOn(
-      mockedVcModule,
-      "issueVerifiableCredential",
-    );
+    const spiedIssueRequest = jest
+      .spyOn(mockedVcModule, "issueVerifiableCredential")
+      .mockResolvedValueOnce(mockAccessGrantVc());
+
     const accessRequestWithPurpose = mockAccessRequestVc({
       purpose: ["https://example.org/some-purpose"],
     });

diff --git a/src/gConsent/manage/denyAccessRequest.ts b/src/gConsent/manage/denyAccessRequest.ts
@@ -31,6 +31,8 @@ import type { AccessBaseOptions } from "../type/AccessBaseOptions";
 import { getGrantBody, issueAccessVc } from "../util/issueAccessVc";
 import { getBaseAccessRequestVerifiableCredential } from "../util/getBaseAccessVerifiableCredential";
 import { initializeGrantParameters } from "../util/initializeGrantParameters";
+import { normalizeAccessGrant } from "./approveAccessRequest";
+import type { AccessGrant } from "../type/AccessGrant";
 
 // Merge back in denyAccessRequest after the deprecated signature has been removed.
 // eslint-disable-next-line camelcase
@@ -71,7 +73,7 @@ async function internal_denyAccessRequest(
 async function denyAccessRequest(
   vc: VerifiableCredential | URL | UrlString,
   options?: AccessBaseOptions,
-): Promise<VerifiableCredential>;
+): Promise<AccessGrant>;
 /**
  * @deprecated Please remove the `resourceOwner` parameter.
  */
@@ -95,12 +97,12 @@ async function denyAccessRequest(
     return internal_denyAccessRequest(
       vcOrOptions as VerifiableCredential | URL | UrlString,
       options ?? {},
-    );
+    ).then(normalizeAccessGrant);
   }
   return internal_denyAccessRequest(
     resourceOwnerOrVc,
     (vcOrOptions as AccessBaseOptions) ?? {},
-  );
+  ).then(normalizeAccessGrant);
 }
 
 export { denyAccessRequest };