-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: token invalid throw 401 instead of return public incoming payment #3062
base: main
Are you sure you want to change the base?
Changes from 4 commits
ced6d16
e0909bf
c242202
97afcdf
dd1b8e3
e90fb80
92b887e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -67,26 +67,31 @@ function toOpenPaymentsAccess( | |||||
export function createTokenIntrospectionMiddleware({ | ||||||
requestType, | ||||||
requestAction, | ||||||
bypassError = false | ||||||
canSkipAuthValidation = false | ||||||
}: { | ||||||
requestType: AccessType | ||||||
requestAction: RequestAction | ||||||
bypassError?: boolean | ||||||
canSkipAuthValidation?: boolean | ||||||
}) { | ||||||
return async ( | ||||||
ctx: WalletAddressUrlContext, | ||||||
next: () => Promise<void> | ||||||
): Promise<void> => { | ||||||
const config = await ctx.container.use('config') | ||||||
try { | ||||||
const parts = ctx.request.headers.authorization?.split(' ') | ||||||
if (parts?.length !== 2 || parts[0] !== 'GNAP') { | ||||||
if (canSkipAuthValidation && !ctx.request.headers.authorization) { | ||||||
await next() | ||||||
return | ||||||
} | ||||||
|
||||||
const authSplit = ctx.request.headers.authorization?.split(' ') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
if (authSplit?.length !== 2 || authSplit[0] !== 'GNAP') { | ||||||
throw new OpenPaymentsServerRouteError( | ||||||
401, | ||||||
'Missing or invalid authorization header value' | ||||||
) | ||||||
} | ||||||
const token = parts[1] | ||||||
const token = authSplit[1] | ||||||
const tokenIntrospectionClient = await ctx.container.use( | ||||||
'tokenIntrospectionClient' | ||||||
) | ||||||
|
@@ -147,14 +152,10 @@ export function createTokenIntrospectionMiddleware({ | |||||
} | ||||||
} catch (err) { | ||||||
if (!(err instanceof OpenPaymentsServerRouteError)) { | ||||||
DarianM marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
throw err | ||||||
ctx.set('WWW-Authenticate', `GNAP as_uri=${config.authServerGrantUrl}`) | ||||||
} | ||||||
|
||||||
ctx.set('WWW-Authenticate', `GNAP as_uri=${config.authServerGrantUrl}`) | ||||||
|
||||||
if (!bypassError) { | ||||||
throw err | ||||||
} | ||||||
throw err | ||||||
} | ||||||
|
||||||
await next() | ||||||
|
@@ -166,6 +167,11 @@ export const authenticatedStatusMiddleware = async ( | |||||
next: () => Promise<unknown> | ||||||
): Promise<void> => { | ||||||
ctx.authenticated = false | ||||||
if (!ctx.request.headers.authorization) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now that we expect the request to be for sure authorized after this check, we can get rid of the try-catch below altogether |
||||||
await next() | ||||||
return | ||||||
} | ||||||
|
||||||
try { | ||||||
await throwIfSignatureInvalid(ctx) | ||||||
ctx.authenticated = true | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,6 +178,18 @@ You can either trigger the debugger by adding `debugger` statements in the code | |
#### Debugging with VS Code: | ||
|
||
To debug with VS Code, add this configuration to your `.vscode/launch.json`: | ||
```json | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{ | ||
"name": "Attach to docker (cloud-nine-backend)", | ||
"type": "node", | ||
"request": "attach", | ||
"port": 9229, | ||
"address": "localhost", | ||
"localRoot": "${workspaceFolder}", | ||
"remoteRoot": "/home/rafiki/", | ||
"restart": true | ||
}, | ||
``` | ||
|
||
The `localRoot` variable will depend on the location of the `launch.json` file relative to Rafiki’s root directory. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
authenticatedStatusMiddleware
we should return early if we know that we ended up skipping auth validation increateTokenIntrospectionMiddleware
(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 makeclient
optional inHttpSigWithAuthenticatedStatusContext
, and only proceed to callthrowIfSignatureInvalid
ifctx.client
is defined (settingctx.client
is the main function ofcreateTokenIntrospectionMiddleware
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about checking for
if (!ctx.request.headers.authorization) await next() return }
in authenticatedStatusMiddlewareinstead of making the
client
optional (string | undefined) for the rest of the code? I also see thatauthenticatedStatusMiddleware
is only used for open payments