Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/backend/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ export class App {
createTokenIntrospectionMiddleware({
requestType: AccessType.IncomingPayment,
requestAction: RequestAction.Read,
bypassError: true
canSkipAuthValidation: true
}),
authenticatedStatusMiddleware,
Copy link
Contributor

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 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)

Copy link
Author

@DarianM DarianM Nov 1, 2024

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 authenticatedStatusMiddleware
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

getWalletAddressForSubresource,
Expand Down
65 changes: 61 additions & 4 deletions packages/backend/src/open_payments/auth/middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ describe('Auth Middleware', (): void => {
await appContainer.shutdown()
})

describe('bypassError option', (): void => {
test('calls next for HTTP errors', async (): Promise<void> => {
describe('canSkipAuthValidation option', (): void => {
test('calls next for undefined authorization header', async (): Promise<void> => {
const middleware = createTokenIntrospectionMiddleware({
requestType: type,
requestAction: action,
bypassError: true
canSkipAuthValidation: true
})
ctx.request.headers.authorization = ''

Expand All @@ -107,7 +107,7 @@ describe('Auth Middleware', (): void => {
const middleware = createTokenIntrospectionMiddleware({
requestType: AccessType.OutgoingPayment,
requestAction: action,
bypassError: true
canSkipAuthValidation: true
})

jest.spyOn(tokenIntrospectionClient, 'introspect').mockResolvedValueOnce({
Expand Down Expand Up @@ -140,6 +140,63 @@ describe('Auth Middleware', (): void => {
)
expect(next).not.toHaveBeenCalled()
})

test('proceeds with validation when authorization header exists, even with canSkipAuthValidation true', async (): Promise<void> => {
const middleware = createTokenIntrospectionMiddleware({
requestType: type,
requestAction: action,
canSkipAuthValidation: true
})
ctx.request.headers.authorization = 'GNAP valid_token'
jest.spyOn(tokenIntrospectionClient, 'introspect').mockResolvedValueOnce({
active: true,
access: [{ type: type, actions: [action] }],
client: 'test-client'
} as TokenInfo)

await middleware(ctx, next)

expect(tokenIntrospectionClient.introspect).toHaveBeenCalled()
expect(ctx.client).toBe('test-client')
expect(next).toHaveBeenCalled()
})

test('throws OpenPaymentsServerRouteError for invalid token with skipAuthValidation true', async (): Promise<void> => {
const middleware = createTokenIntrospectionMiddleware({
requestType: type,
requestAction: action,
canSkipAuthValidation: true
})
ctx.request.headers.authorization = 'GNAP invalid_token'
jest
.spyOn(tokenIntrospectionClient, 'introspect')
.mockRejectedValueOnce(new Error())

await expect(middleware(ctx, next)).rejects.toThrow(
OpenPaymentsServerRouteError
)
expect(ctx.response.get('WWW-Authenticate')).toBe(
`GNAP as_uri=${Config.authServerGrantUrl}`
)
expect(next).not.toHaveBeenCalled()
})

test('throws OpenPaymentsServerRouteError when canSkipAuthValidation is false and no authorization header', async (): Promise<void> => {
const middleware = createTokenIntrospectionMiddleware({
requestType: type,
requestAction: action,
canSkipAuthValidation: false
})
ctx.request.headers.authorization = ''

await expect(middleware(ctx, next)).rejects.toThrow(
OpenPaymentsServerRouteError
)
expect(ctx.response.get('WWW-Authenticate')).toBe(
`GNAP as_uri=${Config.authServerGrantUrl}`
)
expect(next).not.toHaveBeenCalled()
})
})

test.each`
Expand Down
28 changes: 17 additions & 11 deletions packages/backend/src/open_payments/auth/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(' ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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'
)
Expand Down Expand Up @@ -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()
Expand All @@ -166,6 +167,11 @@ export const authenticatedStatusMiddleware = async (
next: () => Promise<unknown>
): Promise<void> => {
ctx.authenticated = false
if (!ctx.request.headers.authorization) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,6 @@ async function getIncomingPayment(
accessToken: grant.accessToken
})

// TODO: remove after #2889 is completed
if (!incomingPayment.walletAddress) {
throw new OpenPaymentsClientError('Got invalid incoming payment', {
status: 401,
description: 'Received public incoming payment instead of private'
})
}

return incomingPayment
} catch (err) {
const errorMessage = 'Could not get remote incoming payment'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The 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.
if out of scope PR, can make another PR

{
"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.

Expand Down
Loading