[Extension Telemetry] Add potential password theft signal. [chromium/src : main]
Zack Han (Gerrit)
Attention is currently required from: Zack Han.
Set Ready For Review
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Zack Han (Gerrit)
1 comment:
File chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc:
Patch Set #2, Line 192:
new RemoteHostContactedSignalProcessor();Use smart pointer instead.
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Zack Han (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr, Xinghui Lu.
Zack Han would like Jean-Francois Cyr, Anunoy Ghosh and Xinghui Lu to review this change.
[Extension Telemetry] Add potential password theft signal.
Change-Id: Icb1a374cd513fc0c0c9acabcb5fc2c99962b53ff
---
M chrome/browser/BUILD.gn
M chrome/browser/password_manager/BUILD.gn
M chrome/browser/password_manager/chrome_password_manager_client.cc
M chrome/browser/password_manager/chrome_password_manager_client.h
M chrome/browser/safe_browsing/BUILD.gn
M chrome/browser/safe_browsing/extension_telemetry/extension_signal.h
M chrome/browser/safe_browsing/extension_telemetry/extension_signal_processor.h
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h
A chrome/browser/safe_browsing/extension_telemetry/password_reuse_signal.cc
A chrome/browser/safe_browsing/extension_telemetry/password_reuse_signal.h
A chrome/browser/safe_browsing/extension_telemetry/potential_password_theft_signal_processor.cc
A chrome/browser/safe_browsing/extension_telemetry/potential_password_theft_signal_processor.h
A chrome/browser/safe_browsing/extension_telemetry/potential_password_theft_signal_processor_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/remote_host_contacted_signal_processor.cc
M chrome/browser/safe_browsing/extension_telemetry/remote_host_contacted_signal_processor.h
M chrome/browser/safe_browsing/extension_telemetry/remote_host_contacted_signal_processor_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/tabs_execute_script_signal_processor.cc
M chrome/browser/safe_browsing/extension_telemetry/tabs_execute_script_signal_processor.h
M chrome/browser/safe_browsing/extension_telemetry/tabs_execute_script_signal_processor_unittest.cc
M chrome/test/BUILD.gn
M components/password_manager/core/browser/password_manager_client.cc
M components/password_manager/core/browser/password_manager_client.h
M components/password_manager/core/browser/password_manager_test_utils.h
M components/password_manager/core/browser/password_reuse_detection_manager.cc
M components/password_manager/core/browser/password_reuse_detection_manager.h
M components/password_manager/core/browser/password_reuse_detector.cc
M components/password_manager/core/browser/password_reuse_detector.h
M components/password_manager/core/browser/password_reuse_detector_consumer.h
M components/password_manager/core/browser/password_reuse_manager_impl.cc
M components/safe_browsing/content/browser/password_protection/password_protection_service.cc
M components/safe_browsing/content/browser/password_protection/password_protection_service.h
M components/safe_browsing/core/browser/password_protection/metrics_util.cc
M components/safe_browsing/core/browser/password_protection/metrics_util.h
M components/safe_browsing/core/browser/password_protection/password_protection_service_base.cc
M components/safe_browsing/core/browser/password_protection/password_protection_service_base.h
M components/safe_browsing/core/common/features.cc
M components/safe_browsing/core/common/features.h
M components/safe_browsing/core/common/proto/csd.proto
M tools/metrics/histograms/enums.xml
40 files changed, 1,184 insertions(+), 40 deletions(-)
Anunoy Ghosh (Gerrit)
Attention is currently required from: Jean-Francois Cyr, Xinghui Lu, Zack Han.
12 comments:
Patchset:
Thanks Zack! I have an initial set of comments about how to organize the signals and processors in the ExtensionTelemetry service. I also want to make sure we are not introducing unnecessary dependencies on telemetry service in non-embedder specific code. We should probably talk offline about that.
Xing, JF: I suggest you hold off reviewing till Zack and I discuss some refactoring.
JF: you are welcome to review the whole CL but you can limit your review to the csd.proto changes which show what data is being sent up to the server.
File chrome/browser/password_manager/chrome_password_manager_client.h:
Patch Set #2, Line 268:
void NotifyExtensionPotentialPasswordTheft(This should be named something like NotifyPasswordReuseInExtension since that is what is being detected here. The PasswordTheftSignal is generated by the corresponding processor by consuming the PasswordReuse signal AND the RemoteHostContacted signal.
File chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h:
Patch Set #2, Line 171:
SignalProcessors signal_processors_;We actually need 2 containers:
1. signal processors
- a map of (signal -> processor that generates corresponding telemetry report)
- Suggest keeping this the same as the previous code.
2. signal subscribers
- a map of (signal -> vector of processors that consume the signal)
- Rename your SignalProcessors type and member (as defined in the lines above) to
- SignalSubscribers
Also declare signal_subscribers_ after signal_processors_ for proper shutdown for the service (may not be strictly necessary though)
Patch Set #2, Line 173:
ExtensionSignalProcessor* tab_execute_signal_processor_;We don't need to individually declare these as they will be contained in the SignalProcessors container. Please remove.
File chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc:
Patch Set #2, Line 152:
ExtensionTelemetryService::~ExtensionTelemetryService() {}use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Lint observed on `linux-chromeos-clang-tidy-rel` and `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Please fix.
Patch Set #2, Line 192:
new RemoteHostContactedSignalProcessor();Use smart pointer instead.
Put the original code back here that created the map of signal processors.
Patch Set #2, Line 196:
// Create signal processors.This comment line applies to the code immediately preceding it. The code that follows should have a comment such as:
// Create subscriber lists for each telemetry signal typePatch Set #2, Line 197:
std::vector<ExtensionSignalProcessor*>replace these lines to use the signal_processors_ map. So something like:
```
std::vector<ExtensionSignalProcessor*> subscribers_for_remote_host_contacted = {
signal_processors_[ExtensionSignalType::kRemoteHostContacted].get(),
signal_processors_[ExtensionSignalType::kPasswordReuse].get()};
```Patch Set #2, Line 257:
signal_processors_.clear();here we should first clear the signal_subscribers_ and then the signal_processors_
Patch Set #2, Line 276:
if (tab_execute_signal_processor_) {We won't need these once we make the signal processor members unique ptrs.
Patch Set #2, Line 317:
for (auto* processor : signal_processors_[signal_type]) {we will iterate over signal_subscribers_ for the signal (see previous comment about the 2 containers)
Patch Set #2, Line 444:
for (auto* processor : processor_it.second) {you should just have to iterate over signal_procesors_ since there is only 1 instance of each and each processor will generate the necessary report.
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Zack Han (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr, Xinghui Lu.
11 comments:
Patchset:
Thanks anunoy@ for the quick review! I've addressed the issues and go through the diffs again to make some revisions again.
File chrome/browser/password_manager/chrome_password_manager_client.h:
Patch Set #2, Line 268:
void NotifyExtensionPotentialPasswordTheft(
This should be named something like NotifyPasswordReuseInExtension since that is what is being detec […]
Done
File chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h:
Patch Set #2, Line 173:
ExtensionSignalProcessor* tab_execute_signal_processor_;We don't need to individually declare these as they will be contained in the SignalProcessors contai […]
Done
File chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc:
Patch Set #2, Line 152:
ExtensionTelemetryService::~ExtensionTelemetryService() {}
> use '= default' to define a trivial destructor (https://clang.llvm. […]
Done
Patch Set #2, Line 192:
new RemoteHostContactedSignalProcessor();
Put the original code back here that created the map of signal processors.
Done
Patch Set #2, Line 196:
// Create signal processors.
This comment line applies to the code immediately preceding it. […]
Done
Patch Set #2, Line 197:
std::vector<ExtensionSignalProcessor*>
replace these lines to use the signal_processors_ map. So something like: […]
Done
Patch Set #2, Line 257:
signal_processors_.clear();here we should first clear the signal_subscribers_ and then the signal_processors_
Done
Patch Set #2, Line 276:
if (tab_execute_signal_processor_) {We won't need these once we make the signal processor members unique ptrs.
Done
Patch Set #2, Line 317:
for (auto* processor : signal_processors_[signal_type]) {
we will iterate over signal_subscribers_ for the signal (see previous comment about the 2 containers […]
Done
Patch Set #2, Line 444:
for (auto* processor : processor_it.second) {
you should just have to iterate over signal_procesors_ since there is only 1 instance of each and ea […]
Done
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Xinghui Lu (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr.
1 comment:
Patchset:
Thanks Zack! I have an initial set of comments about how to organize the signals and processors in t […]
Sounds good, please add me back to the attention set when it is ready.
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Xinghui Lu (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr, Zack Han.
1 comment:
Patchset:
Just a quick high-level suggestion: Consider splitting this CL into multiple smaller CLs (go/small-cls).
At a glance, I think it can be splitted into at least three parts:
1 The signal processors in extension telemetry.
2 Sending signals from password reuse detection manager to password protection service.
3 Sending signals from password protection service to extension telemetry.
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Zack Han (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr, Xinghui Lu, Zack Han.
1 comment:
Patchset:
Just a quick high-level suggestion: Consider splitting this CL into multiple smaller CLs (go/small-c […]
Thanks Xing for the suggestion, it makes sense. I will separate this CL into 2 instead. Leaving the password protection service related code in one CL because they are related and the changes aren't a lot and signal processors in extension telemetry in another CL.
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Xinghui Lu (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr, Zack Han, Zack Han.
1 comment:
Patchset:
Thanks Xing for the suggestion, it makes sense. I will separate this CL into 2 instead. […]
That makes sense. Thanks Zack!
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Zack Han (Gerrit)
Attention is currently required from: Anunoy Ghosh, Jean-Francois Cyr, Zack Han.
1 comment:
Patchset:
Hey all, I have split this CL into 2:
http://crrev/c/3788301
http://crrev/c/3788302
Please feel free to take a look at those two. The first has the password detection changes and the second includes the telemetry service changes. Thanks!
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.
Zack Han (Gerrit)
Zack Han abandoned this change.
To view, visit change 3770662. To unsubscribe, or for help writing mail filters, visit settings.