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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ post {
auth: none
}

headers {
~x-operator-secret: tXhxCNRWuVOJs5W/CUgjsc+vBnKj0CVdqSb87EXN64E=
}

body:graphql {
mutation CreateOutgoingPayment($input: CreateOutgoingPaymentInput!) {
createOutgoingPayment(input: $input) {
Expand Down Expand Up @@ -46,7 +50,7 @@ body:graphql:vars {
{
"input": {
"walletAddressId": "{{gfranklinWalletAddressId}}",
"quoteId": "{{quoteId}}"
"quoteId": "{{quoteId}}", "tenantId": ""
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ post {
auth: none
}

headers {
~x-operator-secret: {{operatorApiSecret}}
}

body:graphql {
mutation CreateQuote($input: CreateQuoteInput!) {
createQuote(input: $input) {
Expand Down Expand Up @@ -38,7 +42,8 @@ body:graphql:vars {
{
"input": {
"walletAddressId": "{{gfranklinWalletAddressId}}",
"receiver": "{{receiverId}}"
"receiver": "{{receiverId}}",
"tenantId": ""
}
}
}
Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ post {
auth: none
}

headers {
~x-operator-secret: {{operatorApiSecret}}
}

body:graphql {
mutation CreateReceiver($input: CreateReceiverInput!) {
createReceiver(input: $input) {
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ post {
auth: none
}

headers {
~x-operator-secret: tXhxCNRWuVOJs5W/CUgjsc+vBnKj0CVdqSb87EXN64E=
}

body:graphql {
query GetOutgoingPayment($id: String!) {
outgoingPayment(id: $id) {
Expand Down
3 changes: 2 additions & 1 deletion bruno/collections/Rafiki/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
25 changes: 24 additions & 1 deletion packages/backend/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -439,15 +441,36 @@ export class App {
await getTenantIdFromOperatorSecret(ctx, this.config)
return next()
} else {
// 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')
}
Expand Down
17 changes: 17 additions & 0 deletions packages/backend/src/graphql/resolvers/incoming_payment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,23 @@ export const cancelIncomingPayment: MutationResolvers<ApolloContext>['cancelInco
'incomingPaymentService'
)

// ACCESS CONTROL CASE: Update/Deletes. Check existing resource's tenantId before mutating.

// TODO: move into incomingPaymentService.canAccess(isOperator, tenantId, incomingPaymentId)?
if (!ctx.isOperator) {
const incomingPayment = await incomingPaymentService.get({
id: args.input.id
})
if (!incomingPayment || ctx.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
)
Expand Down
67 changes: 59 additions & 8 deletions packages/backend/src/graphql/resolvers/outgoing_payment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,17 +631,62 @@ describe('OutgoingPayment Resolvers', (): void => {
})

test('Fails if walletAddress unknown', async (): Promise<void> => {
const createSpy = jest
.spyOn(outgoingPaymentService, 'create')
.mockResolvedValueOnce(OutgoingPaymentError.UnknownWalletAddress)

const input = {
walletAddressId: uuid(),
tenantId,
quoteId: uuid()
}

expect.assertions(2)
try {
await appContainer.apolloClient
.query({
query: gql`
mutation CreateOutgoingPayment(
$input: CreateOutgoingPaymentInput!
) {
createOutgoingPayment(input: $input) {
payment {
id
state
}
}
}
`,
variables: { input }
})
.then(
(query): OutgoingPaymentResponse =>
query.data?.createOutgoingPayment
)
} catch (error) {
expect(error).toBeInstanceOf(ApolloError)
expect((error as ApolloError).graphQLErrors).toContainEqual(
expect.objectContaining({
message: errorToMessage[OutgoingPaymentError.UnknownWalletAddress],
extensions: expect.objectContaining({
code: errorToCode[OutgoingPaymentError.UnknownWalletAddress]
})
})
)
}
})

test('Fails if tenantId doesnt match walletAddress.tenantId', async (): Promise<void> => {
const { id: walletAddressId } = await createWalletAddress(
deps,
tenantId,
{
assetId: asset.id
}
)
const input = {
walletAddressId: walletAddressId,
tenantId: uuid(),
quoteId: uuid()
}

expect.assertions(3)
expect.assertions(2)
try {
await appContainer.apolloClient
.query({
Expand Down Expand Up @@ -674,17 +719,23 @@ describe('OutgoingPayment Resolvers', (): void => {
})
)
}
expect(createSpy).toHaveBeenCalledWith(input)
})

test('internal server error', async (): Promise<void> => {
const { id: walletAddressId } = await createWalletAddress(
deps,
tenantId,
{
assetId: asset.id
}
)
const createSpy = jest
.spyOn(outgoingPaymentService, 'create')
.mockRejectedValueOnce(new Error('unexpected'))

const input = {
walletAddressId: uuid(),
tenantId: uuid(),
walletAddressId,
tenantId,
quoteId: uuid()
}

Expand Down
51 changes: 46 additions & 5 deletions packages/backend/src/graphql/resolvers/outgoing_payment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import {
import {
isOutgoingPaymentError,
errorToMessage,
errorToCode
errorToCode,
OutgoingPaymentError
} from '../../open_payments/payment/outgoing/errors'
import { OutgoingPayment } from '../../open_payments/payment/outgoing/model'
import { ApolloContext } from '../../app'
Expand All @@ -26,7 +27,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
Expand Down Expand Up @@ -104,12 +125,32 @@ export const createOutgoingPayment: MutationResolvers<ApolloContext>['createOutg
args,
ctx
): Promise<ResolversTypes['OutgoingPaymentResponse']> => {
const tenantId = ctx.isOperator ? args.input.tenantId : ctx.tenantId

// tenantId should match tenantId on wallet address. fail should appear 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(
errorToMessage[OutgoingPaymentError.UnknownWalletAddress],
{
extensions: {
code: errorToCode[OutgoingPaymentError.UnknownWalletAddress],
walletAddressId: 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: {
Expand Down
Loading
Loading