fix: always pass in identifier into token introspection (#2907)

mkurapov · web-flow · commit e20228f7be7d · 2024-08-27T18:33:36.000+03:00
* feat(auth): check identifier only if present in grant

* chore(backend): always pass in identifier during token introspection
diff --git a/packages/auth/src/access/utils.test.ts b/packages/auth/src/access/utils.test.ts
@@ -155,6 +155,66 @@ describe('Access utilities', (): void => {
     ).toBe(true)
   })
 
+  test('Can compare an access item on a grant and an access item from a request with different action ordering', async (): Promise<void> => {
+    const grantAccessItemSuperAction = await Access.query(trx).insertAndFetch({
+      grantId: grant.id,
+      type: AccessType.OutgoingPayment,
+      actions: [AccessAction.Create, AccessAction.ReadAll, AccessAction.List],
+      identifier,
+      limits: {
+        receiver,
+        debitAmount: {
+          value: '400',
+          assetCode: 'USD',
+          assetScale: 2
+        }
+      }
+    })
+
+    const requestAccessItem: AccessItem = {
+      type: 'outgoing-payment',
+      actions: ['read', 'list', 'create'],
+      identifier,
+      limits: {
+        receiver,
+        debitAmount: {
+          value: '400',
+          assetCode: 'USD',
+          assetScale: 2
+        }
+      }
+    }
+
+    expect(
+      compareRequestAndGrantAccessItems(
+        requestAccessItem,
+        toOpenPaymentsAccess(grantAccessItemSuperAction)
+      )
+    ).toBe(true)
+  })
+
+  test('Can compare an access item on a grant without an identifier with a request with an identifier', async (): Promise<void> => {
+    const grantAccessItemSuperAction = await Access.query(trx).insertAndFetch({
+      grantId: grant.id,
+      type: AccessType.IncomingPayment,
+      actions: [AccessAction.ReadAll],
+      identifier: undefined
+    })
+
+    const requestAccessItem: AccessItem = {
+      type: 'incoming-payment',
+      actions: [AccessAction.ReadAll],
+      identifier
+    }
+
+    expect(
+      compareRequestAndGrantAccessItems(
+        requestAccessItem,
+        toOpenPaymentsAccess(grantAccessItemSuperAction)
+      )
+    ).toBe(true)
+  })
+
   test('access comparison fails if grant action items are insufficient', async (): Promise<void> => {
     const identifier = `https://example.com/${v4()}`
     const receiver =
@@ -206,4 +266,46 @@ describe('Access utilities', (): void => {
       )
     ).toBe(false)
   })
+
+  test('access comparison fails if identifier mismatch', async (): Promise<void> => {
+    const grantAccessItemSuperAction = await Access.query(trx).insertAndFetch({
+      grantId: grant.id,
+      type: AccessType.IncomingPayment,
+      actions: [AccessAction.ReadAll],
+      identifier
+    })
+
+    const requestAccessItem: AccessItem = {
+      type: 'incoming-payment',
+      actions: [AccessAction.ReadAll],
+      identifier: `https://example.com/${v4()}`
+    }
+
+    expect(
+      compareRequestAndGrantAccessItems(
+        requestAccessItem,
+        toOpenPaymentsAccess(grantAccessItemSuperAction)
+      )
+    ).toBe(false)
+  })
+
+  test('access comparison fails if type mismatch', async (): Promise<void> => {
+    const grantAccessItemSuperAction = await Access.query(trx).insertAndFetch({
+      grantId: grant.id,
+      type: AccessType.Quote,
+      actions: [AccessAction.Read]
+    })
+
+    const requestAccessItem: AccessItem = {
+      type: 'incoming-payment',
+      actions: [AccessAction.Read]
+    }
+
+    expect(
+      compareRequestAndGrantAccessItems(
+        requestAccessItem,
+        toOpenPaymentsAccess(grantAccessItemSuperAction)
+      )
+    ).toBe(false)
+  })
 })
diff --git a/packages/auth/src/access/utils.ts b/packages/auth/src/access/utils.ts
@@ -42,13 +42,22 @@ export function compareRequestAndGrantAccessItems(
     return false
   }
 
