Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

Google Sign-in iOS #5

Open
gbenvenuti opened this issue Jun 17, 2015 · 16 comments
Open

Google Sign-in iOS #5

gbenvenuti opened this issue Jun 17, 2015 · 16 comments

Comments

@gbenvenuti
Copy link
Contributor

Hi,

Is there plans to integrate the Google Sigin-in for iOS into this plugin?
https://developers.google.com/identity/sign-in/ios/

Apple has rejected one of our apps for jumping to safari to complete oauth.

@gbenvenuti
Copy link
Contributor Author

Hi @agrieve,

I'm happy to get this started as we need it to resubmit, any chance you could set up the framework repo and then npm module etc? that way MobileChromeApps maintains control. Then I can PR.

Something like:

cordova-plugin-google-sign-in-ios

@agrieve
Copy link
Member

agrieve commented Jun 18, 2015

Hi!

Certainly would be a good idea to update to use the newer google sign-in library. It'll be tougher than doing what was done for the googleplus framework though, as this library requires cocoapods (and has a few dependencies).

Support for cocoapods within cordova has started, although I think might be somewhat stalled due to lack of a committer to champion it:

https://issues.apache.org/jira/browse/CB-5921

@gbenvenuti
Copy link
Contributor Author

Hi @agrieve,

As the library was available without coco-pods (https://developers.google.com/identity/sign-in/ios/sdk/) creating the framework plugin was a simple process:

https://github.com/gbenvenuti/cordova-plugin-google-sign-in-ios

Happy to transfer ownership to MobileChromeApps if you like. I also have not yet published to NPM.

As for the identity plugin I'll submit a PR once it's complete.

@agrieve
Copy link
Member

agrieve commented Jun 19, 2015

Oh, nice!

SGTM then! Only thing I'd need from you is for you to sign the Google contributor agreement (can be done online):

https://github.com/MobileChromeApps/mobile-chrome-apps/blob/master/CONTRIBUTING.md#legal

Once you do (or confirm that you already did, I can just fork your framework repo and publish it.

@gbenvenuti
Copy link
Contributor Author

Hey, I've already signed, did it for the signout feature on this plugin.

@agrieve
Copy link
Member

agrieve commented Jun 19, 2015

@gbenvenuti
Copy link
Contributor Author

Hi @agrieve,

Excellent. One question on the removeCachedAuthToken implementation in iOS.

The current version nils the auth token.

https://github.com/MobileChromeApps/cordova-plugin-chrome-apps-identity/blob/master/src/ios/ChromeIdentity.m#L76

This is not possible with the sign-in plugin as these are read-only properties. I'm not sure what exactly the use case is here. The token is revoked on the JS side so the account is effectively disconnected. Is there are method from https://developers.google.com/identity/sign-in/ios/api/interface_g_i_d_sign_in that you would recommend?

@agrieve
Copy link
Member

agrieve commented Jun 19, 2015

The use case here is that you've requested an auth token, tried to use it, and then discover that it has expired.

In this case, you need to tell the library that "hey, it didn't work! Give me a new one".

Looks like the equivalent in the new lib is [GIDAuthentication refreshAccessTokenWithHandler]

@gbenvenuti
Copy link
Contributor Author

Any chance you could point me to where you found reference to refreshAccessTokenWithHandler I can't find it in the docs or SDK.

@agrieve
Copy link
Member

agrieve commented Jun 19, 2015

bah, apparently the lack of this function is a known issue and the replacement I suggested is from a work-in-progress fix.

For now, I'd suggest just replacing it with a TODO.

@gbenvenuti
Copy link
Contributor Author

Hi @agrieve ,

Ok, here's where I'm at:

master...gbenvenuti:iosGoogleSignIn

I haven't created a pull request yet, as there's a few things still to sort out.

  • Finish removeCachedAuthToken, as above awaiting sdk update.
  • What approach would you prefer for the required plist file GoogleService-Info.plist and URLTypes? I have looked at making this with plugin variables. This works quite well with the current version of cordova as you can save them in your config.xml file. Unfortunately from what I can tell these become required for all platforms.
    <plugin name="cordova-plugin-chrome-apps-identity" spec="^X.X.X">
        <variable name="IOS_BUNDLE_ID" value="value" />
        <variable name="IOS_CLIENT_ID" value="value" />
        <variable name="IOS_REVERSED_CLIENT_ID" value="value" />
    </plugin>

The install command becomes

cordova plugin add --save cordova-plugin-chrome-apps-identity  --variable IOS_BUNDLE_ID=value --variable IOS_CLIENT_ID=value --variable IOS_REVERSED_CLIENT_ID=value

When working with plugins I prefer not to have to copy other files etc over just to make the plugin functional. The ideal workflow is to have everything defined in config.xml and any developer can then build the app without jumping through a bunch of hoops.

Having said that, if you'd rather a more manual approach I'll work around that.

  • Readme still needs a bit of work, based on above.
  • We'll also need cordova-plugin-google-sign-in-ios v2.0.2 published on npm with the Bundle.

I have enough now to resubmit our app to the store, I'm happy to finish it off after your feedback, or for you to take it from there.

Thanks for your help.

@agrieve
Copy link
Member

agrieve commented Jun 24, 2015

Had a look at your diff. Looks great!

The <variable> thing is unfortunate, and it's also confusing that if you change the values in your config.xml, they don't update. <preference> along with a plugin hook might be better, but we'd probably need to bundle a plist library (or attempt to edit xml via regexp) as part of the hook.

@agrieve
Copy link
Member

agrieve commented Jun 27, 2015

@gbenvenuti
Copy link
Contributor Author

I've submitted a PR for the SDK 2.1.0

So now in the removeCachedAuthToken method we call this new method? I'm just not sure this matches the android implementation as it simply calls GoogleAuthUtil.clearToken(context, token); and doesn't make an attempt to retrieve a new one. If this isn't an issue I'll add the new call with a completion block here.

@agrieve
Copy link
Member

agrieve commented Jun 29, 2015

Great! I've published 2.1.0 to npm.

Yeah, the new API isn't exactly the same in semantics, but it's close enough. The expected usage is:

  1. Make an request
  2. If request comes back with not authorized:
    1. Clear cached auth token
    2. Request new auth token
    3. Retry request

I can't see a reason to clear the cached auth token and not request a new one right afterwards.

It could be a bit weird that clearAuthToken will timeout when not online (since it's actually refreshing under-the-hood). Perhaps instead of calling it right away, we set a flag in the class that tells it to call refreshAccessTokenWithHandler the next time getAuthToken() is called. WDYT?

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

No branches or pull requests

2 participants