[cleanup] SAML is always enabled.

[cleanup] SAML is always enabled.

Removed "disable-saml-signin" flag and non-SAML auth extension manifest.
Now SAML is always enabled for both desktop and Chrome OS.
Plus:
* Prevented potential races during extension initialization.
* Made "manifest_keyboard.json" up to date with "manifest.json".

BUG=394342
TEST=none
TBR=jochen (c/b/browser_resources.grd)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284120

[dzhioev (left Google), 2014-07-11 13:45:59]

Hi, please review.
I've checked that GAIA works right for both regular and SAML accounts in all
possible places: Chrome OS login (regular and red carpet), Chrome and Chrome OS
mirror.

[dzhioev (left Google), 2014-07-11 13:54:25]

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:165: }.bind(this), 200);
Hui, why this check is needed? On Chrome OS we connected to channel much earlier
(on 'enableSAML' message, which were dispatched from 'saml_injected.js) and
never experienced problems with connection.

[guohui, 2014-07-11 19:29:36]

lgtm

a side question, a while ago we talked about disabling password scraping when
adding a secondary SAML user on cros, i assume that must have been implemented
before we enable SAMl support for everyone?

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:165: }.bind(this), 200);
initSupportChannel_ is called from onPageLoad_ which is an event listener for
DOMContentLoaded, this may happen before the background script is set up, thus
the 'authMain' message may get lost, and thus we need to listen for a
confirmation message 'channelConnected', and if it is not received within
timeout then we resend the 'authMain' message.

[bartfab (slow), 2014-07-14 10:12:31]

On 2014/07/11 19:29:36, guohui wrote:
> lgtm
> 
> a side question, a while ago we talked about disabling password scraping when
> adding a secondary SAML user on cros, i assume that must have been implemented
> before we enable SAMl support for everyone?

This is the first time I hear about that idea. AFAICT, we do nothing special for
secondary users. In most cases, SAML logins will be done for primary users
because every new Chrome OS user must always start out as primary. Subsequent
authentication normally, such as when adding a secondary user to an existing
session, happens offline, using the cached password. But there are ways to force
online authentication. If that was to happen, I expect that we do run the
password scraping code while adding a secondary user. Is this a concern? What
profile would be used for this auth flow?

> 
>
https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
> File chrome/browser/resources/gaia_auth/main.js (right):
> 
>
https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
> chrome/browser/resources/gaia_auth/main.js:165: }.bind(this), 200);
> initSupportChannel_ is called from onPageLoad_ which is an event listener for
> DOMContentLoaded, this may happen before the background script is set up, thus
> the 'authMain' message may get lost, and thus we need to listen for a
> confirmation message 'channelConnected', and if it is not received within
> timeout then we resend the 'authMain' message.

[bartfab (slow), 2014-07-14 10:12:40]

lgtm

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
File chrome/browser/extensions/signin/gaia_auth_extension_loader.cc (right):

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:24: #include
"chrome/browser/chromeos/profiles/profile_helper.h"
Nit: No longer used.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:27: #include
"chromeos/chromeos_switches.h"
Nit: No longer used.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:28: #include
"components/signin/core/common/profile_management_switches.h"
Nit: No longer used.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:60:
manifest_resource_id = IDR_GAIA_AUTH_KEYBOARD_MANIFEST;
Did you verify that things do not break when the keyboard manifest is active,
now that the extension host expects SAML to be always enabled?

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:163: this.initDesktopChannel_();
This calls initDesktopChannel_(), which no longer exists.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:211: * Invoded when support channel
is connected.
Nit: s/Invoded/Invoked/

[dzhioev (left Google), 2014-07-15 18:34:08]

+xiyuan@
Please take another look. I've slightly changed initialization process to
prevent races between iframe 'load' event and connection to channel.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
File chrome/browser/extensions/signin/gaia_auth_extension_loader.cc (right):

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:24: #include
"chrome/browser/chromeos/profiles/profile_helper.h"
On 2014/07/14 10:12:39, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:27: #include
"chromeos/chromeos_switches.h"
On 2014/07/14 10:12:40, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:28: #include
"components/signin/core/common/profile_management_switches.h"
On 2014/07/14 10:12:39, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/extensions/si...
chrome/browser/extensions/signin/gaia_auth_extension_loader.cc:60:
manifest_resource_id = IDR_GAIA_AUTH_KEYBOARD_MANIFEST;
On 2014/07/14 10:12:39, bartfab wrote:
> Did you verify that things do not break when the keyboard manifest is active,
> now that the extension host expects SAML to be always enabled?
You are right, keyboard manifest was broken. I made it up to date with changes
in main manifest, so now it supports SAML too.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:163: this.initDesktopChannel_();
On 2014/07/14 10:12:40, bartfab wrote:
> This calls initDesktopChannel_(), which no longer exists.

