feat(auth): check that token value matches during rotation & revocation

[njlie]

Changes proposed in this pull request

• Checks the token value during revocation and rotation.
• Adds some in-line issues in the comments.

Context

Closes #832.

When rotating and revoking a token, the provided management id in the URL was used to retrieve the token, but the token value that's provided during these requests wasn't being verified against the token that was retrieved.

Also, some TODOs did not have an associated Github issue attached to them. These have been created and their URIs have been added to the original associted in-line comments.

Checklist

• Related issues linked using fixes #number
• Tests added/updated
• Documentation added
• Make sure that all checks pass

[wilsonianb]

Would it be possible to check access tokens in a separate middleware (maybe the signature middleware)?
The GNAP spec talks about binding client keys to access tokens (both access token and continue access tokens). Could client and clientKeyId (on its way out) and maybe even continue details be moved off of grants, which would avoid doing grant service lookups in signature middleware

rafiki/packages/auth/src/signature/middleware.ts

Line 151 in 83a6a54

const grant = await grantService.get(accessToken.grantId)

rafiki/packages/auth/src/signature/middleware.ts

Lines 84 to 89 in 83a6a54

	const grantService = await ctx.container.use('grantService')
	const grant = await grantService.getByContinue(
	ctx.params['id'],
	continueToken,
	interactRef
	)

[njlie]

Moving client & clientKeyId into a new table would make sense since they may be bound to multiple grants anyways. In the case of continue details it can be done but I think it's a little trickier to justify because it'd punt the call to a different, more narrow service instead since they're one-to-one with grants.

What service calls are "in-scope" for the signature (or some other) middleware? As I understand it it seems like the idea is to just lookup via the client service if the token/grant continuation is associated with the client via its bound key.

[wilsonianb]

Maybe there can be path specific auth middlewares (ahead of the signature middleware) that store client on ctx like the backend currently does (via the grant)

rafiki/packages/backend/src/open_payments/auth/middleware.ts

Line 63 in ec15723

ctx.grant = grant

Some of these could store other route specific data like the access token or the grant on the ctx. The backend does this with payment pointer routes

rafiki/packages/backend/src/open_payments/payment_pointer/middleware.ts

Line 20 in ec15723

ctx.paymentPointer = paymentPointer

rafiki/packages/backend/src/open_payments/payment_pointer/routes.ts

Lines 26 to 37 in ec15723

	export async function getPaymentPointer(
	deps: ServiceDependencies,
	ctx: PaymentPointerContext
	): Promise<void> {
	if (!ctx.paymentPointer) {
	return ctx.throw(404)
	}
	ctx.body = ctx.paymentPointer.toOpenPaymentsType({
	authServer: deps.authServer
	})
	}

Then there can be a single signature middleware.

[njlie]

Going to capture this for now in #882, in case I don't get to it before the holidays.

[wilsonianb]

I think the findOne and delete should otherwise use a lock.

  const oldToken = await AccessToken.query(deps.knex)
    .delete()
    .returning('*')
    .findOne({
      managementId,
      value: tokenValue
    })
  if (oldToken) {
    const token = await AccessToken.query(deps.knex).insertAndFetch({

The order of the delete/returning/findOne is a workaround for:

• New types in v2 have some issues with single/multiple return types, e.g. with returning("*") Vincit/objection.js#1649 (comment)

[wilsonianb]

🤔
Maybe this should all happen inside a transaction though.
Both the delete and insert.

[wilsonianb]

  await AccessToken.query(deps.knex)
    .findOne({
      managementId: id,
      value: tokenValue
    })
    .delete()

[wilsonianb]

What about

Suggested change

	): Promise<Rotation> {
	): Promise<Token | undefined> {

instead of the success, and let the calling route format the response body

[wilsonianb]

Or everyone's favorite

Suggested change

	): Promise<Rotation> {
	): Promise<Token | TokenError> {

a la

rafiki/packages/backend/src/open_payments/payment/incoming/service.ts

Line 44 in 40af362

complete(id: string): Promise<IncomingPayment | IncomingPaymentError>

to enable the route to return 404 for unmatched management id and 401 for unmatched token value (which would require re-splitting up my magic delete/returning query).
But maybe we just wait for:

• Bind access token to clients; verify binding in middlewares for token requests #882

[wilsonianb]

I prefer 👇 so we don't have to manage trx.commit/rollback

Suggested change

	const trx = await AccessToken.startTransaction()
	try {
	try {
	return await AccessToken.transaction(async trx => {

[wilsonianb]

I think this is a single query and doesn't need a transaction