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

Bc/2929/filter op resources by tenantid #3026

Draft
wants to merge 15 commits into
base: 2893-multi-tenant-rafiki
Choose a base branch
from

Conversation

BlairCurrey
Copy link
Contributor

@BlairCurrey BlairCurrey commented Oct 9, 2024

Trying to determine where to put access control. Basic options are:

  • in resolver/middleware
  • in service, ie an interface like incomingPaymentService.get(id, tenantId, isOperator). Not sure if this is ultimately viable

There are basically 3 classes of access control that I see so far:

  • Gets: check tenantIds after retrieving if doing in resolver, or filter by if doing in service
  • Creates: check the resource-to-be-created's walletAddress.tenantId before creating (either in resolver or service)
  • Update/deletes: checks the existing resource's tenantId before mutating (either in resolver or service)

This getIncomingPayment resolver and the comment shows the issue more clearly. It does access control in the resolver after getting the resource:

export const getOutgoingPayment: QueryResolvers<ApolloContext>['outgoingPayment'] =
async (parent, args, ctx): Promise<ResolversTypes['OutgoingPayment']> => {
const outgoingPaymentService = await ctx.container.use(
'outgoingPaymentService'
)
const payment = await outgoingPaymentService.get({
id: args.id
})
// 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
}
})
}
return paymentToGraphql(payment)
}

This cancelIncomingPayment resolver exemplifies the Create and Update/Delete cases (largely similar, only difference is if we compare given tenantId with existing resource or resource-to-be-created's walletAddress.tenantId):

export const cancelIncomingPayment: MutationResolvers<ApolloContext>['cancelIncomingPayment'] =
async (
parent,
args,
ctx
): Promise<ResolversTypes['CancelIncomingPaymentResponse']> => {
const incomingPaymentService = await ctx.container.use(
'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
)
if (isIncomingPaymentError(incomingPaymentOrError)) {
throw new GraphQLError(errorToMessage[incomingPaymentOrError], {
extensions: {
code: errorToCode[incomingPaymentOrError]
}
})
}
return {
payment: paymentToGraphql(incomingPaymentOrError)
}
}

will fix: #2929

@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Oct 9, 2024
@github-actions github-actions bot added the type: tests Testing related label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant