+Martin as this relates to privacy

Hey Martin, I think this may have been something we missed during our privacy review for Dice. If a user has specifically gone to Content settings and configured their cookies to clear on exit (either just their Google cookies or all of their cookies), we believe we should honor this decision with Dice and sign the user out completely (instead of rebuilding their cookies). This setting doesn't exist on mobile with Mirror, which is probably why we missed it. This is mostly an FYI for you, since I think you would agree with the change we're making :)

cc'ing Nick as well since he had some comments on the thread about what domains we should look for in the "Clear on exit" list. I realize that it's a regexp, so we can probably just look for [*.]google.com instead of [*.]accounts.google.com. Since we're revoking the user's token, will the reconcilor also take care of signing the user out from other TLDs (e.g. youtube.com)?

Looking at the code, the "Clear cookie on exit" functionality is implemented by turning all cookies into session cookies. Which means that they are technically not deleted on exit, but rather not loaded on restart.

This means that we have to delete the tokens on restart, and the easiest place to do this would be on the first run of the reconcilor (instead of rebuilding the cookie, we would delete the token if cookies for accounts.google.com are flagged as "delete on exit").

QuotaPolicyCookieStore::~QuotaPolicyCookieStore() and SessionDataDeleter::DeleteSessionOnlyOriginCookies() delete "Clear cookie on exit" cookies when Chrome terminates or the last window of a profile is closed.

Thanks for the pointer. That means that there are two paths to delete these cookies: on exit, and also on the next restart (by not loading them). I guess doing it on restart is necessary, because it's the only way to safely delete them if Chrome crashes.

I wonder if we need to delete the tokens from these two paths, or if doing it on restart only is enough.

Do you know why this DeleteSessionOnlyOriginCookies() code exists? Why wasn't it enough to just not reload them on next restart? 

I'm not exactly sure why this exists. I assume that "Clear on Exit" should actually happen when Chrome closes because the user decided this. We shouldn't leave the data on disk until next start. 
Normal session-only cookies are not removed by ~QuotaPolicyCookieStore. This is probably done for performance reasons to keep shutdown quick.
Most users don't have "Clear on exit" rules but many websites create session-only cookies so almost everyone has some of these.

I'm not sure why cookie deletion is duplicated in SessionDataDeleter and ~QuotaPolicyCookieStore but the first one handles closing the last window of a profile if there is still any other window from a different profile or a Chrome App. ~QuotaPolicyCookieStore only gets called when Chrome closes completely. 

> Since we're revoking the user's token, will the reconcilor also take care of signing the user out from other TLDs (e.g. youtube.com)?

Chrome (including the reconcilor) shouldn't manually try to clear other cookies. The remaining auth cookies should already be invalidated by revoking the LST.

Cool, thanks for the input everyone. This sounds good to me.

David, clarifying one thing you said:

> ...we would delete the token if cookies for accounts.google.com are flagged as "delete on exit".

If the user puts [*.]google.com, that would count as "accounts.google.com being flagged," since the regexp matches, right?

I'll only clear the token on the next restart at first, because it handles all the cases including  browser crashes.
I'm still unsure if we need to do more than this, and even if we do, it probably does not need to block dice and can be done in M68 to avoid extra cherry-picking.

I don't think we should look at other domains such as youtube, there is no reason to do this. The tokens we have in Chrome are kept in sync with the APISID cookie on accounts.google.com, and thus this is the only one that matters here.

ewald: I think we should not completely sign the user out of Sync, if Sync is enabled. I would prefer putting them in error state, or maybe even rebuild their cookie in this case.

Re #8: correct, we should just put the user into a "sync paused" error state (with a dummy token), as we discussed this morning.

David, I also want to clarify my comment from #7: if the user puts [*.]google.com, that would count as "accounts.google.com being flagged," since the regexp matches, correct?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/355781b89354303660792752cf9e0c9552d5e875

commit 355781b89354303660792752cf9e0c9552d5e875
Author: David Roger <droger@chromium.org>
Date: Thu May 03 16:51:49 2018

[signin] Clear refresh tokens when signin cookies are cleared on exit

When signin cookies are "cleared on exit" through the Chrome settings,
Dice would rebuild them on next startup, which defeats the point of
that cookie settings.
To fix this, tokens are not loaded when signin cookies are cleared on
exit. Instead, the tokens are revoked on the server, and deleted from
the database.
The secondary accounts are completely removed from Chrome, but the
primary account is kept in authentication error state, so that Sync
can display an error.

TBR=treib, eugenebut

Bug:  836862 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I5a72b3a173242494ad1c996765c838bb9a7f09f1
Reviewed-on: https://chromium-review.googlesource.com/1042392
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555771}
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/chrome/browser/search/one_google_bar/one_google_bar_service_factory.cc
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/chrome/browser/signin/profile_oauth2_token_service_factory.cc
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/components/signin/core/browser/BUILD.gn
[add] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/components/signin/core/browser/cookie_settings_util.cc
[add] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/components/signin/core/browser/cookie_settings_util.h
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/components/signin/core/browser/signin_header_helper.cc
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/components/signin/core/browser/signin_header_helper.h
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/ios/chrome/browser/signin/ios_chrome_signin_client.mm
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/ios/web_view/internal/signin/ios_web_view_signin_client.mm
[modify] https://crrev.com/355781b89354303660792752cf9e0c9552d5e875/tools/metrics/histograms/enums.xml

