-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat(backend): start of handling grant token rotation more gracefully #2887
Changes from 7 commits
a16e8fa
96a1d7c
c913df5
27fc5aa
c192197
af51d1b
b21d89e
8e49521
4bff426
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* @param { import("knex").Knex } knex | ||
* @returns { Promise<void> } | ||
*/ | ||
exports.up = function (knex) { | ||
return knex.schema.alterTable('grants', function (table) { | ||
table.dropUnique(['authServerId', 'accessType', 'accessActions']) | ||
}) | ||
} | ||
|
||
/** | ||
* @param { import("knex").Knex } knex | ||
* @returns { Promise<void> } | ||
*/ | ||
exports.down = function (knex) { | ||
return knex.schema.alterTable('grants', function (table) { | ||
table.unique(['authServerId', 'accessType', 'accessActions']) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why not combine this migration with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, makes sense for this one, let me do that |
||
* @param { import("knex").Knex } knex | ||
* @returns { Promise<void> } | ||
*/ | ||
exports.up = function (knex) { | ||
return knex.schema.alterTable('grants', (table) => { | ||
table.timestamp('deletedAt').nullable() | ||
}) | ||
} | ||
|
||
/** | ||
* @param { import("knex").Knex } knex | ||
* @returns { Promise<void> } | ||
*/ | ||
exports.down = function (knex) { | ||
return knex.schema.alterTable('grants', (table) => { | ||
table.dropColumn('deletedAt') | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,6 +98,8 @@ import { | |
} from './open_payments/wallet_address/middleware' | ||
|
||
import { LoggingPlugin } from './graphql/plugin' | ||
import { GrantService } from './open_payments/grant/service' | ||
import { AuthServerService } from './open_payments/authServer/service' | ||
export interface AppContextData { | ||
logger: Logger | ||
container: AppContainer | ||
|
@@ -232,6 +234,8 @@ export interface AppServices { | |
incomingPaymentService: Promise<IncomingPaymentService> | ||
remoteIncomingPaymentService: Promise<RemoteIncomingPaymentService> | ||
receiverService: Promise<ReceiverService> | ||
grantService: Promise<GrantService> | ||
authServerService: Promise<AuthServerService> | ||
Comment on lines
+237
to
+238
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. weren't included before, adding here so we get proper types There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. easy to miss this |
||
streamServer: Promise<StreamServer> | ||
webhookService: Promise<WebhookService> | ||
quoteService: Promise<QuoteService> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export enum GrantError { | ||
GrantRequiresInteraction = 'GrantRequiresInteraction', | ||
InvalidGrantRequest = 'InvalidGrantRequest' | ||
} | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types | ||
export const isGrantError = (o: any): o is GrantError => | ||
Object.values(GrantError).includes(o) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping the unique constraint on authServerId accessType and accessActions such that we can properly soft delete grants (see migration below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems similar to this case? #2863 (comment)
So could we use a partial unique index (where
deletedAt
is not null)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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