feat: add PKCE to 3 Legged OAuth exchange #471

bajajneha27 · 2024-01-29T13:56:21Z

b/270203889

clundin25 · 2024-01-30T17:49:16Z

README.md

+  request.session['code_verifier'] ||= authorizer.generate_code_verifier
+  authorizer.code_verifier = request.session['code_verifier']


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

Yes. Cool, added an overview.

dazuma · 2024-01-31T18:06:58Z

lib/googleauth/user_authorizer.rb

+      #  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


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.)

Updated code_verifier to be a keyword argument.

I'll raise another PR for deprecating callback_uri .

dazuma · 2024-01-31T18:12:44Z

lib/googleauth/user_authorizer.rb

-      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


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.

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.

dazuma · 2024-01-31T18:14:16Z

lib/googleauth/user_authorizer.rb

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


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.

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

dazuma · 2024-01-31T18:14:45Z

lib/googleauth/user_authorizer.rb

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


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

No, we don't. Missed that.

bajajneha27 · 2024-02-05T06:59:37Z

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

clundin25 · 2024-02-05T16:23:12Z

README.md

+  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


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?

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 ?

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?

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:

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

Assign it to Authorizer.
authorizer = Google::Auth::WebUserAuthorizer.new(..,.., code_verifier: session[:code_verifier])

authorizer passes on the code with additional_parameter when you call authorizer.get_credential() which happens here

Okay got it, thank you!

bajajneha27 marked this pull request as ready for review January 30, 2024 07:55

bajajneha27 requested a review from a team as a code owner January 30, 2024 07:55

bajajneha27 changed the title ~~feat: add PKCE to 3LO exchange~~ feat: add PKCE to 3 Legged OAuth exchange Jan 30, 2024

clundin25 reviewed Jan 30, 2024

View reviewed changes

dazuma reviewed Jan 31, 2024

View reviewed changes

bajajneha27 force-pushed the b/270203889 branch from 7e6b9dc to 556c6c0 Compare February 1, 2024 07:43

dazuma approved these changes Feb 1, 2024

View reviewed changes

This was referenced Feb 4, 2024

fix: Client library credentials provide correct self-signed JWT and external account behavior when loading from a file path or JSON data #474

Merged

chore(main): release googleauth 1.10.0 #473

Merged

bajajneha27 force-pushed the b/270203889 branch from b10b100 to bcb501d Compare February 5, 2024 07:04

clundin25 reviewed Feb 5, 2024

View reviewed changes

clundin25 approved these changes Feb 6, 2024

View reviewed changes

bajajneha27 added 7 commits February 8, 2024 07:04

feat: add PKCE to 3LO exchange

4194c5e

fix test cases

2c11944

Fix Rubocop failures

f3ece76

update README with PKCE example

9eb647c

address review comments

43c6c1c

fix linkinator issue

47c421d

Make generate_code_verifier a class method

6cdcd21

bajajneha27 force-pushed the b/270203889 branch from bcb501d to 6cdcd21 Compare February 8, 2024 07:04

bajajneha27 merged commit 65bae29 into googleapis:main Feb 8, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add PKCE to 3 Legged OAuth exchange #471

feat: add PKCE to 3 Legged OAuth exchange #471

bajajneha27 commented Jan 29, 2024 •

edited

clundin25 Jan 30, 2024

bajajneha27 Feb 1, 2024

dazuma Jan 31, 2024

bajajneha27 Feb 1, 2024

bajajneha27 Feb 1, 2024

dazuma Jan 31, 2024

bajajneha27 Feb 1, 2024

dazuma Jan 31, 2024

bajajneha27 Feb 1, 2024 •

edited

dazuma Jan 31, 2024

bajajneha27 Feb 1, 2024

bajajneha27 commented Feb 5, 2024

clundin25 Feb 5, 2024

bajajneha27 Feb 6, 2024

clundin25 Feb 6, 2024

bajajneha27 Feb 7, 2024

clundin25 Feb 7, 2024

		request.session['code_verifier'] \|\|= authorizer.generate_code_verifier
		authorizer.code_verifier = request.session['code_verifier']

feat: add PKCE to 3 Legged OAuth exchange #471

feat: add PKCE to 3 Legged OAuth exchange #471

Conversation

bajajneha27 commented Jan 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 commented Jan 29, 2024 •

edited

bajajneha27 Feb 1, 2024 •

edited