Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
880943002:
UMA statistics for Linux password storage backend usage (Closed)
|
Created:
5 years, 11 months ago by dvadym Modified:
5 years, 10 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, asvitkine+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUMA statistics for Linux password storage backend usage
Our main goal is to understand how often is the situation when no security storage is available on Linux and plain text (actually SQL lite db) is used for password storage.
BUG=355223
Committed: https://crrev.com/563ef4b9ff570ae32dad945685e49eac18c1a337
Cr-Commit-Position: refs/heads/master@{#313691}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Added more cases for statistics. #
Total comments: 5
Patch Set 3 : Fixed comments. #
Total comments: 6
Patch Set 4 : Fix comments. #
Messages
Total messages: 21 (6 generated)
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Could you please review this CL? Best regards, Vadym
Hi Vadym, This looks good, only I'm not sure if we cover all the cases, see my comment. Cheers, Vaclav https://codereview.chromium.org/880943002/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_store_factory.cc (right): https://codereview.chromium.org/880943002/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_store_factory.cc:306: LinuxBackendUsage usage = OTHER_PLAINTEXT; But what about cases when the user forces a store through the flag without running the corresponding backend? I'm mostly thinking about people not running any of the mentioned desktop environments, who still somehow manage to run an instance of the keyring or the wallet, and use those. If that can happen, we should log it. On a related note -- if the above can happen, than perhaps, e.g., a person running KDE could still use the Gnome Keyring? If yes, we need to reflect it as well. https://codereview.chromium.org/880943002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/880943002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23211: + Information about usage password storage backends on Linux. It also includes usage -> the usage of https://codereview.chromium.org/880943002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23212: + if command line flag for specific backend is given. if -> whether a specific -> a specific
Thanks, Vaclav. I added new cases. PTAL https://codereview.chromium.org/880943002/diff/1/chrome/browser/password_mana... File chrome/browser/password_manager/password_store_factory.cc (right): https://codereview.chromium.org/880943002/diff/1/chrome/browser/password_mana... chrome/browser/password_manager/password_store_factory.cc:306: LinuxBackendUsage usage = OTHER_PLAINTEXT; On 2015/01/27 16:15:51, vabr (Chromium) wrote: > But what about cases when the user forces a store through the flag without > running the corresponding backend? I'm mostly thinking about people not running > any of the mentioned desktop environments, who still somehow manage to run an > instance of the keyring or the wallet, and use those. If that can happen, we > should log it. > > On a related note -- if the above can happen, than perhaps, e.g., a person > running KDE could still use the Gnome Keyring? If yes, we need to reflect it as > well. Done. https://codereview.chromium.org/880943002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/880943002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23212: + if command line flag for specific backend is given. On 2015/01/27 16:15:51, vabr (Chromium) wrote: > if -> whether a > specific -> a specific Done.
Thanks, Vadym. This LGTM, even though I wonder if we should also log the flag usage outside KDE/Gnome (see the comment below). Cheers, Vaclav https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_store_factory.cc (right): https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_store_factory.cc:347: // It is not Gnome nor KDE environment. grammar: not -> neither https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_store_factory.h (right): https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_store_factory.h:99: OTHER_PLAINTEXT, Should we also care about if the user uses the flag outside of KDE/Gnome?
uikmano@gmail.com changed reviewers: + uikmano@gmail.com
lgtm https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_store_factory.h (right): https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_store_factory.h:99: OTHER_PLAINTEXT, On 2015/01/28 12:26:23, vabr (Chromium) wrote: > Should we also care about if the user uses the flag outside of KDE/Gnome? Done.
The CQ bit was checked by uikmano@gmail.com
lgtm lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880943002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks Vaclav. https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_store_factory.cc (right): https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_store_factory.cc:347: // It is not Gnome nor KDE environment. On 2015/01/28 12:26:23, vabr (Chromium) wrote: > grammar: not -> neither Done. https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_store_factory.h (right): https://codereview.chromium.org/880943002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_store_factory.h:99: OTHER_PLAINTEXT, On 2015/01/28 12:26:23, vabr (Chromium) wrote: > Should we also care about if the user uses the flag outside of KDE/Gnome? After offline discussion we decided that it doen't give more information to find out cases when on other environment flags are unsuccessful used.
dvadym@chromium.org changed reviewers: + isherman@chromium.org - uikmano@gmail.com
Hi Ilya, Could you please review histograms.xml? Best regards, Vadym
Histograms LGTM % nits: https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23211: + Information about usage password storage backends on Linux. It also includes nit: "usage password storage" -> "usage of password storage" https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23212: + wheather a command line flag for specific backend is given. nit: "wheather" -> "whether" https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23212: + wheather a command line flag for specific backend is given. nit: "for specific" -> "for a specific"
Thanks Ilya! Fixed. https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23211: + Information about usage password storage backends on Linux. It also includes On 2015/01/28 19:56:26, Ilya Sherman wrote: > nit: "usage password storage" -> "usage of password storage" Done. https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23212: + wheather a command line flag for specific backend is given. On 2015/01/28 19:56:26, Ilya Sherman wrote: > nit: "wheather" -> "whether" Done. https://codereview.chromium.org/880943002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23212: + wheather a command line flag for specific backend is given. On 2015/01/28 19:56:26, Ilya Sherman wrote: > nit: "for specific" -> "for a specific" Done.
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880943002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/563ef4b9ff570ae32dad945685e49eac18c1a337 Cr-Commit-Position: refs/heads/master@{#313691} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