CL listed at #10 is not yet in canary, pls update the bug with canary result tomorrow. Thank you.

Also we took multiple merges in for this feature [Dice], May I pls know why multiple merge request after branch? I know few CLs are to collect metrics.

Here are Merged CLs so far:
https://chromium.googlesource.com/chromium/src.git/+/27133efece77f85202f484e1518260d83016685e

https://chromium.googlesource.com/chromium/src.git/+/fef85e327bdb83770166daaa891bde2559520a8d

https://chromium.googlesource.com/chromium/src.git/+/8286a9215b61b11ae63671d9f796df681b1099a6

https://chromium.googlesource.com/chromium/src.git/+/09d9553cd58341d6e09bf896d0ceef2ac8a00098
 
https://chromium.googlesource.com/chromium/src.git/+/a8db414a44d1c54295f8f89c0b8e556643d7f2e8

https://chromium.googlesource.com/chromium/src.git/+/a2f515ad27573e34ac34be23f9233743954144a6

I will test the CL in #10 tomorrow on Canary and verify it's working as expected before we request merge approval.

Re merges for Dice: yes, we're aware that we've taken multiple merges for Dice. Justification:
- Dice is a hugely important multi-year project, with a lot of teams depending on us launching on a specific timeline (M67)
- We landed 95%+ of the code and CLs before feature freeze & branch point; these merges that are coming in now are for tweaks we realized we needed after feature freeze and branch
- We've been baking Dice on Canary/Dev since literally February (https://cs.corp.google.com/piper///depot/google3/googledata/googleclient/chrome/finch/studies/AccountConsistencyCanaryDev.json?dr=C&g=0), and have had no stability/performance regressions or issues. All of the CLs we're landing at this point are just polish & metrics tracking
- With the new merge policies (can merge these types of fixes through the first four weeks of Beta), we assumed this should be non-controversial

Hope that helps clarify!

I should add one more not to #13, so you are not surprised: there's still at least one more CL we'll plan to merge back for Dice. You can see the list of launch-blocking bugs that we're planning to merge back at go/chrome-dice-m3-mvp-hotlist (note one of those bugs is just tracking writing a pre-mortem, and so won't have any CLs associated with it to merge back).

Thank you ewald@ for clarification. Pls update the bug with canary result tomorrow.

Also it would be great if one more CL which you're planning to request a merge for landed in trunk by end of this week and request a merge to M67 on Monday morning. So we can pick it up for next week Beta release. 

The NextAction date has arrived: 2018-05-04

I checked that the fix works on Canary.

I've prepared the cherry-pick CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1044221

Approving merge to M67 branch 3396 based on comment #13 and #17. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/650354e40e1a474700ae4948ed9bc88b52135ffc

commit 650354e40e1a474700ae4948ed9bc88b52135ffc
Author: David Roger <droger@chromium.org>
Date: Fri May 04 15:13:21 2018

[signin] Clear refresh tokens when signin cookies are cleared on exit

When signin cookies are "cleared on exit" through the Chrome settings,
Dice would rebuild them on next startup, which defeats the point of
that cookie settings.
To fix this, tokens are not loaded when signin cookies are cleared on
exit. Instead, the tokens are revoked on the server, and deleted from
the database.
The secondary accounts are completely removed from Chrome, but the
primary account is kept in authentication error state, so that Sync
can display an error.

TBR=droger@chromium.org, eugenebut, treib

(cherry picked from commit 355781b89354303660792752cf9e0c9552d5e875)

Bug:  836862 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I5a72b3a173242494ad1c996765c838bb9a7f09f1
Reviewed-on: https://chromium-review.googlesource.com/1042392
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#555771}
Reviewed-on: https://chromium-review.googlesource.com/1044221
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#472}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/chrome/browser/search/one_google_bar/one_google_bar_service_factory.cc
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/chrome/browser/signin/profile_oauth2_token_service_factory.cc
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/components/signin/core/browser/BUILD.gn
[add] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/components/signin/core/browser/cookie_settings_util.cc
[add] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/components/signin/core/browser/cookie_settings_util.h
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/components/signin/core/browser/signin_header_helper.cc
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/components/signin/core/browser/signin_header_helper.h
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/ios/chrome/browser/signin/ios_chrome_signin_client.mm
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/ios/web_view/internal/signin/ios_web_view_signin_client.mm
[modify] https://crrev.com/650354e40e1a474700ae4948ed9bc88b52135ffc/tools/metrics/histograms/enums.xml

Removing the Restrict-View-Google. I don't see a reason to have this issue restricted, and I'd like to link it to external people.