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

Prefer provided credentials if they are present in FirestoreOptions #3460

Closed
wants to merge 1 commit into from
Closed

Conversation

andrewparmet
Copy link

@andrewparmet andrewparmet commented Jul 12, 2018

Addresses #3458.

As of 0.52.0-beta, calls to setCredentials() on FirestoreOptions are effectively ignored. This is because GrpcTransportOptions.setUpCredentialsProvider() is no longer called in GrpcFirestoreRpc - instead the settings builder just pulls the credentials provider regardless of whether or not a fixed credential was supplied in the original options.

It used to be:

settingsBuilder.setCredentialsProvider(
    GrpcTransportOptions.setUpCredentialsProvider(options));

Today it reads:

settingsBuilder.setCredentialsProvider(options.getCredentialsProvider());

This was changed in #3320. (Diff line)

The call can be restored in the same class with some conditional logic, or the FirestoreOptions can try to return the correct CredentialsProvider.

@andrewparmet andrewparmet requested a review from pongad as a code owner July 12, 2018 16:59
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2018
@pongad
Copy link
Contributor

pongad commented Jul 12, 2018

@schmidt-sebastian Can you take a look?

@schmidt-sebastian
Copy link
Contributor

Thanks for sending this over! The problem you described exists in both the CredentialsProvider and the ChannelProvider. I have opened a PR to deal with both: #3472

Let's leave this open until 3472 is approved.

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.

4 participants