chore(backend): update wallet address middleware & error handling #2722

mkurapov · 2024-05-13T17:48:16Z

Changes proposed in this pull request

To finalize the standardization of Open Payments route errors in backend, this PR splits up walletAddressMiddleware into several separate middlewares. One group of them deals with adding the walletAddressUrl onto the ctx (from several resources), while getWalletAddressForSubresource simply polls for the active wallet address by using the ctx.walletAddressUrl itself. Each of the middlewares is then added explicity to the route.

The benefits:

We now handle all possible Open Payments API errors properly in the openPaymentsServerErrorMiddleware (i.e. we don't use ctx.throw at all in other places)
walletAddressUrl is added to the ctx as a middleware specific to the route route, meaning we can remove all-encompassing logic in walletAddressMiddleware. Likewise, we don't need to actually fetch the wallet address before doing token introspection and http sig verification

Other changes:

When wallet address is requested, return the Open Payments one by default.
SPSP payment pointer can only be fetched when setting accept header to include 'application/spsp4+json' explicitly

Context

Fixes #1905 for backend

Checklist

…eware

netlify · 2024-05-13T17:48:32Z

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

Name	Link
🔨 Latest commit	`bffcaa5`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/664c9ffc9d1af20008f24f03
😎 Deploy Preview	https://deploy-preview-2722--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-05-13T17:48:52Z

packages/backend/src/index.ts

@@ -318,8 +318,7 @@ export function initIocContainer(
    return createWalletAddressKeyRoutes({
      config: await deps.use('config'),
      logger: await deps.use('logger'),
-      walletAddressKeyService: await deps.use('walletAddressKeyService'),
-      walletAddressService: await deps.use('walletAddressService')


mkurapov · 2024-05-13T17:50:26Z

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

@@ -95,7 +95,7 @@ export function createTokenIntrospectionMiddleware({
      const access = tokenInfo.access.find((access: Access) => {
        if (
          access.type !== requestType ||
-          (access.identifier && access.identifier !== ctx.walletAddress.url)
+          (access.identifier && access.identifier !== ctx.walletAddressUrl)


at this point, in the token introspection middleware, we only have ctx.walletAddressUrl in the context

mkurapov · 2024-05-13T17:54:32Z

packages/backend/src/open_payments/wallet_address/routes.ts

@@ -30,16 +30,6 @@ export async function getWalletAddress(
  deps: ServiceDependencies,
  ctx: WalletAddressContext
 ): Promise<void> {
-  if (!ctx.walletAddress) {


we're already handling this in the wallet address middleware

mkurapov · 2024-05-13T17:56:56Z

packages/backend/src/app.ts

@@ -426,7 +429,6 @@ export class App {
    // Create incoming payment
    router.post<DefaultState, SignedCollectionContext<IncomingCreateBody>>(


now, each route follows as such:

validate against the OpenAPI spec (since we need all of the request parameters, including the walletAddressUrl ones to be always valid)

Get wallet address URL, from either request body, query params, or the resource itself

do token introspection

do http sig verification (if required)

fetch (poll for) walletAddress via the walletAddressUrl from step 2, add it onto the ctx

Call the actual route

mkurapov · 2024-05-13T17:58:27Z

packages/backend/src/app.ts

        walletAddressServerSpec,
        {
          path: '/',
          method: HttpMethod.GET
        },
        validatorMiddlewareOptions
      ),
+      getWalletAddressUrlFromPath,
+      createWalletAddressMiddleware({ errorCodeOnMissing: 404 }),


usually, we call 400 (since 404 doesn't really make sense when doing a e.g. GET/POST outgoing payment, since its not the resource that's missing, its the wallet address request body or query param that points to an invalid wallet address).

…essForSubresource

…on routes themselves

mkurapov · 2024-05-13T19:28:03Z

packages/backend/src/app.ts

+      getWalletAddressUrlFromPath,
+      createSpspMiddleware(this.config.spspEnabled),


do not call getWalletAddressForSubresource here and in the jwks.json route since we have different behaviours in these routes for handling deps.config.walletAddressUrl === ctx.walletAddressUrl

BlairCurrey · 2024-05-14T16:27:58Z

walletAddressUrl is added to the ctx as a middleware specific to the route route, meaning we can remove all-encompassing logic in walletAddressMiddleware. Likewise, we don't need to actually fetch the wallet address before doing token introspection and http sig verification

This is much better. Low coupling/high cohesion. 👍

BlairCurrey · 2024-05-14T16:57:25Z

packages/backend/src/open_payments/wallet_address/routes.ts

 ): Promise<void> {
-  if (!ctx.walletAddress) {
+  // The instance's wallet address is only relevant for the /jwks.json route, so "hide" it when directly requested


Does this mean we just shouldn't have a /GET route for the wallet address? If we still need this route then when would we need it? Comment seems to suggest we only need the /jwks.json route IMO but maybe im not following.

When I call the Examples > Open Payments > Get receiver wallet address bruno request (which creates the wallet address) then call Open Payments APIs > Get a Wallet Address I receive the the wallet address.

If I copy/paste the url into the browser http://localhost:4000/accounts/asmith then I see Could not get wallet address.

How should it be working? It seems like the bruno request should not return the wallet address according to this comment.

@BlairCurrey

deps.config.walletAddressUrl is the configured wallet address for the Rafiki instance itself, i.e. its the wallet address used to get incoming payments/incoming payment grants when creating "remote" incoming payments at other Rafikis. e.g. https://ilp.rafiki.money/.well-known/pay/.

Maybe I can word this comment better, but we actually don't want to resolve https://ilp.rafiki.money/.well-known/pay/ ever (this shouldn't be a wallet address which resources can be created on, its not for an actual user account). As a result, we only care about the https://ilp.rafiki.money/.well-known/pay/jwks.json path, since it gets looked up to check the keys of the rafiki instance when validating the grant on another rafiki.

All other "normal" wallet addresses, should get resolved as usual with the correct asset, publicName, auth/resourceServer urls (since they represent actual user accounts)

Actually, it is odd you don't see http://localhost:4000/accounts/asmith. Can you try a different wallet address?

Ah that makes sense, thank you for the context.

Same result with http://localhost:3000/accounts/gfranklin in the browser, but not with bruno. This is not the case on main - this request works as expected in the browser. The only difference between bruno and the browser I see is the host header that gets added in the pre-request script. Not sure if that's the actual difference.

Here is the error:

{"level":30,"time":1715787978022,"pid":1,"hostname":"cloud-nine-wallet-backend","method":"GET","path":"/accounts/gfranklin","message":"Could not get wallet address","msg":"Received error when handling SPSP request"}

Looks like its failing at the if (!walletAddress?.isActive) { check of the spspsMiddleware.

going back just 1 step further it looks like if (ctx.accepts('application/spsp4+json')) { resolves to false with the bruno request, and true for the browser.

What happens when we call http://localhost:3000/accounts/gfranklin in the browser, or without the host header in bruno, is we actually end up updating the wallet address via the wallet_address.not_found webhook event. (ie if you request http://localhost:3000/accounts/gfranklin on main, the wallet address will end up changing from https://cloud-nine-wallet-backend/accounts/gfranklin to http://localhost:3000/accounts/gfranklin.

This PR is different for SPSP request since we use walletAddressService.getByUrl instead of walletAddressService.getOrPollByUrl. However, I think this PR has same behaviour when we request a wallet address without going through SPSP as well just by requesting the wallet address with Accept: application/json. I think we should update the MASE code to prevent this, and seems like a separate issue in general.

I'm not sure why I decided to use walletAddressService.getByUrl in SPSP instead of polling (maybe I assume if you are using SPSP then we should already have a wallet address)?

This makes me wonder if we should default to returning the Open Payments wallet address actually, and only return the SPSP details if explicitly requested?

This makes me wonder if we should default to returning the Open Payments wallet address actually, and only return the SPSP details if explicitly requested?

Chatted w Sabine regarding this, decision made to return Open Payments wallet address by default when fetching it. Also made a change to return SPSP wallet address only if 'application/spsp4+json' is present in accept header explicitly, and decided that using getByUrl was sufficient in the SPSP lookup.

…ET spsp accept header explicit

mkurapov · 2024-05-21T13:10:07Z

packages/backend/src/app.ts

-      createWalletAddressMiddleware(),
+      getWalletAddressUrlFromPath,


Basically, if we are fetching the SPSP payment pointer, we don't need to call the validator middleware since its not Open Payments related

mkurapov · 2024-05-21T13:12:22Z

packages/backend/src/payment-method/ilp/spsp/middleware.ts

-  if (ctx.accepts('application/spsp4+json')) {
+  if (ctx.request.header.accept?.includes(SPSP_CONTENT_TYPE_V4)) {


this ctx.accepts was the reason why by default when fetching the wallet address, the SPSP details were returned, since usually, the accept header included catch all */* route, which to ctx.accepts would be truthy.

Now, we need to explicitly add application/spsp4+json in the header to get the SPSP details

mkurapov · 2024-05-21T13:33:12Z

Going to close and re-open to see if I can get past the stuck actions (I think affected by their degradation here)

mkurapov added 4 commits May 10, 2024 22:36

chore(backend): specify WalletAddressUrlContext

7dfef7a

chore(backend): use ctx.walletAddressUrl in token introspection middl…

82ba51c

…eware

feat(backend): update wallet address middleware & its app.ts usage

2dbf255

feat(backend): use new WalletAddressContext

02abf51

mkurapov changed the title ~~1905/mk/update wallet address middleware~~ chore(backend): update wallet address middleware May 13, 2024

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels May 13, 2024

chore(backend): formatting

9641dc7

mkurapov commented May 13, 2024

View reviewed changes

mkurapov added 4 commits May 13, 2024 21:22

chore(backend): rename createWalletAddressMiddleware to getWalletAddr…

7f1ac04

…essForSubresource

chore(backend): update logging in getOrPollByUrl

eeb2d51

chore(backend): update spsp middleware to use WalletAddressUrlContext

1b22bc8

chore(backend): move wallet fetching into wallet address and /jwks.js…

110bd68

…on routes themselves

mkurapov commented May 13, 2024

View reviewed changes

mkurapov changed the title ~~chore(backend): update wallet address middleware~~ chore(backend): update wallet address middleware & error handling May 13, 2024

mkurapov added 2 commits May 13, 2024 21:30

chore(backend): fix wallet address routes tests

b52207d

Merge branch 'main' into 1905/mk/update-wallet-address-middleware

0e8e820

mkurapov marked this pull request as ready for review May 13, 2024 19:40

mkurapov requested review from BlairCurrey, sabineschaller and njlie May 13, 2024 19:42

BlairCurrey reviewed May 14, 2024

View reviewed changes

feat(backend): return Open Payments wallet address by default, make G…

7f6a65a

…ET spsp accept header explicit

mkurapov force-pushed the 1905/mk/update-wallet-address-middleware branch from 15cc4a6 to 7f6a65a Compare May 21, 2024 13:08

mkurapov commented May 21, 2024

View reviewed changes

Merge branch 'main' into 1905/mk/update-wallet-address-middleware

bffcaa5

BlairCurrey approved these changes May 21, 2024

View reviewed changes

mkurapov closed this May 21, 2024

mkurapov reopened this May 21, 2024

mkurapov merged commit 3d6c1d4 into main May 21, 2024
79 checks passed

mkurapov deleted the 1905/mk/update-wallet-address-middleware branch May 21, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(backend): update wallet address middleware & error handling #2722

chore(backend): update wallet address middleware & error handling #2722

mkurapov commented May 13, 2024 •

edited

Loading

netlify bot commented May 13, 2024 •

edited

Loading

mkurapov May 13, 2024

mkurapov May 13, 2024

mkurapov May 13, 2024

mkurapov May 13, 2024

mkurapov May 13, 2024

mkurapov May 13, 2024

BlairCurrey commented May 14, 2024

BlairCurrey May 14, 2024

BlairCurrey May 14, 2024

mkurapov May 15, 2024 •

edited

Loading

mkurapov May 15, 2024 •

edited

Loading

BlairCurrey May 15, 2024 •

edited

Loading

BlairCurrey May 15, 2024

mkurapov May 15, 2024

mkurapov May 21, 2024 •

edited

Loading

mkurapov May 21, 2024

mkurapov May 21, 2024

mkurapov commented May 21, 2024

		@@ -426,7 +429,6 @@ export class App {
		// Create incoming payment
		router.post<DefaultState, SignedCollectionContext<IncomingCreateBody>>(

		getWalletAddressUrlFromPath,
		createSpspMiddleware(this.config.spspEnabled),

		createWalletAddressMiddleware(),
		getWalletAddressUrlFromPath,

		if (ctx.accepts('application/spsp4+json')) {
		if (ctx.request.header.accept?.includes(SPSP_CONTENT_TYPE_V4)) {

chore(backend): update wallet address middleware & error handling #2722

chore(backend): update wallet address middleware & error handling #2722

Conversation

mkurapov commented May 13, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented May 13, 2024 • edited Loading

✅ 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov May 15, 2024 • edited Loading

Choose a reason for hiding this comment

mkurapov May 15, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov commented May 21, 2024

mkurapov commented May 13, 2024 •

edited

Loading

netlify bot commented May 13, 2024 •

edited

Loading

mkurapov May 15, 2024 •

edited

Loading

mkurapov May 15, 2024 •

edited

Loading

BlairCurrey May 15, 2024 •

edited

Loading

mkurapov May 21, 2024 •

edited

Loading