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: Implement support for exposing refreshHandler callback to handle token refresh. #1213

Merged
merged 13 commits into from Aug 27, 2021

Conversation

xil222
Copy link
Contributor

@xil222 xil222 commented Jul 20, 2021

Design Doc: link
For this PR, developer has to provide a callback function returns access_token and expire_time both mandatory.
@bojeil-google @bcoe @lsirac Your feedback is highly appreciated!

There're 8 test cases:
getRequestHeader():

  1. expired token, calling refreshHandler
  2. token not expired, return authorized header with current token
  3. no credentials in client, calling refreshHandler

request():

  1. one with calling refreshHandler callback case
  2. no credentials in client, calling refreshHandler

getAccessToken():

  1. call refreshHandler when token expires and no refresh token
  2. no credentials in client, calling refreshHandler

RefreshHandler returns no access token:

  1. define refresh handler callback with empty access token, rejected when trying to refresh in getAccessToken()

Modify other error message and description in tests due to addition of refreshHandler logic.
Modify other test cases in request() to add another mock for 2nd successful request.

Note:
Since the logic of throwing error is in refreshTokenNoCache I didn’t add "no refreshHandler callback related information" to the error message in the test case

@xil222 xil222 requested a review from a team as a code owner July 20, 2021 19:25
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2021
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

One nit about the edit in samples/package.json, otherwise this looks reasonable to me.

samples/package.json Outdated Show resolved Hide resolved
@bojeil-google bojeil-google self-requested a review July 31, 2021 05:25
Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

Hey @xil222 I took a first look. I will continue next week. Thanks.

src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
@xil222 xil222 requested a review from a team as a code owner August 5, 2021 07:41
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Show resolved Hide resolved
test/test.oauth2.ts Show resolved Hide resolved
test/test.oauth2.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Outdated Show resolved Hide resolved
@xil222
Copy link
Contributor Author

xil222 commented Aug 20, 2021

There're 5 test cases:
getRequestHeader():

  1. expired token, calling refreshHandler
  2. token not expired, return authorized header with current token

request():

  1. one with calling refreshHandler callback case

getAccessToken():

  1. call refreshHandler when token expires and no refresh token

RefreshHandler returns no access token:

  1. define refresh handler callback with empty access token, rejected when trying to refresh in getAccessToken()

Modify other error message and description in tests due to addition of refreshHandler logic.

Note:
Since the logic of throwing error is in refreshTokenNoCache I didn’t add "no refreshHandler callback related information" to the error message in the test case

@xil222 xil222 closed this Aug 20, 2021
@xil222 xil222 reopened this Aug 20, 2021
@xil222 xil222 requested review from bojeil-google and removed request for lsirac August 20, 2021 00:34
src/auth/oauth2client.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Show resolved Hide resolved
test/test.oauth2.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Outdated Show resolved Hide resolved
test/test.oauth2.ts Show resolved Hide resolved
test/test.oauth2.ts Show resolved Hide resolved
Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

The tests look a lot better now. Glad we uncovered this. Thanks for fixing them.

@bcoe bcoe added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 27, 2021
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 27, 2021
@bcoe bcoe merged commit 2fcab77 into googleapis:master Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants