chore(backend): do not store entire grant on context #911

wilsonianb · 2022-12-20T22:22:47Z

Changes proposed in this pull request

Make access limits specific to outgoing payment creation.

Context

fixes Do not store entire grant on context #688
setup for Add token introspection package #844

Checklist

Related issues linked using fixes #number
Tests added/updated
Documentation added
Make sure that all checks pass

Make access limits specific to outgoing payment creation.

mkurapov

Going to come back to this tomorrow (getting late here), just a few minor comments for now

mkurapov · 2022-12-21T00:19:45Z

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

+          (!access['identifier'] ||
+            access['identifier'] === ctx.paymentPointer.url) &&
+          access.actions.find((tokenAction) => {
+            if (tokenAction == action) {


Suggested change

if (tokenAction == action) {

if (tokenAction === action) {

I don't think a triple-equals will cause issues here, it'll be just a string comparison at runtime

same below

Oops, I originally had this based on a branch using generated OpenAPI types where it was an issue.
I tripled the access type check but missed the actions.

mkurapov · 2022-12-21T00:33:01Z

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

+          (!access['identifier'] ||
+            access['identifier'] === ctx.paymentPointer.url) &&


Suggested change

(!access['identifier'] ||

access['identifier'] === ctx.paymentPointer.url) &&

(access.identifier === ctx.paymentPointer.url) &&

if its optional

mkurapov · 2022-12-21T00:36:48Z

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

+              // Unless the relevant grant action is ReadAll/ListAll add the
+              // clientId to ctx for Read/List filtering


I'm not sure if I understand the comment fully, but it reads as though

return ( (action === AccessAction.Read && tokenAction == AccessAction.ReadAll) || (action === AccessAction.List && tokenAction == AccessAction.ListAll) )

should be before

if (tokenAction == action) { ... }

?

action will never be ReadAll or ListAll

rafiki/packages/backend/src/app.ts

Lines 279 to 296 in 4db6f8e

const toAction = ({

path,

method

}: {

path: string

method: HttpMethod

}): AccessAction | undefined => {

switch (method) {

case HttpMethod.GET:

return path.endsWith('{id}') ? AccessAction.Read : AccessAction.List

case HttpMethod.POST:

return path.endsWith('/complete')

? AccessAction.Complete

: AccessAction.Create

default:

return undefined

}

}

so this should be fine, but I'll update the types to make that explicit.

🤔 Actually this is still dependent on Read and ReadAll (and List/ListAll) being mutually exclusive, which is no longer the case (in OpenAPI)

feat: remove allof open-payments#215

mkurapov

LGTM, non-blocking comments

mkurapov · 2022-12-22T00:43:25Z

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

-    return new TokenInfo(options, data.key)
+    return data.active ? data : undefined
+
+    return data


Suggested change

return data

mkurapov · 2022-12-22T00:46:25Z

packages/backend/src/app.ts

@@ -282,7 +289,7 @@ export class App {
    }: {
      path: string
      method: HttpMethod
-    }): AccessAction | undefined => {
+    }): RequestAction | undefined => {


I like the RequestAction type 👍 . I propose we call the variable also that: action -> requestAction

mkurapov · 2022-12-22T00:48:35Z

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

 import { HttpSigContext, PaymentPointerContext } from '../../app'

+export type RequestAction = Exclude<


🚀 I now appreciate Exclude after my types adventure

mkurapov · 2022-12-22T01:02:12Z

packages/backend/src/open_payments/payment/outgoing/service.ts

@@ -92,7 +98,7 @@ async function createOutgoingPayment(
  deps: ServiceDependencies,
  options: CreateOutgoingPaymentOptions
 ): Promise<OutgoingPayment | OutgoingPaymentError> {
-  const grantId = options.grant ? options.grant.grant : undefined
+  const grantId = options.grant?.id


Will this be required in the future? i.e. will this end up throwing if there is no grant on the ctx?

I think grant is mainly optional for creating outgoing payments via the admin api

rafiki/packages/backend/src/graphql/schema.graphql

Lines 97 to 99 in e773fb7

createOutgoingPayment(

input: CreateOutgoingPaymentInput!

): OutgoingPaymentResponse!

mkurapov · 2022-12-22T01:07:43Z

packages/backend/src/open_payments/payment/outgoing/service.ts

@@ -32,6 +32,11 @@ import {
 import { Interval } from 'luxon'
 import { knex } from 'knex'

+export interface Grant {


Maybe this interface first better in the auth/middleware.ts file?

mkurapov · 2022-12-22T01:27:56Z

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

-      expect(next).toHaveBeenCalled()
-      expect(ctx.grant).toEqual(tokenInfo)
+      if (identifierOption !== IdentifierOption.Conflicting) {
+        test('sets the context client info and calls next', async (): Promise<void> => {


More for my own understanding, why are we ok to allow tokenInfo to not have an identifier?

Optional grant access identifier open-payments#124

but then I'm not sure why it was subsequently re-required for outgoing payments 😬

fix: add quote, fix outgoing, drop incoming limits open-payments#144

identifier (string):
A string identifier indicating a specific resource at the RS. For example, a patient identifier for a medical API or a bank account number for a financial API.

Based on the description, this feels like it should be reference to specific sub resource, and not just a payment pointer (of how we are currently treating it). I'm ok with keeping it optional across the board, thoughts?

🤔 Our own descriptions also seem to suggest it would refer to a specific (sub)resource...
https://github.com/interledger/open-payments/blob/main/openapi/schemas.yaml#L56-L59

description: A string identifier indicating a specific resource at the RS.

I think we do want to be able to refer to a payment pointer, especially for create and list action grants.
If/when we switch to Adrian's grant requests to the payment pointer itself, the client will no longer need to specify the payment pointer. And he's proposed just having a paymentPointer field in the token introspection response for the grant.

mkurapov · 2023-01-10T23:39:37Z

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

-      expect(next).toHaveBeenCalled()
-      expect(ctx.grant).toEqual(tokenInfo)
+      if (identifierOption !== IdentifierOption.Conflicting) {
+        test('sets the context client info and calls next', async (): Promise<void> => {


identifier (string):
A string identifier indicating a specific resource at the RS. For example, a patient identifier for a medical API or a bank account number for a financial API.

Based on the description, this feels like it should be reference to specific sub resource, and not just a payment pointer (of how we are currently treating it). I'm ok with keeping it optional across the board, thoughts?

chore(backend): do not store entire grant on context

0d87450

Make access limits specific to outgoing payment creation.

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related labels Dec 20, 2022

wilsonianb marked this pull request as ready for review December 20, 2022 22:40

wilsonianb requested review from sabineschaller, dclipp, mkurapov and JoblersTune December 20, 2022 22:41

mkurapov reviewed Dec 21, 2022

View reviewed changes

wilsonianb added 3 commits December 21, 2022 12:35

chore(backend): make access action equality check strict

e88e4fd

chore(backend): restrict token introspection middleware access actions

c2e2d92

chore(backend): match ReadAll/ListAll before Read/List

7f2102c

wilsonianb requested a review from mkurapov December 21, 2022 19:26

mkurapov previously approved these changes Dec 22, 2022

View reviewed changes

chore(backend): rename createTokenIntrospectionMiddleware args

a5f4000

wilsonianb dismissed mkurapov’s stale review via a5f4000 December 22, 2022 04:56

wilsonianb requested a review from mkurapov December 22, 2022 05:07

mkurapov previously approved these changes Jan 10, 2023

View reviewed changes

Merge branch 'main' into bw-no-grant

5ad7a14

wilsonianb dismissed mkurapov’s stale review via 5ad7a14 January 11, 2023 00:27

wilsonianb requested a review from mkurapov January 11, 2023 00:55

mkurapov approved these changes Jan 11, 2023

View reviewed changes

wilsonianb merged commit dc909a6 into main Jan 11, 2023

wilsonianb deleted the bw-no-grant branch January 11, 2023 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(backend): do not store entire grant on context #911

chore(backend): do not store entire grant on context #911

wilsonianb commented Dec 20, 2022 •

edited

Loading

mkurapov left a comment

mkurapov Dec 21, 2022

wilsonianb Dec 21, 2022

mkurapov Dec 21, 2022

mkurapov Dec 21, 2022 •

edited

Loading

wilsonianb Dec 21, 2022

wilsonianb Dec 21, 2022 •

edited

Loading

mkurapov left a comment

mkurapov Dec 22, 2022

mkurapov Dec 22, 2022

mkurapov Dec 22, 2022

mkurapov Dec 22, 2022

wilsonianb Dec 22, 2022

mkurapov Dec 22, 2022

mkurapov Dec 22, 2022

wilsonianb Dec 22, 2022

mkurapov Jan 10, 2023

wilsonianb Jan 11, 2023 •

edited

Loading

wilsonianb Jan 11, 2023

mkurapov Jan 10, 2023

		(!access['identifier'] \|\|
		access['identifier'] === ctx.paymentPointer.url) &&

	(!access['identifier'] \|\|
	access['identifier'] === ctx.paymentPointer.url) &&
	(access.identifier === ctx.paymentPointer.url) &&

		// Unless the relevant grant action is ReadAll/ListAll add the
		// clientId to ctx for Read/List filtering

	const toAction = ({
	path,
	method
	}: {
	path: string
	method: HttpMethod
	}): AccessAction \| undefined => {
	switch (method) {
	case HttpMethod.GET:
	return path.endsWith('{id}') ? AccessAction.Read : AccessAction.List
	case HttpMethod.POST:
	return path.endsWith('/complete')
	? AccessAction.Complete
	: AccessAction.Create
	default:
	return undefined
	}
	}

		import { HttpSigContext, PaymentPointerContext } from '../../app'

		export type RequestAction = Exclude<

	createOutgoingPayment(
	input: CreateOutgoingPaymentInput!
	): OutgoingPaymentResponse!

chore(backend): do not store entire grant on context #911

chore(backend): do not store entire grant on context #911

Conversation

wilsonianb commented Dec 20, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

mkurapov left a 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 Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb Dec 21, 2022 • edited Loading

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

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

wilsonianb Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb commented Dec 20, 2022 •

edited

Loading

mkurapov Dec 21, 2022 •

edited

Loading

wilsonianb Dec 21, 2022 •

edited

Loading

wilsonianb Jan 11, 2023 •

edited

Loading