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.

Sign up for GitHub

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

Jump to bottom

feat(backend): serve jwks.json from client payment pointer #758

Merged
merged 5 commits into from
Nov 22, 2022
Merged

feat(backend): serve jwks.json from client payment pointer #758

merged 5 commits into from
Nov 22, 2022

Conversation

wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb commented Nov 19, 2022 •
edited
Loading

Changes proposed in this pull request

  • Configure the RS's client payment pointer and serve its jwk (derived from the configured private key)

Context

Checklist

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

@github-actions github-actions bot added pkg: backend Changes in the backend package. pkg: open-payments type: source Changes business logic type: tests Testing related labels Nov 19, 2022
wilsonianb
wilsonianb commented Nov 19, 2022
View reviewed changes
packages/open-payments/src/client/index.ts Outdated
@@ -125,7 +126,8 @@ export const createAuthenticatedClient = async (
grant: createGrantRoutes({
axiosInstance,
openApi: authServerOpenApi,
logger
logger,
client: args.client
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to have createAuthenticatedClient query ${client}/jwks.json to validate that the key with the specified keyid is served there, but backend calls createAuthenticatedClient long before its routes are set up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was thinking you'd possibly get some kind of race condition type behaviour - could add some kind of retry mechanism but gets too complicated for what it's worth.

Is there a more straightforward name than client? Even just paymentPointerUrl? or clientPaymentPointer?

@wilsonianb wilsonianb marked this pull request as ready for review November 19, 2022 00:42
mkurapov
mkurapov reviewed Nov 19, 2022
View reviewed changes
packages/open-payments/src/client/index.ts Outdated
@@ -125,7 +126,8 @@ export const createAuthenticatedClient = async (
grant: createGrantRoutes({
axiosInstance,
openApi: authServerOpenApi,
logger
logger,
client: args.client
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was thinking you'd possibly get some kind of race condition type behaviour - could add some kind of retry mechanism but gets too complicated for what it's worth.

Is there a more straightforward name than client? Even just paymentPointerUrl? or clientPaymentPointer?

packages/backend/src/open_payments/payment_pointer/middleware.ts Outdated
ctx.throw(404)
const url = `https://${ctx.request.host}/${ctx.params.paymentPointerPath}`
const config = await ctx.container.use('config')
if (url !== config.paymentPointerUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In which situations will this equality happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this is an Open Payments resource request for the RS's own payment pointer, which will now/soon be happening when a separate AS is querying RS's client payment pointer /jwks.json

mkurapov
mkurapov previously approved these changes Nov 21, 2022
View reviewed changes
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

Few comments, otherwise LGTM 👍

packages/backend/src/paymentPointerKey/routes.ts Outdated
Comment on lines 50 to 51
deps.config.paymentPointerUrl ===
`https://${ctx.request.host}/${ctx.params.paymentPointerPath}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be made into a function we can use across this package? Any function name will also help with the self-commenting of the code IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to be able to do this via a paymentPointerUrl getter on AppContext(?), but I'm struggling to pull it off.

packages/open-payments/src/client/index.ts
@@ -93,6 +93,7 @@ export interface CreateAuthenticatedClientArgs
extends CreateUnauthenticatedClientArgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of outside the scope of this PR, but I'm a bit confused why TS doesn't complain about

const axiosInstance = createAxiosInstance({
    privateKey: args.privateKey,
    keyId: args.keyId,
    requestTimeoutMs:
      args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS
}

when privateKey and keyId are optional in createUnauthenticatedClient.

Copy link
Contributor Author

@wilsonianb wilsonianb Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this due to

rafiki/tsconfig.build.json

Line 28 in 2f17be3

"strict": false /* Enable all strict type-checking options. */,

which was toggled in

@matdehaast @traviscrist Do either of you remember why we disabled strict type checking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wilsonianb I think its was due to it not being run properly so when run it was showing tons of issues and we didn't want to fix them all when getting pnpm setup. If you enable it how many issues does it find?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you enable it how many issues does it find?

tbd

@wilsonianb wilsonianb mentioned this pull request Nov 21, 2022
Closed
4 tasks
@wilsonianb wilsonianb merged commit cb3940b into main Nov 22, 2022
@wilsonianb wilsonianb deleted the bw-rs-jwks branch November 22, 2022 12:57
This was referenced Nov 23, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traviscrist traviscrist traviscrist left review comments

@mkurapov mkurapov mkurapov approved these changes

@omertoast omertoast Awaiting requested review from omertoast

@sabineschaller sabineschaller Awaiting requested review from sabineschaller

@njlie njlie Awaiting requested review from njlie

Assignees
No one assigned
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wilsonianb @traviscrist @mkurapov