chore: token invalid throw 401 instead of return public incoming payment

[DarianM]

Changes proposed in this pull request

Moving the await next() inside the try catch as I believe this is a good practice.
Letting next() call outside the try-catch creates potential issues:

• error handling inconsistency: some errors are caught and handled, but other errors from next() can propagate up unhandled I think
• unexpected flow: the middleware might partially fail but still continue execution. If an error occurs during token validation, the middleware might still call next() with invalid/incomplete context state

Renaming bypassError to canSkipAuthValidation to not introspect an access token and respond with "public" incoming payment that does not require an access token at all

Context

If an accessToken is provided with the request to get an incoming payment, and the token is invalid, it fails with a 401 error instead of returning a public incoming payment
Link issues here - fixes #2889

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)

[netlify]

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	dd1b8e3
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6724e6f07324a40008e38fc4

[DarianM]

I'm also adding the json from the localenv README file here as it's missing here and I find it useful as it helped me debug the code.
if out of scope PR, can make another PR

[mkurapov]

Suggested change

	const authSplit = ctx.request.headers.authorization?.split(' ')
	const authSplit = ctx.request.headers.authorization.split(' ')

[DarianM]

authorization can still be undefined here, when the canSkipAuthValidation is false it won't enter the early return statement and will return 401 because of undefined authorization;
this is the reason i left the ? there

[mkurapov]

Just need to run pnpm format to fix the check.

We also have a line in packages/backend/src/open_payments/payment/incoming_remote/service.ts which we can remove
("// TODO: remove after #2889 is completed").

[mkurapov]

Let's keep this in the outside of try-catch. Otherwise, the catch block in this middleware will apply to all of the following middlewares called in await next() which might lead to unexpected behaviour, e.g., adding WWW-Authenticate when we don't want to.

[mkurapov]

Suggested change

	if (!(err instanceof OpenPaymentsServerRouteError)) {
	throw err
	}
	ctx.set('WWW-Authenticate', `GNAP as_uri=${config.authServerGrantUrl}`)
	if (!bypassError) {
	throw err
	}
	throw err
	if (err instanceof OpenPaymentsServerRouteError) {
	ctx.set('WWW-Authenticate', `GNAP as_uri=${config.authServerGrantUrl}`)
	}
	throw err

[mkurapov]

Since we are here, this seemed more straightforward

[mkurapov]

In authenticatedStatusMiddleware we should return early if we know that we ended up skipping auth validation in createTokenIntrospectionMiddleware (since it doesn't make sense to do a lot of work for something we already know shouldn't succeed). One way we can do this is to make client optional in HttpSigWithAuthenticatedStatusContext, and only proceed to call throwIfSignatureInvalid if ctx.client is defined (setting ctx.client is the main function of createTokenIntrospectionMiddleware)

[DarianM]

what do you think about checking for
if (!ctx.request.headers.authorization) await next() return }
instead of making the client optional (string | undefined) for the rest of the code? I also see that authenticatedStatusMiddleware is only used for open payments