feat: adding support for authentication type on UserAuthorizer

[BigTailWolf]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

The change basically following the logic that NodeJS change: googleapis/google-auth-library-nodejs#1814

The key point is telling the client how are the UserAuthorizer going to provide auth with token URI.
Our current way is to have client_secret sending as part of the post url parameter. The STS endpoint won't allow that and they are not accepting client_secret field. Instead, the STS is using basic auth which takes a base64 encoding of client_id:client_secret.

Here the change is to provide a parameter to UserAuthorizer which auth from #RFC we are using and set the POST (Current way) as default.
Then in the implementation, when sending the token request, we apply a basic auth header if the authentication type is set to BASIC.

If you write sample code, please follow the samples format.

[lsirac]

I don't think we are validating the auth header anywhere.

[BigTailWolf]

This is the limitation of MockTokenServerTransport as it doesn't capture request header during it's execute method. So we have to use an addHeader method to simulate what has been added to the headers. And then in the test code call we cal this addHeader prior to make the http request.

[lsirac]

You can get the request header -- we do it in other tests I'm pretty sure

[lsirac]

The library methods here return UserCredentials. Is this compatible with BYOID? We have ExternalAccountAuthorizedUserCredentials for BYOID.

[BigTailWolf]

The library methods here return UserCredentials. Is this compatible with BYOID? We have ExternalAccountAuthorizedUserCredentials for BYOID.

ExternalAccountAuthorizedUserCredentials won't need any changes to have the capability of BYOID. Cloud Code can choose this one if they want to switch.
Per the sync with Cloud Code, they currently using UserAuthorizer for existing flow, what we do is just adding the capability to UserAuthorizer calling token endpoint with basic auth header.

[sonarcloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[lsirac]

The library methods here return UserCredentials. Is this compatible with BYOID? We have ExternalAccountAuthorizedUserCredentials for BYOID.

ExternalAccountAuthorizedUserCredentials won't need any changes to have the capability of BYOID. Cloud Code can choose this one if they want to switch. Per the sync with Cloud Code, they currently using UserAuthorizer for existing flow, what we do is just adding the capability to UserAuthorizer calling token endpoint with basic auth header.

The method you've updated here returns UserCredentials. This is not compatible with the BYOID flow.

[BigTailWolf]

The library methods here return UserCredentials. Is this compatible with BYOID? We have ExternalAccountAuthorizedUserCredentials for BYOID.

ExternalAccountAuthorizedUserCredentials won't need any changes to have the capability of BYOID. Cloud Code can choose this one if they want to switch. Per the sync with Cloud Code, they currently using UserAuthorizer for existing flow, what we do is just adding the capability to UserAuthorizer calling token endpoint with basic auth header.

The method you've updated here returns UserCredentials. This is not compatible with the BYOID flow.

UserCredentials is holding clientId, tokenServerUri, refreshToken, access_token etc. That's all the user wants.
And during the authentication flow. Now with modification. We have the generate auth URL done inside the getCredentialsFromCode with the addtionalParameters properly set. That will give the auth url we need, then we make the http request to sts.googleapis.com/v1/oauthtoken and exchange the access token. The access token is going to be set in the property of UserCredentials. And this will be return to the user agent (Cloud Code in our case).

Maybe this is not the same flow with our existing BYOID flow. I believe the flow is still work as help the UserAuthorizer class able to get the access token with minimum changes.

[lqiu96]

Since this is a private constructor, can we update this to take in the Builder class instead of additional parameters?

private UserAuthorizer(Builder builder) {
  // Assign the fields.
}

If we change this constructor to take a Builder, then the javadocs suggestion above doesn't matter.

[BigTailWolf]

I don't think so. The Builder's build() method is using this parameter to initialize the value.

[lqiu96]

I believe you can change the builder's build() method so that it pass the builder as the sole parameter to the constructor:

    public UserAuthorizer build() {
      return new UserAuthorizer(this);
    }

I think this would be cleaner because future implementations can add additional params without having to explicitly new fields to the constructor:

  private UserAuthorizer(UserAuthorizer.Builder builder) {
    this.clientId = Preconditions.checkNotNull(builder.clientId);
    this.scopes = ImmutableList.copyOf(Preconditions.checkNotNull(builder.scopes));
    this.callbackUri = (builder.callbackUri == null) ? DEFAULT_CALLBACK_URI : builder.callbackUri;
    this.transportFactory =
        (builder.transportFactory == null) ? OAuth2Utils.HTTP_TRANSPORT_FACTORY : builder.transportFactory;
    this.tokenServerUri = (builder.tokenServerUri == null) ? OAuth2Utils.TOKEN_SERVER_URI : builder.tokenServerUri;
    this.userAuthUri = (builder.userAuthUri == null) ? OAuth2Utils.USER_AUTH_URI : builder.userAuthUri;
    this.tokenStore = (builder.tokenStore == null) ? new MemoryTokensStorage() : builder.tokenStore;
    this.pkce = builder.pkce;
    this.clientAuthenticationType =
        (builder.clientAuthenticationType == null)
            ? ClientAuthenticationType.CLIENT_SECRET_POST
            : builder.clientAuthenticationType;
  }

I think this can be refactored even more, so that the builder sets default values and it doesn't need to be validated in the constructor:

  public static class Builder {

    private ClientId clientId;
    private TokenStore tokenStore = new MemoryTokensStorage();
    private URI callbackUri = DEFAULT_CALLBACK_URI;
    private URI tokenServerUri = OAuth2Utils.TOKEN_SERVER_URI;
    private URI userAuthUri = OAuth2Utils.USER_AUTH_URI;
    private Collection<String> scopes;
    private HttpTransportFactory transportFactory = OAuth2Utils.HTTP_TRANSPORT_FACTORY;
    private PKCEProvider pkce;
    private ClientAuthenticationType clientAuthenticationType = ClientAuthenticationType.CLIENT_SECRET_POST;
...

and the constructor could be something like:

  private UserAuthorizer(UserAuthorizer.Builder builder) {
    this.clientId = Preconditions.checkNotNull(builder.clientId);
    this.scopes = ImmutableList.copyOf(Preconditions.checkNotNull(builder.scopes));
    this.callbackUri = builder.callbackUri;
    this.transportFactory = builder.transportFactory;
    this.tokenServerUri = builder.tokenServerUri;
    this.userAuthUri = builder.userAuthUri;
    this.tokenStore = builder.tokenStore;
    this.pkce = builder.pkce;
    this.clientAuthenticationType = builder.clientAuthenticationType;
  }

[BigTailWolf]

The refactor make sense to me. But that's sort of a bigger refactor than this change and irrelevant to the purpose of this change.
Can I do a separate one for that? Maybe create an issue and resolve it through the refactor change.

[lqiu96]

Sure, it's a private constructor and we can change implementation later. If you could create an issue in this repo and follow up on it sometime in the coming weeks, that would be great! Thanks!

[sonarcloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud