feat(auth): add subject to grant #3440

cozminu · 2025-05-27T14:04:38Z

Changes proposed in this pull request

Context

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

netlify · 2025-05-27T14:04:42Z

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

Name	Link
🔨 Latest commit	`4b6f81c`
🔍 Latest deploy log	https://app.netlify.com/projects/brilliant-pasca-3e80ec/deploys/68c0374dc0e20a0008ee53d0
😎 Deploy Preview	https://deploy-preview-3440--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

github-actions · 2025-06-02T11:57:08Z

🚀 Performance Test Results

Test Configuration:

VUs: 4
Duration: 1m0s

Test Metrics:

Requests/s: 38.08
Iterations/s: 12.72
Failed Requests: 0.00% (0 of 2299)

📜 Logs


> [email protected] run-tests:testenv /home/runner/work/rafiki/rafiki/test/performance
> ./scripts/run-tests.sh -e test "-k" "-q" "--vus" "4" "--duration" "1m"

Cloud Nine GraphQL API is up: http://localhost:3101/graphql
Cloud Nine Wallet Address is up: http://localhost:3100/
Happy Life Bank Address is up: http://localhost:4100/
cloud-nine-wallet-test-backend already set
cloud-nine-wallet-test-auth already set
happy-life-bank-test-backend already set
happy-life-bank-test-auth already set
     data_received..................: 830 kB 14 kB/s
     data_sent......................: 1.8 MB 29 kB/s
     http_req_blocked...............: avg=7.64µs   min=3.1µs   med=6.01µs   max=740.24µs p(90)=7.13µs   p(95)=7.73µs  
     http_req_connecting............: avg=832ns    min=0s      med=0s       max=685.9µs  p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=104.29ms min=7.77ms  med=85.63ms  max=600.3ms  p(90)=177.49ms p(95)=205.07ms
       { expected_response:true }...: avg=104.29ms min=7.77ms  med=85.63ms  max=600.3ms  p(90)=177.49ms p(95)=205.07ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 2299
     http_req_receiving.............: avg=98.73µs  min=27.22µs med=85.8µs   max=955.85µs p(90)=127.54µs p(95)=161.1µs 
     http_req_sending...............: avg=45.06µs  min=11.66µs med=31.19µs  max=6.49ms   p(90)=46.4µs   p(95)=63.84µs 
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=104.15ms min=7.56ms  med=85.48ms  max=600.23ms p(90)=177.35ms p(95)=204.94ms
     http_reqs......................: 2299   38.076114/s
     iteration_duration.............: avg=313.93ms min=201.7ms med=299.83ms max=1.18s    p(90)=383.76ms p(95)=419.52ms
     iterations.....................: 768    12.719642/s
     vus............................: 4      min=4       max=4 
     vus_max........................: 4      min=4       max=4

packages/auth/src/subject/service.ts

mkurapov · 2025-06-12T16:41:05Z

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

+jest.mock('../access/types', () => ({
+  isIncomingPaymentAccessRequest: (access: AccessRequest) =>
+    access.type === 'incoming-payment',
+  isQuoteAccessRequest: (access: AccessRequest) => access.type === 'quote'
+}))


Why do we need this mock?

canSkipInteraction function uses them and the purpose of the unit test is to test only the unit and not the deps, but I see there are also benefits from using the unmocked functions. removed the mock

njlie

Overall LGTM, just a couple things

packages/auth/src/graphql/resolvers/grant.ts

njlie · 2025-06-16T17:52:44Z

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

