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.

Sign up for GitHub

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(backend): remove wallet address middleware #2708

Closed
wants to merge 15 commits into from
Closed

chore(backend): remove wallet address middleware #2708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4936272
feat(backend): SPSPRouteError
mkurapov May 6, 2024
c5bfc9f
chore(backend): remove wallet address middleware, replace with inline…
mkurapov May 6, 2024
6cf1e7f
feat(backend): SPSPRouteError
mkurapov May 6, 2024
16d95bf
Merge branch '1905/mk/spsp-errors' into 1905/mk/remove-wallet-address…
mkurapov May 7, 2024
b0b6e13
feat(backend): SPSPRouteError
mkurapov May 6, 2024
67a67b5
Merge branch '1905/mk/spsp-errors' into 1905/mk/remove-wallet-address…
mkurapov May 7, 2024
18a4ba5
chore(backend): use getOrPollByUrl in SPSP middleware
mkurapov May 7, 2024
a123c69
Merge branch 'main' into 1905/mk/spsp-errors
mkurapov May 7, 2024
221f55f
Merge branch '1905/mk/spsp-errors' into 1905/mk/remove-wallet-address…
mkurapov May 7, 2024
9307437
chore(backend): update log during wallet address polling
mkurapov May 7, 2024
62b3e9a
chore(backend): minor cleanup in app.ts
mkurapov May 7, 2024
6725937
chore(backend): convert error log to debug statement in getOrPollByUrl
mkurapov May 7, 2024
d657a8d
chore(backend): updating wallet address get route
mkurapov May 7, 2024
5b59c71
Merge branch 'main' into 1905/mk/remove-wallet-address-middleware
mkurapov May 10, 2024
2082015
chore(backend): remove unused walletAddress arg
mkurapov May 10, 2024
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
132 changes: 105 additions & 27 deletions packages/backend/src/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import { HttpTokenService } from './payment-method/ilp/peer-http-token/service'
import { AssetService, AssetOptions } from './asset/service'
import { AccountingService } from './accounting/service'
import { PeerService } from './payment-method/ilp/peer/service'
import { createWalletAddressMiddleware } from './open_payments/wallet_address/middleware'
import { WalletAddress } from './open_payments/wallet_address/model'
import { WalletAddressService } from './open_payments/wallet_address/service'
import {
createTokenIntrospectionMiddleware,
Expand Down Expand Up @@ -86,16 +84,17 @@ import { PaymentMethodHandlerService } from './payment-method/handler/service'
import { IlpPaymentService } from './payment-method/ilp/service'
import { TelemetryService } from './telemetry/service'
import { ApolloArmor } from '@escape.tech/graphql-armor'
import { openPaymentsServerErrorMiddleware } from './open_payments/route-errors'
import {
OpenPaymentsServerRouteError,
openPaymentsServerErrorMiddleware
} from './open_payments/route-errors'
import { verifyApiSignature } from './shared/utils'

export interface AppContextData {
logger: Logger
container: AppContainer
// Set by @koa/router.
params: { [key: string]: string }
walletAddress?: WalletAddress
walletAddressUrl?: string
}

export interface ApolloContext {
Expand All @@ -112,19 +111,12 @@ export type AppRequest<ParamsT extends string = string> = Omit<
}

export interface WalletAddressContext extends AppContext {
walletAddress: WalletAddress
walletAddressUrl: string
grant?: Grant
client?: string
accessAction?: AccessAction
}

export type WalletAddressKeysContext = Omit<
WalletAddressContext,
'walletAddress'
> & {
walletAddress?: WalletAddress
}

type HttpSigHeaders = Record<'signature' | 'signature-input', string>

type HttpSigRequest = Omit<AppContext['request'], 'headers'> & {
Expand Down Expand Up @@ -426,7 +418,10 @@ export class App {
// Create incoming payment
router.post<DefaultState, SignedCollectionContext<IncomingCreateBody>>(
'/incoming-payments',
createWalletAddressMiddleware(),
async (ctx, next) => {
ctx.walletAddressUrl = ctx.request.body.walletAddress
await next()
},
createValidatorMiddleware<
ContextType<SignedCollectionContext<IncomingCreateBody>>
>(
Expand All @@ -452,7 +447,10 @@ export class App {
SignedCollectionContext<never, GetCollectionQuery>
>(
'/incoming-payments',
createWalletAddressMiddleware(),
async (ctx, next) => {
ctx.walletAddressUrl = ctx.request.query['wallet-address']
await next()
},
createValidatorMiddleware<
ContextType<SignedCollectionContext<never, GetCollectionQuery>>
>(
Expand All @@ -475,7 +473,10 @@ export class App {
// Create outgoing payment
router.post<DefaultState, SignedCollectionContext<OutgoingCreateBody>>(
'/outgoing-payments',
createWalletAddressMiddleware(),
async (ctx, next) => {
ctx.walletAddressUrl = ctx.request.body.walletAddress
await next()
},
createValidatorMiddleware<
ContextType<SignedCollectionContext<OutgoingCreateBody>>
>(
Expand All @@ -501,7 +502,10 @@ export class App {
SignedCollectionContext<never, GetCollectionQuery>
>(
'/outgoing-payments',
createWalletAddressMiddleware(),
async (ctx, next) => {
ctx.walletAddressUrl = ctx.request.query['wallet-address']
await next()
},
createValidatorMiddleware<
ContextType<SignedCollectionContext<never, GetCollectionQuery>>
>(
Expand All @@ -524,7 +528,10 @@ export class App {
// Create quote
router.post<DefaultState, SignedCollectionContext<QuoteCreateBody>>(
'/quotes',
createWalletAddressMiddleware(),
async (ctx, next) => {
ctx.walletAddressUrl = ctx.request.body.walletAddress
await next()
},
createValidatorMiddleware<
ContextType<SignedCollectionContext<QuoteCreateBody>>
>(
Expand All @@ -547,7 +554,24 @@ export class App {
// Read incoming payment
router.get<DefaultState, SubresourceContextWithAuthenticatedStatus>(
'/incoming-payments/:id',
createWalletAddressMiddleware(),
async (ctx, next) => {
const incomingPaymentService = await ctx.container.use(
'incomingPaymentService'
)
const incomingPayment = await incomingPaymentService.get({
id: ctx.params.id
})

if (!incomingPayment?.walletAddress) {
throw new OpenPaymentsServerRouteError(401, 'Unauthorized', {
description: 'Failed to get wallet address from incoming payment',
id: ctx.params.id
})
}

ctx.walletAddressUrl = incomingPayment.walletAddress.url
Comment on lines +558 to +572
Copy link
Contributor Author

@mkurapov mkurapov May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We do not get a specific wallet address in the following OP requests (either in the query params or request body):

GET /incoming-payments/:id
GET /outgoing-payments/:id
GET /quotes/:id
POST /incoming-payments/:id/complete

We need the walletAddressUrl in order to pass down into createTokenIntrospectionMiddleware, as the middleware needs to determine whether the possible identifier on the grant matches up to the resource for the particular wallet address.

This is why we still need to get the resource first, before we can proceed with handling the request. The status quo is returning a 401/Unauthorized response code (to not give away information about whether resource exists or not). Maybe this should be 400, but I don't have a strong opinion here.

Copy link
Contributor Author

@mkurapov mkurapov May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I need to add tests for this, but I would need to do some work about splitting apart these in-line middlewares into separate functions. I can handle it in a separate PR where I do a little more refactoring, since I don't want to balloon this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the following:

GET /incoming-payments/:id
GET /outgoing-payments/:id
GET /quotes/:id

we could just store these on ctx.state and not have to call the <resource>Routes.get/fetch the resource from the DB again, but a) could be done in separate PR, or b) not worth doing at all for the consistency of having the routes as they are

await next()
},
createValidatorMiddleware<
ContextType<SubresourceContextWithAuthenticatedStatus>
>(
Expand All @@ -571,7 +595,24 @@ export class App {
// Complete incoming payment
router.post<DefaultState, SignedSubresourceContext>(
'/incoming-payments/:id/complete',
createWalletAddressMiddleware(),
async (ctx, next) => {
const incomingPaymentService = await ctx.container.use(
'incomingPaymentService'
)
const incomingPayment = await incomingPaymentService.get({
id: ctx.params.id
})

if (!incomingPayment?.walletAddress) {
throw new OpenPaymentsServerRouteError(401, 'Unauthorized', {
description: 'Failed to get wallet address from incoming payment',
id: ctx.params.id
})
}

ctx.walletAddressUrl = incomingPayment.walletAddress.url
await next()
},
createValidatorMiddleware<ContextType<SignedSubresourceContext>>(
resourceServerSpec,
{
Expand All @@ -592,7 +633,24 @@ export class App {
// Read outgoing payment
router.get<DefaultState, SignedSubresourceContext>(
'/outgoing-payments/:id',
createWalletAddressMiddleware(),
async (ctx, next) => {
const outgoingPaymentService = await ctx.container.use(
'outgoingPaymentService'
)
const outgoingPayment = await outgoingPaymentService.get({
id: ctx.params.id
})

if (!outgoingPayment?.walletAddress) {
throw new OpenPaymentsServerRouteError(401, 'Unauthorized', {
description: 'Failed to get wallet address from outgoing payment',
id: ctx.params.id
})
}

ctx.walletAddressUrl = outgoingPayment.walletAddress.url
await next()
},
createValidatorMiddleware<ContextType<SignedSubresourceContext>>(
resourceServerSpec,
{
Expand All @@ -613,7 +671,22 @@ export class App {
// Read quote
router.get<DefaultState, SignedSubresourceContext>(
'/quotes/:id',
createWalletAddressMiddleware(),
async (ctx, next) => {
const quoteService = await ctx.container.use('quoteService')
const quote = await quoteService.get({
id: ctx.params.id
})

if (!quote?.walletAddress) {
throw new OpenPaymentsServerRouteError(401, 'Unauthorized', {
description: 'Failed to get wallet address from quote',
id: ctx.params.id
})
}

ctx.walletAddressUrl = quote.walletAddress.url
await next()
},
createValidatorMiddleware<ContextType<SignedSubresourceContext>>(
resourceServerSpec,
{
Expand All @@ -632,24 +705,29 @@ export class App {

router.get(
WALLET_ADDRESS_PATH + '/jwks.json',
createWalletAddressMiddleware(),
createValidatorMiddleware<WalletAddressKeysContext>(
async (ctx, next) => {
ctx.walletAddressUrl = `https://${ctx.request.host}/${ctx.params.walletAddressPath}`
await next()
},
createValidatorMiddleware<WalletAddressContext>(
walletAddressServerSpec,
{
path: '/jwks.json',
method: HttpMethod.GET
},
validatorMiddlewareOptions
),
async (ctx: WalletAddressKeysContext): Promise<void> =>
await walletAddressKeyRoutes.getKeysByWalletAddressId(ctx)
walletAddressKeyRoutes.getKeysByWalletAddressId
)

// Add the wallet address query route last.
// Otherwise it will be matched instead of other Open Payments endpoints.
router.get(
WALLET_ADDRESS_PATH,
createWalletAddressMiddleware(),
async (ctx, next) => {
ctx.walletAddressUrl = `https://${ctx.request.host}/${ctx.params.walletAddressPath}`
await next()
},
createSpspMiddleware(this.config.spspEnabled),
createValidatorMiddleware<WalletAddressContext>(
walletAddressServerSpec,
Expand Down
12 changes: 8 additions & 4 deletions packages/backend/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,14 +304,16 @@ export function initIocContainer(
config: await deps.use('config'),
logger: await deps.use('logger'),
incomingPaymentService: await deps.use('incomingPaymentService'),
streamCredentialsService: await deps.use('streamCredentialsService')
streamCredentialsService: await deps.use('streamCredentialsService'),
walletAddressService: await deps.use('walletAddressService')
})
})
container.singleton('walletAddressRoutes', async (deps) => {
const config = await deps.use('config')
return createWalletAddressRoutes({
authServer: config.authServerGrantUrl,
resourceServer: config.openPaymentsUrl
resourceServer: config.openPaymentsUrl,
walletAddressService: await deps.use('walletAddressService')
})
})
container.singleton('walletAddressKeyRoutes', async (deps) => {
Expand Down Expand Up @@ -458,7 +460,8 @@ export function initIocContainer(
return createQuoteRoutes({
config: await deps.use('config'),
logger: await deps.use('logger'),
quoteService: await deps.use('quoteService')
quoteService: await deps.use('quoteService'),
walletAddressService: await deps.use('walletAddressService')
})
})

Expand All @@ -485,7 +488,8 @@ export function initIocContainer(
return createOutgoingPaymentRoutes({
config: await deps.use('config'),
logger: await deps.use('logger'),
outgoingPaymentService: await deps.use('outgoingPaymentService')
outgoingPaymentService: await deps.use('outgoingPaymentService'),
walletAddressService: await deps.use('walletAddressService')
})
})

Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/auth/middleware.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ describe('Auth Middleware', (): void => {

beforeEach(async (): Promise<void> => {
if (identifierOption === IdentifierOption.Matching) {
identifier = ctx.walletAddress.url
identifier = ctx.walletAddressUrl
} else if (identifierOption === IdentifierOption.Conflicting) {
identifier = faker.internet.url({ appendSlash: false })
}
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/auth/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
) {
return false
}
Expand Down
Loading
Loading