Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949243004:
Distinguish G Suite accounts from regular gmail/googlemail accounts (Closed)
|
Created:
3 years, 6 months ago by Jialiu Lin Modified:
3 years, 5 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, grt+watch_chromium.org, timvolodine, asvitkine+watch_chromium.org, gcasto+watchlist_chromium.org, vakh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDistinguish G Suite accounts from regular gmail/googlemail accounts
BUG=733720
Review-Url: https://codereview.chromium.org/2949243004
Cr-Commit-Position: refs/heads/master@{#483034}
Committed: https://chromium.googlesource.com/chromium/src/+/7f09ef38dd2e5ab393cfe2d37bac05c9de4ee21d
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 7
Patch Set 3 : nit #Patch Set 4 : fix chrome os compilation #Patch Set 5 : Adding account_id #Patch Set 6 : nits #
Total comments: 4
Patch Set 7 : address lpz's comments #
Total comments: 8
Patch Set 8 : address nparker's comments #Patch Set 9 : nits #Messages
Total messages: 55 (39 generated)
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:20001) has been deleted
jialiul@chromium.org changed reviewers: + rogerta@chromium.org
Hi rogerta@, Could you take a look at the change in chrome_password_protection_service.cc file? Basically, we want to distinguish if a safe browsing ping is from a signed in gmail user , @google.com user or a dasher account user. Let me know if my implementation makes sense. https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:153: ChromePasswordProtectionService::GetSyncAccountType() { +rogerta@, Does this function make sense? Thanks!
Looks fine Jialiu. One comment and one question for you below. https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:164: signin_manager->GetAuthenticatedAccountId()); No need to get the account tracker in this case. Just get the signin manager and call: AccountInfo account_info = signin_manager->GetAuthenticatedAccountInfo(); https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:171: std::string(AccountTrackerService::kNoHostedDomainFound)) { In theory "kNoHostedDomainFound" means we don't know. Do you want to treat this as gmail.com?
https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:164: signin_manager->GetAuthenticatedAccountId()); On 2017/06/23 15:25:38, Roger Tawa wrote: > No need to get the account tracker in this case. Just get the signin manager > and call: > > AccountInfo account_info = signin_manager->GetAuthenticatedAccountInfo(); Ah, that simplifies things. Good to know! Thanks! https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:171: std::string(AccountTrackerService::kNoHostedDomainFound)) { On 2017/06/23 15:25:38, Roger Tawa wrote: > In theory "kNoHostedDomainFound" means we don't know. Do you want to treat this > as gmail.com? Oh, I misunderstood. In that case, I'd better treat it as user not signed in. Can you give me some examples where "kNoHostedDomainFound" can happen?
Hi Roger, Another thing I notice during testing. If I pass in an empty string as hosted_domain when I call FakeUserInfoFetchSuccess(), the resulting AccountInfo will have kNoHostedDomainFound host. How should I test for the gmail.com case then? What hostname should I pass into FakeUserInfoFetchSuccess() ? Thanks!
+Anthony, Mike Hi Jialiu, Maybe Anthony or Mike might be able to answer you question. Thanks, Roger On Jun 23, 2017 11:51, <jialiul@chromium.org> wrote: Hi Roger, Another thing I notice during testing. If I pass in an empty string as hosted_domain when I call FakeUserInfoFetchSuccess(), the resulting AccountInfo will have kNoHostedDomainFound host. How should I test for the gmail.com case then? What hostname should I pass into FakeUserInfoFetchSuccess() ? Thanks! https://codereview.chromium.org/2949243004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/06/23 18:01:26, Roger Tawa wrote: > +Anthony, Mike > > Hi Jialiu, > > Maybe Anthony or Mike might be able to answer you question. > > Thanks, > Roger > > On Jun 23, 2017 11:51, <mailto:jialiul@chromium.org> wrote: > > Hi Roger, > Another thing I notice during testing. If I pass in an empty string as > hosted_domain when I call FakeUserInfoFetchSuccess(), the resulting > AccountInfo > will have kNoHostedDomainFound host. > > How should I test for the http://gmail.com case then? What hostname should I pass > into > FakeUserInfoFetchSuccess() ? > > Thanks! > > https://codereview.chromium.org/2949243004/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. If I recall correctly, we get an empty value in this field for any non-SAML sign in. Which means for gmail.com, this field will and should be empty.
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Roger! I have resolved all my questions with mlerman@. https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:153: ChromePasswordProtectionService::GetSyncAccountType() { On 2017/06/23 15:08:16, Jialiu Lin wrote: > +rogerta@, > Does this function make sense? Thanks! Done. https://codereview.chromium.org/2949243004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_password_protection_service.cc:171: std::string(AccountTrackerService::kNoHostedDomainFound)) { On 2017/06/23 15:34:34, Jialiu Lin wrote: > On 2017/06/23 15:25:38, Roger Tawa wrote: > > In theory "kNoHostedDomainFound" means we don't know. Do you want to treat > this > > as gmail.com? > > Oh, I misunderstood. In that case, I'd better treat it as user not signed in. > > Can you give me some examples where "kNoHostedDomainFound" can happen? Resolved with mlerman offline. For gmail/googlemail accounts, GAIA service show empty hosted domain, and the account fetcher will set it to kNoHostedDomainFound.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:140001) has been deleted
jialiul@chromium.org changed reviewers: + lpz@chromium.org, nparker@chromium.org
nparker@ and lpz@, PTAL. Thanks!
lgtm https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... components/safe_browsing/csd.proto:232: DASHER = 3; nit: dasher is an internal name, right? Maybe rename to something more recognizable (eg: GSUITE)? https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:159: 1); nit: move to prev line?
jialiul@chromium.org changed reviewers: + dvadym@chromium.org, holte@chromium.org
Thanks, lpz@! +holte@ for histogram review +dvadym@ for chrome/browser/password_manager/* Thanks! https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... components/safe_browsing/csd.proto:232: DASHER = 3; On 2017/06/27 18:50:10, lpz wrote: > nit: dasher is an internal name, right? Maybe rename to something more > recognizable (eg: GSUITE)? Ah! thanks for catching this. I totally forgot "Dasher" is an internal name. https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2949243004/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:159: 1); On 2017/06/27 18:50:10, lpz wrote: > nit: move to prev line? oops, git cl format has weird taste.
Description was changed from ========== Distinguish Dasher accounts from regular gmail/google accounts BUG=733720 ========== to ========== Distinguish G Suite accounts from regular gmail/google accounts BUG=733720 ==========
lgtm https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:174: return account_info.hosted_domain == "google.com" Is this google.com as in Googlers? Is there a use case for distinguishing this from other gsuite-like accounts? Maybe... https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:175: ? LoginReputationClientRequest_PasswordReuseEvent::GOOGLE s/_/:: here, to match that above. https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc (right): https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:37: const char kTestGaia[] = "gaia"; What is this string? Is this an account type? https://codereview.chromium.org/2949243004/diff/200001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2949243004/diff/200001/components/safe_browsi... components/safe_browsing/csd.proto:223: NOT_SIGNED_IN = 0; Any idea when/why this could happen? Would we expect to see any users in this bucket?
histograms lgtm
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Distinguish G Suite accounts from regular gmail/google accounts BUG=733720 ========== to ========== Distinguish G Suite accounts from regular gmail/googlemail accounts BUG=733720 ==========
chrome/browser/password_manager/* LGTM
Thank you all! https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right): https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:174: return account_info.hosted_domain == "google.com" On 2017/06/27 23:06:30, Nathan Parker wrote: > Is this http://google.com as in Googlers? Is there a use case for distinguishing this > from other gsuite-like accounts? Maybe... Initially, I think this could be a strong predictor (i.e. a large group of googlers reused their password -> false positives). But after second thought, these cases might be caught by PasswordAlert before us.... Let me remove this category. https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service.cc:175: ? LoginReputationClientRequest_PasswordReuseEvent::GOOGLE On 2017/06/27 23:06:30, Nathan Parker wrote: > s/_/:: here, to match that above. Done. https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc (right): https://codereview.chromium.org/2949243004/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:37: const char kTestGaia[] = "gaia"; On 2017/06/27 23:06:30, Nathan Parker wrote: > What is this string? Is this an account type? It is a test account id. Changed into a more generic string. https://codereview.chromium.org/2949243004/diff/200001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2949243004/diff/200001/components/safe_browsi... components/safe_browsing/csd.proto:223: NOT_SIGNED_IN = 0; On 2017/06/27 23:06:30, Nathan Parker wrote: > Any idea when/why this could happen? Would we expect to see any users in this > bucket? It covers the case such as we cannot get the sync account info from profile, or somehow between password reuse detection and sending out ping, user state changes from sign-in to sign-out. (Given a slow network, and an async whitelist checking, this in theory could happen).
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpz@chromium.org, nparker@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2949243004/#ps240001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1498664435883170,
"parent_rev": "eb7b7e12f341305f3db7e0dfe02fc1c2bc28874d", "commit_rev":
"7f09ef38dd2e5ab393cfe2d37bac05c9de4ee21d"}
Message was sent while issue was closed.
Description was changed from ========== Distinguish G Suite accounts from regular gmail/googlemail accounts BUG=733720 ========== to ========== Distinguish G Suite accounts from regular gmail/googlemail accounts BUG=733720 Review-Url: https://codereview.chromium.org/2949243004 Cr-Commit-Position: refs/heads/master@{#483034} Committed: https://chromium.googlesource.com/chromium/src/+/7f09ef38dd2e5ab393cfe2d37bac... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/7f09ef38dd2e5ab393cfe2d37bac... |