-  // Validate remaining keys
+  if (restOfRequestAccessItem.type !== restOfGrantAccessItem.type) {
+    return false
+  }
+
+  // Validate identifier, if present on the grant
+  const grantAccessIdentifier = (
+    restOfGrantAccessItem as OutgoingPaymentOrIncomingPaymentAccess
+  ).identifier
+
+  const requestAccessIdentifier = (
+    restOfRequestAccessItem as OutgoingPaymentOrIncomingPaymentAccess
+  ).identifier
+
   if (
-    restOfRequestAccessItem.type !== restOfGrantAccessItem.type ||
-    (restOfRequestAccessItem as OutgoingPaymentOrIncomingPaymentAccess)
-      .identifier !==
-      (restOfGrantAccessItem as OutgoingPaymentOrIncomingPaymentAccess)
-        .identifier
+    grantAccessIdentifier &&
+    requestAccessIdentifier !== grantAccessIdentifier
   ) {
     return false
   }
diff --git a/packages/backend/src/open_payments/auth/middleware.test.ts b/packages/backend/src/open_payments/auth/middleware.test.ts
@@ -46,7 +46,7 @@ type IntrospectionCallObject = {
   access: {
     type: AccessType
     actions: AccessAction[]
-    identifier?: string
+    identifier: string
   }[]
 }
 
@@ -193,7 +193,8 @@ describe('Auth Middleware', (): void => {
       access: [
         {
           type: type,
-          actions: [action]
+          actions: [action],
+          identifier: ctx.walletAddressUrl
         }
       ]
     })
@@ -217,7 +218,8 @@ describe('Auth Middleware', (): void => {
       access: [
         {
           type: type,
-          actions: [action]
+          actions: [action],
+          identifier: ctx.walletAddressUrl
         }
       ]
     })
@@ -286,15 +288,12 @@ describe('Auth Middleware', (): void => {
             access: [
               {
                 type,
-                actions: [action]
+                actions: [action],
+                identifier: ctx.walletAddressUrl
               }
             ]
           }
 
-          if (type === AccessType.OutgoingPayment) {
-            expectedCallObject.access[0].identifier = ctx.walletAddressUrl
-          }
-
           expect(introspectSpy).toHaveBeenCalledWith(expectedCallObject)
           expect(next).not.toHaveBeenCalled()
         }
@@ -313,7 +312,8 @@ describe('Auth Middleware', (): void => {
             access: [
               {
                 type,
-                actions: [action]
+                actions: [action],
+                identifier: ctx.walletAddressUrl
               }
             ]
           })
@@ -349,7 +349,8 @@ describe('Auth Middleware', (): void => {
               access: [
                 {
                   type,
-                  actions: [subAction]
+                  actions: [subAction],
+                  identifier: ctx.walletAddressUrl
                 }
               ]
             })
@@ -376,7 +377,8 @@ describe('Auth Middleware', (): void => {
               access: [
                 {
                   type,
-                  actions: [superAction]
+                  actions: [superAction],
+                  identifier: ctx.walletAddressUrl
                 }
               ]
             })
@@ -436,13 +438,11 @@ describe('Auth Middleware', (): void => {
                 access: [
                   {
                     type,
-                    actions: [action]
+                    actions: [action],
+                    identifier: ctx.walletAddressUrl
                   }
                 ]
               }
-              if (type === AccessType.OutgoingPayment) {
-                expectedCallObject.access[0].identifier = ctx.walletAddressUrl
-              }
 
               expect(introspectSpy).toHaveBeenCalledWith(expectedCallObject)
               expect(next).toHaveBeenCalled()
@@ -473,7 +473,8 @@ describe('Auth Middleware', (): void => {
             access: [
               {
                 type,
-                actions: [action]
+                actions: [action],
+                identifier: ctx.walletAddressUrl
               }
             ]
           })
diff --git a/packages/backend/src/open_payments/auth/middleware.ts b/packages/backend/src/open_payments/auth/middleware.ts
@@ -36,7 +36,7 @@ export interface Grant {
 export interface Access {
   type: string
   actions: AccessAction[]
-  identifier?: string
+  identifier: string
 }
 
 function contextToRequestLike(ctx: HttpSigContext): RequestLike {
@@ -95,13 +95,11 @@ export function createTokenIntrospectionMiddleware({
         tokenInfo = await tokenIntrospectionClient.introspect({
           access_token: token,
           access: [
-            requestType === AccessType.OutgoingPayment
-              ? toOpenPaymentsAccess(
-                  requestType,
-                  requestAction,
-                  ctx.walletAddressUrl
-                )
-              : toOpenPaymentsAccess(requestType, requestAction)
+            toOpenPaymentsAccess(
+              requestType,
+              requestAction,
+              ctx.walletAddressUrl
+            )
           ]
         })
       } catch (err) {

Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ type IntrospectionCallObject = {`
`46`	`46`	`access: {`
`47`	`47`	`type: AccessType`
`48`	`48`	`actions: AccessAction[]`
`49`		`- identifier?: string`
	`49`	`+ identifier: string`
`50`	`50`	`}[]`
`51`	`51`	`}`
`52`	`52`
`@@ -193,7 +193,8 @@ describe('Auth Middleware', (): void => {`
`193`	`193`	`access: [`
`194`	`194`	`{`
`195`	`195`	`type: type,`
`196`		`- actions: [action]`
	`196`	`+ actions: [action],`
	`197`	`+ identifier: ctx.walletAddressUrl`
`197`	`198`	`}`
`198`	`199`	`]`
`199`	`200`	`})`
`@@ -217,7 +218,8 @@ describe('Auth Middleware', (): void => {`
`217`	`218`	`access: [`
`218`	`219`	`{`
`219`	`220`	`type: type,`
`220`		`- actions: [action]`
	`221`	`+ actions: [action],`
	`222`	`+ identifier: ctx.walletAddressUrl`
`221`	`223`	`}`
`222`	`224`	`]`
`223`	`225`	`})`
`@@ -286,15 +288,12 @@ describe('Auth Middleware', (): void => {`
`286`	`288`	`access: [`
`287`	`289`	`{`
`288`	`290`	`type,`
`289`		`- actions: [action]`
	`291`	`+ actions: [action],`
	`292`	`+ identifier: ctx.walletAddressUrl`
`290`	`293`	`}`
`291`	`294`	`]`
`292`	`295`	`}`
`293`	`296`
`294`		`- if (type === AccessType.OutgoingPayment) {`
`295`		`- expectedCallObject.access[0].identifier = ctx.walletAddressUrl`
`296`		`- }`
`297`		`-`
`298`	`297`	`expect(introspectSpy).toHaveBeenCalledWith(expectedCallObject)`
`299`	`298`	`expect(next).not.toHaveBeenCalled()`
`300`	`299`	`}`
`@@ -313,7 +312,8 @@ describe('Auth Middleware', (): void => {`
`313`	`312`	`access: [`
`314`	`313`	`{`
`315`	`314`	`type,`
`316`		`- actions: [action]`
	`315`	`+ actions: [action],`
	`316`	`+ identifier: ctx.walletAddressUrl`
`317`	`317`	`}`
`318`	`318`	`]`
`319`	`319`	`})`
`@@ -349,7 +349,8 @@ describe('Auth Middleware', (): void => {`
`349`	`349`	`access: [`
`350`	`350`	`{`
`351`	`351`	`type,`
`352`		`- actions: [subAction]`
	`352`	`+ actions: [subAction],`
	`353`	`+ identifier: ctx.walletAddressUrl`
`353`	`354`	`}`
`354`	`355`	`]`
`355`	`356`	`})`
`@@ -376,7 +377,8 @@ describe('Auth Middleware', (): void => {`
`376`	`377`	`access: [`
`377`	`378`	`{`
`378`	`379`	`type,`
`379`		`- actions: [superAction]`
	`380`	`+ actions: [superAction],`
	`381`	`+ identifier: ctx.walletAddressUrl`
`380`	`382`	`}`
`381`	`383`	`]`
`382`	`384`	`})`
`@@ -436,13 +438,11 @@ describe('Auth Middleware', (): void => {`
`436`	`438`	`access: [`
`437`	`439`	`{`
`438`	`440`	`type,`
`439`		`- actions: [action]`
	`441`	`+ actions: [action],`
	`442`	`+ identifier: ctx.walletAddressUrl`
`440`	`443`	`}`
`441`	`444`	`]`
`442`	`445`	`}`
`443`		`- if (type === AccessType.OutgoingPayment) {`
`444`		`- expectedCallObject.access[0].identifier = ctx.walletAddressUrl`
`445`		`- }`
`446`	`446`
`447`	`447`	`expect(introspectSpy).toHaveBeenCalledWith(expectedCallObject)`
`448`	`448`	`expect(next).toHaveBeenCalled()`
`@@ -473,7 +473,8 @@ describe('Auth Middleware', (): void => {`
`473`	`473`	`access: [`
`474`	`474`	`{`
`475`	`475`	`type,`
`476`		`- actions: [action]`
	`476`	`+ actions: [action],`
	`477`	`+ identifier: ctx.walletAddressUrl`
`477`	`478`	`}`
`478`	`479`	`]`
`479`	`480`	`})`