feat(auth): inspect access during token introspection #2788

njlie · 2024-06-26T17:45:29Z

Changes proposed in this pull request

Adds access parameter to token introspection request
auth service now verifies if provided access is sufficiently allowed by grant retrieved during token introspection
token-introspection extends capability of verifying a token has sufficient access for a requested access scope.

Context

Fixes #835.

Checklist

netlify · 2024-06-26T17:45:44Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`017699f`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66a2da4a63d64200081a7a62

BlairCurrey · 2024-07-18T14:21:58Z

packages/token-introspection/src/client/introspection.test.ts

@@ -98,17 +99,15 @@ describe('introspection', (): void => {
    })
  })

-  describe('validateTokenInfo', (): void => {
-    test('returns valid token info', async (): Promise<void> => {
+  describe('validateTokenAccess', (): void => {


can you add a case where the validation fails?

Ended up removing it, actually

BlairCurrey · 2024-07-18T15:08:17Z

packages/backend/src/open_payments/auth/middleware.ts

+function toOpenPaymentsAccess(
+  type: AccessType,
+  action: RequestAction,
+  identifier?: string
+): AccessItem {
+  return {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    type: type as any,
+    actions: [action],
+    identifier: identifier ?? undefined
+  }
+}


i tried to make this fully typesafe and it got brutal 😆

Effectively the same, but perhaps a little cleaner?

Suggested change

function toOpenPaymentsAccess(

type: AccessType,

action: RequestAction,

identifier?: string

): AccessItem {

return {

// eslint-disable-next-line @typescript-eslint/no-explicit-any

type: type as any,

actions: [action],

identifier: identifier ?? undefined

}

}

function toOpenPaymentsAccess(

type: AccessType,

action: RequestAction,

identifier?: string

): AccessItem {

return {

type: type,

actions: [action],

identifier: identifier ?? undefined

} as AccessItem

}

BlairCurrey · 2024-07-18T15:39:11Z

packages/backend/src/open_payments/auth/middleware.ts

+      if (
+        requestAction === AccessAction.Read &&
+        (access as Access).actions.includes(AccessAction.ReadAll)
+      ) {
+        ctx.accessAction = AccessAction.ReadAll
+      } else if (
+        requestAction === AccessAction.List &&
+        (access as Access).actions.includes(AccessAction.ListAll)
+      ) {
+        ctx.accessAction = AccessAction.ListAll
+      } else {
+        ctx.accessAction = requestAction
+      }
+


Could we de-duplicate this logic some by returning RequestAction | undefined from validateTokenAccess instead of the AccessItem? Then using that to set ctx.accessAction. Just looks like we're doing the same thing in validateTokenAccess.

Looks like the only other place we use the AccessItem is to set the grant limits but I think the tokenInfo should work fine for that instead, if we wanted.

er... maybe not. I didn't fully understand the type of tokenInfo. Dont think we can use that for grant limit since we won't know which access item we're dealing with. Guess we could return both access and accessAction from validateToken if we cared to.

Looks like we can just return a filtered version of access with a single item from the introspection endpoint, and have the RS just assume that the lone item in that array is the relevant access.

Then the backend can just check if there is a Read-All or List-All action in the list if the request for a Read or List action, respectively.

From the Resource Server Spec (emphasis mine):

access (array of strings/objects):
REQUIRED. The access rights associated with this access token. This MUST be in the format described in the Section 8 of [GNAP]. This array MAY be filtered or otherwise limited for consumption by the identified RS, including being an empty array, indicating that the token has no explicit access rights that can be disclosed to the RS.

BlairCurrey · 2024-07-18T16:25:47Z

packages/auth/src/access/utils.ts

@@ -0,0 +1,47 @@
+import { isDeepStrictEqual } from 'util'


how did I not realize this was in the node standard library? Cool.

I just discovered it when completing this PR. I knew there surely was a library that could handle this but I wasn't expecting a node-native solution either!

BlairCurrey · 2024-07-18T16:41:10Z

packages/auth/src/access/utils.ts

+    ) {
+      return false
+    } else if (
+      restOfRequestAccessItem[key as keyof typeof restOfRequestAccessItem] !==


could use requestAccessItemValue, since it was already initialized on L28

Suggested change

restOfRequestAccessItem[key as keyof typeof restOfRequestAccessItem] !==

requestAccessItemValue !==

mkurapov · 2024-07-23T10:23:22Z

packages/backend/src/open_payments/auth/middleware.ts

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    type: type as any,
+    actions: [action],
+    identifier: identifier ?? undefined


Suggested change

identifier: identifier ?? undefined

identifier

mkurapov · 2024-07-23T13:55:42Z

packages/backend/src/open_payments/auth/middleware.ts

+          access: [
+            toOpenPaymentsAccess(
+              requestType,
+              requestAction,
+              ctx.walletAddressUrl
+            )
+          ]


Why do we need this to be an array?

The GNAP Resource Server spec specifies that this be an array.

access (array of strings/objects):
OPTIONAL. The minimum access rights required to fulfill the request. This MUST be in the format described in Section 8 of [GNAP].

Got it, makes sense!

mkurapov · 2024-07-23T13:57:34Z

packages/token-introspection/src/client/introspection.ts

+      return true
+    }
+    return tokenAccess.actions.find((tokenAction: AccessAction) => {
+      if (isActiveTokenInfo(tokenInfo) && tokenAction === access.action) {


do we need to check for isActiveTokenInfo earlier? ie the two ifs before this find -> is it valid to return true without checking isActiveTokenInfo first?

mkurapov · 2024-07-23T16:48:11Z

packages/auth/src/accessToken/service.ts

+      for (const accessItem of access) {
+        const { access: grantAccess } = token.grant
+        if (
+          !grantAccess.find((grantAccessItem) =>
+            compareRequestAndGrantAccessItems(
+              accessItem,
+              toOpenPaymentsAccess(grantAccessItem)
+            )
+          )
+        ) {
+          return undefined
+        }


to understand this correctly, in this implementation, the access will consist of an array with a single item of { identifier access type }. We then compare this object to the existing grantAccessItem.
And even though accessItem will never have limits, compareRequestAndGrantAccessItems checks if all the other keys are equivalent, correct?

Yeah that's pretty much it. I wrote it to be able to handle multiple access items as an input, though.

Makes sense. Just as an edge case, we are parsing access.limits correctly from Postgres, right?
Don't want to run into an instance where for example, the limit.debit/receiveAmount.accessScale gets parsed out as a string and then we can't properly compare it to it being a number in the token introspection request

mkurapov · 2024-07-23T16:48:45Z

packages/auth/src/accessToken/service.ts

@@ -42,7 +48,8 @@ export async function createAccessTokenService({
  return {
    getByManagementId: (managementId: string) =>
      getByManagementId(managementId),
-    introspect: (tokenValue: string) => introspect(deps, tokenValue),
+    introspect: (tokenValue: string, access?: AccessItem[]) =>


will/should access ever be undefined?

It's optional in the GNAP spec. It's not required in the OpenAPI spec for the token-introspection package, but we certainly could make it required. Payments are pretty sensitive entities, I'd be in favor of making it required as part of Open Payments.

We can change that later if we wanted, for now it's ok I think.

Edit, I think if we are doing this, ie returning the single relevant access item, we should make this required

njlie · 2024-07-23T22:44:08Z

packages/backend/src/open_payments/auth/middleware.ts

-          return false
-        })
-      })
+      if (tokenInfo.access.length > 1) {


Whether or not we need this check depends on if it is decided that the token introspection endpoint will always return ONLY the request's matching access item in the response.

Looks like GNAP is flexible on this as well:

access (array of strings/objects):
REQUIRED. The access rights associated with this access token. This MUST be in the format described in the Section 8 of [GNAP]. This array MAY be filtered or otherwise limited for consumption by the identified RS, including being an empty array, indicating that the token has no explicit access rights that can be disclosed to the RS.

However, looking at

https://github.com/interledger/rafiki/pull/2788/files#diff-4c4c1504c8978d5fcd7a7bfa8210903c7dada6ec793ac0ad8e895fda2f0a5115R120-R122

since it looks like its "possible" to return multiple accesses, we should keep the original behavior of allowing multiple? Or we make accessItem required in the auth introspection route handler.

Here's what I decided - the introspection endpoint on the auth server will always return whatever access rights were presented in the request if the token is valid, no more, no less.

The backend service can then expect the response to have exactly one item since the middleware only ever presents one access during the introspection request.

@njlie makes sense to me 👍

mkurapov · 2024-07-24T16:20:46Z

packages/backend/src/open_payments/auth/middleware.ts

-          return false
-        })
-      })
+      if (tokenInfo.access.length > 1) {


Looks like GNAP is flexible on this as well:

access (array of strings/objects):
REQUIRED. The access rights associated with this access token. This MUST be in the format described in the Section 8 of [GNAP]. This array MAY be filtered or otherwise limited for consumption by the identified RS, including being an empty array, indicating that the token has no explicit access rights that can be disclosed to the RS.

mkurapov · 2024-07-24T16:30:50Z

packages/auth/src/access/utils.test.ts

+      compareRequestAndGrantAccessItems(
+        requestAccessItem,
+        toOpenPaymentsAccess(grantAccessItemSuperAction)
+      )


do we need

Suggested change

)

).toBe(true)

or does expect() just work?

I think it works, updated just to be sure

mkurapov · 2024-07-24T16:35:48Z

packages/auth/src/access/utils.ts

+  Object.keys(restOfRequestAccessItem).forEach((key) => {
+    const requestAccessItemValue =
+      restOfRequestAccessItem[key as keyof typeof restOfRequestAccessItem]
+    if (
+      typeof requestAccessItemValue === 'object' &&
+      !isDeepStrictEqual(
+        requestAccessItemValue,
+        restOfgrantAccessItem[key as keyof typeof restOfgrantAccessItem]
+      )
+    ) {
+      return false
+    } else if (
+      restOfRequestAccessItem[key as keyof typeof restOfRequestAccessItem] !==
+      restOfgrantAccessItem[key as keyof typeof restOfgrantAccessItem]
+    ) {
+      return false
+    }
+  })


I think I wouldn't mind comparing keys explicitly here (ie requestAccessItem.type === grantAccessItem.type), since we don't have too many, but this works as well

I decided to update this to compare the keys explicitly. I added a test in light of this comment to ensure that only the requested access rights get returned, and it wouldn't work without explicit key comparison.

mkurapov · 2024-07-24T16:41:26Z

packages/auth/src/accessToken/service.ts

+      for (const accessItem of access) {
+        const { access: grantAccess } = token.grant
+        if (
+          !grantAccess.find((grantAccessItem) =>
+            compareRequestAndGrantAccessItems(
+              accessItem,
+              toOpenPaymentsAccess(grantAccessItem)
+            )
+          )
+        ) {
+          return undefined
+        }


Makes sense. Just as an edge case, we are parsing access.limits correctly from Postgres, right?
Don't want to run into an instance where for example, the limit.debit/receiveAmount.accessScale gets parsed out as a string and then we can't properly compare it to it being a number in the token introspection request

mkurapov · 2024-07-24T16:43:34Z

packages/auth/src/accessToken/service.ts

@@ -42,7 +48,8 @@ export async function createAccessTokenService({
  return {
    getByManagementId: (managementId: string) =>
      getByManagementId(managementId),
-    introspect: (tokenValue: string) => introspect(deps, tokenValue),
+    introspect: (tokenValue: string, access?: AccessItem[]) =>


We can change that later if we wanted, for now it's ok I think.

mkurapov · 2024-07-24T17:06:07Z

packages/backend/src/open_payments/auth/middleware.ts

-          return false
-        })
-      })
+      if (tokenInfo.access.length > 1) {


However, looking at

https://github.com/interledger/rafiki/pull/2788/files#diff-4c4c1504c8978d5fcd7a7bfa8210903c7dada6ec793ac0ad8e895fda2f0a5115R120-R122

since it looks like its "possible" to return multiple accesses, we should keep the original behavior of allowing multiple? Or we make accessItem required in the auth introspection route handler.

mkurapov

Looks good! just a minor comment

mkurapov · 2024-07-25T13:00:47Z

packages/auth/src/accessToken/service.ts

  const token = await AccessToken.query(deps.knex)
    .findOne({ value: tokenValue })
    .withGraphFetched('grant.access')

+  let foundAccessItem: Access | undefined


Does this need to be defined here or just in the L95 loop?

mkurapov · 2024-07-25T13:02:34Z

Feel free to re-request after fixing the test

mkurapov

🚀

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels Jun 26, 2024

njlie added 8 commits July 12, 2024 16:53

feat(auth): inspect access during token introspection

c3cefa5

feat: don't require strict equal for rest of access object

aafc86d

feat: submit access to introspect endpoint in backend

9544d8e

chore: formatting

f2a7b38

fix: access token routes test

f3ca550

feat: move access inspection to token-introspection package

c04ab0a

fix: backend build errors

f695813

fix: token-introspection tests

4aa0a03

njlie force-pushed the nl/835/token-introspection-access branch from 0c50082 to 4aa0a03 Compare July 12, 2024 23:53

fix: remove unused service function

eb96853

njlie marked this pull request as ready for review July 15, 2024 18:01

njlie requested review from mkurapov, sabineschaller and BlairCurrey July 15, 2024 18:57

BlairCurrey reviewed Jul 18, 2024

View reviewed changes

BlairCurrey previously approved these changes Jul 18, 2024

View reviewed changes

mkurapov reviewed Jul 23, 2024

View reviewed changes

feat: return single access item during introspection if it is provided

dcee655

njlie dismissed BlairCurrey’s stale review via dcee655 July 23, 2024 22:32

njlie commented Jul 23, 2024

View reviewed changes

fix: remove test for removed function

c3bc37b

njlie force-pushed the nl/835/token-introspection-access branch from a98360f to c3bc37b Compare July 23, 2024 22:47

fix: auth tests

e8159af

mkurapov reviewed Jul 24, 2024

View reviewed changes

njlie added 3 commits July 24, 2024 14:23

fix: backend tests

36a284e

feat: token introspection error if unexpected access returned

b7c909e

feat: new auth server introspection behavior

90d82ba

njlie force-pushed the nl/835/token-introspection-access branch from d030e41 to 90d82ba Compare July 24, 2024 23:37

fix: update expect in auth tests

fe8175b

njlie requested review from mkurapov and BlairCurrey July 24, 2024 23:42

mkurapov previously approved these changes Jul 25, 2024

View reviewed changes

fix tests, compare access keys explicitly

345cd5e

njlie dismissed mkurapov’s stale review via 345cd5e July 25, 2024 18:03

njlie requested a review from mkurapov July 25, 2024 18:08

fix: only submit identifier for outgoing payment accesses

017699f

mkurapov approved these changes Jul 26, 2024

View reviewed changes

njlie merged commit d6d65ff into main Jul 30, 2024
36 of 42 checks passed

njlie deleted the nl/835/token-introspection-access branch July 30, 2024 15:20

	restOfRequestAccessItem[key as keyof typeof restOfRequestAccessItem] !==
	requestAccessItemValue !==

feat(auth): inspect access during token introspection #2788

feat(auth): inspect access during token introspection #2788

Conversation

njlie commented Jun 26, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Jun 26, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov commented Jul 25, 2024

mkurapov left a comment

Choose a reason for hiding this comment

njlie commented Jun 26, 2024 •

edited

Loading

netlify bot commented Jun 26, 2024 •

edited

Loading

BlairCurrey Jul 18, 2024 •

edited

Loading

BlairCurrey Jul 18, 2024 •

edited

Loading

njlie Jul 23, 2024 •

edited

Loading

njlie Jul 23, 2024 •

edited

Loading

njlie Jul 23, 2024 •

edited

Loading

njlie Jul 24, 2024 •

edited

Loading