Skip to content
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 7 commits into from
Feb 8, 2024

Conversation

bajajneha27
Copy link
Contributor

@bajajneha27 bajajneha27 commented Jan 29, 2024

b/270203889

@bajajneha27 bajajneha27 marked this pull request as ready for review January 30, 2024 07:55
@bajajneha27 bajajneha27 requested a review from a team as a code owner January 30, 2024 07:55
@bajajneha27 bajajneha27 changed the title feat: add PKCE to 3LO exchange feat: add PKCE to 3 Legged OAuth exchange Jan 30, 2024
README.md Outdated
Comment on lines 120 to 123
request.session['code_verifier'] ||= authorizer.generate_code_verifier
authorizer.code_verifier = request.session['code_verifier']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this how a user can generate the code verifier? It may be useful to give a short overview on what PKCE is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Cool, added an overview.

# Random string of 43-128 chars used to verify the key exchange using
# PKCE. Auto-generated if not provided
def initialize client_id, scope, token_store,
callback_uri = nil, code_verifier = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using keyword arguments for additional optional arguments when the list gets this long. (I also recommend deprecating the existing callback_uri optional argument and providing a keyword argument replacement.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code_verifier to be a keyword argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll raise another PR for deprecating callback_uri .

def initialize client_id, scope, token_store, callback_uri = nil
# @param [String] code_verifier
# Random string of 43-128 chars used to verify the key exchange using
# PKCE. Auto-generated if not provided
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code doesn't look like it "auto-generates" the verifier if not provided. It looks like it simply doesn't include the code challenge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes! That was the implementation I was trying to follow but it doesn't work given the way our 3LO code is implemented. Removed that.


# Generate the code verifier needed to be sent while fetching
# authorization URL.
def generate_code_verifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a class method instead of an instance method? If it's an instance method, it kinda implies it might set @code_verifier which it doesn't.

Copy link
Contributor Author

@bajajneha27 bajajneha27 Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking which will be a better implementation. Class method makes more sense here and I've changed it accordingly.

# authorization URL.
def generate_code_verifier
random_number = rand 32..96
SecureRandom.alphanumeric(random_number).to_str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SecureRandom.alphanumeric returns a string. Do we need to call to_str on the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't. Missed that.

@bajajneha27
Copy link
Contributor Author

Hey @clundin25 , I have addressed all the review comments. Can you please take a look at it again?

user_id = request.session['user_id']
# User needs to take care of generating the code_verifier and storing it in
# the session.
request.session['code_verifier'] ||= Google::Auth::WebUserAuthorizer.generate_code_verifier

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?

Copy link
Contributor Author

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 same code_verifier in the subsequent call. Either we store the code_verifier itself in the session or we make sure that we're using the same authorizer 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 ?

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?

Copy link
Contributor Author

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

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.

Is the code reading it from the session somewhere, or is the user supposed to read it from the session later?

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:

  1. Generate a new code ( if it's not already there in the session ) and store it in the session for future call.
    session[:code_verifier] ||= Google::Auth::WebUserAuthorizer.generate_code_verifier
  2. Assign it to Authorizer.
    authorizer = Google::Auth::WebUserAuthorizer.new(..,.., code_verifier: session[:code_verifier])
  3. authorizer passes on the code with additional_parameter when you call authorizer.get_credential() which happens here

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!

@bajajneha27 bajajneha27 merged commit 65bae29 into googleapis:main Feb 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants