feat(backend): request grant to query incoming payment receiver #779

wilsonianb · 2022-11-23T01:47:34Z

Changes proposed in this pull request

The receiver service now uses a stored grant if one exists or requests a new grant when querying a remote incoming payment.

Context

fixes Request grant at other Open Payments server(s) #583

Checklist

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

Add services for managing grants.

Update environment variables.

Remove unused locations and interval fields and account access.

wilsonianb · 2022-11-23T02:08:48Z

Using a combination of this branch + #777, I was able to run localenv with AS signature verification enabled:

rafiki/infrastructure/local/peer-docker-compose.yml

Line 17 in 632bdfb

BYPASS_SIGNATURE_VALIDATION: "true"

and successfully create a quote (with backend auto-requesting a grant to read the receiver incoming payment)

wilsonianb · 2022-11-23T02:10:55Z

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

+    return existingGrant
+  }
+
+  const grant = await deps.openPaymentsClient.grant.request({


Is it worth (get or creating and) locking the auth server's db row here to avoid simultaneously sending redundant grant requests?

Is this a case for when we request the receiver on the outgoing payment lifecycle hook?

I was thinking if (multiple instances of) the backend were handling two quotes with the same receiver at the same time, should it avoid performing multiple grant requests?

Pretty edge-casey IMO, not sure if worth adding just yet

wilsonianb · 2022-11-23T14:35:13Z

infrastructure/local/peer-docker-compose.yml

@@ -69,3 +70,6 @@ services:
      - ./seed.peer.yml:/workspace/seed.peer.yml
    depends_on:
      - peer-backend
+networks:
+  local_rafiki:
+    external: true


I added this after noticing 👇 while testing:

$ docker compose -f infrastructure/local/peer-docker-compose.yml ps service "peer-auth" refers to undefined network rafiki: invalid compose project

However, I am now sometimes getting the following when starting the localenv:

network local_rafiki declared as external, but could not be found

I wonder if that can be avoided by splitting these into separate/subsequent docker compose commands:

rafiki/package.json

Lines 20 to 21 in 632bdfb

"localenv": "docker compose -f ./infrastructure/local/docker-compose.yml -f ./infrastructure/local/peer-docker-compose.yml",

"localenv:build": "docker compose -f ./infrastructure/local/docker-compose.yml -f ./infrastructure/local/peer-docker-compose.yml -f ./infrastructure/local/build-override.yml build",

mkurapov · 2022-11-23T18:40:31Z

Using a combination of this branch + #777, I was able to run localenv with AS signature verification enabled:

rafiki/infrastructure/local/peer-docker-compose.yml

Line 17 in 632bdfb

BYPASS_SIGNATURE_VALIDATION: "true"

and successfully create a quote (with backend auto-requesting a grant to read the receiver incoming payment)

what sig headers did you use for making it work? You mean the example P2P payment, correct?

wilsonianb · 2022-11-23T18:53:20Z

This query 👆
The signature headers are unchanged (TODO)
But the signature headers are there for the RS->AS grant request under the hood.

mkurapov · 2022-11-23T19:04:02Z

Is accessToken just dev-access-token for now? I'm working on updating those seed scripts.

wilsonianb · 2022-11-23T19:06:11Z

Yeah, I changed it from the (Postman) default example-access-token.

packages/backend/src/paymentPointerKey/routes.test.ts

mkurapov · 2022-11-28T15:50:37Z

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

+  const grant = await deps.openPaymentsClient.grant.request({
+    url: paymentPointer.authServer,
+    request: {
+      access_token: {
+        access: [
+          {
+            type: grantOptions.accessType,
+            actions: grantOptions.accessActions
+          }
+        ]
+      },
+      interact: {
+        start: ['redirect']
+      }
+    } as GrantRequest


Suggested change

const grant = await deps.openPaymentsClient.grant.request({

url: paymentPointer.authServer,

request: {

access_token: {

access: [

{

type: grantOptions.accessType,

actions: grantOptions.accessActions

}

]

},

interact: {

start: ['redirect']

}

} as GrantRequest

const grant = await deps.openPaymentsClient.grant.request({

url: paymentPointer.authServer,

request: {

access_token: {

access: [

{

type: grantOptions.accessType as 'incoming-payment',

actions: grantOptions.accessActions

}

]

},

interact: {

start: ['redirect']

}

}

Should we export accessType from open-payments, along with a mapper? So we don't need the as GrantRequest. Otherwise, I would prefer type asserting only the single thing that needs it

Or could these be exported

rafiki/packages/open-payments/src/generated/auth-server-types.ts

Lines 250 to 287 in 48cdfa2

"access-incoming": {

/** @description The type of resource request as a string. This field defines which other fields are allowed in the request object. */

type: "incoming-payment";

/** @description The types of actions the client instance will take at the RS as an array of strings. */

actions: (

| "create"

| "complete"

| "read"

| "read-all"

| "list"

| "list-all"

)[];

/**

* Format: uri

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

*/

identifier?: string;

};

/** access-outgoing */

"access-outgoing": {

/** @description The type of resource request as a string. This field defines which other fields are allowed in the request object. */

type: "outgoing-payment";

/** @description The types of actions the client instance will take at the RS as an array of strings. */

actions: ("create" | "read" | "read-all" | "list" | "list-all")[];

/**

* Format: uri

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

*/

identifier: string;

limits?: external["https://raw.githubusercontent.com/interledger/open-payments/b363d33038fe789e5388f04f80ddd06a4fa97093/openapi/schemas.yaml"]["components"]["schemas"]["limits-outgoing"];

};

/** access-quote */

"access-quote": {

/** @description The type of resource request as a string. This field defines which other fields are allowed in the request object. */

type: "quote";

/** @description The types of actions the client instance will take at the RS as an array of strings. */

actions: ("create" | "read" | "read-all")[];

};

as IncomingPaymentAccess, etc?
Then backend/auth could utilize these types instead having a bunch of pairings of arbitrary AccessType/AccessAction.

I've gone with your original suggestion but opened:

Reconcile string enums with generated union types #808

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

mkurapov · 2022-11-28T16:01:20Z

packages/backend/src/open_payments/grant/model.ts

+import { AuthServer } from '../authServer/model'
+import { BaseModel } from '../../shared/baseModel'
+
+export class Grant extends BaseModel {


Anything we can do here (naming wise) to make it more explicit what the difference between Grant and GrantReference is?

What about GrantReference -> ResourceGrant (or (PaymentPointer)SubresourceGrant)?

I'm also tempted to backtrack on this: #535 (comment)

We could instead store clientId (soon to be client payment pointer #737) on subresources.
Rename GrantReference as OutgoingPaymentGrants (since it's only really needed to lock on limit checking 👇) and drop the clientId column

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

Lines 255 to 256 in a5c36d7

//lock grant

await deps.grantReferenceService.lock(grant.grant, deps.knex)

😬

chore(backend): replace GrantReference with OutgoingPaymentGrant #800

Renaming is the right move since we only store references to outgoing payment grants. I'll have to look into the other PR to understand why we can drop clientId.

At the moment, we store references for grants for incoming payments, quotes, and outgoing payments because it is how we associate clientId with those resources.

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

mkurapov · 2022-11-28T16:13:13Z

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

+  const grantOptions = {
+    authServer: paymentPointer.authServer,
+    accessType: AccessType.IncomingPayment,
+    accessActions: [AccessAction.ReadAll]


I guess it all depends on the AS, but would just Read be "safer" here? Having to create a grant per incoming payment isn't great, but makes it easier on auth servers to restrict access

I don't have a strong opinion either way, and I'm unsure which would make auth server locking more of a necessity:
#779 (comment)
👇

I think just Read wouldn't work because I'd imagine in most cases, this Rafiki instance wasn't the client creating the incoming payment. It just wants to read it to pay it.

mkurapov · 2022-11-28T16:28:53Z

packages/backend/src/open_payments/grant/service.test.ts

+    test.each`
+      newAuthServer
+      ${false}
+      ${true}
+    `(
+      'Grant can be created and fetched (new auth server: $newAuthServer)',


imo might be easier to split into two separate tests to avoid the if (newAuthServer) checks

I'm not sure if I made it better or worse by moving it to the describe.each
aa571e1

mkurapov · 2022-11-28T16:31:41Z

packages/backend/src/open_payments/payment_pointer/model.ts

+    authServer: string
+  }): OpenPaymentsPaymentPointer {
+    return {
+      id: this.url,


Would returning url: this.url be more clear from an Open Payments API perspective?

As in, change id to url in the Open Payments spec?
https://github.com/interledger/open-payments/blob/b363d33038fe789e5388f04f80ddd06a4fa97093/openapi/resource-server.yaml#L809-L813

As in, change id to url in the Open Payments spec?

Exactly, wasn't a comment of the code, more in general. ids for other resources (incoming/outgoing payments) have a different meaning that the id for the payment pointer. NABD, it felt slightly off to me in the spec in the first place

🤔 Want to elaborate on how the id is different for the payment pointer (here or in an open-payments issue)?
Part of me wants to say payment pointers are probably going to be out of scope for Open Payments soon.
Also:

fix: payment pointer id description open-payments#223

Well, for example in our case, we call incoming payments by baseUrl/incoming-payment/{{id}}, id being a uuid, a sub resource identifier. You don't necessarily have a similar "baseUrl/payment-pointer" resource, where you could GET a payment pointer using the id path param.

Likewise in backend, when we use paymentPointer.id its usually the model's uuid. I don't think it's worth changing, just might not be always 100% clear, is all.

mkurapov · 2022-11-28T16:48:31Z

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

+    return existingGrant
+  }
+
+  const grant = await deps.openPaymentsClient.grant.request({


Is this a case for when we request the receiver on the outgoing payment lifecycle hook?

packages/auth/src/access/types.ts

infrastructure/local/peer-docker-compose.yml

sabineschaller · 2022-11-30T10:14:18Z

packages/backend/migrations/20221012013413_create_grants_table.js

+exports.up = function (knex) {
+  return knex.schema.createTable('grants', function (table) {
+    table.uuid('id').notNullable().primary()
+    table.uuid('authServerId').notNullable()
+    table.foreign('authServerId').references('authServers.id')
+    table.string('continueId').nullable()
+    table.string('continueToken').nullable()
+    table.string('accessToken').nullable().unique()
+    table.string('accessType').notNullable()
+    table.specificType('accessActions', 'text[]')
+
+    table.timestamp('expiresAt').nullable()
+
+    table.timestamp('createdAt').defaultTo(knex.fn.now())
+    table.timestamp('updatedAt').defaultTo(knex.fn.now())
+
+    table.unique(['authServerId', 'accessType', 'accessActions'])
+  })
+}


Is it sufficient to not store the entire access array of the grant?

It is for now since the RS is only requesting read-all incoming payment grants, and the new GrantService only supports creating grants of a single access type.
Do we anticipate the RS requesting multi-type grants that we should consider future-proofing for?

sabineschaller · 2022-11-30T10:18:21Z

packages/backend/src/open_payments/grant/model.ts

+import { AuthServer } from '../authServer/model'
+import { BaseModel } from '../../shared/baseModel'
+
+export class Grant extends BaseModel {


Renaming is the right move since we only store references to outgoing payment grants. I'll have to look into the other PR to understand why we can drop clientId.

sabineschaller · 2022-11-30T10:26:17Z

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

+  const grantOptions = {
+    authServer: paymentPointer.authServer,
+    accessType: AccessType.IncomingPayment,
+    accessActions: [AccessAction.ReadAll]


I think just Read wouldn't work because I'd imagine in most cases, this Rafiki instance wasn't the client creating the incoming payment. It just wants to read it to pay it.

mkurapov · 2022-12-05T23:18:42Z

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

+    return existingGrant
+  }
+
+  const grant = await deps.openPaymentsClient.grant.request({


Pretty edge-casey IMO, not sure if worth adding just yet

wilsonianb added 5 commits November 22, 2022 07:18

feat(backend): request grant to query incoming payment receiver

ce5c135

Add services for managing grants.

fix(localenv): connect Docker network

6f7b31f

Update environment variables.

fix(auth): add grant access actions

2988e54

Remove unused locations and interval fields and account access.

fix(backend): properly export public key jwk

3763cd6

fix(open-payments): properly construct grant request body

da07fea

github-actions bot added pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: open-payments type: localenv Local playground type: source Changes business logic type: tests Testing related labels Nov 23, 2022

wilsonianb marked this pull request as ready for review November 23, 2022 02:05

wilsonianb requested review from njlie, mkurapov and sabineschaller and removed request for njlie November 23, 2022 02:08

wilsonianb commented Nov 23, 2022

View reviewed changes

wilsonianb mentioned this pull request Nov 23, 2022

feat(backend): add services for managing RS grants #693

Closed

4 tasks

wilsonianb commented Nov 23, 2022

View reviewed changes

mkurapov reviewed Nov 28, 2022

View reviewed changes

wilsonianb mentioned this pull request Nov 28, 2022

Rotate expired grant access token for incoming payment lookup from one Rafiki to another #795

Closed

wilsonianb added 3 commits November 28, 2022 12:32

Merge branch 'main' into bw-rs-client

417e6a3

chore(backend): import open-payments generateJwk

4a453a9

chore(backend): store and check grant expiresAt

aa571e1

njlie reviewed Nov 29, 2022

View reviewed changes

packages/auth/src/access/types.ts Show resolved Hide resolved

chore(auth): add interval to OutgoingPaymentLimit

19fe828

wilsonianb requested review from mkurapov and njlie November 29, 2022 13:29

njlie previously approved these changes Nov 29, 2022

View reviewed changes

wilsonianb mentioned this pull request Nov 30, 2022

chore(backend): replace GrantReference with OutgoingPaymentGrant #800

Merged

4 tasks

sabineschaller reviewed Nov 30, 2022

View reviewed changes

chore(localenv): re-format networks

291ec8c

wilsonianb dismissed njlie’s stale review via 291ec8c November 30, 2022 13:29

wilsonianb requested a review from sabineschaller November 30, 2022 13:35

wilsonianb added 2 commits December 1, 2022 14:08

Merge branch 'main' into bw-rs-client

f29d867

chore(backend): don't cast GrantRequest

2e2a3a9

wilsonianb mentioned this pull request Dec 2, 2022

Reconcile string enums with generated union types #808

Closed

mkurapov approved these changes Dec 5, 2022

View reviewed changes

wilsonianb merged commit a0302b3 into main Dec 6, 2022

wilsonianb deleted the bw-rs-client branch December 6, 2022 14:30

wilsonianb mentioned this pull request Jan 19, 2023

Sporadic localenv network error #1003

Closed

	"localenv": "docker compose -f ./infrastructure/local/docker-compose.yml -f ./infrastructure/local/peer-docker-compose.yml",
	"localenv:build": "docker compose -f ./infrastructure/local/docker-compose.yml -f ./infrastructure/local/peer-docker-compose.yml -f ./infrastructure/local/build-override.yml build",

	"access-incoming": {
	/** @description The type of resource request as a string. This field defines which other fields are allowed in the request object. */
	type: "incoming-payment";
	/** @description The types of actions the client instance will take at the RS as an array of strings. */
	actions: (
	\| "create"
	\| "complete"
	\| "read"
	\| "read-all"
	\| "list"
	\| "list-all"
	)[];
	/**
	* Format: uri
	* @description A string identifier indicating a specific resource at the RS.
	*/
	identifier?: string;
	};
	/** access-outgoing */
	"access-outgoing": {
	/** @description The type of resource request as a string. This field defines which other fields are allowed in the request object. */
	type: "outgoing-payment";
	/** @description The types of actions the client instance will take at the RS as an array of strings. */
	actions: ("create" \| "read" \| "read-all" \| "list" \| "list-all")[];
	/**
	* Format: uri
	* @description A string identifier indicating a specific resource at the RS.
	*/
	identifier: string;
	limits?: external["https://raw.githubusercontent.com/interledger/open-payments/b363d33038fe789e5388f04f80ddd06a4fa97093/openapi/schemas.yaml"]["components"]["schemas"]["limits-outgoing"];
	};
	/** access-quote */
	"access-quote": {
	/** @description The type of resource request as a string. This field defines which other fields are allowed in the request object. */
	type: "quote";
	/** @description The types of actions the client instance will take at the RS as an array of strings. */
	actions: ("create" \| "read" \| "read-all")[];
	};

	//lock grant
	await deps.grantReferenceService.lock(grant.grant, deps.knex)

feat(backend): request grant to query incoming payment receiver #779

feat(backend): request grant to query incoming payment receiver #779

Conversation

wilsonianb commented Nov 23, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

wilsonianb commented Nov 23, 2022

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 commented Nov 23, 2022 • edited Loading

wilsonianb commented Nov 23, 2022

mkurapov commented Nov 23, 2022

wilsonianb commented Nov 23, 2022

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 Nov 28, 2022 • 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

wilsonianb Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb Nov 28, 2022 • 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

mkurapov Nov 30, 2022 • 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

wilsonianb commented Nov 23, 2022 •

edited

Loading

mkurapov commented Nov 23, 2022 •

edited

Loading

wilsonianb Nov 28, 2022 •

edited

Loading

wilsonianb Nov 30, 2022 •

edited

Loading

wilsonianb Nov 28, 2022 •

edited

Loading

mkurapov Nov 30, 2022 •

edited

Loading