-
Notifications
You must be signed in to change notification settings - Fork 257
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: add PKCE to 3 Legged OAuth exchange #471
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4194c5e
feat: add PKCE to 3LO exchange
bajajneha27 2c11944
fix test cases
bajajneha27 f3ece76
Fix Rubocop failures
bajajneha27 9eb647c
update README with PKCE example
bajajneha27 43c6c1c
address review comments
bajajneha27 47c421d
fix linkinator issue
bajajneha27 6cdcd21
Make generate_code_verifier a class method
bajajneha27 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What is the reason for storing the
code_verifier
in the session? I understand the code was stateless so state needs to be stored elsewhere.In the example, does it make sense to illustrate where
code_verifier
is needed again?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.
What is the reason for storing the `code_verifier` in the session? I understand the code was stateless so state needs to be stored elsewhere.
Yes, the reason behind storing
code_verifier
in the session is to make sure we maintain the state and pass on the samecode_verifier
in the subsequent call. Either we store thecode_verifier
itself in the session or we make sure that we're using the sameauthorizer
object for further call, is upto the user how they want to design it.In the example, does it make sense to illustrate where `code_verifier` is needed again?
I have done that on Line #123. I can write a comment on top and explain it further if it's not clear. WDYT ?
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.
Hmm I think the disconnect for me was in this example, why have the step of storing it in the session, instead of just setting it directly
Is the code reading it from the session somewhere, or is the user supposed to read it from the session later?
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.
The code is a property of Authorizer. But we may create a new instance of the Authorizer for the second call and lose the code.
User is supposed to store it in the session when we create it for the first time. And then retrieve it from the session and set in the Authorizer object.
So it goes like:
session[:code_verifier] ||= Google::Auth::WebUserAuthorizer.generate_code_verifier
authorizer = Google::Auth::WebUserAuthorizer.new(..,.., code_verifier: session[:code_verifier])
authorizer
passes on the code withadditional_parameter
when you callauthorizer.get_credential()
which happens hereThere 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.
Okay got it, thank you!