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

Fix expired access token bug in the proxy #217

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@jianglai
Copy link
Member

commented Aug 6, 2019

#129 migrated GoogleCredential
to GoogleCredentialsBundle and introduced a subtle bug. I don't fully
understand why but there are times when the access token is null but
credentials.refresh() is not called, resulting in NullPointerException
when credentials.getAccessToken().getTokenValue() is called.

Since the new GoogleCredentials class supports shouldRefresh(), we now
just rely on it to make sure that we always get a value access token.


This change is Reviewable

@jianglai jianglai requested a review from hstonec Aug 6, 2019

@hstonec

hstonec approved these changes Aug 6, 2019

Copy link
Collaborator

left a comment

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @hstonec and @jianglai)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 240 at r1 (raw file):

        }
      }
      return credentials.getAccessToken().getTokenValue();

Technically, if accessToken is null, credentials.refresh() should be invoked. I think maybe it just didn't work?

@hstonec

hstonec approved these changes Aug 6, 2019

Copy link
Collaborator

left a comment

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @hstonec and @jianglai)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 240 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Technically, if accessToken is null, credentials.refresh() should be invoked. I think maybe it just didn't work?

BTW, did you manage to reproduce the issue somewhere and see the new code fixed the issue?

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 6, 2019

This pull request fixes 1 alert when merging 4af494e into cf3f960 - view on LGTM.com

fixed alerts:

  • 1 for Result of multiplication cast to wider type

@jianglai jianglai requested a review from hstonec Aug 6, 2019

@jianglai
Copy link
Member Author

left a comment

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @hstonec)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 240 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

BTW, did you manage to reproduce the issue somewhere and see the new code fixed the issue?

Right, like I said I couldn't really figure out what is the problem. The change here is to make things as simple as possible and reduce the potential for bugs.

The problem only happens once or twice a day on production, so it's hard to reproduce or verify the fix. I pushed the code to crash in the hope that the prober traffic would be enough to provide some evidence.

@jianglai jianglai force-pushed the jianglai:credential branch from 4af494e to b7accdd Aug 7, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 7, 2019

This pull request fixes 1 alert when merging b7accdd into ce791e8 - view on LGTM.com

fixed alerts:

  • 1 for Result of multiplication cast to wider type
Fix expired access token bug in the proxy
#129 migrated `GoogleCredential`
to `GoogleCredentialsBundle` and introduced a subtle bug. I don't fully
understand why but there are times when the access token is null but
`credentials.refresh()` is not called, resulting in NullPointerException
when `credentials.getAccessToken().getTokenValue()` is called.

Since the new GoogleCredentials class supports `shouldRefresh()`, we now
just rely on it to make sure that we always get a value access token.

@jianglai jianglai force-pushed the jianglai:credential branch from b7accdd to 0ac6f16 Aug 7, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 7, 2019

This pull request fixes 1 alert when merging 0ac6f16 into a68b1a1 - view on LGTM.com

fixed alerts:

  • 1 for Result of multiplication cast to wider type

@jianglai jianglai merged commit c7478fc into google:master Aug 8, 2019

4 of 7 checks passed

code-review/reviewable 3 files, 1 discussion left (hstonec)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java 1 fixed alert
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro Kokoro build finished
Details

@jianglai jianglai deleted the jianglai:credential branch Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.