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(android): get webClientId automatically #1003

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hannojg
Copy link

@hannojg hannojg commented Nov 5, 2021

Motivation

We have for our app multiple environment configurations, so we have multiple firebase projects. As the configuration for google/firebase happens on the native side it was too impractical for us to pass the webClientId on the JS side (the JS side has no clue about the google/firebase configuration).
We initially came to this when we mentioned that after the signing in the idToken was empty. We then read several issues which explained that you need to pass the webClientId when calling configure.

Solution

During the build process, the google-services.json seems to get read and its content is added to the resources. We simply get the resource value for the webClientId from the resources, which is equivalent to the value of client.oauth_client.client_id.

Concerns

I have zero knowledge about this code base or firebase/google mechanisms in general. For us, this solution works fairly well, so I thought it might do for others. But maybe I've missed a big important point somewhere 🤷

Copy link
Member

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

hello and thanks for the PR!

This looks useful, but we need to handle this with care:

with this addition, does the iOS + Android behavior of configure() call without webClientId provided in JS "drift away" from each other? You see, previously if users didn't pass the webClientId in JS, it was null in java. Now it's not, which could be confusing.
Is this right / is this wrong? I haven't used the package in a while, I don't even remember what the option does :D

What are your thoughts?
Either way, if we add some sort of "magic", it needs to be documented.

Thank you!

@hannojg
Copy link
Author

hannojg commented Dec 6, 2021

To my understanding, with this change, iOS and android work now exactly the same.
While previously to setup this library on ios, you didn't need to pass anything to configure [as it is getting the webClientId automatically from the config], for the android setup you'd have to call configure with the webClientId.

It is still fine to set the webClientId from JS side. That will lead to the same output on iOS and android.

I think not all features/functionality needs the webClientId so that's maybe why it hasn't been noticed before? See for example this code:

configure(options: ConfigureParams = {}): void {
if (options.offlineAccess && !options.webClientId) {
throw new Error('RNGoogleSignin: offline use requires server web ClientID');
}

Even with this change in place, an error would be thrown on JS side. So, imagine the native code would have picked up the webClientId from the native config itself, the JS side doesn't know that and would throw an error. So to make this really feature complete we would need to move that change to the native side and reject the promise there in case the user wants to use offline access and doesn't pass a webClientId.
However, users that pass a webClientId probably do so, because they also need offline access I guess. So, either way, this isn't a breaking change or behavioural change I feel.

@vonovak
Copy link
Member

vonovak commented Dec 20, 2021

So to make this really feature complete we would need to move that change to the native side and reject the promise there in case the user wants to use offline access and doesn't pass a webClientId.

can you please make that change? Thank you :)

@BrodaNoel
Copy link

@hannojg any updates here?

@robsonsr
Copy link

@hannojg any updates here?

Copy link

@Ankush-Hegde Ankush-Hegde left a comment

Choose a reason for hiding this comment

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

that's an useful update @hannojg

Copy link

@shivansh-rajpoot-vas shivansh-rajpoot-vas left a comment

Choose a reason for hiding this comment

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

I have seen this issue 4 years ago in the repo and its still here, this will resolve a lot of confusion for devs, p.s. docs need to be updated too

@raahimkhan
Copy link

I have seen this issue 4 years ago in the repo and its still here, this will resolve a lot of confusion for devs, p.s. docs need to be updated too

Can you please guide me how did you resolve your issue? I have added sha1 fingerprints of debug as well as release key store along with the sha1 finger print found in google play console but still google login is not working in release mode for android. It is working fine in development build.

I will be grateful if you can please guide how you got it working for android (react native).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants