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

feat(backend): unique keys per wallet address #2863

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c0e4574
feat(backend): make keys unique
sabineschaller Aug 1, 2024
8378b0b
fix: only make keys unique per wallet address
sabineschaller Aug 15, 2024
705bf44
fix(frontend): It is ambiguous on what scale is the withdrawal and de…
Emanuel-Palestino Jul 31, 2024
ba203bc
chore: sync docs and readmes (#2830)
JoblersTune Jul 31, 2024
a5f953a
chore(deps): update dependency @apollo/client to ^3.11.2 (#2822)
renovate[bot] Aug 1, 2024
612f671
feat(frontend): ux improvements to liquidity dialog component (#2839)
Emanuel-Palestino Aug 5, 2024
7ef4d44
feat(docker): switch to alpine3.19 (#2842)
golobitch Aug 6, 2024
60a9148
fix(auth): interact redirect (#2832)
sabineschaller Aug 7, 2024
c3671dd
feat(interaction): return grantId (#2843)
golobitch Aug 7, 2024
d2ef6b8
feat(outgoing-payment): add grantId to admin api (#2841)
golobitch Aug 7, 2024
8f3c147
feat(auth): soft delete access tokens and grant accesses (#2837)
njlie Aug 8, 2024
3dbd870
feat(auth): set session expiry based on interaction expiry env (#2851)
njlie Aug 9, 2024
aea3361
feat(localenv): span metrics generation (#2849)
BlairCurrey Aug 9, 2024
ef8f2da
chore(deps): update dependency @types/node to ^20.14.15 (#2838)
renovate[bot] Aug 13, 2024
2bf62f3
chore(deps): update dependency @apollo/client to ^3.11.4 (#2845)
renovate[bot] Aug 13, 2024
af91a0d
feat(2737): add fees as metric for outgoing payment. (#2831)
koekiebox Aug 14, 2024
31bf57c
refactor(dependencies): axios to 1.7.4 (#2861)
golobitch Aug 15, 2024
f01fd23
chore: add tests and better error handling
sabineschaller Aug 15, 2024
6b43cef
chore: formatting
sabineschaller Aug 15, 2024
fdaf29b
Merge branch 'main' into s2-unique-key-upload
sabineschaller Aug 15, 2024
5e81569
fix: build
sabineschaller Aug 15, 2024
0653ff9
fix: add camelcase quotes and make `up` async
sabineschaller Aug 16, 2024
56aba1e
chore: keep latest version of key
sabineschaller Aug 16, 2024
83664de
fix: formatting
sabineschaller Aug 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* @param { import("knex").Knex } knex
* @returns { Promise<void> }
*/
exports.up = async function (knex) {
// delete any existing duplicates per wallet address, keep latest version
await knex.raw(`
DELETE FROM "walletAddressKeys"
WHERE ctid NOT IN (
SELECT ctid FROM (
SELECT "walletAddressId", kid, x, MAX("createdAt") AS latest_added, MAX(ctid) AS ctid
FROM "walletAddressKeys"
GROUP BY "walletAddressId", kid, x
) subquery
);
`)
Copy link
Contributor

@BlairCurrey BlairCurrey Aug 15, 2024

Choose a reason for hiding this comment

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

played around with it and it seems to be working. Had to lookup ctid but I guess it probably makes for an easier query than using the uuid.

What's the user experience going to be for the people using the keys being deleted here? I recall the discussion and read the issue but I guess I'm still not quite connecting the dots. For example, is some normal user going to see invalid signature or similar from the grant request process and have no idea they need to upload some new key? I guess probably not because they should still have a key right (just not duplicates of it)? I suppose maybe it just fixes the bug in that issue and otherwise people wont be impacted from these duplicates being deleted.

I see the error for duplicate writes in gql - that makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

The users should not have any issues since they still have another copy of the key. But this makes me think I need to check what happens when a user re-adds a key that they have previously revoked. That should be un-revoked and not throw the duplicate error. And I'll also double check that we actually delete the older versions of the key in the migration and keep the most recent.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah thats a good point... since we're not deleting these and instead setting revoked to true I imagine re-adding the previously revoked key will trigger the unique constraint error. I dont think we can just add the revoked to the unique constraint because you couldnt revoke it twice.

  1. add key, OK
  2. revoke key, OK
  3. re-add same key, OK
  4. revoke same key, NOT OK. already have a revoked key with these unique fields.

Copy link
Contributor

@BlairCurrey BlairCurrey Aug 16, 2024

Choose a reason for hiding this comment

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

Not tested, but I think a partial index like this would work:

CREATE UNIQUE INDEX wallet_address_keys_revoked_false_idx
ON "walletAddressKeys" (walletAddressId, kid, x)
WHERE revoked = false;

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really want to unrevoke the key? if we do this, we lose the information that sometime in the past, this key was revoked. I think that we need to have new entry in the database if revoked key was again added

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @golobitch re: unrevoking (we should just create a new row instead) & @BlairCurrey's partial index suggestion


return knex.schema.alterTable('walletAddressKeys', (table) => {
table.unique(['walletAddressId', 'kid', 'x'])
})
}

/**
* @param { import("knex").Knex } knex
* @returns { Promise<void> }
*/
exports.down = function (knex) {
return knex.schema.alterTable('walletAddressKeys', (table) => {
table.dropUnique(['walletAddressId', 'kid', 'x'])
})
}
67 changes: 67 additions & 0 deletions packages/backend/src/graphql/resolvers/walletAddressKey.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ import { createWalletAddress } from '../../tests/walletAddress'
import { getPageTests } from './page.test'
import { createWalletAddressKey } from '../../tests/walletAddressKey'
import { GraphQLErrorCode } from '../errors'
import {
errorToCode,
errorToMessage,
isWalletAddressKeyError,
WalletAddressKeyError
} from '../../open_payments/wallet_address/key/errors'

const TEST_KEY = generateJwk({ keyId: uuid() })

Expand Down Expand Up @@ -99,6 +105,66 @@ describe('Wallet Address Key Resolvers', (): void => {
})
})

test('Cannot add duplicate key', async (): Promise<void> => {
const walletAddress = await createWalletAddress(deps)

const input: CreateWalletAddressKeyInput = {
walletAddressId: walletAddress.id,
jwk: TEST_KEY as JwkInput
}

await walletAddressKeyService.create(input)

expect.assertions(2)

try {
await appContainer.apolloClient
.mutate({
mutation: gql`
mutation CreateWalletAddressKey(
$input: CreateWalletAddressKeyInput!
) {
createWalletAddressKey(input: $input) {
walletAddressKey {
id
walletAddressId
jwk {
kid
x
alg
kty
crv
}
revoked
createdAt
}
}
}
`,
variables: {
input
}
})
.then((query): CreateWalletAddressKeyMutationResponse => {
if (query.data) {
return query.data.createWalletAddressKey
} else {
throw new Error('Data was empty')
}
})
} catch (error) {
expect(error).toBeInstanceOf(ApolloError)
expect((error as ApolloError).graphQLErrors).toContainEqual(
expect.objectContaining({
message: errorToMessage[WalletAddressKeyError.DuplicateKey],
extensions: expect.objectContaining({
code: errorToCode[WalletAddressKeyError.DuplicateKey]
})
})
)
}
})

test('internal server error', async (): Promise<void> => {
jest
.spyOn(walletAddressKeyService, 'create')
Expand Down Expand Up @@ -170,6 +236,7 @@ describe('Wallet Address Key Resolvers', (): void => {
walletAddressId: walletAddress.id,
jwk: TEST_KEY
})
assert.ok(!isWalletAddressKeyError(key))

const response = await appContainer.apolloClient
.mutate({
Expand Down
16 changes: 14 additions & 2 deletions packages/backend/src/graphql/resolvers/walletAddressKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import { GraphQLError } from 'graphql'
import { GraphQLErrorCode } from '../errors'
import { Pagination } from '../../shared/baseModel'
import { getPageInfo } from '../../shared/pagination'
import {
errorToCode,
errorToMessage,
isWalletAddressKeyError
} from '../../open_payments/wallet_address/key/errors'

export const getWalletAddressKeys: WalletAddressResolvers<ApolloContext>['walletAddressKeys'] =
async (
Expand Down Expand Up @@ -85,10 +90,17 @@ export const createWalletAddressKey: MutationResolvers<ApolloContext>['createWal
'walletAddressKeyService'
)

const key = await walletAddressKeyService.create(args.input)
const keyOrError = await walletAddressKeyService.create(args.input)
if (isWalletAddressKeyError(keyOrError)) {
throw new GraphQLError(errorToMessage[keyOrError], {
extensions: {
code: errorToCode[keyOrError]
}
})
}

return {
walletAddressKey: walletAddressKeyToGraphql(key)
walletAddressKey: walletAddressKeyToGraphql(keyOrError)
}
}

Expand Down
21 changes: 21 additions & 0 deletions packages/backend/src/open_payments/wallet_address/key/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { GraphQLErrorCode } from '../../../graphql/errors'

export enum WalletAddressKeyError {
DuplicateKey = 'DuplicateKey'
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
export const isWalletAddressKeyError = (o: any): o is WalletAddressKeyError =>
Object.values(WalletAddressKeyError).includes(o)

export const errorToCode: {
[key in WalletAddressKeyError]: GraphQLErrorCode
} = {
[WalletAddressKeyError.DuplicateKey]: GraphQLErrorCode.Duplicate
}

export const errorToMessage: {
[key in WalletAddressKeyError]: string
} = {
[WalletAddressKeyError.DuplicateKey]: 'Key already exists'
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { createWalletAddress } from '../../../tests/walletAddress'
import { WalletAddressService } from '../service'
import { OpenPaymentsServerRouteError } from '../../route-errors'
import assert from 'assert'
import { isWalletAddressKeyError } from './errors'

const TEST_KEY = generateJwk({ keyId: uuid() })

Expand Down Expand Up @@ -52,6 +53,7 @@ describe('Wallet Address Keys Routes', (): void => {
jwk: TEST_KEY
}
const key = await walletAddressKeyService.create(keyOption)
assert.ok(!isWalletAddressKeyError(key))

const ctx = createContext<WalletAddressUrlContext>({
headers: { Accept: 'application/json' },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import assert from 'assert'
import { generateJwk } from '@interledger/http-signature-utils'
import { v4 as uuid } from 'uuid'

Expand All @@ -13,6 +14,7 @@ import { getPageTests } from '../../../shared/baseModel.test'
import { createWalletAddressKey } from '../../../tests/walletAddressKey'
import { WalletAddress } from '../model'
import { Pagination, SortOrder } from '../../../shared/baseModel'
import { isWalletAddressKeyError, WalletAddressKeyError } from './errors'

const TEST_KEY = generateJwk({ keyId: uuid() })

Expand Down Expand Up @@ -51,6 +53,18 @@ describe('Wallet Address Key Service', (): void => {
walletAddressKeyService.create(options)
).resolves.toMatchObject(options)
})

test('cannot add duplicate key to a wallet address', async (): Promise<void> => {
const options = {
walletAddressId: walletAddress.id,
jwk: TEST_KEY
}

await walletAddressKeyService.create(options)
await expect(walletAddressKeyService.create(options)).resolves.toEqual(
WalletAddressKeyError.DuplicateKey
)
})
})

describe('Fetch Wallet Address Keys', (): void => {
Expand Down Expand Up @@ -79,6 +93,8 @@ describe('Wallet Address Key Service', (): void => {

const key1 = await walletAddressKeyService.create(keyOption1)
const key2 = await walletAddressKeyService.create(keyOption2)
assert.ok(!isWalletAddressKeyError(key1))
assert.ok(!isWalletAddressKeyError(key2))
await walletAddressKeyService.revoke(key1.id)

await expect(
Expand Down Expand Up @@ -107,6 +123,7 @@ describe('Wallet Address Key Service', (): void => {
}

const key = await walletAddressKeyService.create(keyOption)
assert.ok(!isWalletAddressKeyError(key))
const revokedKey = await walletAddressKeyService.revoke(key.id)
expect(revokedKey).toEqual({
...key,
Expand All @@ -129,6 +146,7 @@ describe('Wallet Address Key Service', (): void => {
}

const key = await walletAddressKeyService.create(keyOption)
assert.ok(!isWalletAddressKeyError(key))

const revokedKey = await walletAddressKeyService.revoke(key.id)
await expect(walletAddressKeyService.revoke(key.id)).resolves.toEqual(
Expand Down
32 changes: 24 additions & 8 deletions packages/backend/src/open_payments/wallet_address/key/service.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { TransactionOrKnex } from 'objection'
import { TransactionOrKnex, UniqueViolationError } from 'objection'

import { WalletAddressKey } from './model'
import { BaseService } from '../../../shared/baseService'
import { JWK } from '@interledger/http-signature-utils'
import { Pagination, SortOrder } from '../../../shared/baseModel'
import { WalletAddressKeyError } from './errors'

export interface WalletAddressKeyService {
create(options: CreateOptions): Promise<WalletAddressKey>
create(
options: CreateOptions
): Promise<WalletAddressKey | WalletAddressKeyError>
revoke(id: string): Promise<WalletAddressKey | undefined>
getPage(
walletAddressId: string,
Expand Down Expand Up @@ -52,12 +55,25 @@ export interface CreateOptions {
async function create(
deps: ServiceDependencies,
options: CreateOptions
): Promise<WalletAddressKey> {
const key = await WalletAddressKey.query(deps.knex).insertAndFetch({
walletAddressId: options.walletAddressId,
jwk: options.jwk
})
return key
): Promise<WalletAddressKey | WalletAddressKeyError> {
try {
const key = await WalletAddressKey.query(deps.knex).insertAndFetch({
walletAddressId: options.walletAddressId,
jwk: options.jwk
})
return key
} catch (err) {
if (err instanceof UniqueViolationError) {
return WalletAddressKeyError.DuplicateKey
}
deps.logger.error(
{
err
},
'error adding key'
)
throw err
}
}

async function revoke(
Expand Down
7 changes: 6 additions & 1 deletion packages/backend/src/tests/walletAddressKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { WalletAddressKey } from '../open_payments/wallet_address/key/model'
import { CreateOptions } from '../open_payments/wallet_address/key/service'
import { v4 as uuidv4 } from 'uuid'
import { generateJwk, generateKey } from '@interledger/http-signature-utils'
import { isWalletAddressKeyError } from '../open_payments/wallet_address/key/errors'
export async function createWalletAddressKey(
deps: IocContract<AppServices>,
walletAddressId: string
Expand All @@ -18,5 +19,9 @@ export async function createWalletAddressKey(
})
}

return walletAddressKeyService.create(options)
const keyOrError = await walletAddressKeyService.create(options)
if (isWalletAddressKeyError(keyOrError)) {
throw keyOrError
}
return keyOrError
}
Loading