search

MobileChromeApps / cordova-plugin-chrome-apps-identity

BSD 3-Clause "New" or "Revised" License
9 stars 12 forks source link

Google Sign-in iOS #5

Open gbenvenuti opened 9 years ago

gbenvenuti commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

agrieve commented 9 years ago

Bam! https://github.com/MobileChromeApps/cordova-plugin-google-sign-in-ioshttps://www.npmjs.com/package/cordova-plugin-google-sign-in-ios

Thanks for being awesome :)

gbenvenuti commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

agrieve commented 9 years ago

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.

agrieve commented 9 years ago

FYI - http://stackoverflow.com/questions/30522519/how-do-i-get-refresh-token-using-gidsignin-and-gtmoauth2authentication-in-ios

gbenvenuti commented 9 years ago

Hi @agrieve ,

Ok, here's where I'm at:

https://github.com/MobileChromeApps/cordova-plugin-chrome-apps-identity/compare/master...gbenvenuti:iosGoogleSignIn

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

    <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.

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 commented 9 years ago

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 commented 9 years ago

Looks like it's live now: https://developers.google.com/identity/sign-in/ios/api/interface_g_i_d_authentication#ad529aecd6162d65690cae95959a3a965

gbenvenuti commented 9 years ago

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 commented 9 years ago

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?