Sync's method for G-authorize causes conflicts

MysticJay commented 4 years ago

(Forwarded from TG) Original reporter: CharlieNasa, deviousness The sync plugin was changed from using gapi.auth2.authorize() to gapi.auth2.init(), which pollutes the namespace and makes it so that it conflicts with any other plugin using gapi.auth2.* ; Had it stayed with gapi.auth2.authorize() it wouldn't conflict.

https://developers.google.com/identity/sign-in/web/reference#gapiauth2authorizeparams_callback

"Warning: do not use this method alongside the recommended gapi.auth2.init and signIn flow. These are two distinct behaviors (Authorization for gapi.auth2.authorize vs Authentication for gapi.auth2.init/signIn) and will have unexpected issues if used within the same application."

Does anyone recall why this change was made? Does anyone have a working solution for multiple plugins using google auth?

MysticJay commented 4 years ago

This is where sync changed from AUTH to INIT: https://github.com/IITC-CE/ingress-intel-total-conversion/commit/0ae25e1e152a45c26082b7e6539d2b78de8ce4e9#diff-63dc9087c660611bdf3fcb1a1257247a

johnd0e commented 4 years ago

Link to original report: https://t.me/iitc_group/7885

And here I quote original reporter (@cloudkucooland):

I'll know more after I've had time to seriously test

I'm going to work on an alternative solution.

(one of such solution proposed here: https://t.me/iitc_group/7917)

So there is no real issue evidence yet. Please note that I do not claim that there is no such issue. But to confirm it we need real code sample.

MysticJay commented 4 years ago

After deeper investigation the situation shows as follows:

plugins that use gapi.auth2.authorize can load in any order
a plugin that uses gapi.auth2.init will make any subsequent plugin fail to load, regardless of that later plugin using gapi.auth2.authorize or gapi.auth2.init.
as it can not be determined which plugin loads first there is no Workaround for the user.

Steps to resolve the situation:

[ ] all plugins must be switched to gapi.auth2.authorize -> provide sample code
[ ] to detect a conflict with a plugin still using gapi.auth2.init, each plugin should check if an gapi.auth2.init was used before starting its own code else issue an Alert to the user -> provide sample code
[ ] investigate the possibility for the main script to identify a plugin that will use gapi.auth2.init to exclude it from loading.

johnd0e commented 4 years ago

I see conflicting statements. (1) and (2) mean that gapi.auth2.init is more compatible than gapi.auth2.authorize

MysticJay commented 4 years ago

Got mixed up myself. Fixed the original comment.

johnd0e commented 4 years ago

Please use full names (gapi.auth2.init and gapi.auth2.authorize) instead of 'INIT' and 'AUTH'.

investigate the possibility for the main script to identify a plugin that will use INIT to exclude it from loading.

This is not possible.

each plugin should check if an INIT was used before starting

This is not possible (AFAIK), and not needed anyway.

johnd0e commented 4 years ago

[ ] Alternative approach (to be tested): load own instance of gapi into sandboxed window namespace (feature of TM/GM/VM).

This have to be done outside of plugin wrapper.

Note, that @grant none may prevent sandboxing in TM.

Todo: test with https://greasyfork.org/en/scripts/395541-iitc-plugin-draw-tools-sync

cloudkucooland commented 4 years ago

[ ] Alternative approach (to be tested): load own instance of gapi into sandboxed window namespace (feature of TM/GM/VM).

This have to be done outside of plugin wrapper.

Note, that @grant none may prevent sandboxing in TM.

Todo: test with https://greasyfork.org/en/scripts/395541-iitc-plugin-draw-tools-sync

I'm willing to test this solution with my code, if there is clear bit of documentation on what the new wrapper code looks like. In the meantime, if I detect the sync plugin or anything initializing gapi.auth2.init, I throw alerts to the user about the conflict.

johnd0e commented 4 years ago

if there is clear bit of documentation on what the new wrapper code looks like.

You do not actually need new wrapper for that (and we are not going to change wrapper for now).

cloudkucooland commented 4 years ago

if there is clear bit of documentation on what the new wrapper code looks like.

You do not actually need new wrapper for that (and we are not going to change wrapper for now).

So there is no real proposed solution evidence yet. Please note that I do not claim that there is no such solution. But to confirm it we need real code sample.

johnd0e commented 4 years ago

You also can try from another side - change sync.user.js to use old .authorise() method and check if this really fixes your issue.

johnd0e commented 4 years ago

But to confirm it we need real code sample.

I have no ready to use sample, but if you want to give it a try - I could assist you.

And you are right - there is no evidence that it will work with gapi lib.

cloudkucooland commented 4 years ago

But to confirm it we need real code sample.

I have no ready to use sample, but if you want to give it a try - I could assist you.

And you are right - there is no evidence that it will work with gapi lib.

If you don't have even a proof-of-concept, I can't help you test. Let me know when you've got something worth testing.

cloudkucooland commented 4 years ago

You also can try from another side - change sync.user.js to use old .authorise() method and check if this really fixes your issue.

I did, it does.

johnd0e commented 4 years ago

@cloudkucooland > If you don't have even a proof-of-concept, I can't help you test.

Perhaps my explanation was bad: I never intended to implement it myself, being too busy with other parts.

I did, it does.

So go ahead, open PR. And provide us with working sample of alternative plugin to test in pair (copy of standard sync, or anything)

MysticJay commented 4 years ago

Why are we in a dead-lock here? I try to summarize:

1) On the one hand Google recommends to use gapi.auth2.init, but OTOH in same moment warns about using it, when gapi.auth2.authorize is used too:

"Warning: do not use this [gapi.auth2.authorize] method alongside the recommended gapi.auth2.init and signIn flow. These are two distinct behaviors (Authorization for gapi.auth2.authorize vs Authentication for gapi.auth2.init/signIn) and will have unexpected issues if used within the same application." https://developers.google.com/identity/sign-in/web/reference#gapiauth2authorizeparams_callback`

Seems to be a clear statement to me that requires action.

2) Using gapi.auth2.init would require a "framework" in IITC and ALL plugins that use gapi.auth2 to hook up on that. This will not happen for various reasons:

plugins need/use their own Client ID to sign in with google (and for faction specific reasons this is a requiremment). For this situation Google even recommends to use gapi.auth2.authorize (see link above).
gapi.auth2.authorize makes it easy for developers to get their plugin working without the need to check anything (now they check if "sync" is loaded and ask to disable that).

3) Except SYNC the plugins I have found and need to authorize with Google are faction specific. But all have implemented or switched to gapi.auth2.authorize. So, to become compatible with those faction plugins SYNC must revert back to gapi.auth2.authorize, at least there should be a "conflict-free" alternative.

As I naturally only have access to ENL tools I would like a RES Agent to check their tools and confirm the situation.

MysticJay commented 4 years ago

As even Google recommends to use authorize in case of conflicts a fix has been implemented in #389

IITC-CE / ingress-intel-total-conversion

Sync's method for G-authorize causes conflicts #334