feat: begin implementing some gql access control, annotated w/ comments

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": ""
     }
   }
 }

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

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

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

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

packages/backend/src/app.ts

@@ -116,6 +116,8 @@ export interface AppContextData {
 export interface ApolloContext {
   container: IocContract<AppServices>
   logger: Logger
+  tenantId: string
+  isOperator: boolean
 }
 export type AppContext = Koa.ParameterizedContext<DefaultState, AppContextData>
 
@@ -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<ApolloContext> => {
+        context: async ({ ctx }) => {
           return {
+            tenantId: ctx.tenantId,
+            isOperator: ctx.isOperator,
             container: this.container,
             logger: await this.container.use('logger')
           }

packages/backend/src/graphql/resolvers/incoming_payment.ts

@@ -146,6 +146,23 @@ export const cancelIncomingPayment: MutationResolvers<ApolloContext>['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
     )

packages/backend/src/graphql/resolvers/outgoing_payment.ts

@@ -26,7 +26,27 @@ export const getOutgoingPayment: QueryResolvers<ApolloContext>['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<ApolloContext>['createOutg
     args,
     ctx
   ): Promise<ResolversTypes['OutgoingPaymentResponse']> => {
+    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: {

packages/backend/src/graphql/resolvers/quote.ts

@@ -39,7 +39,35 @@ export const getQuote: QueryResolvers<ApolloContext>['quote'] = async (
 
 export const createQuote: MutationResolvers<ApolloContext>['createQuote'] =
   async (parent, args, ctx): Promise<ResolversTypes['QuoteResponse']> => {
+    // 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,

packages/backend/src/graphql/resolvers/receiver.ts

@@ -34,9 +34,14 @@ export const getReceiver: QueryResolvers<ApolloContext>['receiver'] = async (
 
 export const createReceiver: MutationResolvers<ApolloContext>['createReceiver'] =
   async (_, args, ctx): Promise<ResolversTypes['CreateReceiverResponse']> => {
+    // 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)

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

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