+  })
+
+  describe('getByGrant', (): void => {
+    test('gets access', async () => {


nit: this is getting subject, not access

mkurapov

Few comments!

mkurapov · 2025-06-23T15:18:35Z

packages/auth/src/grant/routes.ts

+        400,
+        GNAPErrorCode.InvalidRequest,
+        err.message || 'invalid request'


We should match GrantErrors to their respective HTTP codes and GNAPErrorCodes.
This is similar to how @oana-lolea maps AccessErrors to the respective code & error codes in her PR.

mkurapov · 2025-06-23T15:23:33Z

packages/auth/src/grant/utils.ts

+  const emptyAccess = (body.access_token?.access?.length ?? 0) === 0
+
+  if (emptySubject && emptyAccess) {
+    throw new Error('subject or access_token required')


Should this throw a GrantError instead?

yes, adapted to throw GrantError

packages/auth/src/graphql/schema.graphql

mkurapov · 2025-06-23T15:27:07Z

packages/auth/src/accessToken/routes.ts

 interface IntrospectBody {
  access_token: string
  access?: AccessItem[]
+  subjects?: SubjectItem[]


If we don't use subjects anywhere (given that we don't pass it in during the introspection request), then its OK to leave out

packages/auth/src/subject/service.ts

Co-authored-by: Max Kurapov <[email protected]>

njlie

Just one suggestion to be applied accross all the places where trx was cast in order to silence a Typescript error.

njlie · 2025-07-01T08:52:50Z

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

  let appContainer: TestContainer
  let accessService: AccessService
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = null as unknown as Knex.Transaction


It might be healthier to actually initialize trx in the beforeAll block so that the tables get properly truncated when the test is finished, but the editor also doesn't flag a Typescript error. I assume this type casting was done to address the TS error.

beforeAll(async (): Promise<void> => { ... trx = await appContainer.knex.transaction() })

Yes, It was a fix to a TS error

Variable 'trx' is used before being assigned.ts(2454)

Using a transaction is tricky because it can deadlock tests because some queries are done within a transaction and others are not. Managed to fix it, but in another manner.

let trx: KnexOrTransaction beforeAll(async (): Promise<void> => { ... trx = await appContainer.knex })

Is this ok with you?

njlie · 2025-07-01T08:56:04Z

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

  let deps: IocContract<AppServices>
  let appContainer: TestContainer
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = undefined as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T08:56:10Z

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

  let deps: IocContract<AppServices>
  let appContainer: TestContainer
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = undefined as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T08:56:42Z

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

  let deps: IocContract<AppServices>
  let appContainer: TestContainer
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = undefined as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T09:02:17Z

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

  let appContainer: TestContainer
  let grantService: GrantService
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = null as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T09:03:52Z

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

@@ -0,0 +1,86 @@
+import { AccessRequest } from '../access/types'


Thanks for adding these tests! Especially since they cover logic that existed before this PR

njlie · 2025-07-01T09:11:17Z

packages/auth/src/signature/middleware.test.ts

    let interaction: Interaction
    let token: AccessToken
-    let trx: Knex.Transaction
+    const trx: Knex.Transaction = null as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

njlie · 2025-07-01T09:21:18Z

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

  let client: string
  let amtDelivered: bigint
-  let trx: Knex.Transaction
+  const trx: Knex.Transaction = null as unknown as Knex.Transaction


Ditto to prior comment on initializing trx.

mkurapov · 2025-07-21T16:01:53Z

packages/auth/src/graphql/resolvers/grant.ts

  client: grant.client,
-  access: grant.access?.map((item) => accessToGraphql(item)),
+  access: grant.access?.map((item) => accessToGraphql(item)) || [],
+  subject: grant.subjects?.map((item) => subjectToGraphql(item)) || [],


grant.subjects only exists if we add it to the withGraphFetched request in the service methods OR we have subjects as a separate "child" resolver under grants, and call the subjectService.getByGrantId directly.

An example of this is the Asset > sendingFee resolver in backend.

fixed with withGraphFetched('subjects') in the service methods

mkurapov · 2025-07-21T16:06:09Z

packages/auth/src/graphql/schema.graphql

  "Wallet address of the grantee's account."
  client: String!
  "Details of the access provided by the grant."
-  access: [Access!]!


I think we can keep it as [Access!]!, if there is no access it can simply be an empty array.

mkurapov · 2025-07-21T16:07:56Z

packages/auth/src/grant/routes.ts

      400,
      GNAPErrorCode.InvalidRequest,
-      'access identifier required'
+      err instanceof Error ? err.message : ''


to avoid empty string, let's provide some generic message instead at least. It's less important for the client, its more so for us to find where to debug it.

changed to 'unknown error while checking interaction requirement'

mkurapov · 2025-07-21T16:08:53Z

packages/auth/src/subject/service.ts

+      'subject id must be a valid https url'
+    )
+  }
+  if (subject.format != 'uri') {


Suggested change

if (subject.format != 'uri') {

if (subject.format !== 'uri') {

mkurapov · 2025-07-21T16:23:10Z

packages/auth/src/subject/service.ts

+
+function validateSubjectRequest(subject: SubjectRequest): void {
+  try {
+    if (!subject.id.startsWith('https://')) throw 1


not sure if @njlie agrees, but I think just checking that it is a valid URL is sufficient, without checking the https://. IMO it can be up to the ASE to determine how strict they want to be with the specifics, like protocol.

For the record, I also agree.

mkurapov · 2025-07-21T16:25:40Z

packages/auth/src/grant/model.ts

+  subjectItems: Subject[]
 ): OpenPaymentsGrant {
  return {
    access_token: toOpenPaymentsAccessToken(accessToken, accessItems, {


access_token could be undefined now, just like subject

mkurapov · 2025-07-21T16:28:07Z

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

  let appContainer: TestContainer
  let accessService: AccessService
-  let trx: Knex.Transaction
+  let trx: TransactionOrKnex


We should remove the import { Knex } from 'knex' import

mkurapov · 2025-07-21T16:36:19Z

packages/auth/src/interaction/routes.ts

  ctx.body = {
    grantId: interaction.grant.id,
    access: access.map(toOpenPaymentsAccess),
+    subjects: subjects.map(toOpenPaymentsSubject),


there can only be one subject for a grant, but the sub_ids can be many. So this can either be a subject object with sub_ids array, (like the Open Payments spec), or we can directly return sub_ids here.

The same applies for the subject updates in the GraphQL schema

changed to subject object with sub_ids array.
I'm not aware of a subject updates.

njlie · 2025-07-30T18:18:46Z

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

-        const fetchedGrant = await grantService.getByIdWithAccess(grant.id)
+        const grantRequest: GrantRequest = {
+          ...BASE_GRANT_REQUEST,
+          subject: {


access should still be included here so that this service call can properly test that it also retrieves an access field along with the grant

mkurapov

Thinking through this a bit, I think we need to update the response types.
Since subject should only be returned after we have gone through a successful interaction, we should:

Make sure we do not return subject in the initial grant request
Return subject in the grant continue request only

Section 3.4 of GNAP:

The grant request MUST be in the approved state to return this field in the response.

This will require open-payments-specification changes, but we can work together @cozminu and I can take care of those changes

CC @njlie for a double check :)

mkurapov · 2025-08-07T11:46:01Z

packages/auth/src/grant/routes.ts

+      err instanceof Error
+        ? err.message
+        : 'unknown error while checking interaction requirement'


Looking at what canSkipInteraction throws, we should instead use err.description, since that will always be set.

casted e as GrantError to fix this.

... } catch (e) { const err = e as GrantError throw new GNAPServerRouteError( 400, GNAPErrorCode.InvalidRequest, err.message ) ...

Other option would be to throw 500 if e is not GrantError

mkurapov · 2025-08-07T11:46:42Z

packages/auth/src/grant/errors.ts

+
+export class GrantError extends Error {
+  code: GrantErrorCode
+  constructor(code: GrantErrorCode, description?: string) {


Suggested change

constructor(code: GrantErrorCode, description?: string) {

constructor(code: GrantErrorCode, description: string) {

Looks like description is set everywhere already

mkurapov · 2025-08-07T11:49:34Z

packages/auth/src/grant/routes.ts

-        err
+        errorToHTTPCode[err.code],
+        errorToGNAPCode[err.code],
+        err.message || 'invalid request'


Suggested change

err.message || 'invalid request'

err.description || 'invalid request'

same as the other comment, we can use err.description here, since it will always be set and is most useful. Or rename GrantError.description to GrantError.message

renamed description to message to fit Error class

mkurapov · 2025-08-07T11:53:15Z

packages/auth/src/grant/routes.ts

    )
  }
  const access = await deps.accessService.getByGrant(grant.id)
+  const subjects = await deps.subjectService.getByGrant(grant.id)


Since subjects always requires interaction, I think we don't need to fetch subjects for the non-interactive grant creation.

njlie · 2025-08-13T16:30:48Z

Thinking through this a bit, I think we need to update the response types. Since subject should only be returned after we have gone through a successful interaction, we should:

Make sure we do not return subject in the initial grant request

Return subject in the grant continue request only

Section 3.4 of GNAP:

The grant request MUST be in the approved state to return this field in the response.

This will require open-payments-specification changes, but we can work together @cozminu and I can take care of those changes

CC @njlie for a double check :)

Based on the quoted excerpt, I thought that the AS could deem subject information something noninteractive - the grant just needed to be approved. But I think ultimately the interpretation is correct, based on the following:

The AS MUST return the subject field only in cases where the AS is sure that the RO and the end user are the same party. This can be accomplished through some forms of interaction with the RO (Section 4).

mkurapov

Looks good, just a few minor comments

packages/auth/src/grant/routes.ts

packages/auth/src/grant/model.ts

mkurapov · 2025-08-18T13:54:49Z

packages/auth/src/grant/service.ts

    if (!trx) {
      await grantTrx.rollback()
    }


Suggested change

if (!trx) {

await grantTrx.rollback()

}

await grantTrx.rollback()

as grantTrx is always defined. Same for L281-283

Tests will fail if I do this. Reason being that the transaction starts on a higher level if trx is provided and rollback or commit will be called twice: once here and once on the higher level.

Co-authored-by: Max Kurapov <[email protected]>

mkurapov

Looks good! Will just wait for RAF-1147 / #3608

package.json

Co-authored-by: Max Kurapov <[email protected]>

mkurapov · 2025-09-05T10:34:55Z

@cozminu if you merge in latest changes from main, you should see open-payments-specifications as a submodule. You can now go to the submodule, checkout v1.1.0 tag of the specifications (the change that contains the subject additions), and finalize the PR 👍

mkurapov · 2025-09-15T17:38:59Z

packages/auth/src/openapi/specs/id-provider.yaml

                properties:
                  access:
                    $ref: ./auth-server.yaml#/components/schemas/access
+                  subject:


We should make this spec standalone, and move over the correct components (access/subject) into this file, similar to how it was done for token-introspection

feat(auth): add subject to grant

6b05778

cozminu self-assigned this May 27, 2025

github-actions bot added type: tests Testing related type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels May 27, 2025

feat(auth): add subject to grant

c6a310e

fix(auth): grant access when is undefined

ed8411d

cozminu requested review from njlie and mkurapov June 2, 2025 12:30

mkurapov reviewed Jun 12, 2025

View reviewed changes

njlie reviewed Jun 16, 2025

View reviewed changes

cozminu added 2 commits June 20, 2025 18:12

feat(auth): subject id validation

fdfbba3

fix(auth): fix tests

03b1ec5

github-actions bot added the pkg: backend Changes in the backend package. label Jun 20, 2025

mkurapov reviewed Jun 23, 2025

View reviewed changes

cozminu and others added 3 commits June 25, 2025 11:04

Update packages/auth/src/graphql/schema.graphql

9b224b2

Co-authored-by: Max Kurapov <[email protected]>

feat(auth): throw GrantError instead of Error

d9d7982

fix(auth): description for subIdFormat

7a7d8e6

cozminu requested a review from mkurapov June 25, 2025 14:08

njlie reviewed Jul 1, 2025

View reviewed changes

fix(auth): trx in tests is knex

efd3042

mkurapov reviewed Jul 21, 2025

View reviewed changes

cozminu added 5 commits July 23, 2025 17:14

Merge branch 'main' into cozmin/raf-996

454195e

fix: grant service to throw only grant errors

99e11b9

fix(auth): address change requests

a2db4d6

fix(auth): address change requests

223e69e

fix tests

eb28179

github-actions bot removed the pkg: backend Changes in the backend package. label Jul 28, 2025

cozminu added 2 commits July 28, 2025 18:30

access token optional in grant response

bc07cae

fix(auth): move accessErrorsMap to grant

1b89c37

njlie reviewed Jul 30, 2025

View reviewed changes

cozminu added 4 commits July 31, 2025 15:18

Merge branch 'main' into cozmin/raf-996

7b0fe22

fix(auth): fix test for grant access

b823bbf

fix(auth): changed interaction generic error message

523da68

use main OpenAPI spec for Auth

86a0618

cozminu requested a review from mkurapov August 5, 2025 13:40

mkurapov reviewed Aug 11, 2025

View reviewed changes

mkurapov mentioned this pull request Aug 18, 2025

feat: adding subject to AS spec interledger/open-payments-specifications#18

Merged

fix(auth): approved grant does not return subject

cfb3ca8

mkurapov reviewed Aug 18, 2025

View reviewed changes

cozminu and others added 4 commits August 25, 2025 11:52

fix(auth): remove unused import

bc22b5b

Co-authored-by: Max Kurapov <[email protected]>

fix(auth): make access_token optional in response

8127553

fix(deps): sha.js critical update

108cb6e

fix(deps): critical update

e9a8f08

cozminu requested a review from mkurapov September 1, 2025 14:15

mkurapov reviewed Sep 2, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

cozminu and others added 2 commits September 2, 2025 17:06

Update package.json

6c735a0

Co-authored-by: Max Kurapov <[email protected]>

update pnpm lock file

69ad04f

mkurapov previously approved these changes Sep 2, 2025

View reviewed changes

cozminu added 2 commits September 5, 2025 16:11

Merge branch 'main' into cozmin/raf-996

805795b

change op specs version

4b6f81c

cozminu dismissed mkurapov’s stale review via 4b6f81c September 9, 2025 14:18

mkurapov reviewed Sep 15, 2025

View reviewed changes

		@@ -0,0 +1,86 @@
		import { AccessRequest } from '../access/types'

	if (subject.format != 'uri') {
	if (subject.format !== 'uri') {

	constructor(code: GrantErrorCode, description?: string) {
	constructor(code: GrantErrorCode, description: string) {

	err.message \|\| 'invalid request'
	err.description \|\| 'invalid request'

feat(auth): add subject to grant #3440

Are you sure you want to change the base?

feat(auth): add subject to grant #3440

Uh oh!

Conversation

cozminu commented May 27, 2025

Changes proposed in this pull request

Context

Checklist

Uh oh!

netlify bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

Uh oh!

github-actions bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Performance Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njlie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njlie Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurapov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented May 27, 2025 •

edited

Loading

github-actions bot commented Jun 2, 2025 •

edited

Loading

njlie Jun 16, 2025 •

edited

Loading

mkurapov left a comment •

edited

Loading