-
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(auth): use gnap error middleware on idp api #3094
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
'500': | ||
description: Internal Server Error |
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.
Would it be good to provide the actual GNAP response error objects in the IDP spec?
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'll add them since it's a pretty bare PR as it is right now
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.
We can make my comment into a separate issue, if you'd like
1735c6e
to
0282619
Compare
content: | ||
application/json: | ||
schema: | ||
$ref: '#/components/schemas/gnap-error' |
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 figured it might make sense to include this in the auth server spec as well for Open Payments, so I captured it in an issue here.
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.
Makes sense! if you want, we could probably merge the open payments issue first, bump the client, and import the yaml files in here so we can reference them as ./auth-server.yaml#/components/schemas/...
, what do you think? Unless we want to decouple the IDP spec from the auth server spec?
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.
Let's bump the OP client for this 👍
Changes proposed in this pull request
gnapServerErrorMiddleware
to the routes on the Identity Provider API on the auth server.Context
Closes #3029.
This ended up being a one-line change, as the IDP responses were added already in the spec from #850 and #3024. The code already throws these errors and had tests added as of #2400. There was one potential response missing from the spec, however.
Checklist
fixes #number
user-docs
label (if necessary)