Done.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:165: }.bind(this), 200);
You are right.
Note that we have potential race here: if we are not able to connect to channel
for significant amount of time, but already received 'load' event (or
'clearOldAttempts' message) -- we show Gaia to user, but extension doesn't work
right, because channel is not initialized.
I've fixed this issue in next patch set.

https://codereview.chromium.org/384003002/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/main.js:211: * Invoded when support channel
is connected.
On 2014/07/14 10:12:40, bartfab wrote:
> Nit: s/Invoded/Invoked/

Done.

[xiyuan, 2014-07-15 18:55:23]

LGTM

[Nikita (slow), 2014-07-16 10:14:01]

>> BUG=none 

Should this point to http://crbug.com/374810 instead?
If not this CL should reference a newly created issue though.

[bartfab (slow), 2014-07-16 11:32:14]

Still LGTM.

https://codereview.chromium.org/384003002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/384003002/diff/40001/chrome/browser/resources...
chrome/browser/resources/gaia_auth/main.js:179: this.initSupportChannel_();
What happens if the initialization of a support channel is already in flight and
this call tries to create another 'authMain' channel? Can it happen that the
injected script ens up using one channel and the main script the other,
preventing communication between the two?

[dzhioev (left Google), 2014-07-16 13:25:18]

https://codereview.chromium.org/384003002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/384003002/diff/40001/chrome/browser/resources...
chrome/browser/resources/gaia_auth/main.js:179: this.initSupportChannel_();
On 2014/07/16 11:32:13, bartfab wrote:
> What happens if the initialization of a support channel is already in flight
and
> this call tries to create another 'authMain' channel? Can it happen that the
> injected script ens up using one channel and the main script the other,
> preventing communication between the two?
Yes, it can. 'chrome.runtime.connect' is a strange method, because there is no
way to check if connection was successful. I think we need implement something
smarter here to be sure that connection establishing works right (like custom
handshake algorithm). Probably I'll do it in other CL.
Same applies to connection from saml_injected to background, where we don't have
even such timeout protection.

[dzhioev (left Google), 2014-07-16 13:28:11]

On 2014/07/16 10:14:01, Nikita Kostylev wrote:
> >> BUG=none 
> 
> Should this point to http://crbug.com/374810 instead?
> If not this CL should reference a newly created issue though.

Nikita, this CL doesn't change observable behavior and doesn't fix any of
reported issues.

[Nikita (slow), 2014-07-16 14:05:52]

On 2014/07/16 13:28:11, dzhioev wrote:
> On 2014/07/16 10:14:01, Nikita Kostylev wrote:
> > >> BUG=none 
> > 
> > Should this point to http://crbug.com/374810 instead?
> > If not this CL should reference a newly created issue though.
> 
> Nikita, this CL doesn't change observable behavior and doesn't fix any of
> reported issues.

Then I advice filing a separate bug for SAML code cleanup and reference is here.
Makes sense to mention in the bug that verification would be that SAML flow
works fine.

This is needed so that QA is aware of such changes even though it's just a code
cleanup.

[dzhioev (left Google), 2014-07-16 15:01:25]

On 2014/07/16 14:05:52, Nikita Kostylev wrote:
> On 2014/07/16 13:28:11, dzhioev wrote:
> > On 2014/07/16 10:14:01, Nikita Kostylev wrote:
> > > >> BUG=none 
> > > 
> > > Should this point to http://crbug.com/374810 instead?
> > > If not this CL should reference a newly created issue though.
> > 
> > Nikita, this CL doesn't change observable behavior and doesn't fix any of
> > reported issues.
> 
> Then I advice filing a separate bug for SAML code cleanup and reference is
here.
> Makes sense to mention in the bug that verification would be that SAML flow
> works fine.
> 
> This is needed so that QA is aware of such changes even though it's just a
code
> cleanup.

Done. Created http://crbug.com/394342.

[Nikita (slow), 2014-07-17 06:20:23]

lgtm

[dzhioev (left Google), 2014-07-18 13:36:21]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-07-18 13:37:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dzhioev@chromium.org/384003002/40001

[commit-bot: I haz the power, 2014-07-18 16:07:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 16:10:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:10:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[dzhioev (left Google), 2014-07-18 16:21:15]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:22:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dzhioev@chromium.org/384003002/40001

[commit-bot: I haz the power, 2014-07-18 16:28:35]

Change committed as 284120