feat(auth): allow non-interactive grants for incoming payments #588

sabineschaller · 2022-09-02T10:58:54Z

Changes proposed in this pull request

if the access type is incoming-payment, the grant will be issued right away, without interaction

Context

fixes Support grant issuance without interaction #415

Checklist

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

sabineschaller · 2022-09-02T11:00:02Z

Q: Should we have the same behavior for quote grant requests? @wilsonianb

wilsonianb · 2022-09-02T15:16:37Z

Q: Should we have the same behavior for quote grant requests? @wilsonianb

I'm not sure.
Creating a quote can indirectly reveal the sender's available balance at the account provider.
OTOH a grant allowing quote creation is likely to also include outgoing payment creation, which would require interaction.

packages/auth/src/accessToken/service.test.ts

wilsonianb · 2022-09-02T15:27:54Z

packages/auth/src/grant/routes.ts

@@ -66,7 +71,8 @@ export function createGrantRoutes({
  }
 }

-function validateGrantRequest(
+// exported for testing
+export function validateGrantRequest(


Does this have checks that aren't covered by the OpenAPI validator middleware?

rafiki/packages/auth/src/app.ts

Line 239 in 0642a0f

createValidatorMiddleware<ContextType<typeof route>>(openApi, {

I don't think so but I'd like @njlie to confirm.

I think we're missing out on the benefits of the OpenAPI validator middleware type guard by not specifying the ctx.body type in createGrantInitiation and elsewhere

rafiki/packages/openapi/src/index.ts

Lines 47 to 51 in 668e7cb

export type ValidateFunction<T> = (data: any) => data is T

export interface OpenAPI {

paths: Paths

createRequestValidator<T>(options: RequestOptions): ValidateFunction<T>

See:

rafiki/packages/backend/src/open_payments/payment/incoming/routes.ts

Lines 79 to 88 in 668e7cb

export type CreateBody = {

description?: string

expiresAt?: string

incomingAmount?: AmountJSON

externalRef?: string

}

async function createIncomingPayment(

deps: ServiceDependencies,

ctx: CreateContext<CreateBody>

Maybe reopen Validate GNAP requests with OpenAPI #351 ?

I'd hold off on any work there now that Adrian has been working on his auth server implementation.

I don't think so but I'd like @njlie to confirm.

Yeah I'm pretty sure this can be excised now, I wrote this before we had brought OpenAPI into the auth service. This basically is a type guard against the POST / request which is definitely covered.

I removed the validation function and re-opened #351

packages/auth/src/grant/routes.ts

packages/auth/src/grant/service.test.ts

packages/auth/src/accessToken/service.test.ts

packages/auth/src/config/app.ts

sabineschaller · 2022-09-05T09:50:38Z

Creating a quote can indirectly reveal the sender's available balance at the account provider.
OTOH a grant allowing quote creation is likely to also include outgoing payment creation, which would require interaction.

Then let's leave it as is for now.

wilsonianb

Should config.authServerSpec be updated to include feat(auth-spec): grant request changes open-payments#168 ?

packages/auth/src/accessToken/service.test.ts

wilsonianb · 2022-09-08T14:27:22Z

packages/auth/src/config/app.ts

@@ -10,6 +10,10 @@ function envInt(name: string, value: number): number {
  return envValue == null ? value : parseInt(envValue)
 }

+function envBool(name: string, value: boolean): boolean {
+  const envValue = process.env[name]
+  return envValue == null ? value : envValue === 'true'


I prefer your version over:

rafiki/packages/backend/src/config/app.ts

Lines 20 to 23 in 668e7cb

// function envBool(name: string, value: boolean): boolean {

// const envValue = process.env[name]

// return envValue == null ? value : Boolean(envValue)

// }

I didn't know we had that.

wilsonianb · 2022-09-08T14:57:39Z

packages/auth/src/grant/routes.ts

@@ -66,7 +71,8 @@ export function createGrantRoutes({
  }
 }

-function validateGrantRequest(
+// exported for testing
+export function validateGrantRequest(


I think we're missing out on the benefits of the OpenAPI validator middleware type guard by not specifying the ctx.body type in createGrantInitiation and elsewhere

rafiki/packages/openapi/src/index.ts

Lines 47 to 51 in 668e7cb

export type ValidateFunction<T> = (data: any) => data is T

export interface OpenAPI {

paths: Paths

createRequestValidator<T>(options: RequestOptions): ValidateFunction<T>

See:

rafiki/packages/backend/src/open_payments/payment/incoming/routes.ts

Lines 79 to 88 in 668e7cb

export type CreateBody = {

description?: string

expiresAt?: string

incomingAmount?: AmountJSON

externalRef?: string

}

async function createIncomingPayment(

deps: ServiceDependencies,

ctx: CreateContext<CreateBody>

Maybe reopen Validate GNAP requests with OpenAPI #351 ?

wilsonianb · 2022-09-08T14:58:31Z

packages/auth/src/grant/routes.ts

+    !deps.config.incomingPaymentInteraction &&
+    body.access_token.access
+      .map((acc) => {
+        return isIncomingPaymentAccessRequest(acc as IncomingPaymentRequest)


Suggested change

return isIncomingPaymentAccessRequest(acc as IncomingPaymentRequest)

return isIncomingPaymentAccessRequest(acc as AccessRequest)

I think the isIncomingPaymentAccessRequest argument type should be changed to AccessRequest.

Also, if we specified ctx.body type we wouldn't need to cast here at all (#588 (comment))

I didn't want to touch this because I thought I would add the change to have the ctx.body typed but that's probably going to be a subsequent PR so yes, I'll change that.

But it complains with

Types of property 'type' are incompatible. Type 'AccessType.OutgoingPayment' is not assignable to type 'AccessType.IncomingPayment'.ts(2345)

BaseAccessRequest might work, but I just noticed the other type guards also have the target type as the argument type.
Maybe they can all be updated as part of #351 instead of this pr.

I think that is a good idea.

packages/auth/src/grant/routes.ts

packages/auth/src/grant/service.ts

njlie · 2022-09-13T16:48:16Z

packages/auth/migrations/20220504161932_create_grants_table.js

-    table.string('continueToken').unique()
-    table.string('continueId').unique()
+    table.string('continueToken').notNullable().unique()
+    table.string('continueId').notNullable().unique()


It shouldn't be necessary to generate continuation information if interaction (or further continuation) isn't necessary for a grant. The POST / should just return the access token without any continuation information. Continuation information could just be generated if the grant is modified in a way that requires interaction to be issued again (if we end up supporting that again).

I think we discussed that while you were away. We always want to be able to delete/revoke the grant and for that you need the continue token and id. Hence, it must always be present. We also already changed the spec to make it always required.

njlie · 2022-09-13T20:06:07Z

packages/auth/src/grant/routes.ts

      access: access.map((a: Access) => accessToBody(a)),
-      expiresIn: accessToken.expiresIn
+      expiresIn: accessToken.expiresIn,
+      continue: {


We can exclude the continue object if there's no interaction required and the AS can just supply an access_token instead. Also vice/versa for if interaction is required. It's basically an either/or situation between continue and access_token with how we're using GNAP.

…t is present

wilsonianb · 2022-09-22T14:44:12Z

packages/auth/src/grant/routes.ts

+    !deps.config.incomingPaymentInteraction &&
+    body.access_token.access
+      .map((acc) => {
+        return isIncomingPaymentAccessRequest(acc as IncomingPaymentRequest)


packages/auth/src/grant/routes.ts

packages/auth/src/grant/service.test.ts

packages/auth/src/grant/routes.ts

wilsonianb · 2022-09-27T13:21:49Z

packages/auth/src/grant/routes.ts

+      access_token: {
+        value: grant.continueToken
+      },
+      uri: domain + `continue/${grant.continueId}`


Should this still include wait (RECOMMENDED)?
https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-3.1

I don't think so because the grant is granted right away. At least that was my interpretation of it.

feat(auth): allow non-interactive grants for incoming payments

52e4f55

github-actions bot added pkg: auth Changes in the GNAP auth package. type: source Changes business logic type: tests Testing related labels Sep 2, 2022

sabineschaller requested review from wilsonianb and njlie September 2, 2022 11:00

sabineschaller added 4 commits September 2, 2022 13:06

test(auth): remove tests that are caught by request validator

96767a9

test(auth): fix typing

c11c566

fix(auth): add not nullable field continueId to grant

add9552

fix(auth): create grant request test suite

a438f8e

wilsonianb reviewed Sep 2, 2022

View reviewed changes

sublimator reviewed Sep 5, 2022

View reviewed changes

packages/auth/src/grant/service.test.ts Outdated Show resolved Hide resolved

sublimator reviewed Sep 5, 2022

View reviewed changes

packages/auth/src/accessToken/service.test.ts Outdated Show resolved Hide resolved

sublimator reviewed Sep 5, 2022

View reviewed changes

packages/auth/src/config/app.ts Outdated Show resolved Hide resolved

sabineschaller added 2 commits September 5, 2022 10:58

fix(auth): unnecessary typecast, check for incoming payment type

0c3f58f

feat(auth): add envBool function

d938fce

sabineschaller requested a review from wilsonianb September 5, 2022 09:02

fix(auth): remove another unneccessary typecast

d5d1c52

sabineschaller marked this pull request as ready for review September 5, 2022 09:47

wilsonianb reviewed Sep 8, 2022

View reviewed changes

matdehaast requested a review from adrianhopebailie September 13, 2022 13:34

njlie reviewed Sep 13, 2022

View reviewed changes

wilsonianb mentioned this pull request Sep 20, 2022

feat(auth): refactor route mounting to be more easily readable #613

Merged

4 tasks

Merge branch 'main' into s2-no-interaction-grant

bc0e41d

sabineschaller mentioned this pull request Sep 22, 2022

Validate GNAP requests with OpenAPI #351

Closed

fix(auth): remove unneccessary validation function

c9b0d17

sabineschaller added 3 commits September 22, 2022 13:40

chore(auth): update reference to API spec

172d0dc

test(auth): avoid typecasting

dee2671

feat(auth): check that on non-incoming-payment grant request, interac…

aed10d4

…t is present

sabineschaller requested review from wilsonianb and njlie September 22, 2022 12:46

wilsonianb reviewed Sep 22, 2022

View reviewed changes

refactor(aut): add Brandon's suggestions

bb99546

sabineschaller requested a review from wilsonianb September 22, 2022 15:47

feat(auth): create grant and access token within trx

428cd92

sabineschaller commented Sep 26, 2022

View reviewed changes

packages/auth/src/grant/routes.ts Show resolved Hide resolved

fix(auth): deal with internal server error on grant and token creation

bd2c4c9

wilsonianb approved these changes Sep 27, 2022

View reviewed changes

sabineschaller merged commit 3d30ce7 into main Sep 27, 2022

sabineschaller deleted the s2-no-interaction-grant branch September 27, 2022 13:55

wilsonianb mentioned this pull request Oct 3, 2022

Discuss adding support for rejecting/completing an incoming payment to the Admin API #648

Closed

This was referenced Nov 4, 2022

Adding AS types & grant requests support to open-payments client #727

Merged

feat(backend): bypass sig validation #741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): allow non-interactive grants for incoming payments #588

feat(auth): allow non-interactive grants for incoming payments #588

sabineschaller commented Sep 2, 2022 •

edited

Loading

sabineschaller commented Sep 2, 2022

wilsonianb commented Sep 2, 2022

wilsonianb Sep 2, 2022

sabineschaller Sep 5, 2022

wilsonianb Sep 8, 2022

sabineschaller Sep 13, 2022

njlie Sep 13, 2022

sabineschaller Sep 22, 2022

sabineschaller commented Sep 5, 2022

wilsonianb left a comment

wilsonianb Sep 8, 2022

sabineschaller Sep 13, 2022

wilsonianb Sep 8, 2022

wilsonianb Sep 8, 2022 •

edited

Loading

wilsonianb Sep 22, 2022

sabineschaller Sep 22, 2022

sabineschaller Sep 22, 2022

wilsonianb Sep 22, 2022 •

edited

Loading

sabineschaller Sep 23, 2022

njlie Sep 13, 2022 •

edited

Loading

sabineschaller Sep 22, 2022

njlie Sep 13, 2022

sabineschaller Sep 22, 2022

wilsonianb Sep 22, 2022

wilsonianb Sep 27, 2022

sabineschaller Sep 27, 2022

	export type ValidateFunction<T> = (data: any) => data is T

	export interface OpenAPI {
	paths: Paths
	createRequestValidator<T>(options: RequestOptions): ValidateFunction<T>

	export type CreateBody = {
	description?: string
	expiresAt?: string
	incomingAmount?: AmountJSON
	externalRef?: string
	}

	async function createIncomingPayment(
	deps: ServiceDependencies,
	ctx: CreateContext<CreateBody>

	// function envBool(name: string, value: boolean): boolean {
	// const envValue = process.env[name]
	// return envValue == null ? value : Boolean(envValue)
	// }

	return isIncomingPaymentAccessRequest(acc as IncomingPaymentRequest)
	return isIncomingPaymentAccessRequest(acc as AccessRequest)

feat(auth): allow non-interactive grants for incoming payments #588

feat(auth): allow non-interactive grants for incoming payments #588

Conversation

sabineschaller commented Sep 2, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

sabineschaller commented Sep 2, 2022

wilsonianb commented Sep 2, 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

Choose a reason for hiding this comment

sabineschaller commented Sep 5, 2022

wilsonianb 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

wilsonianb Sep 8, 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 Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie Sep 13, 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

sabineschaller commented Sep 2, 2022 •

edited

Loading

wilsonianb Sep 8, 2022 •

edited

Loading

wilsonianb Sep 22, 2022 •

edited

Loading

njlie Sep 13, 2022 •

edited

Loading