OSCrypt supports encryption with KWallet

components/os_crypt/key_storage_kwallet.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OS_CRYPT_KEY_STORAGE_KWALLET_H_
+#define COMPONENTS_OS_CRYPT_KEY_STORAGE_KWALLET_H_
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "components/os_crypt/key_storage_linux.h"
+#include "components/os_crypt/kwallet_dbus.h"
+
+class KeyStorageKWallet : public KeyStorageLinux {
+ public:
+  explicit KeyStorageKWallet(base::nix::DesktopEnvironment desktop_env,

[Lei Zhang, 2016/07/20 01:08:26]

nit explicit

[cfroussios, 2016/07/20 15:22:46]

Done.

+                             std::string app_name);

[Lei Zhang, 2016/07/20 01:08:26]

Just pass by ref and set app_name_?

[cfroussios, 2016/07/20 15:22:46]

For constructors, this is supposed to perform better on average than passing by
reference. Not that this is likely to ever become hot. Is that a pattern we
don't care for in Chrome?

[Lei Zhang, 2016/07/20 19:15:07]

Well, this is definitely not hot code, and I think (please double check) if you
want to use move semantics, you do "std::string&&" but the Chromium code base
has 2 instances of that, so in general, we are not quite there yet. Plus, I like
|g_product_name| being set once and staying the same.

[cfroussios, 2016/07/21 11:49:46]

You don't need the "std::string&&" to take advantage of move semantics, because
std::string already has a move constructor. The idea is that |app_name| will be
constructed with the best constructor in each case:

1. If the input can be moved, it will be moved into |app_name| and then into
|app_name_|.
2. If stuff can't be moved, |app_name| will be a new copy, which will be moved
into |app_name_|. We would have had to create a copy anyway.

I don't quite understand your comment about |g_product_name|. Are you saying
that, since it won't change, we can pass pointers to it? Or that we would have
to cannibalize it, thus changing it? In the way that KeyStorageKWallet is used
now, it won't actually be moved. It is just a good pattern for constructors,
because it works both when you can and can't move.

[Lei Zhang, 2016/07/21 21:25:31]

+danakj to help provide guidance for the more academic portion of this
discussion. In general, are we ok with having functions that pass objects by
value with intention to std::move it? How do we make that intention clear?
Always pass objects by const-ref is a bit engrained in my head.
I expect |g_product_name| to be set once by the higher level, and remain the
same for the lifetime of its existence. Then it can be passed around by
const-ref and be used and copied as needed. The std::move() in the
KeyStorageKWallet ctor made me think this CL is trying to:
- setting |g_product_name| once
- clearing it as its value is being moved into KeyStorageKWallet::app_name

but now I see it's not actually being moved... this is confusing. . o O
(std::move() only casts, std::move() only casts)

[cfroussios, 2016/07/22 11:25:25]

Yes, I am only defending this pattern in general. In this particular case, it
makes no real difference. We agree that |g_product_name| should not be a
temporary.

I tried to find documentation on the pattern, but I didn't find any specific
mentions. That's a bit weird, since I only learned about it from people within
Google. I'm really curious why it's not more common. If there is a pitfall I am
not aware of, or if it is considered too foreign and confusing for Chrome code,
I should know before slipping it past other reviews.

+  ~KeyStorageKWallet() override;
+
+  // KeyStorageLinux
+  std::string GetKey() override;
+
+  // Initialize using an optional KWalletDBus mock.
+  // A DBus session will not be created if a mock is provided.
+  bool InitWithKWalletDBus(
+      std::unique_ptr<KWalletDBus> optional_kwallet_dbus_ptr);
+
+ protected:
+  // KeyStorageLinux
+  bool Init() override;
+
+ private:
+  enum class InitResult {
+    SUCCESS,
+    TEMPORARY_FAIL,
+    PERMANENT_FAIL,
+  };
+
+  // Checks whether KWallet is enabled and sets |wallet_name_|
+  InitResult InitWallet();
+
+  // Create Chrome's folder in the wallet, if it doesn't exist.
+  bool InitFolder();
+
+  const base::nix::DesktopEnvironment desktop_env_;
+  int32_t handle_;
+  std::string wallet_name_;
+  const std::string app_name_;
+  std::unique_ptr<KWalletDBus> kwallet_dbus_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyStorageKWallet);
+};
+
+#endif  // COMPONENTS_OS_CRYPT_KEY_STORAGE_KWALLET_H_