Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929573002:
Changed location of LibsecretLoader (Closed)
|
Created:
4 years, 7 months ago by cfroussios Modified:
4 years, 7 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_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. |
DescriptionChanged location of Libsecret utils
Moved classes LibsecretLoader and LibsecretAttributeBuilder
from password_manager to os_crypt. In the future,
password_manager will not interact with libsecret directly,
while os_crypt will. Responsibility is therefore
transfered. For now, password_manager continues to use
those utilites.
Currently, Password Manager stores passwords one by one in
libsecret. With this and future CL(s), we will only store
an encryption key with libsecret and use said key to
protect a local database, managed by the Password Manager.
BUG=602624
TBR=caitkp@chromium.org for components/os_crypt.gypi
Committed: https://crrev.com/df99cec1248d89188156bf53aba322c3a9e0f0bb
Cr-Commit-Position: refs/heads/master@{#390706}
Patch Set 1 #Patch Set 2 : password_manager explicitly includes libsecret.h #
Total comments: 19
Patch Set 3 : Fixed gyp for os_crypt #Patch Set 4 : Applied refactoring recommendations #Patch Set 5 : Added omitted reference to open bug #Patch Set 6 : Fixed compilation error #
Total comments: 8
Patch Set 7 : Moved omitted implementation and also moved LibsecretAttributesBuilder #
Total comments: 8
Patch Set 8 : LibsecretLoader no longer inherited from + linting #
Total comments: 16
Patch Set 9 : Documentation, renaming to libsecret_utils_posix, introduced EnsureLibsecretLoaded method #Patch Set 10 : Documentation reflects change #
Total comments: 10
Patch Set 11 : Nits #Patch Set 12 : Caught required attribute in dummy schema #Patch Set 13 : Fixed memory errors #
Total comments: 1
Patch Set 14 : Added TODO and reference to bug #
Messages
Total messages: 34 (12 generated)
Description was changed from ========== Changed location of LibsecretLoader Moved class LibsecretLoader from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use LibsecretLoader BUG=602624 ========== to ========== Changed location of LibsecretLoader Moved class LibsecretLoader from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use LibsecretLoader BUG=602624 ==========
cfroussios@chromium.org changed reviewers: + vabr@chromium.org
Thanks, this looks mostly good, I only had a couple of comments to improve the already existing code while you are moving it. But note that asan trybot error is real. I'm not sure, but it might be that that bot is still running GYP, and you will need to update components/os_crypt.gypi to make it work. (All Linux bots are just being converted to GN, so updating GYP for Linux-only changes will not be required, but probably that's still not completed yet.) Cheers, Vaclav https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.cc (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: No (c). https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:11: typeof(&::secret_password_store_sync) Please use C++11 decltype instead of the non-standard typeof. decltype is already allowed to use in Chromium (http://chromium-cpp.appspot.com/), it might not have been at the time the moved code was written. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:44: bool LibsecretLoader::LoadLibsecret() { nit: For all static methods the convention is to add a line // static just before the first line of the definition. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:53: LOG(WARNING) << "Could not load libsecret-1.so.0: " << dlerror(); LOG(WARNING) is discouraged [1]. Please replace this with VLOG(1) and add // TODO(crbug.com/607435) Channel this message to the user-facing log. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:57: for (size_t i = 0; functions[i].name; ++i) { If you use the range-based for loop, you can get rid of the nullptr,nullptr pair in functions[]: for (const auto& function : functions) { ... } https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: No (c), see http://dev.chromium.org/developers/coding-style#TOC-File-headers . https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; Please add DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); as the very last line in the private section, separated by a blank line above. (The purpose of the macro is to prevent creating instances of the class. This is an all-static class, so creating an instance would typically mean a typo in the code.)
Also, before you loop in the os_crypt OWNER, thestig@, it might be a good idea to elaborate in the CL description on why os_crypt will use LibsecretLoader in the future: because the key storage for encryption will be used through libsecret, and that change comes in a separate CL. Cheers, Vaclav
Description was changed from ========== Changed location of LibsecretLoader Moved class LibsecretLoader from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use LibsecretLoader BUG=602624 ========== to ========== Changed location of LibsecretLoader Moved class LibsecretLoader from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use LibsecretLoader Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by Chrome. BUG=602624 ==========
https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.cc (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/04/28 07:53:21, vabr (Chromium) wrote: > nit: No (c). Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:11: typeof(&::secret_password_store_sync) On 2016/04/28 07:53:20, vabr (Chromium) wrote: > Please use C++11 decltype instead of the non-standard typeof. decltype is > already allowed to use in Chromium (http://chromium-cpp.appspot.com/), it might > not have been at the time the moved code was written. Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:44: bool LibsecretLoader::LoadLibsecret() { On 2016/04/28 07:53:21, vabr (Chromium) wrote: > nit: For all static methods the convention is to add a line > // static > just before the first line of the definition. Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:53: LOG(WARNING) << "Could not load libsecret-1.so.0: " << dlerror(); On 2016/04/28 07:53:21, vabr (Chromium) wrote: > LOG(WARNING) is discouraged [1]. Please replace this with VLOG(1) and add > // TODO(crbug.com/607435) Channel this message to the user-facing log. Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.cc:57: for (size_t i = 0; functions[i].name; ++i) { On 2016/04/28 07:53:21, vabr (Chromium) wrote: > If you use the range-based for loop, you can get rid of the nullptr,nullptr pair > in functions[]: > > for (const auto& function : functions) { ... } Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/04/28 07:53:21, vabr (Chromium) wrote: > nit: No (c), see > http://dev.chromium.org/developers/coding-style#TOC-File-headers . Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; On 2016/04/28 07:53:21, vabr (Chromium) wrote: > Please add > DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); > > as the very last line in the private section, separated by a blank line above. > > (The purpose of the macro is to prevent creating instances of the class. This is > an all-static class, so creating an instance would typically mean a typo in the > code.) NativeBackendLibsecret inherits from LibsecretLoader and therefore LibsecretLoader needs to be instantiated. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... I'm adding DISALLOW_COPY_AND_ASSIGN instead.
Thanks! Some more comments below. Cheers, Vaclav https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; On 2016/04/28 12:18:11, cfroussios wrote: > On 2016/04/28 07:53:21, vabr (Chromium) wrote: > > Please add > > DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); > > > > as the very last line in the private section, separated by a blank line above. > > > > (The purpose of the macro is to prevent creating instances of the class. This > is > > an all-static class, so creating an instance would typically mean a typo in > the > > code.) > > NativeBackendLibsecret inherits from LibsecretLoader and therefore > LibsecretLoader needs to be instantiated. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... > > I'm adding DISALLOW_COPY_AND_ASSIGN instead. It does not really make sense to derive from a static-only class, including the questionable benefits of the protected visibility. When we are already cleaning this up, I suggest: (1) Make LoadLibsecret() and IsLibsecretAvailable() public. (2) Make libsecret_loaded private. (3) Change NativeBackendLibsecret to stop inheriting from LibsecretLoader, and add LibsecretLoader:: to the calls in NativeBackendLibsecret::Init. (4) Apply DISALLOW_IMPLICIT_CONSTRUCTORS to LibsecretLoader and delete the default constructor. https://codereview.chromium.org/1929573002/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_libsecret.cc (left): https://codereview.chromium.org/1929573002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_libsecret.cc:270: bool LibsecretLoader::LibsecretIsAvailable() { Move this implementation to os_crypt as well. https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... File components/os_crypt/libsecret_loader.cc (right): https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... components/os_crypt/libsecret_loader.cc:43: //static nit: The style requires a space between the "//" and the "static". https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... components/os_crypt/libsecret_loader.cc:53: // TODO(crbug.com/607435) nit: Please also add the TODO message itself, like: "Channel this message to the user-facing log." The bug itself does not mention this code, so it's not clear what the TODO means without the message. https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... components/os_crypt/libsecret_loader.h:34: DISALLOW_COPY_AND_ASSIGN(LibsecretLoader); nit: This still needs to be at the very end of the private section, see https://google.github.io/styleguide/cppguide.html#Declaration_Order
I have addressed your comments. I also moved an additional utility, LibsecretAttributeBuilder, which was used by LibsecretLoader. I therefore renamed the files to os_crypt_posix_util_libsecret https://codereview.chromium.org/1929573002/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_libsecret.cc (left): https://codereview.chromium.org/1929573002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_libsecret.cc:270: bool LibsecretLoader::LibsecretIsAvailable() { On 2016/04/28 12:44:25, vabr (Chromium) wrote: > Move this implementation to os_crypt as well. Done. https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... File components/os_crypt/libsecret_loader.cc (right): https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... components/os_crypt/libsecret_loader.cc:43: //static On 2016/04/28 12:44:25, vabr (Chromium) wrote: > nit: The style requires a space between the "//" and the "static". Done. https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... components/os_crypt/libsecret_loader.cc:53: // TODO(crbug.com/607435) On 2016/04/28 12:44:25, vabr (Chromium) wrote: > nit: Please also add the TODO message itself, like: "Channel this message to the > user-facing log." > The bug itself does not mention this code, so it's not clear what the TODO means > without the message. Done. https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/100001/components/os_crypt/li... components/os_crypt/libsecret_loader.h:34: DISALLOW_COPY_AND_ASSIGN(LibsecretLoader); On 2016/04/28 12:44:25, vabr (Chromium) wrote: > nit: This still needs to be at the very end of the private section, see > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done.
Description was changed from ========== Changed location of LibsecretLoader Moved class LibsecretLoader from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use LibsecretLoader Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by Chrome. BUG=602624 ========== to ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 ==========
Thanks, Christos. Please see the comments below (+one ping of an older comment which got lost). With those, LGTM. The move and name change makes sense to me, but that's ultimately to the directory OWNER to approve. Cheers, Vaclav https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; On 2016/04/28 12:44:25, vabr (Chromium) wrote: > On 2016/04/28 12:18:11, cfroussios wrote: > > On 2016/04/28 07:53:21, vabr (Chromium) wrote: > > > Please add > > > DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); > > > > > > as the very last line in the private section, separated by a blank line > above. > > > > > > (The purpose of the macro is to prevent creating instances of the class. > This > > is > > > an all-static class, so creating an instance would typically mean a typo in > > the > > > code.) > > > > NativeBackendLibsecret inherits from LibsecretLoader and therefore > > LibsecretLoader needs to be instantiated. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... > > > > I'm adding DISALLOW_COPY_AND_ASSIGN instead. > > It does not really make sense to derive from a static-only class, including the > questionable benefits of the protected visibility. When we are already cleaning > this up, I suggest: > (1) Make LoadLibsecret() and IsLibsecretAvailable() public. > (2) Make libsecret_loaded private. > (3) Change NativeBackendLibsecret to stop inheriting from LibsecretLoader, and > add LibsecretLoader:: to the calls in NativeBackendLibsecret::Init. > (4) Apply DISALLOW_IMPLICIT_CONSTRUCTORS to LibsecretLoader and delete the > default constructor. Ping. https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.cc (right): https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.cc:75: bool LibsecretLoader::LibsecretIsAvailable() { // static https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.h (right): https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:5: #ifndef COMPONENTS_OS_CRYPT_LIBSECRET_LOADER_H_ nit: Don't forget to update the #include guard. (If you run "git cl lint", it will even tell you which one you should use.) https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:49: void Append(const std::string& name, const std::string& value); nit: Please add blank lines between method declarations. https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:59: }; Add DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; On 2016/04/28 12:44:25, vabr (Chromium) wrote: > On 2016/04/28 12:18:11, cfroussios wrote: > > On 2016/04/28 07:53:21, vabr (Chromium) wrote: > > > Please add > > > DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); > > > > > > as the very last line in the private section, separated by a blank line > above. > > > > > > (The purpose of the macro is to prevent creating instances of the class. > This > > is > > > an all-static class, so creating an instance would typically mean a typo in > > the > > > code.) > > > > NativeBackendLibsecret inherits from LibsecretLoader and therefore > > LibsecretLoader needs to be instantiated. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... > > > > I'm adding DISALLOW_COPY_AND_ASSIGN instead. > > It does not really make sense to derive from a static-only class, including the > questionable benefits of the protected visibility. When we are already cleaning > this up, I suggest: > (1) Make LoadLibsecret() and IsLibsecretAvailable() public. > (2) Make libsecret_loaded private. > (3) Change NativeBackendLibsecret to stop inheriting from LibsecretLoader, and > add LibsecretLoader:: to the calls in NativeBackendLibsecret::Init. > (4) Apply DISALLOW_IMPLICIT_CONSTRUCTORS to LibsecretLoader and delete the > default constructor. Done. https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; On 2016/04/28 12:44:25, vabr (Chromium) wrote: > On 2016/04/28 12:18:11, cfroussios wrote: > > On 2016/04/28 07:53:21, vabr (Chromium) wrote: > > > Please add > > > DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); > > > > > > as the very last line in the private section, separated by a blank line > above. > > > > > > (The purpose of the macro is to prevent creating instances of the class. > This > > is > > > an all-static class, so creating an instance would typically mean a typo in > > the > > > code.) > > > > NativeBackendLibsecret inherits from LibsecretLoader and therefore > > LibsecretLoader needs to be instantiated. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... > > > > I'm adding DISALLOW_COPY_AND_ASSIGN instead. > > It does not really make sense to derive from a static-only class, including the > questionable benefits of the protected visibility. When we are already cleaning > this up, I suggest: > (1) Make LoadLibsecret() and IsLibsecretAvailable() public. > (2) Make libsecret_loaded private. > (3) Change NativeBackendLibsecret to stop inheriting from LibsecretLoader, and > add LibsecretLoader:: to the calls in NativeBackendLibsecret::Init. > (4) Apply DISALLOW_IMPLICIT_CONSTRUCTORS to LibsecretLoader and delete the > default constructor. The libsecret field needs to stay protected, so that it is mockable for tests. https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.cc (right): https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.cc:75: bool LibsecretLoader::LibsecretIsAvailable() { On 2016/04/28 14:42:13, vabr (Chromium) wrote: > // static Done. https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.h (right): https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:5: #ifndef COMPONENTS_OS_CRYPT_LIBSECRET_LOADER_H_ On 2016/04/28 14:42:13, vabr (Chromium) wrote: > nit: Don't forget to update the #include guard. > (If you run "git cl lint", it will even tell you which one you should use.) Done. https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:49: void Append(const std::string& name, const std::string& value); On 2016/04/28 14:42:13, vabr (Chromium) wrote: > nit: Please add blank lines between method declarations. Done. https://codereview.chromium.org/1929573002/diff/120001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:59: }; On 2016/04/28 14:42:13, vabr (Chromium) wrote: > Add DISALLOW_COPY_AND_ASSIGN Done.
cfroussios@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please review changes in components/os_crypt/os_crypt_posix_util_libsecret.{cc,h} and corresponding build. Thanks!
Can you keep the text in the CL description to 72 chars / line to be more git friendly? https://codereview.chromium.org/1929573002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_libsecret.cc (right): https://codereview.chromium.org/1929573002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_libsecret.cc:10: #include <libsecret/secret.h> I'm curious what this is needed for if "password_manager will not interact with libsecret directly" https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt.gypi File components/os_crypt.gypi (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:61: 'include_dirs' : [ There's a slight discrepancy - here we have include_dirs, but in GN we have a dependency. What's up with that? https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.h (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:5: #ifndef COMPONENTS_OS_CRYPT_OS_CRYPT_POSIX_UTIL_LIBSECRET_H_ Can we name the file libsecret_util_posix.h? https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:15: class LibsecretLoader { Can you add some documentation to explain what this class does? I'm guessing one must call LibsecretIsAvailable() to see if the library exists. Then LoadLibsecret() to initialize the function pointers. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:54: // |LibsecretAttributesBuilder| desctructor. typo https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:59: std::list<std::string> name_values_; I realize this is code being moved, but what's the advantage of having a list, vs just using a vector?
Still LGTM, some comments for cfroussios@, some answers to Lei. Cheers, Vaclav https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... File components/os_crypt/libsecret_loader.h (right): https://codereview.chromium.org/1929573002/diff/20001/components/os_crypt/lib... components/os_crypt/libsecret_loader.h:38: }; On 2016/04/28 16:26:57, cfroussios wrote: > On 2016/04/28 12:44:25, vabr (Chromium) wrote: > > On 2016/04/28 12:18:11, cfroussios wrote: > > > On 2016/04/28 07:53:21, vabr (Chromium) wrote: > > > > Please add > > > > DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader); > > > > > > > > as the very last line in the private section, separated by a blank line > > above. > > > > > > > > (The purpose of the macro is to prevent creating instances of the class. > > This > > > is > > > > an all-static class, so creating an instance would typically mean a typo > in > > > the > > > > code.) > > > > > > NativeBackendLibsecret inherits from LibsecretLoader and therefore > > > LibsecretLoader needs to be instantiated. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas... > > > > > > I'm adding DISALLOW_COPY_AND_ASSIGN instead. > > > > It does not really make sense to derive from a static-only class, including > the > > questionable benefits of the protected visibility. When we are already > cleaning > > this up, I suggest: > > (1) Make LoadLibsecret() and IsLibsecretAvailable() public. > > (2) Make libsecret_loaded private. > > (3) Change NativeBackendLibsecret to stop inheriting from LibsecretLoader, and > > add LibsecretLoader:: to the calls in NativeBackendLibsecret::Init. > > (4) Apply DISALLOW_IMPLICIT_CONSTRUCTORS to LibsecretLoader and delete the > > default constructor. > > The libsecret field needs to stay protected, so that it is mockable for tests. Acknowledged. https://codereview.chromium.org/1929573002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_libsecret.cc (right): https://codereview.chromium.org/1929573002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_libsecret.cc:10: #include <libsecret/secret.h> On 2016/04/28 21:49:09, Lei Zhang wrote: > I'm curious what this is needed for if "password_manager will not interact with > libsecret directly" Not interacting with libsecret is the ultimate goal, but we are not there yet. Even with os_crypt hosting the libsecret loader, this native backend still needs to save and retrieve data to the Keyring via libsecret. The plan is as follows: (1) Implement encryption on Linux with libsecret. (2) Stop saving passwords through libsecret and store them encrypted in the LoginDatabase instead. After step (2) there will be no need for referencing libsecret in the native backend. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt.gypi File components/os_crypt.gypi (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:61: 'include_dirs' : [ On 2016/04/28 21:49:09, Lei Zhang wrote: > There's a slight discrepancy - here we have include_dirs, but in GN we have a > dependency. What's up with that? It is a shortcut, given that GYP is just being deprecated on Linux. The separate target for libsecret we have in GN will be useful if we start depending on it somewhere else. But that's unlikely to happen in the time remaining for GYP on Linux. Please speak up if you don't think this is a good idea and would prefer to have a dependency in GYP as well. In that case we would need to add a GYP configuration there as well. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.cc (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.cc:77: if (!LoadLibsecret()) Here you changed the semantics of LibsecretIsAvailable -- it no longer allows to check availability without actually loading it. That might be fine, because we do not seem to use it without actually wanting to load libsecret, but I'm curious what made you change it. If you keep it as it is, them please consider renaming the method. It is more of a EnsureLibsecretLoaded. And with this, there is no need for a public LoadLibsecret, and NativeBackendLibsecret::Init can just call EnsureLibsecretLoaded.
Description was changed from ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 ========== to ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 ==========
Description was changed from ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 ========== to ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 ==========
Hi all, I've updated the description as requested and addressed your comments. One thing pending is your thestig@'s opinion on whether we should properly support gyp builds on a linux-only feature. I'm fine either way. Thanks! https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.cc (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.cc:77: if (!LoadLibsecret()) On 2016/04/29 07:36:45, vabr (Chromium) wrote: > Here you changed the semantics of LibsecretIsAvailable -- it no longer allows to > check availability without actually loading it. That might be fine, because we > do not seem to use it without actually wanting to load libsecret, but I'm > curious what made you change it. > > If you keep it as it is, them please consider renaming the method. It is more of > a EnsureLibsecretLoaded. And with this, there is no need for a public > LoadLibsecret, and NativeBackendLibsecret::Init can just call > EnsureLibsecretLoaded. It seemed unnecessarily hazardous to require a particular sequence of calls when they never appear separately and, as thestig@ noticed, the required order can be counter-intuitive. I've now returned the function to the previous semantics and I've introduced EnsureLibsecretLoaded as a separate method, which will do things in the right order. I made both preexisting methods private. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.h (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:5: #ifndef COMPONENTS_OS_CRYPT_OS_CRYPT_POSIX_UTIL_LIBSECRET_H_ On 2016/04/28 21:49:09, Lei Zhang wrote: > Can we name the file libsecret_util_posix.h? Done. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:15: class LibsecretLoader { On 2016/04/28 21:49:09, Lei Zhang wrote: > Can you add some documentation to explain what this class does? > > I'm guessing one must call LibsecretIsAvailable() to see if the library exists. > Then LoadLibsecret() to initialize the function pointers. I understand the confusion. I've introduced a method that does things in the correct order and made the two previous methods private. I've also added documentation. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:54: // |LibsecretAttributesBuilder| desctructor. On 2016/04/28 21:49:09, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:59: std::list<std::string> name_values_; On 2016/04/28 21:49:09, Lei Zhang wrote: > I realize this is code being moved, but what's the advantage of having a list, > vs just using a vector? There are no benefits that I can see. I've switched to a vector
Thanks, still LGTM, mostly nits below. Cheers, Vaclav https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... File components/os_crypt/libsecret_util_posix.h (right): https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:27: // Loads the libsecret binaries and checks that it responds to queries. nit: Plural "binaries" vs. singular "it". What about binaries -> library? https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:28: // False if either step fails. optional nit: "Returns false" instead of just "false"? https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:32: return LoadLibsecret() && LibsecretIsAvailable(); Please do not inline this method, define it in the .cc file. http://dev.chromium.org/developers/coding-style#TOC-Inline-functions https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:36: static bool libsecret_loaded; nit: Even static data members should have a trailing underscore in the name. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:44: static const FunctionInfo functions[]; nit: This is a constant, so let's call it kFunctions.
https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... File components/os_crypt/libsecret_util_posix.h (right): https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:27: // Loads the libsecret binaries and checks that it responds to queries. On 2016/04/29 13:16:07, vabr (Chromium) wrote: > nit: Plural "binaries" vs. singular "it". What about binaries -> library? Done. https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:28: // False if either step fails. On 2016/04/29 13:16:07, vabr (Chromium) wrote: > optional nit: "Returns false" instead of just "false"? Returns is redundant, but let's go with the norm. Done. https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:32: return LoadLibsecret() && LibsecretIsAvailable(); On 2016/04/29 13:16:07, vabr (Chromium) wrote: > Please do not inline this method, define it in the .cc file. > http://dev.chromium.org/developers/coding-style#TOC-Inline-functions Done. https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:36: static bool libsecret_loaded; On 2016/04/29 13:16:07, vabr (Chromium) wrote: > nit: Even static data members should have a trailing underscore in the name. > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/1929573002/diff/180001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:44: static const FunctionInfo functions[]; On 2016/04/29 13:16:07, vabr (Chromium) wrote: > nit: This is a constant, so let's call it kFunctions. Done.
lgtm
lgtm https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt.gypi File components/os_crypt.gypi (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:61: 'include_dirs' : [ On 2016/04/29 07:36:45, vabr (Chromium) wrote: > On 2016/04/28 21:49:09, Lei Zhang wrote: > > There's a slight discrepancy - here we have include_dirs, but in GN we have a > > dependency. What's up with that? > > It is a shortcut, given that GYP is just being deprecated on Linux. The separate > target for libsecret we have in GN will be useful if we start depending on it > somewhere else. But that's unlikely to happen in the time remaining for GYP on > Linux. Please speak up if you don't think this is a good idea and would prefer > to have a dependency in GYP as well. In that case we would need to add a GYP > configuration there as well. Ok, I won't worry about it given GYP deprecation on Linux. https://codereview.chromium.org/1929573002/diff/240001/components/os_crypt/li... File components/os_crypt/libsecret_util_posix.h (right): https://codereview.chromium.org/1929573002/diff/240001/components/os_crypt/li... components/os_crypt/libsecret_util_posix.h:73: std::list<std::string> name_values_; I thought you were going to switch to a vector.
https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_posix_util_libsecret.h (right): https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_posix_util_libsecret.h:59: std::list<std::string> name_values_; On 2016/04/28 21:49:09, Lei Zhang wrote: > I realize this is code being moved, but what's the advantage of having a list, > vs just using a vector? It turns out that list is necessary. Switching to vector is what caused the asan bot to keep failing. The failures go away if this is a list or if we call a sufficient vector.reserve before appending attributes. name_values_ serves as storage for others to refer to. My hypothesis is that those references can be broken if the vector moves things while changing its capacity. In contract, list nodes will stay where they are. I'm keeping it as a list for now. It might make sense in the future to explore alternative ways of doing the same thing without depending on the internals of containers.
On 2016/04/29 16:39:37, cfroussios wrote: > https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... > File components/os_crypt/os_crypt_posix_util_libsecret.h (right): > > https://codereview.chromium.org/1929573002/diff/140001/components/os_crypt/os... > components/os_crypt/os_crypt_posix_util_libsecret.h:59: std::list<std::string> > name_values_; > On 2016/04/28 21:49:09, Lei Zhang wrote: > > I realize this is code being moved, but what's the advantage of having a list, > > vs just using a vector? > > It turns out that list is necessary. Switching to vector is what caused the asan > bot to keep failing. The failures go away if this is a list or if we call a > sufficient vector.reserve before appending attributes. > > name_values_ serves as storage for others to refer to. My hypothesis is that > those references can be broken if the vector moves things while changing its > capacity. In contract, list nodes will stay where they are. > > I'm keeping it as a list for now. It might make sense in the future to explore > alternative ways of doing the same thing without depending on the internals of > containers. Good job figuring this out, Christos. Could you please add a comment to the declaration of name_values_ to explain what you just explained here? Otherwise the next person changing this to a vector might need to rely on ASAN to prevent them from doing so. Exploring the alternatives sounds also like a good idea, perhaps you could file a bug for that and reference it in that same comment through TODO(crbug.com/XXXX)? Thanks! Vaclav
Just for the record, I encouraged cfroussios@ to use TBR for components/OWNERS for components/os_crypt.gypi, given that the owner of components/os_crypt approved the CL. I'll suggest a change to have components/os_crypt.gypi covered by the components/os_crypt owners in a separate CL. Cheers, Vaclav
Description was changed from ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 ========== to ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 TBR=caitkp@chromium.org for components/os_crypt.gypi ==========
The CQ bit was checked by cfroussios@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1929573002/#ps260001 (title: "Added TODO and reference to bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929573002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929573002/260001
Message was sent while issue was closed.
Description was changed from ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 TBR=caitkp@chromium.org for components/os_crypt.gypi ========== to ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 TBR=caitkp@chromium.org for components/os_crypt.gypi ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/df99cec1248d89188156bf53aba322c3a9e0f0bb Cr-Commit-Position: refs/heads/master@{#390706}
Message was sent while issue was closed.
Description was changed from ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 TBR=caitkp@chromium.org for components/os_crypt.gypi ========== to ========== Changed location of Libsecret utils Moved classes LibsecretLoader and LibsecretAttributeBuilder from password_manager to os_crypt. In the future, password_manager will not interact with libsecret directly, while os_crypt will. Responsibility is therefore transfered. For now, password_manager continues to use those utilites. Currently, Password Manager stores passwords one by one in libsecret. With this and future CL(s), we will only store an encryption key with libsecret and use said key to protect a local database, managed by the Password Manager. BUG=602624 TBR=caitkp@chromium.org for components/os_crypt.gypi Committed: https://crrev.com/df99cec1248d89188156bf53aba322c3a9e0f0bb Cr-Commit-Position: refs/heads/master@{#390706} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
