From a5d678352e70dc6c51abba17f87227615f67e176 Mon Sep 17 00:00:00 2001 From: Matt Schoch Date: Mon, 26 Aug 2024 11:46:02 -0400 Subject: [PATCH] Fixing race condition with PERMITs before EvaluationLog is insertted --- apps/armory/Makefile | 2 +- .../service/authorization-request.service.ts | 44 ++--- .../authorization-request.repository.ts | 154 ++++++++---------- .../src/lib/__test__/e2e/scenarios.spec.ts | 66 ++++---- 4 files changed, 122 insertions(+), 144 deletions(-) diff --git a/apps/armory/Makefile b/apps/armory/Makefile index 440848548..697e9cc49 100644 --- a/apps/armory/Makefile +++ b/apps/armory/Makefile @@ -5,7 +5,7 @@ ARMORY_DATABASE_SCHEMA := ${ARMORY_PROJECT_DIR}/src/shared/module/persistence/sc # === Start === armory/start/dev: - npx nx serve ${ARMORY_PROJECT_NAME} + npx nx serve ${ARMORY_PROJECT_NAME} -- --port 9230 # === Setup === diff --git a/apps/armory/src/orchestration/core/service/authorization-request.service.ts b/apps/armory/src/orchestration/core/service/authorization-request.service.ts index 8f1342ebe..d4823572c 100644 --- a/apps/armory/src/orchestration/core/service/authorization-request.service.ts +++ b/apps/armory/src/orchestration/core/service/authorization-request.service.ts @@ -143,25 +143,9 @@ export class AuthorizationRequestService { }) const status = getStatus(evaluation.decision) - - const authzRequest = await this.authzRequestRepository.update({ - id: input.id, - clientId: input.clientId, - status, - evaluations: [ - { - id: uuid(), - decision: evaluation.decision, - signature: evaluation.accessToken?.value || null, - approvalRequirements: evaluation.approvals, - transactionRequestIntent: evaluation.transactionRequestIntent, - createdAt: new Date() - } - ] - }) - + // NOTE: we will track the transfer before we update the status to PERMITTED so that we don't have a brief window where a second transfer can come in before the history is tracked. // TODO: (@wcalderipe, 01/02/24) Move to the TransferTrackingService. - if (authzRequest.request.action === Action.SIGN_TRANSACTION && status === AuthorizationRequestStatus.PERMITTED) { + if (input.request.action === Action.SIGN_TRANSACTION && status === AuthorizationRequestStatus.PERMITTED) { // TODO: (@wcalderipe, 08/02/24) Remove the cast `as Intent`. const intent = evaluation.transactionRequestIntent as Intent @@ -183,13 +167,13 @@ export class AuthorizationRequestService { } const transfer = { - resourceId: authzRequest.request.resourceId, - clientId: authzRequest.clientId, - requestId: authzRequest.id, + resourceId: input.request.resourceId, + clientId: input.clientId, + requestId: input.id, from: intent.from, to: intent.to, token: intent.token, - chainId: authzRequest.request.transactionRequest.chainId, + chainId: input.request.transactionRequest.chainId, initiatedBy: evaluation.principal?.userId, createdAt: new Date(), amount: BigInt(intent.amount), @@ -200,6 +184,22 @@ export class AuthorizationRequestService { } } + const authzRequest = await this.authzRequestRepository.update({ + id: input.id, + clientId: input.clientId, + status, + evaluations: [ + { + id: uuid(), + decision: evaluation.decision, + signature: evaluation.accessToken?.value || null, + approvalRequirements: evaluation.approvals, + transactionRequestIntent: evaluation.transactionRequestIntent, + createdAt: new Date() + } + ] + }) + this.logger.log('Authorization request status updated', { clientId: authzRequest.clientId, id: authzRequest.id, diff --git a/apps/armory/src/orchestration/persistence/repository/authorization-request.repository.ts b/apps/armory/src/orchestration/persistence/repository/authorization-request.repository.ts index 8cc98de87..7db8281e3 100644 --- a/apps/armory/src/orchestration/persistence/repository/authorization-request.repository.ts +++ b/apps/armory/src/orchestration/persistence/repository/authorization-request.repository.ts @@ -11,7 +11,7 @@ import { Prisma } from '@prisma/client/armory' import { v4 as uuid } from 'uuid' import { ApplicationException } from '../../../shared/exception/application.exception' import { PrismaService } from '../../../shared/module/persistence/service/prisma.service' -import { decodeAuthorizationRequest, decodeEvaluationLog } from '../decode/authorization-request.decode' +import { decodeAuthorizationRequest } from '../decode/authorization-request.decode' import { createRequestSchema } from '../schema/request.schema' @Injectable() @@ -25,39 +25,46 @@ export class AuthorizationRequestRepository { const approvals = (input.approvals || []).map((sig) => ({ sig })) const errors = this.toErrors(clientId, input.errors) - await this.prismaService.authorizationRequest.create({ - data: { - id, - status, - clientId, - request, - idempotencyKey, - createdAt, - updatedAt, - action: request.action, - authnSig: authentication, - metadata, - approvals: { - createMany: { - data: approvals + const evaluationLogs = this.toEvaluationLogs({ requestId: id, clientId, evaluations: input.evaluations }) + const approvalRequirements = this.toApprovalRequirements(input.evaluations) + + await this.prismaService.$transaction(async (prisma) => { + await prisma.authorizationRequest.create({ + data: { + id, + status, + clientId, + request, + idempotencyKey, + createdAt, + updatedAt, + action: request.action, + authnSig: authentication, + metadata, + approvals: { + createMany: { + data: approvals + } + }, + errors: { + createMany: { + data: errors + } } }, - errors: { - createMany: { - data: errors - } + include: { + approvals: true, + errors: true } - }, - include: { - approvals: true, - errors: true - } - }) + }) + + await prisma.evaluationLog.createMany({ + data: evaluationLogs + }) - await this.createEvaluationLogs({ - requestId: id, - clientId, - evaluations: input.evaluations + await prisma.approvalRequirement.createMany({ + data: approvalRequirements + }) }) const authRequest = await this.findById(id) @@ -70,40 +77,6 @@ export class AuthorizationRequestRepository { return authRequest } - private async createEvaluationLogs({ - requestId, - clientId, - evaluations - }: { - requestId: string - clientId?: string - evaluations?: Evaluation[] - }) { - const evaluationLogs = this.toEvaluationLogs({ requestId, clientId, evaluations }) - const approvalRequirements = this.toApprovalRequirements(evaluations) - - await this.prismaService.evaluationLog.createMany({ - data: evaluationLogs - }) - - await this.prismaService.approvalRequirement.createMany({ - data: approvalRequirements - }) - - return this.getEvaluationLogs(requestId) - } - - private async getEvaluationLogs(requestId: string) { - const evaluationLogs = await this.prismaService.evaluationLog.findMany({ - where: { requestId }, - include: { - approvals: true - } - }) - - return evaluationLogs.map(decodeEvaluationLog) - } - private toEvaluationLogs({ requestId, clientId, @@ -185,34 +158,39 @@ export class AuthorizationRequestRepository { (sig) => ({ sig }) ) const errors = this.toErrors(clientId, input.errors) - // TODO (@wcalderipe, 19/01/24): Cover the skipDuplicate with tests. - await this.prismaService.authorizationRequest.update({ - where: { id }, - data: { - status, - approvals: { - createMany: { - data: approvals, - skipDuplicates: true - } - }, - errors: { - createMany: { - data: errors, - skipDuplicates: true - } + + const updateData: Prisma.AuthorizationRequestUpdateInput = { + status, + approvals: { + createMany: { + data: approvals, + skipDuplicates: true } }, - include: { - approvals: true, - errors: true + errors: { + createMany: { + data: errors, + skipDuplicates: true + } } - }) + } + const evaluationLogs = this.toEvaluationLogs({ requestId: id, clientId, evaluations: input.evaluations }) + const approvalRequirements = this.toApprovalRequirements(input.evaluations) + + // Do the update in a tx to avoid partial updates. + await this.prismaService.$transaction(async (prisma) => { + await prisma.authorizationRequest.update({ + where: { id }, + data: updateData + }) - await this.createEvaluationLogs({ - requestId: id, - clientId, - evaluations: input.evaluations + await prisma.evaluationLog.createMany({ + data: evaluationLogs + }) + + await prisma.approvalRequirement.createMany({ + data: approvalRequirements + }) }) const authRequest = await this.findById(id) diff --git a/packages/armory-sdk/src/lib/__test__/e2e/scenarios.spec.ts b/packages/armory-sdk/src/lib/__test__/e2e/scenarios.spec.ts index 232e96864..5a96a9990 100644 --- a/packages/armory-sdk/src/lib/__test__/e2e/scenarios.spec.ts +++ b/packages/armory-sdk/src/lib/__test__/e2e/scenarios.spec.ts @@ -29,11 +29,13 @@ const bobPrivateKey = FIXTURE.UNSAFE_PRIVATE_KEY.Bob const alicePrivateKey = FIXTURE.UNSAFE_PRIVATE_KEY.Alice const carolPrivateKey = FIXTURE.UNSAFE_PRIVATE_KEY.Carol +const genNonce = (request: Request) => ({ ...request, nonce: `${request.nonce}-${v4()}` }) + describe('End to end scenarios', () => { describe('rate limiting by principal', () => { const request: Request = { action: Action.SIGN_TRANSACTION, - nonce: 'test-nonce', + nonce: 'test-nonce-1', transactionRequest: { from: '0x0301e2724a40E934Cce3345928b88956901aA127', to: '0x76d1b7f9b3F69C435eeF76a98A415332084A856F', @@ -167,7 +169,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -178,7 +180,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -189,7 +191,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -202,7 +204,7 @@ describe('End to end scenarios', () => { }) try { - await authClient.requestAccessToken(request) + await authClient.requestAccessToken(genNonce(request)) } catch (error: any) { expect(error.message).toEqual('Unauthorized') } @@ -214,7 +216,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) }) @@ -222,7 +224,7 @@ describe('End to end scenarios', () => { describe('rate limiting by groupId', () => { const request: Request = { action: Action.SIGN_TRANSACTION, - nonce: 'test-nonce', + nonce: 'test-nonce-2', transactionRequest: { from: '0x0301e2724a40E934Cce3345928b88956901aA127', to: '0x76d1b7f9b3F69C435eeF76a98A415332084A856F', @@ -402,7 +404,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -412,7 +414,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -422,31 +424,33 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) it('forbids member bob to exceed the limit', async () => { + expect.assertions(1) const { authClient } = await buildAuthClient(bobPrivateKey, { host: getAuthHost(), clientId }) try { - await authClient.requestAccessToken(request) + await authClient.requestAccessToken(genNonce(request)) } catch (error: any) { expect(error.message).toEqual('Unauthorized') } }) it('forbids member carol to exceed the limit', async () => { + expect.assertions(1) const { authClient } = await buildAuthClient(carolPrivateKey, { host: getAuthHost(), clientId }) try { - await authClient.requestAccessToken(request) + await authClient.requestAccessToken(genNonce(request)) } catch (error: any) { expect(error.message).toEqual('Unauthorized') } @@ -458,7 +462,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) }) @@ -466,7 +470,7 @@ describe('End to end scenarios', () => { describe('spending limits', () => { const request: Request = { action: Action.SIGN_TRANSACTION, - nonce: 'test-nonce', + nonce: 'test-nonce-3', transactionRequest: { from: '0x0301e2724a40E934Cce3345928b88956901aA127', to: '0x76d1b7f9b3F69C435eeF76a98A415332084A856F', @@ -621,7 +625,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -630,8 +634,7 @@ describe('End to end scenarios', () => { host: getAuthHost(), clientId }) - - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -640,8 +643,7 @@ describe('End to end scenarios', () => { host: getAuthHost(), clientId }) - - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -650,8 +652,7 @@ describe('End to end scenarios', () => { host: getAuthHost(), clientId }) - - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -663,7 +664,7 @@ describe('End to end scenarios', () => { }) try { - await authClient.requestAccessToken(request) + await authClient.requestAccessToken(genNonce(request)) } catch (error: any) { expect(error.message).toEqual('Unauthorized') } @@ -674,8 +675,7 @@ describe('End to end scenarios', () => { host: getAuthHost(), clientId }) - - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) }) @@ -683,7 +683,7 @@ describe('End to end scenarios', () => { describe('approvals by groupId and spending limit', () => { const request: Request = { action: Action.SIGN_TRANSACTION, - nonce: 'test-nonce', + nonce: 'test-nonce-4', transactionRequest: { from: '0x0301e2724a40E934Cce3345928b88956901aA127', to: '0x76d1b7f9b3F69C435eeF76a98A415332084A856F', @@ -916,7 +916,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -926,7 +926,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -936,7 +936,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -948,7 +948,7 @@ describe('End to end scenarios', () => { }) try { - await authClient.requestAccessToken(request) + await authClient.requestAccessToken(genNonce(request)) } catch (error: any) { expect(error.message).toEqual('Unauthorized') } @@ -962,7 +962,7 @@ describe('End to end scenarios', () => { }) try { - await authClient.requestAccessToken(request) + await authClient.requestAccessToken(genNonce(request)) } catch (error: any) { expect(error.message).toEqual('Unauthorized') } @@ -974,7 +974,7 @@ describe('End to end scenarios', () => { clientId }) - const response = await authClient.requestAccessToken(request) + const response = await authClient.requestAccessToken(genNonce(request)) expect(response).toMatchObject({ value: expect.any(String) }) }) @@ -991,7 +991,7 @@ describe('End to end scenarios', () => { clientId }) - const res = await authClient.authorize(request) + const res = await authClient.authorize(genNonce(request)) expect(res.decision).toEqual(Decision.CONFIRM) if (res.decision === Decision.CONFIRM) { @@ -1015,7 +1015,7 @@ describe('End to end scenarios', () => { clientId }) - const res = await authClient.authorize(request) + const res = await authClient.authorize(genNonce(request)) expect(res.decision).toEqual(Decision.CONFIRM) if (res.decision === Decision.CONFIRM) {