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(auth): return GNAP 404 error if token cannot be rotated #3101

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(auth): return GNAP 404 error if token cannot be rotated #3101

wants to merge 1 commit into from
+3 −3

Conversation

golobitch
Copy link
Collaborator

@golobitch golobitch commented Nov 17, 2024 •
edited
Loading

Changes proposed in this pull request

  • Instead of returning 400 return 404 if token cannot be rotated

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Make sure that all checks pass
  • Bruno collection updated (if necessary)
  • Documentation issue created with user-docs label (if necessary)
  • OpenAPI specs updated (if necessary)

@golobitch golobitch self-assigned this Nov 17, 2024
@golobitch golobitch linked an issue Nov 17, 2024 that may be closed by this pull request
Authorization Server should throw a GNAPServerRouteError if invalid token is used during rotation #3100
Open
@golobitch golobitch requested a review from njlie November 17, 2024 08:26
@github-actions github-actions bot added type: tests Testing related type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels Nov 17, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 17, 2024 •
edited
Loading

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

Name Link
🔨 Latest commit f053a0c
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6739a9159d289100089d100a

njlie
njlie approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@njlie njlie left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up. For context, this is to match the spec update here: interledger/open-payments#518

mkurapov
mkurapov reviewed Nov 19, 2024
View reviewed changes
packages/auth/src/accessToken/routes.ts
@@ -164,7 +164,7 @@ async function rotateToken(
const errorMessage =
error instanceof Error ? error.message : 'Could not rotate token'
throw new GNAPServerRouteError(
400,
404,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we throw in the other service methods that may lead to this 404? or do we just want this error message for the if (!newToken) case?

Copy link
Contributor

Choose a reason for hiding this comment

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

CC @njlie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njlie njlie njlie approved these changes

@mkurapov mkurapov mkurapov approved these changes

Assignees

@golobitch golobitch

Labels
pkg: auth Changes in the GNAP auth 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.

Authorization Server should throw a GNAPServerRouteError if invalid token is used during rotation
3 participants
@golobitch @mkurapov @njlie