feat(auth): improve GNAP error responses #2400

njlie · 2024-02-07T22:31:36Z

Changes proposed in this pull request

Expands error responses to include a code field that refers to a particular GNAP error and a description field that explains what went wrong.

Context

Fixes #2344.

Just a small PR to update all the GNAP error responses to be more detailed.

Checklist

netlify · 2024-02-07T22:31:41Z

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

Name	Link
🔨 Latest commit	`c335b90`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65f8a655a010500008fbb085
😎 Deploy Preview	https://deploy-preview-2400--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 site configuration.

mkurapov · 2024-02-12T16:52:24Z

packages/auth/src/grant/routes.ts

@@ -364,16 +364,16 @@ async function revokeGrant(
    'GNAP '
  )[1]
  if (!continueId || !continueToken) {
-    ctx.throw(401, { error: 'invalid_request' })
+    ctx.throw(401, { error: { code: 'invalid_request', description: 'invalid continuation information' } })


Similar to how you did above, should we add the description also as the second parameter to have it also exposed to the caller/client (like postman)? Or not for these 401 errors?

mkurapov · 2024-02-12T16:55:58Z

packages/auth/src/interaction/routes.ts

@@ -202,28 +199,48 @@ async function handleInteractionChoice(
      Buffer.from(config.identityServerSecret)


should there be also similar changes for L120 and L125?

I thought at first not to change those, as they're not being served by a GNAP endpoint. But now I think I'll make them uniform across the board.

BlairCurrey

Should error responses in middleware such as this be update as well?

https://github.com/interledger/rafiki/blob/main/packages/auth/src/signature/middleware.ts#L44

njlie · 2024-02-12T22:52:56Z

Should error responses in middleware such as this be update as well?

main/packages/auth/src/signature/middleware.ts#L44

Good catch, I forgot about those.

sabineschaller · 2024-02-15T09:27:24Z

packages/auth/src/grant/routes.ts

+    ctx.throw(400, 'interaction_required', {
+      error: {
+        code: 'interaction_required',
+        description: 'missing required request field'


Should this be different from the invalid_client description? Maybe

missing required request field 'interact'

and

missing required request field 'client'

respectively.

mkurapov

What do you think about making GNAP error responses into an enum and then having a function to handle all of these throws?

mkurapov · 2024-02-22T17:32:04Z

packages/auth/src/grant/routes.ts

  }
  const grant = await deps.grantService.getByContinue(continueId, continueToken)
  if (!grant) {
-    ctx.throw(404, { error: 'unknown_request' })
+    ctx.throw(404, 'unknown_request', {


should this also be invalid_request?

njlie · 2024-03-06T22:26:48Z

packages/auth/src/grant/routes.ts

@@ -101,7 +102,12 @@ async function createGrant(
  try {
    noInteractionRequired = canSkipInteraction(deps.config, ctx.request.body)
  } catch (err) {
-    ctx.throw(400, 'identifier_required', { error: 'identifier_required' })


I have no idea where I got stuff like identifier_required or internal_server_error. I've restricted error responses to the ones that are listed in https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol-18#section-3.6

njlie · 2024-03-06T22:27:48Z

packages/auth/src/grant/routes.ts

@@ -144,12 +157,26 @@ async function createPendingGrant(
  const { body } = ctx.request
  const { grantService, interactionService, config } = deps
  if (!body.interact) {
-    ctx.throw(400, 'interaction_required', { error: 'interaction_required' })


I changed this from interaction_required because that seems to be more for if a resource (likely only a grant) requires interaction in order to be accessed.

njlie · 2024-03-06T22:46:59Z

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

@@ -294,9 +297,11 @@ describe('Signature Service', (): void => {
        resource_server: 'test'
      }
      await expect(tokenHttpsigMiddleware(ctx, next)).rejects.toMatchObject({
-        status: 400,
-        error: 'invalid_request',
-        message: 'invalid signature headers'


I've change this and other error responses to 401 as it is a failure to verify the signature. I also chose invalid_client as the code because of the following description:

"invalid_client":
The request was made from a client that was not recognized or allowed by the AS, or the client's signature validation failed.

(emphasis mine)

sabineschaller · 2024-03-07T08:42:04Z

Just a small PR to update all the GNAP error responses to be more detailed.

Not so small anymore...

sabineschaller · 2024-03-07T08:43:57Z

packages/auth/src/accessToken/routes.ts

@@ -119,20 +120,25 @@ async function rotateToken(
    newToken = await deps.accessTokenService.rotate(ctx.accessToken.id, trx)

    if (!newToken) {
-      ctx.throw()
+      throw new Error('invalid access token')


Why not throw an internal server error with this error message? I know that this is kind of what we are doing here, but should we do a ctx.throw?

I think because we end up catching the error and then call ctx.throw below

mkurapov · 2024-03-08T16:50:51Z

packages/auth/src/grant/routes.ts

+      error: {
+        code: GNAPErrorCode.InvalidRequest,
+        description: 'access identifier required'
+      }


Should we call generateGNAPErrorResponse here?

Good catch, yes we should

mkurapov · 2024-03-08T16:54:18Z

packages/auth/src/accessToken/routes.ts

@@ -119,20 +120,25 @@ async function rotateToken(
    newToken = await deps.accessTokenService.rotate(ctx.accessToken.id, trx)

    if (!newToken) {
-      ctx.throw()
+      throw new Error('invalid access token')


I think because we end up catching the error and then call ctx.throw below

mkurapov · 2024-03-08T16:58:44Z

packages/auth/src/accessToken/routes.ts

+    ctx.throw(
+      400,
+      GNAPErrorCode.InvalidRotation,
+      generateGNAPErrorResponse(GNAPErrorCode.InvalidRotation, errorMessage)
+    )


Suggested change

ctx.throw(

400,

GNAPErrorCode.InvalidRotation,

generateGNAPErrorResponse(GNAPErrorCode.InvalidRotation, errorMessage)

)

throwGNAPError(ctx, 400, GNAPErrorCode.InvalidRotation, message)

what do you think of something like this also? then we wouldn't need to define the GNAPErrorCode.X value twice

I like it, looks cleaner that way too

mkurapov

LGTM ✅
🚢 it

…r GNAP error codes

* feat(auth): improve GNAP error responses * fix: add extra arg to ctx.throw * feat: middleware error improvements * feat: add error messages to grant details endpoint * fix: update error messages in grant routes * feat: create utility function to generate GNAP errors, create enum for GNAP error codes * feat: helper function throws instead of generates response

* feat(backend): webhook max retry * fix: tighten filter * Update packages/documentation/src/content/docs/integration/deployment.md Co-authored-by: Max Kurapov <max@interledger.org> * test: address review feedback * chore: add feature requests to contributing.md (#2542) * chore: enable graphql protection - maxDepth, blockFieldSuggestions, maxCost (#2537) * chore: add graphql depth restriction * feat: properly enable graphql armor * style: remove unnecessary space * chore: add armor to auth admin server * fix(deps): update module github.com/aws/aws-sdk-go to v1.50.38 (#2543) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency @interledger/open-payments to v6.7.2 (#2544) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(frontend): add X-Frame-Options header (#2538) * chore(deps): update dependency @types/lodash to ^4.17.0 (#2546) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency @types/react to ^18.2.66 (#2545) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency @interledger/open-payments to v6.8.0 (#2547) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update module github.com/aws/aws-sdk-go to v1.51.0 (#2548) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency @interledger/docs-design-system to ^0.3.2 (#2549) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency axios to v1.6.8 (#2556) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency isbot to v5 (#2553) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update typescript-eslint monorepo to v7 (#2552) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency koa to ^2.15.1 (#2551) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency postcss to ^8.4.36 (#2559) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update helm release redis to v18.19.3 (#2561) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * feat(bruno): use polling in grant continuation (#2550) * chore: fetch latest OP schemas * feat(bruno): use polling for grant continuation * docs: update * fix: bruno tests * chore(deps): update pnpm to v8.15.5 (#2563) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * feat(telemetry): LIVENET now points to new livenet NLB (#2523) * feat(telemetry): LIVENET now points to new livenet NLB * Update packages/backend/src/config/app.ts Co-authored-by: Max Kurapov <max@interledger.org> * Update packages/backend/src/config/app.ts Co-authored-by: Max Kurapov <max@interledger.org> --------- Co-authored-by: Max Kurapov <max@interledger.org> Co-authored-by: Sabine Schaller <sabine@interledger.org> * feat(auth): improve GNAP error responses (#2400) * feat(auth): improve GNAP error responses * fix: add extra arg to ctx.throw * feat: middleware error improvements * feat: add error messages to grant details endpoint * fix: update error messages in grant routes * feat: create utility function to generate GNAP errors, create enum for GNAP error codes * feat: helper function throws instead of generates response * feat: integration tests (#2380) * feat: setup basic test env * feat: seed integration environment - cmd to start integration environment and run tests - seeds environment on test run - extracts common MASE functionality into new mock-account-servicing-lib * refactor(localenv,integration): move graphql url to config from seed - moves graphql url to config from seed - also removes self section which only contained unused properties * refactor: move testenv configuration into new dir * refactor: cleanup env vars * feat: add webhook server, fix env vars * chore: comment * feat: start grant request test * fix: eslint errors * fix: docker compose env vars * feat: add --build arg * fix: incoming-payment grant initiation request Grant request was failing with invalid signature. This was fixed by directing hte backend to use the same private key being used to create the open payments client. * fix: ts error * feat: implement tests through Create Quote * chore: upgrade op client * feat: partially implemented grant request outgoing payment test * feat: rework to host.docker.internal - switches hostname to host.docker.internal to resolve url mismatch problem - finishes grant request outgoing payment test - cleans up some configuration and test variables * feat: add p2p flow test * feat: add continuation step with consent mocking * fix: rm obsolete type cast to any and comment * feat: add create ougoing payment test * fix: bad pnpm-lock merge * feat: build deps in mock ase job * feat: get non existant wallet address test * fix: update open payments pkg * chore: fix lint warnings * feat: implement continuation polling * chore: test cleanup * refactor: generate gql in tests instead of import from lib * chore: rm old comment * chore: use latest http-singature-utils to match other deps * chore: pnpm i * chore: use latest koa-bodyparser * chore: rm engine strict * chore: revert rm engine strict - fixes netlify ci failure but ultimately not the correct fix * chore(integration): dont ignore env, rm example env * refactor: use docker healthcheck for running tests * chore: move healthcheck to last started docker container * feat: use latest open payments pkg, no body requird on continuation * chore: pnpm i, fix broken lockfile (no apollo version) * refactor: move webhook event enum to types * refactor: move run integration script to test/integration * Update packages/mock-account-servicing-lib/package.json Co-authored-by: Sabine Schaller <sabine@interledger.org> * refactor: simplify mock-account-servicing-entity ci step * feat(integration): exit run-tests early if containers fail to start * feat: use pino logger * refactor: rename mock account servicing lib * refactor: rename class files to camel case * fix: import correct body parser * refactor: poll instead of delay * refactor: simplify call to poll condition only * fix: update filenames * refactor: change filename name to kebab case --------- Co-authored-by: Sabine Schaller <sabine@interledger.org> * fix(deps): update dependency isbot to ^5.1.2 (#2567) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency @types/react to ^18.2.67 (#2566) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update module github.com/aws/aws-sdk-go to v1.51.2 (#2565) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update helm release redis to v18.19.4 (#2570) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * feat(backend): make SPSP optional (#2560) * feat: flag to enable/disable SPSP * test: update to cover all cases * docs: add link to glossary * chore: fix lockfile * test: address feedback * delete lockfile * add lockfile * delete lockfile * fix lockfile this time? * fix: formatting --------- Co-authored-by: Max Kurapov <max@interledger.org> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: xplicit <111863110+beniaminmunteanu@users.noreply.github.com> Co-authored-by: Nathan Lie <lie4nathan@gmail.com> Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com>

njlie requested review from BlairCurrey, mkurapov and sabineschaller February 7, 2024 22:31

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

mkurapov reviewed Feb 12, 2024

View reviewed changes

BlairCurrey reviewed Feb 12, 2024

View reviewed changes

sabineschaller reviewed Feb 15, 2024

View reviewed changes

njlie requested review from BlairCurrey, mkurapov and sabineschaller February 21, 2024 22:44

mkurapov reviewed Feb 22, 2024

View reviewed changes

njlie commented Mar 6, 2024

View reviewed changes

njlie requested a review from mkurapov March 6, 2024 23:12

sabineschaller reviewed Mar 7, 2024

View reviewed changes

mkurapov reviewed Mar 8, 2024

View reviewed changes

njlie requested a review from mkurapov March 15, 2024 20:09

mkurapov approved these changes Mar 15, 2024

View reviewed changes

sabineschaller approved these changes Mar 18, 2024

View reviewed changes

njlie added 2 commits March 18, 2024 13:38

feat(auth): improve GNAP error responses

f4e1bf2

fix: add extra arg to ctx.throw

c0de1f0

njlie added 5 commits March 18, 2024 13:38

feat: middleware error improvements

c5d34eb

feat: add error messages to grant details endpoint

2e2daa8

fix: update error messages in grant routes

7f60e48

feat: create utility function to generate GNAP errors, create enum fo…

1fefe63

…r GNAP error codes

feat: helper function throws instead of generates response

c335b90

njlie force-pushed the nl/2344/improve-auth-errors branch from b1baa45 to c335b90 Compare March 18, 2024 20:38

njlie merged commit 2c930ee into main Mar 18, 2024
22 checks passed

njlie deleted the nl/2344/improve-auth-errors branch March 18, 2024 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): improve GNAP error responses #2400

feat(auth): improve GNAP error responses #2400

njlie commented Feb 7, 2024

netlify bot commented Feb 7, 2024 •

edited

mkurapov Feb 12, 2024

mkurapov Feb 12, 2024

njlie Feb 21, 2024

BlairCurrey left a comment

njlie commented Feb 12, 2024

sabineschaller Feb 15, 2024

mkurapov left a comment

mkurapov Feb 22, 2024

njlie Mar 6, 2024

njlie Mar 6, 2024

njlie Mar 6, 2024

sabineschaller commented Mar 7, 2024

sabineschaller Mar 7, 2024

mkurapov Mar 8, 2024

mkurapov Mar 8, 2024

njlie Mar 15, 2024

mkurapov Mar 8, 2024

mkurapov Mar 8, 2024

njlie Mar 15, 2024

mkurapov left a comment

		@@ -202,28 +199,48 @@ async function handleInteractionChoice(
		Buffer.from(config.identityServerSecret)

feat(auth): improve GNAP error responses #2400

feat(auth): improve GNAP error responses #2400

Conversation

njlie commented Feb 7, 2024

Changes proposed in this pull request

Context

Checklist

netlify bot commented Feb 7, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey left a comment

Choose a reason for hiding this comment

njlie commented Feb 12, 2024

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

sabineschaller commented Mar 7, 2024

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

netlify bot commented Feb 7, 2024 •

edited