Use GAIA headers to distinguish between GAIA and SAML IdP cookies

Use GAIA headers to distinguish between GAIA and SAML IdP cookies

During login, GAIA sends the "Google-Accounts-SAML=Start" and
"Google-Accounts-SAML=End" headers to indicate when a redirect to a SAML
IdP starts and ends. This CL uses these headers to distinguish between
cookies set by GAIA and the IdP:
* Any cookies set between the start and the end of the redirect were
  created by the IdP.
* Any cookies set before the start or after the end of the redirect
  were created by GAIA.

This allows cookies set by the IdP to be copied to the user's session
while ensuring that GAIA cookies are not copied (these need to be
transferred to the session via GAIA's /MergeSession API).

BUG=381123
TEST=Extended browser test; also manual

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

[bartfab (slow), 2014-08-06 19:57:35]

Hi Pavel,

Could you review:

chrome/browser/chromeos/login/profile_auth_data.cc
chrome/browser/chromeos/login/saml/saml_browsertest.cc

Hi Xiyuan,

Could you review:

chrome/browser/resources/gaia_auth/background.js

Hi Roger,

Could you review:

google_apis/gaia/fake_gaia.cc

[Roger Tawa OOO till Jul 10th, 2014-08-06 20:48:54]

lgtm

[xiyuan, 2014-08-06 21:20:12]

lgtm

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

https://codereview.chromium.org/447013003/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/background.js:311: // prepend headers. It is
only possible to delete and append headers
It took me a while to understand why we need to do this. "Prepend" is the
keyword here and we have to make sure the "google-accounts-saml-end" it set
before all other gaia cookies. Can you make it more prominent with the comment.

[dzhioev (left Google), 2014-08-07 09:17:44]

https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/profile_auth_data.cc (right):

https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/profile_auth_data.cc:274: 
I can't understand how does this change help to transfer IdP cookies to user's
profile. I mean IsGAIACookie became more permissive with you change, which means
less cookies will be transferred, right?

[bartfab (slow), 2014-08-07 09:40:28]

On 2014/08/07 09:17:44, dzhioev wrote:
>
https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
> File chrome/browser/chromeos/login/profile_auth_data.cc (right):
> 
>
https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/profile_auth_data.cc:274: 
> I can't understand how does this change help to transfer IdP cookies to user's
> profile. I mean IsGAIACookie became more permissive with you change, which
means
> less cookies will be transferred, right?

The goal is to ensure we never, ever accidentally copy GAIA cookies. So the main
goal for IsGAIACookie() is to avoid false negatives. The original implementation
looked at the domain name only, flagging any "google" or "youtube" cookie as
GAIA. The GAIA team felt that this was insufficient and asked us to look at
headers instead. GAIA sends headers to indicate the start and end of a SAML
redirect. We were asked to only copy cookies set during this redirect phase.
This is what the CL implements. On top of this new mechanism, we still look at
the domain as an extra precaution. This, too, is as discussed and agreed with
the GAIA team.

[dzhioev (left Google), 2014-08-07 11:26:47]

On 2014/08/07 09:40:28, bartfab wrote:
> On 2014/08/07 09:17:44, dzhioev wrote:
> >
>
https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
> > File chrome/browser/chromeos/login/profile_auth_data.cc (right):
> > 
> >
>
https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
> > chrome/browser/chromeos/login/profile_auth_data.cc:274: 
> > I can't understand how does this change help to transfer IdP cookies to
user's
> > profile. I mean IsGAIACookie became more permissive with you change, which
> means
> > less cookies will be transferred, right?
> 
> The goal is to ensure we never, ever accidentally copy GAIA cookies. So the
main
> goal for IsGAIACookie() is to avoid false negatives. The original
implementation
> looked at the domain name only, flagging any "google" or "youtube" cookie as
> GAIA. The GAIA team felt that this was insufficient and asked us to look at
> headers instead. GAIA sends headers to indicate the start and end of a SAML
> redirect. We were asked to only copy cookies set during this redirect phase.
> This is what the CL implements. On top of this new mechanism, we still look at
> the domain as an extra precaution. This, too, is as discussed and agreed with
> the GAIA team.

Got it. LGTM

[bartfab (slow), 2014-08-07 11:59:45]

https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/profile_auth_data.cc (right):

https://codereview.chromium.org/447013003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/profile_auth_data.cc:274: 
On 2014/08/07 09:17:44, dzhioev wrote:
> I can't understand how does this change help to transfer IdP cookies to user's
> profile. I mean IsGAIACookie became more permissive with you change, which
means
> less cookies will be transferred, right?

Done - I replied to this directly and Pavel was happy with the explanation.

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

https://codereview.chromium.org/447013003/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/background.js:311: // prepend headers. It is
only possible to delete and append headers
On 2014/08/06 21:20:11, xiyuan wrote:
> It took me a while to understand why we need to do this. "Prepend" is the
> keyword here and we have to make sure the "google-accounts-saml-end" it set
> before all other gaia cookies. Can you make it more prominent with the
comment.

Done.

[bartfab (slow), 2014-08-07 11:59:49]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-07 12:00:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bartfab@chromium.org/447013003/20001

[commit-bot: I haz the power, 2014-08-07 16:14:20]

Change committed as 288068