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

refactor(backend): add getOrCreate method for grantReferenceService #787

Closed
wants to merge 12 commits into from
Closed
36 changes: 36 additions & 0 deletions packages/backend/src/open_payments/auth/middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,42 @@ describe('Auth Middleware', (): void => {
scope.done()
})

test('if getOrCreateGrantReference throws, return 500', async (): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test('if getOrCreateGrantReference throws, return 500', async (): Promise<void> => {
test('returns 500 if getOrCreateGrantReference throws', async (): Promise<void> => {

the same as comments below

const getOrCreateGrantReferenceSpy = jest
.spyOn(grantReferenceService, 'getOrCreate')
.mockImplementationOnce(async () => {
throw new Error('unexpected')
})

const grant = new TokenInfo(
{
active: true,
clientId: uuid(),
grant: uuid(),
access: [
{
type: AccessType.IncomingPayment,
actions: [AccessAction.Read],
identifier: ctx.paymentPointer.url
}
]
},
mockKeyInfo
)

await grantReferenceService.create({
id: grant.grant,
clientId: uuid()
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this create


const scope = mockAuthServer(grant.toJSON())
await expect(middleware(ctx, next)).rejects.toMatchObject({
status: 500
})
expect(getOrCreateGrantReferenceSpy).toHaveBeenCalled()
scope.done()
})

test.each`
limitAccount
${false}
Expand Down
34 changes: 12 additions & 22 deletions packages/backend/src/open_payments/auth/middleware.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { AccessType, AccessAction } from './grant'
import { Transaction } from 'objection'
import { GrantReference } from '../grantReference/model'
import { HttpSigContext, verifySigAndChallenge } from 'auth'

export function createAuthMiddleware({
Expand Down Expand Up @@ -55,26 +53,18 @@ export function createAuthMiddleware({
ctx.throw(401, `Invalid signature`)
}
}
await GrantReference.transaction(async (trx: Transaction) => {
const grantRef = await grantReferenceService.get(grant.grant, trx)
if (grantRef) {
if (grantRef.clientId !== grant.clientId) {
logger.debug(
`clientID ${grant.clientId} for grant ${grant.grant} does not match internal reference clientId ${grantRef.clientId}.`
)
ctx.throw(500)
}
} else if (action === AccessAction.Create) {
// Grant and client ID's are only stored for create routes
await grantReferenceService.create(
{
id: grant.grant,
clientId: grant.clientId
},
trx
)
}
})

try {
await grantReferenceService.getOrCreate(
{ id: grant.grant, clientId: grant.clientId },
action
)
Comment on lines +58 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await grantReferenceService.getOrCreate(
{ id: grant.grant, clientId: grant.clientId },
action
)
if (action === AccessAction.Create) {
await grantReferenceService.getOrCreate({
id: grant.grant,
clientId: grant.clientId
})
} else {
await grantReferenceService.get(grant.grant)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By moving the action check to the caller, the clientId check

if (grantRef) {
if (grantRef.clientId !== options.clientId) {
throw new Error(
`clientID ${options.clientId} for grant ${options.id} does not match internal reference clientId ${grantRef.clientId}.`
)
}

will probably need to be moved to the getGrantRerefence method
async function getGrantReference(grantId: string, trx: Transaction) {
return await GrantReference.query(trx).findById(grantId)
}

and will have to take both arguments (id and clientId)

async function getGrantReference(
  options: CreateGrantReferenceOptions,
  trx: Transaction
) {
// ...
}

otherwise there will be no validation for the clientId when calling grantReferenceService.get

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards not passing clientId to grantReferenceService.get (even though this is currently the only place calling it).
What if we moved the clientId check out here?

Suggested change
await grantReferenceService.getOrCreate(
{ id: grant.grant, clientId: grant.clientId },
action
)
if (action === AccessAction.Create) {
await grantReferenceService.getOrCreate({
id: grant.grant,
clientId: grant.clientId
})
} else {
const grantRef = await grantReferenceService.get(grant.grant)
if (grantRef && grantRef.clientId !== grant.clientId) {
throw new Error(
`clientID ${grant.clientId} for grant ${grant.id} does not match internal reference clientId ${grantRef.clientId}.`
}
}
}

🤔 that looks a lot like how it was before...

Is there an argument for not actually using getOrCreate?
It looks like the AccessAction.Create check was added after this issue was opened:

} catch (e) {
const errInfo = e && typeof e === 'object' && e.stack ? e.stack : e
logger.debug(errInfo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (e) {
const errInfo = e && typeof e === 'object' && e.stack ? e.stack : e
logger.debug(errInfo)
} catch (error) {
const errorMessage = error?.message ?? 'Failed to get or create grant reference'
logger.error({ error }, errorMessage)

would prefer something like this, without needing to check the type etc

ctx.throw(500)
}

ctx.grant = grant

// Unless the relevant grant action is ReadAll/ListAll add the
Expand Down
57 changes: 57 additions & 0 deletions packages/backend/src/open_payments/grantReference/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { Config } from '../../config/app'
import { GrantReferenceService } from './service'
import { truncateTables } from '../../tests/tableManager'
import { GrantReference } from './model'
import { AccessAction } from '../auth/grant'

describe('Grant Reference Service', (): void => {
let deps: IocContract<AppServices>
Expand Down Expand Up @@ -79,4 +80,60 @@ describe('Grant Reference Service', (): void => {
)
})
})

describe('Get or Create Grant Reference', (): void => {
test('cannot fetch a non-existing grant reference', async (): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test('cannot fetch a non-existing grant reference', async (): Promise<void> => {
test('returns undefined for a non-existing grant reference', async (): Promise<void> => {

expect(
await grantReferenceService.getOrCreate(
{ id: uuid(), clientId: uuid() },
AccessAction.List
)
).toBeUndefined()
})

test('throws an error when clientId does not match', async (): Promise<void> => {
const id = uuid()

await grantReferenceService.create({
id,
clientId: uuid()
})

await expect(
async () =>
await grantReferenceService.getOrCreate(
{ id, clientId: uuid() },
AccessAction.List
)
).rejects.toThrowError('does not match internal reference clientId')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we would have needed a RegEx here, but it seems to work?

Suggested change
).rejects.toThrowError('does not match internal reference clientId')
).rejects.toThrowError(/does not match internal reference clientId/)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same, but based on Jest docs, RegEx and strings are equivalent in this case.

})

test('fetch an existing grant reference', async (): Promise<void> => {
const existingRef = await grantReferenceService.create({
id: uuid(),
clientId: uuid()
})

const retrievedRef = await grantReferenceService.getOrCreate(
existingRef,
AccessAction.List
)

expect(retrievedRef).toEqual(existingRef)
})

test('create a grant reference', async (): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test('create a grant reference', async (): Promise<void> => {
test('creates a grant reference', async (): Promise<void> => {

nitpick, generally naming the test as though it's doing the action: the test creates, returns, throws etc

const receivedRef = {
id: uuid(),
clientId: uuid()
}

const grantRef = await grantReferenceService.getOrCreate(
receivedRef,
AccessAction.Create
)

expect(grantRef).toEqual(receivedRef)
})
})
})
31 changes: 30 additions & 1 deletion packages/backend/src/open_payments/grantReference/service.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Transaction, TransactionOrKnex } from 'objection'
import { AccessAction } from '../auth/grant'
import { GrantReference } from './model'

export interface GrantReferenceService {
Expand All @@ -8,13 +9,18 @@ export interface GrantReferenceService {
trx?: Transaction
): Promise<GrantReference>
lock(grantId: string, trx: TransactionOrKnex): Promise<void>
getOrCreate(
options: CreateGrantReferenceOptions,
action: AccessAction
): Promise<GrantReference>
}

export async function createGrantReferenceService(): Promise<GrantReferenceService> {
return {
get: (grantId, trx) => getGrantReference(grantId, trx),
create: (options, trx) => createGrantReference(options, trx),
lock: (grantId, trx) => lockGrantReference(grantId, trx)
lock: (grantId, trx) => lockGrantReference(grantId, trx),
getOrCreate: (options, action) => getOrCreateGrantReference(options, action)
}
}

Expand Down Expand Up @@ -42,3 +48,26 @@ async function lockGrantReference(grantId: string, trx: TransactionOrKnex) {
.forNoKeyUpdate()
.timeout(5000)
}

async function getOrCreateGrantReference(
options: CreateGrantReferenceOptions,
action: AccessAction
) {
const grant = await GrantReference.transaction(async (trx: Transaction) => {
const grantRef = await getGrantReference(options.id, trx)
if (grantRef) {
if (grantRef.clientId !== options.clientId) {
throw new Error(
`clientID ${options.clientId} for grant ${options.id} does not match internal reference clientId ${grantRef.clientId}.`
)
}

return grantRef
} else if (action === AccessAction.Create) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check should be moved out to the caller.

// Grant and client ID's are only stored for create routes
return await createGrantReference(options, trx)
}
})

return grant
}