Create setting that disables password stores

Create setting that disables password stores

A new setting is implemented, which can be set to prevent 
OSCrypt/Password Manager from using a backend (i.e. keyring/kwallet). 
Since Password Manager is expected to deprecate its backends, OSCrypt 
is given ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, 
therefore profile preferences cannot be used for this setting. The 
setting is stored in a file in the chrome's user data directory. It is 
implemented as a single empty file, whose presence or absence in the 
filesystem defines whether the setting is set to true or false.

Additionally, a new commandline switch is implemented: 
--enable-encryption-selection. This controls whether the feature above 
will be used. When the feature is disabled, current behaviour is for 
Chrome to try and reach the appropriate backend, as determined by the 
OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
Review-Url: https://codereview.chromium.org/2948783002
Cr-Commit-Position: refs/heads/master@{#484595}
Committed: https://chromium.googlesource.com/chromium/src/+/6b340f81b25adc1e97c50247aa3bf235eabcbf7e

[cfroussios, 2017-06-20 14:15:09]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 14:15:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 14:20:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 14:20:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cfroussios, 2017-06-20 15:11:09]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 15:11:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 17:19:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 17:19:03]

Dry run: This issue passed the CQ dry run.

[cfroussios, 2017-07-03 13:52:21]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-03 13:52:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2017-07-03 14:15:37]

Description was changed from

==========
Register preferences for OSCrypt

BUG=
==========

to

==========
A new setting is implemented, which can be set to prevent OSCrypt/Password
Manager from using a backend (i.e. keyring/kwallet).

Since Password Manager is expected to deprecate its backends, OSCrypt is given
ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, therefore
profile preferences cannot be used for this setting. The setting is store in a
file in the chrome's user data directory. It is implemented as a single empty
file, whose presence or absence in the filesystem defines whether the setting is
set to true or false.

Additionally, a new commandline switch is implemented:
--enable-encryption-selection. This controls whether the setting above will be
used.

Additionally, the setting is ignored if it contradicts the --password-store
flag, since the flag is used for development.

BUG=709096
==========

[cfroussios, 2017-07-03 14:29:14]

Description was changed from

==========
A new setting is implemented, which can be set to prevent OSCrypt/Password
Manager from using a backend (i.e. keyring/kwallet).

Since Password Manager is expected to deprecate its backends, OSCrypt is given
ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, therefore
profile preferences cannot be used for this setting. The setting is store in a
file in the chrome's user data directory. It is implemented as a single empty
file, whose presence or absence in the filesystem defines whether the setting is
set to true or false.

Additionally, a new commandline switch is implemented:
--enable-encryption-selection. This controls whether the setting above will be
used.

Additionally, the setting is ignored if it contradicts the --password-store
flag, since the flag is used for development.

BUG=709096
==========

to

==========
A new setting is implemented, which can be set to prevent OSCrypt/Password
Manager from using a backend (i.e. keyring/kwallet).

Since Password Manager is expected to deprecate its backends, OSCrypt is given
ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, therefore
profile preferences cannot be used for this setting. The setting is stored in a
file in the chrome's user data directory. It is implemented as a single empty
file, whose presence or absence in the filesystem defines whether the setting is
set to true or false.

Additionally, a new commandline switch is implemented:
--enable-encryption-selection. This controls whether the feature above will be
used. When the feature is disabled, current behaviour is for Chrome to try and
reach the appropriate backend, as determined by the OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
==========

[cfroussios, 2017-07-03 14:56:21]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-03 14:56:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2017-07-03 14:57:20]

cfroussios@chromium.org changed reviewers:
+ vabr@chromium.org

[cfroussios, 2017-07-03 14:57:21]

Hi! Can you review this change to OSCrypt? Thanks!

[commit-bot: I haz the power, 2017-07-03 16:27:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-03 16:27:43]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-07-03 19:27:14]

