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
[Feature] OpenID response type selection for OAuth 2.0 implicit mode #681
Conversation
💖 Thanks for opening this pull request! 💖 To help make this a smooth process, please be sure you have first read the |
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.
Awesome, just one minor issue @emdfonseca. Thanks for contributing! 😄
@@ -9,30 +9,33 @@ export default async function ( | |||
clientId: string, | |||
redirectUri: string = '', | |||
scope: string = '', | |||
state: string = '' | |||
state: string = '', | |||
responseType: string = '' |
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.
response_type
is a required OAuth 2.0 parameter so this argument default should probably be changed from ''
to c.RESPONSE_TYPE_TOKEN
;
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.
And line 15 can be reverted.
_parseUrl(currentUrl); | ||
}); | ||
|
||
child.webContents.on('did-fail-load', (e, errorCode, errorDescription, url) => { |
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.
Nice 👍
Hi @gschier, thanks for the feedback! I made the changes as requested. I left the name of the constant as RESPONSE_TYPE_ACCESS instead of RESPONSE_TYPE_TOKEN. This helps avoiding confusion since you can get both the 'id_token' and 'access_token' on openid connect servers. Is that ok? |
Hi @gschier, any additional changes needed in order to get this PR merged? |
Sorry, haven't had time to look at it again. It's probably good enough to merge at this point. I'll try to get it merge this week. |
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.
looks good @emdfonseca! Thanks 😄
Congrats on merging your first pull request! 🎉🎉🎉 You're helping make Insomnia awesome! 🙌 |
…ng#681) * Enable response type selection for oauth2 implicit * Parse token even if callback server is unreachable * Fix style: add _ prefix to private method * Fix style: reorder private method declaration * Set OAuth 2.0 default responseType value to token * Add responseType to params * Fix response type constant value * Code styling * Fix authorization request parameters * Don't open dev tools
This PR adds support for OpenID Connect by allowing the configuration of the OAuth 2 (implicit grant) response type. In the advanced options a new dropdown was added requesting the user to select whether he wants to receive the id_token, the access_token or both. By default the access_token is chosen (oauth 2 default). However, the user can also select to receive only the id_token or both the id_token and the access_token to be OpenID Connect compliant.
This PR closes an already closed issue (#501). The issue was previously closed due to inactivity but the solution is not yet available. This PR should reopen the issue for further discussions and, hopefully, help improving insomnia with this new feature.