feat(backend): tenant support for wallet address (#3114)

[koekiebox]

Changes proposed in this pull request

• See: [Multi-Tenant] Tenanted Wallet Addresses #3114

Context

Checklist

• Related issues linked using fixes #number
• Tests added/updated
• Make sure that all checks pass
• Bruno collection updated (if necessary)
• Documentation issue created with user-docs label (if necessary)
• OpenAPI specs updated (if necessary)

[koekiebox]

We want to ensure we are not being 'hacked', the tenant can only access their own wallet addresses.

[BlairCurrey]

Jason what are you thoughts on where we do the access control (ie does the tenantId match) for the different cases? Updating, getting, etc.

In this PR we are mixing whether the tenant id gets passed into the service layer or not here. The update passes it in and uses it in the where to find the resource to update, and the get does not. Looking at the implementation and usage in this PR I think its very simple and like that. I just wonder what happens with the worker where I think the pattern breaks down. And is it a problem if I ask "OK how are you doing access control?" and then we have to go in the resolver for some of it and the service for other parts across all resources. And I guess we need to make access control tests for each resolver/service that does it.

For the worker, we wont have a tenantId since the request doesnt originate from a tenant. So the walletAddressService.update({tenantId ... }) (or similar) wont work. OutgoingPayment update would run into this, for example.

Nathan and I have had some discussions about where the access control happens. There were a few ideas I recall:

1. always doing access control at the resolver or middleware level before the resolver. getting a wallet address? lookup wallet address and check tenantId, then run the resolver as normal and have not concept of the tenant in the service. This does add some extra calls but centralizes it and keeps the services totally tenantId agnostic. Do we need to care about the tenant everywhere in our application or just at the entry? This does have some complexity in terms of getting the resource id from the request in standard way (makes doing in middleware hard because things vary by resolver).
2. make separate functions with shared logic for updating, getting, etc. a resource and doing access control or updating a resource and not doing access control. And maybe always doing the access control in the service layer.

[BlairCurrey]

I had a draft PR for a previous version of multi-tenancy with a helper function in the different services (quote, wallet address, etc) for checking if a tenant could access. Just to elaborate on option #1 above a bit.

https://github.com/interledger/rafiki/pull/3026/files#diff-1695f39120835125efdde9389ae4e400acc244ab1a89105ab531631f1606dff0

[BlairCurrey]

Also, do we need an "is operator" check here?

For things like the create, update, etc. the operator needs to specify a tenant id. But I dont think thats the case for the get, and we need to let the operator bypass this check. Imagine they are on the page listing the wallet addresses. When they click one of them, the tenantId for that resource isnt getting set, and then this resolver fires.

[njlie]

I think with operations like this where a single item is affected, the tenant id on the context should be considered within the permission of the requester - the middleware should take care if the tenant id is the "right" one, so it can be passed into the service layer without any security concern.

I would say isOperator is more relevant to pagination requests, where it would be necessary to know if it needs to be filtered, or if the request is to createTenant which should be operator-only.

[koekiebox]

Ok, I have updated the logic to not have the tenantId be mandatory on the service layer.

In the GraphQL, we ensure that we have flexibility for providing an alternative tenantId (for when isOperator == true) than what is on the signature, as well as ensuring they are correct when fetching.

Example snippet that performs the validation on the resolver:

// The blow is done when fetching the walletAddress:
if (ctx) {
    const tenantId = tenantIdToProceed(
      ctx.isOperator,
      ctx.tenant.id,
      walletAddress.tenantId
    )
    if (!tenantId) {
      const err = WalletAddressError.InvalidTenantIdNotAllowed
      throw new GraphQLError(errorToMessage[err], {
        extensions: {
          code: errorToCode[err]
        }
      })
    }
  }

// Then in operations like create and update walletAddress:
 const tenantId = tenantIdToProceed(
      ctx.isOperator,
      ctx.tenant.id,
      args.input.tenantId
    )
    if (!tenantId) {
      const err = WalletAddressError.InvalidTenantIdNotAllowed
      throw new GraphQLError(errorToMessage[err], {
        extensions: {
          code: errorToCode[err]
        }
      })
    }

The function for determining the correct tenantId to use:

/**
 * The tenantId to use will be determined as follows:
 * - When an operator and the {tenantId} is present, return {tenantId}
 * - When an operator and {tenantId} is not present, return {signatureTenantId}
 * - When NOT an operator and {tenantId} is present, but does not match {signatureTenantId}, return {undefined}
 * - Otherwise return {signatureTenantId}
 *
 * @param isOperator is operator
 * @param signatureTenantId the signature tenantId
 * @param tenantId the intended tenantId
 */
export function tenantIdToProceed(
  isOperator: boolean,
  signatureTenantId: string,
  tenantId?: string
): string | undefined {
  if (isOperator && tenantId) return tenantId
  else if (isOperator) return signatureTenantId
  return tenantId && tenantId !== signatureTenantId
    ? undefined
    : signatureTenantId
}

Let me know what you think? @BlairCurrey and @njlie

[mkurapov]

@koekiebox instead of args.input.tenantId, we would be getting the tenantId from the header, right?

[mkurapov]

@BlairCurrey

we need to let the operator bypass this check. Imagine they are on the page listing the wallet addresses.

When the operator is trying to paginate across all resources (e.g. all wallet addresses, for example), what should the behaviour be? we don't pass in the tenantId at all?

[mkurapov]

ah, we basically got to the same conclusion :)

I ended up setting a tenant-id header in the apollo client during tests: https://github.com/interledger/rafiki/pull/3206/files#diff-5b481c1a1464d3e282df7102a0cb5882701fc2b2af81037e203812e13acf0176R414-R433

Both work, I'm trying to think whether we would actually want to be able to pass in a different tenant id during tests...