Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710583003:
Network traffic annotation added to affiliation_fetcher. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 10 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, gcasto+watchlist_chromium.org, sdefresne+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, droger+watchlist_chromium.org, battre, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to affiliation_fetcher.
Network traffic annotation is added to network request of
password_manager/affiliation_fetcher.
BUG=656607
Review-Url: https://codereview.chromium.org/2710583003
Cr-Commit-Position: refs/heads/master@{#451925}
Committed: https://chromium.googlesource.com/chromium/src/+/cf490d43d2b0e49ff06958cba34c502db192717d
Patch Set 1 #
Total comments: 23
Patch Set 2 : Annotation updated. #
Total comments: 10
Patch Set 3 : Comments addressed. #
Messages
Total messages: 16 (6 generated)
rhalavati@chromium.org changed reviewers: + asanka@chromium.org, vasilii@chromium.org
As you know we are annotating network traffic requests. vasilii@: Please review both files. asanka@: Please review DEPS file as owner of /net. P.S., If you need any reference: Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
vasilii@chromium.org changed reviewers: + engedy@chromium.org
+engedy@ to verify my claims https://codereview.chromium.org/2710583003/diff/1/components/password_manager... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:96: net::DefineNetworkTrafficAnnotation("...", R"( android_affiliation https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:98: sender: "..." affiliation_fetcher https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:99: description: "..." For the users syncing their passwords to Google without a custom passphrase their password storage may contain credentials for Android apps. The service downloads the associations between the Android apps and the corresponding websites. Thus, the android credentials can be used while browsing the web. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:100: trigger: "..." On startup. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:101: data: "..." List of the Android apps the user has credentials for. The passwords and usernames aren't sent. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:102: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:106: setting: "..." One can either stop syncing the passwords to Google or introduce a custom passphrase to disable the service. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:108: [POLICY_NAME] { SyncDisabled https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:109: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} MANDATORY https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:110: value: ... True
All comments addressed, please review. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:96: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/21 10:37:58, vasilii wrote: > android_affiliation Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:98: sender: "..." On 2017/02/21 10:37:58, vasilii wrote: > affiliation_fetcher Done. Can it be "Android Affiliation Fetcher"? https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:99: description: "..." On 2017/02/21 10:37:58, vasilii wrote: > For the users syncing their passwords to Google without a custom passphrase > their password storage may contain credentials for Android apps. The service > downloads the associations between the Android apps and the corresponding > websites. Thus, the android credentials can be used while browsing the web. Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:100: trigger: "..." On 2017/02/21 10:37:58, vasilii wrote: > On startup. Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:101: data: "..." On 2017/02/21 10:37:58, vasilii wrote: > List of the Android apps the user has credentials for. The passwords and > usernames aren't sent. Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:102: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/21 10:37:58, vasilii wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:106: setting: "..." On 2017/02/21 10:37:58, vasilii wrote: > One can either stop syncing the passwords to Google or introduce a custom > passphrase to disable the service. Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:108: [POLICY_NAME] { On 2017/02/21 10:37:58, vasilii wrote: > SyncDisabled Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:109: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/21 10:37:58, vasilii wrote: > MANDATORY Done. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:110: value: ... On 2017/02/21 10:37:58, vasilii wrote: > True Done.
https://codereview.chromium.org/2710583003/diff/1/components/password_manager... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:98: sender: "..." On 2017/02/21 11:59:10, Ramin Halavati wrote: > On 2017/02/21 10:37:58, vasilii wrote: > > affiliation_fetcher > > Done. Can it be "Android Affiliation Fetcher"? I think it's confusing because it's running on desktop (and on Android). Maybe "Android credentials affiliation fetcher"?
Vasilii, thanks a lot! LGTM % comments. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:98: sender: "..." On 2017/02/21 12:03:20, vasilii wrote: > On 2017/02/21 11:59:10, Ramin Halavati wrote: > > On 2017/02/21 10:37:58, vasilii wrote: > > > affiliation_fetcher > > > > Done. Can it be "Android Affiliation Fetcher"? > > I think it's confusing because it's running on desktop (and on Android). Maybe > "Android credentials affiliation fetcher"? +1. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:96: net::DefineNetworkTrafficAnnotation("android_affiliation", R"( nit: How about affiliation_lookup? https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:100: "Password storage of users syncing their passwords to Google " Suggestion for first sentence: Users syncing their passwords may have credentials stored for Android apps. Unless synced data is encrypted with a custom passphrase, this service ... https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:103: "apps and the corresponding websites. Thus, the android " nit: Android https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:105: trigger: "On startup." Yes, it's most often on start-up, but there are also periodic updates and some edge cases, so I would write `Periodically in the background.`. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:116: "Chrome's settings under 'Sign In', 'advanced sync settings') or " nit: Advanced
All comments addressed. https://codereview.chromium.org/2710583003/diff/1/components/password_manager... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/1/components/password_manager... components/password_manager/core/browser/affiliation_fetcher.cc:98: sender: "..." On 2017/02/21 12:03:20, vasilii wrote: > On 2017/02/21 11:59:10, Ramin Halavati wrote: > > On 2017/02/21 10:37:58, vasilii wrote: > > > affiliation_fetcher > > > > Done. Can it be "Android Affiliation Fetcher"? > > I think it's confusing because it's running on desktop (and on Android). Maybe > "Android credentials affiliation fetcher"? Done. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:96: net::DefineNetworkTrafficAnnotation("android_affiliation", R"( On 2017/02/21 12:39:18, engedy (slow) wrote: > nit: How about affiliation_lookup? Done. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:100: "Password storage of users syncing their passwords to Google " On 2017/02/21 12:39:18, engedy (slow) wrote: > Suggestion for first sentence: > > Users syncing their passwords may have credentials stored for Android apps. > Unless synced data is encrypted with a custom passphrase, this service ... > Done. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:103: "apps and the corresponding websites. Thus, the android " On 2017/02/21 12:39:18, engedy (slow) wrote: > nit: Android Done. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:105: trigger: "On startup." On 2017/02/21 12:39:18, engedy (slow) wrote: > Yes, it's most often on start-up, but there are also periodic updates and some > edge cases, so I would write `Periodically in the background.`. Done. https://codereview.chromium.org/2710583003/diff/20001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:116: "Chrome's settings under 'Sign In', 'advanced sync settings') or " On 2017/02/21 12:39:18, engedy (slow) wrote: > nit: Advanced Done.
lgtm
lgtm lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2710583003/#ps40001 (title: "Comments addressed.")
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": 40001, "attempt_start_ts": 1487745439374490,
"parent_rev": "a7b1d4e984b58061e973c31de0c7b403ec14f42c", "commit_rev":
"cf490d43d2b0e49ff06958cba34c502db192717d"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to affiliation_fetcher. Network traffic annotation is added to network request of password_manager/affiliation_fetcher. BUG=656607 ========== to ========== Network traffic annotation added to affiliation_fetcher. Network traffic annotation is added to network request of password_manager/affiliation_fetcher. BUG=656607 Review-Url: https://codereview.chromium.org/2710583003 Cr-Commit-Position: refs/heads/master@{#451925} Committed: https://chromium.googlesource.com/chromium/src/+/cf490d43d2b0e49ff06958cba34c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/cf490d43d2b0e49ff06958cba34c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