Thanks, this LGTM.
I like how you simplified the preference file format! :)
Cheers,
Vaclav

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_factory.cc (left):

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_store_factory.cc:130: //
TODO(crbug.com/715987). Remove when PasswordReuseDetector is decoupled
nit: Let's stick to changes related with the CL goal. Fixing the typo here will
make git blame for this line one step longer.

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_factory.cc (right):

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_store_factory.cc:213:
chrome::GetDefaultUserDataDirectory(&user_data_dir);
optional: GetDefaultUserDataDirectory does not look completely simple inside,
not sure if it is expensive to compute. In case it were expensive, the
alternative would be to only call it if |use_preference| is true.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_linux.h (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_linux.h:39: static void EnablePreference(bool
enable_preference);
optional nit: I'm a bit confused by "enabling". Calling this with false actually
disables the preference, right? So what about something like:

// Controls whether preference on using or ignoring backends is used.
static void RespectPreference(bool should_respect_preference).

No strong opposition on my side, though. Especially if this is already a pattern
in other pieces of code.

In any case, let's keep it consistent with os_crypt.h.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_util_linux.cc (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:10: #include "base/pickle.h"
Unused?

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:16: const char
kPreferencesFileName[] = "Disable OSCrypt";
nit: constexpr?

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:19: namespace {
nit: Why not merge the two consecutive anonymous namespaces?

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:49: if (!use_backend)
nit: Please comment on the need to place this under the block handling |type|
hints, to let --password-store override the preference.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_util_linux.h (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.h:40: bool WriteBackendUse(const
base::FilePath& user_data_dir, bool use);
nit: Let's be clearer about the meaning of |use|. I would not know whether |use|
being true means that the backend should be used, or the LoginDatabase should be
used.

[vabr (Chromium), 2017-07-03 19:40:59]

One more thing:
Please repeat the CL title "Create setting that disables password stores" as the
first line of the description, separated with a blank line from the rest. The
git commit message is only created from the description, so would currently miss
the CL title.

[cfroussios, 2017-07-04 07:38:54]

Description was changed from

==========
A new setting is implemented, which can be set to prevent OSCrypt/Password
Manager from using a backend (i.e. keyring/kwallet).

Since Password Manager is expected to deprecate its backends, OSCrypt is given
ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, therefore
profile preferences cannot be used for this setting. The setting is stored in a
file in the chrome's user data directory. It is implemented as a single empty
file, whose presence or absence in the filesystem defines whether the setting is
set to true or false.

Additionally, a new commandline switch is implemented:
--enable-encryption-selection. This controls whether the feature above will be
used. When the feature is disabled, current behaviour is for Chrome to try and
reach the appropriate backend, as determined by the OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
==========

to

==========
Create setting that disables password stores

A new setting is implemented, which can be set to prevent OSCrypt/Password
Manager from using a backend (i.e. keyring/kwallet). Since Password Manager is
expected to deprecate its backends, OSCrypt is given ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, therefore
profile preferences cannot be used for this setting. The setting is stored in a
file in the chrome's user data directory. It is implemented as a single empty
file, whose presence or absence in the filesystem defines whether the setting is
set to true or false.

Additionally, a new commandline switch is implemented:
--enable-encryption-selection. This controls whether the feature above will be
used. When the feature is disabled, current behaviour is for Chrome to try and
reach the appropriate backend, as determined by the OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
==========

[cfroussios, 2017-07-04 09:57:57]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 09:58:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2017-07-04 09:58:16]

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_factory.cc (left):

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_store_factory.cc:130: //
TODO(crbug.com/715987). Remove when PasswordReuseDetector is decoupled
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> nit: Let's stick to changes related with the CL goal. Fixing the typo here
will
> make git blame for this line one step longer.

Done.

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_factory.cc (right):

https://codereview.chromium.org/2948783002/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_store_factory.cc:213:
chrome::GetDefaultUserDataDirectory(&user_data_dir);
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> optional: GetDefaultUserDataDirectory does not look completely simple inside,
> not sure if it is expensive to compute. In case it were expensive, the
> alternative would be to only call it if |use_preference| is true.

Done.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_linux.h (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_linux.h:39: static void EnablePreference(bool
enable_preference);
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> optional nit: I'm a bit confused by "enabling". Calling this with false
actually
> disables the preference, right? So what about something like:
> 
> // Controls whether preference on using or ignoring backends is used.
> static void RespectPreference(bool should_respect_preference).
> 
> No strong opposition on my side, though. Especially if this is already a
pattern
> in other pieces of code.
> 
> In any case, let's keep it consistent with os_crypt.h.

I have the same concerns with the name, but your proposal doesn't sound very
natural either. To me it implies a policy being introduced from outside.
WDYT of ShouldUsePreference?

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_util_linux.cc (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:10: #include "base/pickle.h"
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> Unused?

Done.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:16: const char
kPreferencesFileName[] = "Disable OSCrypt";
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> nit: constexpr?

Done.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:19: namespace {
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> nit: Why not merge the two consecutive anonymous namespaces?

Done.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.cc:49: if (!use_backend)
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> nit: Please comment on the need to place this under the block handling |type|
> hints, to let --password-store override the preference.

Done.

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_util_linux.h (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_util_linux.h:40: bool WriteBackendUse(const
base::FilePath& user_data_dir, bool use);
On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> nit: Let's be clearer about the meaning of |use|. I would not know whether
|use|
> being true means that the backend should be used, or the LoginDatabase should
be
> used.

Done.

[vabr (Chromium), 2017-07-04 10:11:01]

Still LGTM, thanks for the added tests.
Vaclav

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
File components/os_crypt/key_storage_linux.h (right):

https://codereview.chromium.org/2948783002/diff/120001/components/os_crypt/ke...
components/os_crypt/key_storage_linux.h:39: static void EnablePreference(bool
enable_preference);
On 2017/07/04 09:58:15, cfroussios wrote:
> On 2017/07/03 19:27:14, vabr (Chromium) wrote:
> > optional nit: I'm a bit confused by "enabling". Calling this with false
> actually
> > disables the preference, right? So what about something like:
> > 
> > // Controls whether preference on using or ignoring backends is used.
> > static void RespectPreference(bool should_respect_preference).
> > 
> > No strong opposition on my side, though. Especially if this is already a
> pattern
> > in other pieces of code.
> > 
> > In any case, let's keep it consistent with os_crypt.h.
> 
> I have the same concerns with the name, but your proposal doesn't sound very
> natural either. To me it implies a policy being introduced from outside.
> WDYT of ShouldUsePreference?

ShouldUsePreference sounds good to me.

[commit-bot: I haz the power, 2017-07-04 11:19:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-04 11:19:03]

Dry run: This issue passed the CQ dry run.

[cfroussios, 2017-07-04 11:42:43]

cfroussios@chromium.org changed reviewers:
+ jochen@chromium.org

[cfroussios, 2017-07-04 11:42:44]

jochen@chromium.org: Please review changes in chrome/

Thanks!

[jochen (gone - plz use gerrit), 2017-07-05 07:31:32]

can you please wrap the CL description at 80c?

chrome/ lgtm

can you please forward the design doc to the internal design docs mailing list?

[cfroussios, 2017-07-05 12:04:44]

Description was changed from

==========
Create setting that disables password stores

A new setting is implemented, which can be set to prevent OSCrypt/Password
Manager from using a backend (i.e. keyring/kwallet). Since Password Manager is
expected to deprecate its backends, OSCrypt is given ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, therefore
profile preferences cannot be used for this setting. The setting is stored in a
file in the chrome's user data directory. It is implemented as a single empty
file, whose presence or absence in the filesystem defines whether the setting is
set to true or false.

Additionally, a new commandline switch is implemented:
--enable-encryption-selection. This controls whether the feature above will be
used. When the feature is disabled, current behaviour is for Chrome to try and
reach the appropriate backend, as determined by the OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
==========

to

==========
Create setting that disables password stores

A new setting is implemented, which can be set to prevent 
OSCrypt/Password Manager from using a backend (i.e. keyring/kwallet). 
Since Password Manager is expected to deprecate its backends, OSCrypt 
is given ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, 
therefore profile preferences cannot be used for this setting. The 
setting is stored in a file in the chrome's user data directory. It is 
implemented as a single empty file, whose presence or absence in the 
filesystem defines whether the setting is set to true or false.

Additionally, a new commandline switch is implemented: 
--enable-encryption-selection. This controls whether the feature above 
will be used. When the feature is disabled, current behaviour is for 
Chrome to try and reach the appropriate backend, as determined by the 
OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
==========

[cfroussios, 2017-07-06 13:34:09]

The CQ bit was checked by cfroussios@chromium.org

[commit-bot: I haz the power, 2017-07-06 13:34:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 15:05:16]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499348049778990,
"parent_rev": "5b87618142279f2c84dd02bc21cf58e970413373", "commit_rev":
"6b340f81b25adc1e97c50247aa3bf235eabcbf7e"}

[commit-bot: I haz the power, 2017-07-06 15:05:34]

Description was changed from

==========
Create setting that disables password stores

A new setting is implemented, which can be set to prevent 
OSCrypt/Password Manager from using a backend (i.e. keyring/kwallet). 
Since Password Manager is expected to deprecate its backends, OSCrypt 
is given ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, 
therefore profile preferences cannot be used for this setting. The 
setting is stored in a file in the chrome's user data directory. It is 
implemented as a single empty file, whose presence or absence in the 
filesystem defines whether the setting is set to true or false.

Additionally, a new commandline switch is implemented: 
--enable-encryption-selection. This controls whether the feature above 
will be used. When the feature is disabled, current behaviour is for 
Chrome to try and reach the appropriate backend, as determined by the 
OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096
==========

to

==========
Create setting that disables password stores

A new setting is implemented, which can be set to prevent 
OSCrypt/Password Manager from using a backend (i.e. keyring/kwallet). 
Since Password Manager is expected to deprecate its backends, OSCrypt 
is given ownership of the setting.

OSCrypt needs to be ready for use during profile initialisation, 
therefore profile preferences cannot be used for this setting. The 
setting is stored in a file in the chrome's user data directory. It is 
implemented as a single empty file, whose presence or absence in the 
filesystem defines whether the setting is set to true or false.

Additionally, a new commandline switch is implemented: 
--enable-encryption-selection. This controls whether the feature above 
will be used. When the feature is disabled, current behaviour is for 
Chrome to try and reach the appropriate backend, as determined by the 
OS environment. 
The --password-store flag overrules both of the above behaviours.

BUG=709096

Review-Url: https://codereview.chromium.org/2948783002
Cr-Commit-Position: refs/heads/master@{#484595}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b340f81b25adc1e97c50247aa3b...
==========

[commit-bot: I haz the power, 2017-07-06 15:05:35]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/6b340f81b25adc1e97c50247aa3b...