-
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): handle grant choice params correctly #671
Conversation
@@ -445,7 +445,7 @@ describe('Grant Routes', (): void => { | |||
) | |||
|
|||
const redirectUrl = new URL(config.identityServerDomain) | |||
redirectUrl.searchParams.set('interactRef', grant.interactRef) | |||
redirectUrl.searchParams.set('interactId', grant.interactId) |
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.
These were just changed in
{ | ||
id: grant.interactId, | ||
nonce: grant.interactNonce, | ||
choice: GrantChoices.Accept |
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.
Add test for non-GrantChoices
choice
a8a6dc5
to
ff2502f
Compare
@@ -265,15 +269,19 @@ async function acceptGrant( | |||
ctx.body = { | |||
error: 'request_denied' | |||
} | |||
return |
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.
Not related to the changes, but is it ok to use status = 400 above on L268 even though it's not listed as a possible returned status code in the Open API 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.
Some of this stuff (to my recollection) was written before the OpenAPI spec was brought in, so it's more like an initial stab. There could be something to adding a response to the spec for unexpected stuff, but later I will go over all these responses and decide if they should conform to an existing response or a new one should be added to the spec.
Changes proposed in this pull request
/accept
or/reject
grant endpoints as a query param, so the handler has been modified to reflect that.Context
Checklist
fixes #number