-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add PKCE support #2652
Add PKCE support #2652
Conversation
Thanks for the PR @DASPRiD! Could you please take a look at the breaking tests? Please mark as ready to review again once fixed up. Thanks! |
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.
Apologies for the delayed review! Thank you for working through this feature, I'm looking forward to get it in. 😄
A couple of questions and one minor change, but otherwise LGTM. 👏
id="use-pkce" | ||
onClick={onChange} | ||
value={authentication.usePkce} | ||
title={authentication.usePkce ? 'Disable PKCE' : 'Enable PKCE'}> |
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.
Strange, it seems the Button
component doesn't use the title
prop for anything.
It might have been removed accidentally because there are a lot of instances that provide the title
prop. No need to change this line, we should fix button to consume it. 😄
This is more a note, no need to action it in this PR.
packages/insomnia-app/app/network/o-auth-2/grant-authorization-code.js
Outdated
Show resolved
Hide resolved
…-code.js Co-authored-by: Opender Singh <opender94@gmail.com>
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.
LGTM
…-code.js Co-authored-by: Opender Singh <opender94@gmail.com>
Your suggested change broke prettier D: |
Fixed the prettier report. For some reason the MacOS build is failing now, but that seems to be build server related… shrugs |
No probs, I'll have a look at what's breaking there, should be fine though. |
Can anyone confirm if this impl works with Azure AD? EDIT: Confirmed myself, make sure to specify |
This is a small change to support PKCE with authentication code authorization.
Closes #1653
Closes #2154