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

chore(token-introspection): return client payment pointer, not key #1007

Merged
merged 6 commits into from
Jan 25, 2023

Conversation

wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb commented Jan 20, 2023

Changes proposed in this pull request

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: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: http-signature-utils type: source Changes business logic type: tests Testing related labels Jan 20, 2023
@wilsonianb wilsonianb marked this pull request as ready for review January 20, 2023 20:15
mkurapov
mkurapov previously approved these changes Jan 24, 2023
@@ -171,7 +171,7 @@ export abstract class PaymentPointerSubresource extends BaseModel {
public abstract readonly assetId: string
public abstract asset: Asset

public readonly clientId?: string
public readonly client?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that we wouldn't always want to have this defined here because of subresource lookups in the local service (the service making the request itself)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A payment pointer subresource won't have a client if it was created via the admin api instead of Open Payments

// clientId to ctx for Read/List filtering
ctx.clientId = tokenInfo.client_id
// filter by client for Read/List
ctx.filterClient = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ctx.filterClient = true
ctx.filterByClient = true

@@ -42,7 +42,7 @@ async function getOutgoingPayment(
try {
outgoingPayment = await deps.outgoingPaymentService.get({
id: ctx.params.id,
clientId: ctx.clientId,
client: ctx.filterClient ? ctx.client : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we not want client defined in routes > get methods?

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 (isActiveTokenInfo(tokenInfo) && tokenAction === requestAction) {
// Unless the relevant token action is ReadAll/ListAll add the
// clientId to ctx for Read/List filtering
ctx.clientId = tokenInfo.client_id

^ previous naming with same behavior
We pass client as undefined to get and list methods when the grant allows ReadAll/ListAll, meaning we don't need to filter the query by client.

// clientId to ctx for Read/List filtering
ctx.clientId = tokenInfo.client_id
// filter by client for Read/List
ctx.filterClient = true
Copy link
Contributor

Choose a reason for hiding this comment

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

What if this logic lived at the route level, where we check there if the AccessAction === ReadAll || ListAll there? Can store accessAction or the whole tokenInfo on the ctx itself?

Just a thought

@wilsonianb wilsonianb merged commit 515a525 into main Jan 25, 2023
@wilsonianb wilsonianb deleted the bw-rm-client-key branch January 25, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return client payment pointer in introspection response
2 participants