diff --git a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Outgoing Payment.bru b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Outgoing Payment.bru index 626d324549..2395e61ff5 100644 --- a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Outgoing Payment.bru +++ b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Outgoing Payment.bru @@ -10,6 +10,10 @@ post { auth: none } +headers { + ~x-operator-secret: tXhxCNRWuVOJs5W/CUgjsc+vBnKj0CVdqSb87EXN64E= +} + body:graphql { mutation CreateOutgoingPayment($input: CreateOutgoingPaymentInput!) { createOutgoingPayment(input: $input) { @@ -46,7 +50,7 @@ body:graphql:vars { { "input": { "walletAddressId": "{{gfranklinWalletAddressId}}", - "quoteId": "{{quoteId}}" + "quoteId": "{{quoteId}}", "tenantId": "" } } } diff --git a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Quote.bru b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Quote.bru index b3c20f6802..6946c962ad 100644 --- a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Quote.bru +++ b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Quote.bru @@ -10,6 +10,10 @@ post { auth: none } +headers { + ~x-operator-secret: {{operatorApiSecret}} +} + body:graphql { mutation CreateQuote($input: CreateQuoteInput!) { createQuote(input: $input) { @@ -38,7 +42,8 @@ body:graphql:vars { { "input": { "walletAddressId": "{{gfranklinWalletAddressId}}", - "receiver": "{{receiverId}}" + "receiver": "{{receiverId}}", + "tenantId": "" } } } @@ -53,7 +58,7 @@ script:pre-request { script:post-response { const body = res.getBody(); - + if (body?.data) { bru.setEnvVar("quoteId", body.data.createQuote.quote.id); } diff --git a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Receiver -remote Incoming Payment-.bru b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Receiver -remote Incoming Payment-.bru index 5bbb224b57..5eff5f640a 100644 --- a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Receiver -remote Incoming Payment-.bru +++ b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Create Receiver -remote Incoming Payment-.bru @@ -10,6 +10,10 @@ post { auth: none } +headers { + ~x-operator-secret: {{operatorApiSecret}} +} + body:graphql { mutation CreateReceiver($input: CreateReceiverInput!) { createReceiver(input: $input) { @@ -60,7 +64,7 @@ script:pre-request { script:post-response { const body = res.getBody(); - + if (body?.data) { bru.setEnvVar("receiverId", body.data.createReceiver.receiver.id); } diff --git a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Get Outgoing Payment.bru b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Get Outgoing Payment.bru index cfca035df3..5cabfb310d 100644 --- a/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Get Outgoing Payment.bru +++ b/bruno/collections/Rafiki/Examples/Admin API - only locally/Peer-to-Peer Payment/Get Outgoing Payment.bru @@ -10,6 +10,10 @@ post { auth: none } +headers { + ~x-operator-secret: tXhxCNRWuVOJs5W/CUgjsc+vBnKj0CVdqSb87EXN64E= +} + body:graphql { query GetOutgoingPayment($id: String!) { outgoingPayment(id: $id) { diff --git a/bruno/collections/Rafiki/scripts.js b/bruno/collections/Rafiki/scripts.js index 8ea6b7a614..712fa8d8e4 100644 --- a/bruno/collections/Rafiki/scripts.js +++ b/bruno/collections/Rafiki/scripts.js @@ -181,7 +181,8 @@ const scripts = { method: 'post', headers: { signature: this.generateBackendApiSignature(postBody), - 'Content-Type': 'application/json' + 'Content-Type': 'application/json', + 'x-operator-secret': bru.getEnvVar('operatorApiSecret') }, body: JSON.stringify(postBody) } diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts index f9d27d80f6..f0d7b356dc 100644 --- a/packages/backend/src/app.ts +++ b/packages/backend/src/app.ts @@ -116,6 +116,8 @@ export interface AppContextData { export interface ApolloContext { container: IocContract logger: Logger + tenantId: string + isOperator: boolean } export type AppContext = Koa.ParameterizedContext @@ -439,15 +441,36 @@ export class App { await getTenantIdFromOperatorSecret(ctx, this.config) return next() } else { - await getTenantIdFromRequestHeaders(ctx, this.config) + // TODO: cache. adds network call to every gql resolver and + // not all resolvers need tenantId/isOperator. + // await getTenantIdFromRequestHeaders(ctx, this.config) + + // TODO: remove this and restore getTenantIdFromRequestHeaders. + // This is just a hack to be able to test normal tenant (non-operator) + // case from bruno + const tenantService = await this.container.use('tenantService') + const tenant = await tenantService.getByEmail( + 'c9PrimaryTenant@example.com' ?? + (await tenantService.getByEmail('hlbPrimaryTenant@example.com')) + ) + console.log(tenant) + + if (!tenant) { + throw new Error('could not find tenantId') + } + + ctx.tenantId = tenant.id + ctx.isOperator = false return next() } }) koa.use( koaMiddleware(this.apolloServer, { - context: async (): Promise => { + context: async ({ ctx }) => { return { + tenantId: ctx.tenantId, + isOperator: ctx.isOperator, container: this.container, logger: await this.container.use('logger') } diff --git a/packages/backend/src/graphql/resolvers/incoming_payment.ts b/packages/backend/src/graphql/resolvers/incoming_payment.ts index b9d60b83e6..d5cdca719f 100644 --- a/packages/backend/src/graphql/resolvers/incoming_payment.ts +++ b/packages/backend/src/graphql/resolvers/incoming_payment.ts @@ -146,6 +146,23 @@ export const cancelIncomingPayment: MutationResolvers['cancelInco 'incomingPaymentService' ) + // ACCESS CONTROL CASE: Update/Deletes. Check existing resource's tenantId before mutating. + + // If from operator use tenantId given on input, otherwise use the requestor's tenant's id. + // In other words, dont use the operator's tenantId + const tenantId = ctx.isOperator ? args.input.tenantId : ctx.tenantId + const incomingPayment = await incomingPaymentService.get({ + id: args.input.id + }) + if (!incomingPayment || tenantId !== incomingPayment.tenantId) { + throw new GraphQLError('Unknown incoming payment', { + extensions: { + code: GraphQLErrorCode.NotFound, + id: args.input.id + } + }) + } + const incomingPaymentOrError = await incomingPaymentService.cancel( args.input.id ) diff --git a/packages/backend/src/graphql/resolvers/outgoing_payment.ts b/packages/backend/src/graphql/resolvers/outgoing_payment.ts index a9cdcb4403..03ddc1871b 100644 --- a/packages/backend/src/graphql/resolvers/outgoing_payment.ts +++ b/packages/backend/src/graphql/resolvers/outgoing_payment.ts @@ -26,7 +26,27 @@ export const getOutgoingPayment: QueryResolvers['outgoingPayment' const payment = await outgoingPaymentService.get({ id: args.id }) - if (!payment) { + // ACCESS CONTROL CASE: Gets. No additional query - just compare resource/given tenantId. + + // Simple, non-service based access control that should generally work for gets. + // But is it a good pattern? Is the access control happening too late, and too + // embedded in resolver logic? + // Alternatives (with their own, different considarations): + // - util fn or middleware to lookup resource and check access before getting it. + // Maybe uses a new service method like: outgoingPaymentService.canAccess(tenantId, isOperator) + // - Adds extra service call but better seperation of concerns. Does not enforce + // access control in a central way (have to add middleware/call fn for each resolver). + // - Check in service. Either in existing method or new method like: + // outgoingPaymentService.getTenanted(id, tenantId, isOperator) + // - Updating existing enforces access control in more central way, but perhaps too univerals? + // Wont have tenantId/isOperator in all cases (calling from connector, for example). + // Can add new methods instead in those cases but I think this duplicates a lot of code (and tests). + // And is it really enforcing it more centrally if it's still up to the caller to call the + // right method? tenanted vs. non-tenanted? If not it would be simpler to handle in resolver level. + + // Operator can always get resource. If not the operator and tenantId's don't match, + // it should present as-if the resouce wasn't found. + if (!payment || (!ctx.isOperator && payment.tenantId !== ctx.tenantId)) { throw new GraphQLError('payment does not exist', { extensions: { code: GraphQLErrorCode.NotFound @@ -104,12 +124,30 @@ export const createOutgoingPayment: MutationResolvers['createOutg args, ctx ): Promise => { + if (!ctx.isOperator) { + const walletAddressService = await ctx.container.use( + 'walletAddressService' + ) + const walletAddress = await walletAddressService.get( + args.input.walletAddressId + ) + if (!walletAddress || ctx.tenantId !== walletAddress.tenantId) { + throw new GraphQLError('Unknown wallet address id input', { + extensions: { + code: 400, + walletAddressUrl: args.input.walletAddressId + } + }) + } + } + const outgoingPaymentService = await ctx.container.use( 'outgoingPaymentService' ) - const outgoingPaymentOrError = await outgoingPaymentService.create( - args.input - ) + const outgoingPaymentOrError = await outgoingPaymentService.create({ + ...args.input, + tenantId: ctx.tenantId + }) if (isOutgoingPaymentError(outgoingPaymentOrError)) { throw new GraphQLError(errorToMessage[outgoingPaymentOrError], { extensions: { diff --git a/packages/backend/src/graphql/resolvers/quote.ts b/packages/backend/src/graphql/resolvers/quote.ts index fcd20fb237..a454c37b28 100644 --- a/packages/backend/src/graphql/resolvers/quote.ts +++ b/packages/backend/src/graphql/resolvers/quote.ts @@ -39,7 +39,35 @@ export const getQuote: QueryResolvers['quote'] = async ( export const createQuote: MutationResolvers['createQuote'] = async (parent, args, ctx): Promise => { + // ACCESS CONTROL CASE: Creates. Get associated wallet address tenantId and compare + // to requestor's tenantId before creating. + + // If from operator use tenantId in input, otherwise use the tenant id from the requestor. + // In other words, dont use the operator's tenantId - operator must create + // on behalf of non-operator tenant + const tenantId = ctx.isOperator ? args.input.tenantId : ctx.tenantId + + // tenantId should match tenantId on wallet address - present as-if WA was not found + const walletAddressService = await ctx.container.use('walletAddressService') + const walletAddress = await walletAddressService.get( + args.input.walletAddressId + ) + if (!walletAddress || tenantId !== walletAddress.tenantId) { + throw new GraphQLError('Unknown wallet address id input', { + extensions: { + code: GraphQLErrorCode.BadUserInput, + walletAddressId: args.input.walletAddressId + } + }) + } + const quoteService = await ctx.container.use('quoteService') + // Optionally, quoteService.create could implicity add the correct tenantId + // based on the the wallet address tenantId. This would circumvent need for + // operator to pass in tenantId - is there any downside? Could the operator + // accidentally create it for the wrong tenant this way? I dont think so... + // Haven't fully thought through the implications, but consider this use case is + // atypical and tenant will usually be creating these (does that inform anything?) const options: CreateQuoteOptions = { walletAddressId: args.input.walletAddressId, tenantId: args.input.tenantId, diff --git a/packages/backend/src/graphql/resolvers/receiver.ts b/packages/backend/src/graphql/resolvers/receiver.ts index b9002c30f1..d3755c8c69 100644 --- a/packages/backend/src/graphql/resolvers/receiver.ts +++ b/packages/backend/src/graphql/resolvers/receiver.ts @@ -34,9 +34,14 @@ export const getReceiver: QueryResolvers['receiver'] = async ( export const createReceiver: MutationResolvers['createReceiver'] = async (_, args, ctx): Promise => { + // TODO: do we need to check tenantId for remote args.input.walletAddressUrl? + // Not found on P2P example because wa url is remote - not sure if we can (probably not?) + // nor necessarily need to do such a check + const receiverService = await ctx.container.use('receiverService') const receiverOrError = await receiverService.create({ + tenantId: ctx.tenantId, walletAddressUrl: args.input.walletAddressUrl, expiresAt: args.input.expiresAt ? new Date(args.input.expiresAt) diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 554a931ea4..6a6d1cdd32 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -373,6 +373,9 @@ export function initIocContainer( if (config.enableTelemetry) { telemetry = await deps.use('telemetry') } + // Problem if moving access control check to existing service methods such as: + // walletAddressService, incomingPaymentService. + // Will not have tenantId/isOperator in connector. return await createConnectorService({ logger: await deps.use('logger'), redis: await deps.use('redis'), diff --git a/packages/backend/src/open_payments/receiver/service.ts b/packages/backend/src/open_payments/receiver/service.ts index 10a12f1c57..c93ec2473f 100644 --- a/packages/backend/src/open_payments/receiver/service.ts +++ b/packages/backend/src/open_payments/receiver/service.ts @@ -185,6 +185,8 @@ export async function getLocalIncomingPayment( return undefined } + // Problem if moving access control check to existing service methods: + // - will not have tenantId/isOperator in everywhere this is called (ie outgoing payment worker) const incomingPayment = await deps.incomingPaymentService.get({ id: urlParseResult.id })