GoogleCloudPlatform / ide-login

The plugins-login-common library for authenticating with the Google Cloud Platform shared by several IDE plugins including Google Cloud Tools for Eclipse, Google Cloud Tools for IntelliJ, and Android Studio.
Apache License 2.0
2 stars 9 forks source link

Fix bug #40

Closed chanseokoh closed 7 years ago

chanseokoh commented 7 years ago

Credential itself is an HttpRequestInitializer.

codecov-io commented 7 years ago

Codecov Report

Merging #40 into master will increase coverage by 3.58%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   74.88%   78.47%   +3.58%     
==========================================
  Files          10       10              
  Lines         223      223              
  Branches       20       20              
==========================================
+ Hits          167      175       +8     
+ Misses         50       42       -8     
  Partials        6        6
Impacted Files Coverage Δ
...om/google/cloud/tools/ide/login/OAuth2Wrapper.java 0% <ø> (ø)
...google/cloud/tools/ide/login/GoogleLoginState.java 75% <100%> (+4.12%) :white_check_mark:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8a74722...915a6c4. Read the comment docs.

chanseokoh commented 7 years ago

Credential.initialize() is the right one that will put auth info to the HTTP header section (or so, I think). It seems that Credential.getRequsetInitializer() returns null for our Credential objects. I will look into it further.

As for testing, let me try, but this could be a bit challenging. I may have to further contort the code that was already sharded to enable mocking these network things. (The whole point of UserInfoService was to mock these.)

chanseokoh commented 7 years ago

I think I can write a test taegeting only the returned chained initializer. Will add that.

elharo commented 7 years ago

Is the bug in a recent change, not yet pushed? Otherwise I'm left wondering how the current code works.

chanseokoh commented 7 years ago

This new code is what I merged yesterday. The code we are using currently is unrelated.

chanseokoh commented 7 years ago

I believe I found a right way to do this. Chaining initializers and overriding it in OAuth2 works, but now I think it wasn't the right way.

chanseokoh commented 7 years ago

I believe I found a right way to do this.

Nope, too early to have said that.

chanseokoh commented 7 years ago

The UserInfoService has been significantly simplified. In fact, it's now genuinely a shim wrapper for Oauth2, so I was able to change the name to OAuth2Wrapper.

The refactoring allowed me to add tests that concretely verify whether the credential's initializer is called and the timeouts are set.

chanseokoh commented 7 years ago

@elharo PTAL