feat(backend): start of handling grant token rotation more gracefully #2887

mkurapov · 2024-08-22T13:48:49Z

Changes proposed in this pull request

Adding getOrCreate and delete methods to backend grant service:
- getOrCreate method:
  - soft deletes a grant if token rotation fails
  - uses the open payments client to request grant and store it instead of having it be the responsibility of the caller
- delete method
  - allows callers to soft delete a grant if necessary
- these methods are still unused at this point, I will be updating callers to use them in a later PR, so we can break up the changes
Adding deletedAt field to the grants table to allow soft deletion, as well as dropping the unique constraint on authServerId accessType and accessActions such that we can properly soft delete problematic grants

Context

This is the first part of Try to re-request a grant if token rotation fails during remote incoming payment creation #2852:
- We are making the grant service more resilient to edge cases (like being unable to rotate tokens if they are missing from the receivers Rafiki DB)

Checklist

netlify · 2024-08-22T13:49:04Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`4bff426`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66c88f7043f9a3000747598b

mkurapov · 2024-08-22T19:41:05Z

packages/backend/migrations/20240820101148_drop_unique_grants.js

+ */
+exports.up = function (knex) {
+  return knex.schema.alterTable('grants', function (table) {
+    table.dropUnique(['authServerId', 'accessType', 'accessActions'])


Dropping the unique constraint on authServerId accessType and accessActions such that we can properly soft delete grants (see migration below)

seems similar to this case? #2863 (comment)

So could we use a partial unique index (where deletedAt is not null)?

I don't think this unique constraint is very useful to begin with, particularly because order matters in accessActions array, meaning [Create, Read] is different from [Read, Create], even though they are the same in terms of how we deal with them in code

mkurapov · 2024-08-22T19:41:37Z

packages/backend/src/app.ts

+  grantService: Promise<GrantService>
+  authServerService: Promise<AuthServerService>


weren't included before, adding here so we get proper types

easy to miss this

mkurapov · 2024-08-22T19:41:50Z

packages/backend/src/open_payments/grant/service.ts

+  getOrCreate(options: GrantOptions): Promise<Grant | GrantError>
+  delete(id: string): Promise<Grant>


still unused

mkurapov · 2024-08-22T19:43:56Z

packages/backend/src/open_payments/grant/service.ts

+    })
+    .whereNull('deletedAt')
+    .andWhere('authServer.url', options.authServer)
+    .andWhere('accessActions', '@>', options.accessActions) // all options.accessActions are a subset of saved accessActions


e.g. if we have [AccessActions.Create, AccessActions.Complete, AccessActions.Read] and we are trying to getOrCreate a grant with [AccessActions.Create] we will properly return it, but if one of the elements in the requested list doesnt exist, we will not be able to fetch it

It looks like this differs slightly from the getGrant which finds one that has strictly equal accessActions. Its not quite clear what the ramifications of this will be. Can you speak to this difference? ie that its intentional, working as intended, etc.

before, if we already had a grant for [Read, Create] and then we were trying to getOrCreate a grant with [Read], it would fail to get the existing one, even though the existing grant has the sufficient access requirements

I suppose we're getting rid of the getGrant here then (in subsequent PR)? looks like its unused but if we keep it for some reason I guess we should update to filter out by deletedAt (and maybe this same accessActions where clause). But I imagine we can just remove it.

Yes exactly, will be removing the unused methods

…tions

njlie · 2024-08-23T11:58:03Z

packages/backend/migrations/20240820101201_add_deleted_at_grants.js

@@ -0,0 +1,19 @@
+/**


nit: why not combine this migration with drop_unique_grants?

Yup, makes sense for this one, let me do that

njlie · 2024-08-23T12:27:40Z

packages/backend/src/open_payments/grant/service.test.ts

+      ).resolves.toEqual({ ...existingGrant, authServer })
+    })
+
+    test('gets existing grant (requested actions are a subset of saved actions)', async () => {


Would be necessary to create another test (or modify this one) to ensure that it can retrieve existing grants that have (Read/List)-All rights when (Read/List) is requested?

good point, maybe during create if ReadAll/ListAll is requested, we can also manually add the corresponding Read/List as well

njlie

LGTM

mkurapov added 2 commits August 22, 2024 15:47

feat(backend): add grantService.getOrCreate method

a16e8fa

feat(backend): update grant lookup call

96a1d7c

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Aug 22, 2024

feat(backend): add deletedAt to grants table

c913df5

mkurapov changed the title ~~feat(backend): adding grantService.getOrCreate method~~ feat(backend): adding additional methods in grantService Aug 22, 2024

feat(backend): add migrations for grants table

27fc5aa

mkurapov changed the title ~~feat(backend): adding additional methods in grantService~~ feat(backend): start of handling grant token rotation more gracefully Aug 22, 2024

mkurapov added 2 commits August 22, 2024 21:39

chore(backend): separating into functions

c192197

chore(backend): adding authServerService to list of services

af51d1b

github-actions bot added the type: tests Testing related label Aug 22, 2024

mkurapov commented Aug 22, 2024

View reviewed changes

mkurapov force-pushed the 2852/mk/get-or-create-ip-grants branch from 20c21b2 to 9dcbf78 Compare August 22, 2024 19:44

test(backend): adding tests for getOrCreate and getExistingGrant func…

b21d89e

…tions

mkurapov force-pushed the 2852/mk/get-or-create-ip-grants branch from 9dcbf78 to b21d89e Compare August 22, 2024 19:45

mkurapov marked this pull request as ready for review August 22, 2024 19:45

njlie reviewed Aug 23, 2024

View reviewed changes

chore(backend): merge migrations

8e49521

mkurapov requested review from BlairCurrey and njlie August 23, 2024 12:13

njlie reviewed Aug 23, 2024

View reviewed changes

mkurapov requested a review from njlie August 23, 2024 13:24

feat(backend): add subset actions if present

4bff426

mkurapov force-pushed the 2852/mk/get-or-create-ip-grants branch from 5a69214 to 4bff426 Compare August 23, 2024 13:32

njlie approved these changes Aug 23, 2024

View reviewed changes

mkurapov mentioned this pull request Aug 23, 2024

refactor(backend): handle grant lookup more gracefully in remote incoming payment service #2888

Merged

5 tasks

mkurapov merged commit 175a7cf into main Aug 23, 2024
42 checks passed

mkurapov deleted the 2852/mk/get-or-create-ip-grants branch August 23, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): start of handling grant token rotation more gracefully #2887

feat(backend): start of handling grant token rotation more gracefully #2887

mkurapov commented Aug 22, 2024 •

edited

Loading

netlify bot commented Aug 22, 2024 •

edited

Loading

mkurapov Aug 22, 2024

BlairCurrey Aug 22, 2024

mkurapov Aug 23, 2024 •

edited

Loading

mkurapov Aug 22, 2024

BlairCurrey Aug 22, 2024

mkurapov Aug 22, 2024

mkurapov Aug 22, 2024

BlairCurrey Aug 22, 2024 •

edited

Loading

mkurapov Aug 22, 2024

BlairCurrey Aug 23, 2024

mkurapov Aug 23, 2024

njlie Aug 23, 2024

mkurapov Aug 23, 2024 •

edited

Loading

njlie Aug 23, 2024

mkurapov Aug 23, 2024

njlie left a comment

		grantService: Promise<GrantService>
		authServerService: Promise<AuthServerService>

		getOrCreate(options: GrantOptions): Promise<Grant \| GrantError>
		delete(id: string): Promise<Grant>

feat(backend): start of handling grant token rotation more gracefully #2887

feat(backend): start of handling grant token rotation more gracefully #2887

Conversation

mkurapov commented Aug 22, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Aug 22, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie left a comment

Choose a reason for hiding this comment

mkurapov commented Aug 22, 2024 •

edited

Loading

netlify bot commented Aug 22, 2024 •

edited

Loading

mkurapov Aug 23, 2024 •

edited

Loading

BlairCurrey Aug 22, 2024 •

edited

Loading

mkurapov Aug 23, 2024 •

edited

Loading