Supplimentary identifier for passwords specific

Supplimentary identifier for passwords specific

BUG=638963

[melandory, 2016-08-19 21:00:05]

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

[commit-bot: I haz the power, 2016-08-19 21:01:14]

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, 2016-08-19 21:59:06]

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

[commit-bot: I haz the power, 2016-08-19 21:59:07]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-08-19 23:54:18]

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

[commit-bot: I haz the power, 2016-08-19 23:54:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-08-19 23:55:58]

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

[commit-bot: I haz the power, 2016-08-19 23:56:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-08-19 23:56:59]

melandory@chromium.org changed reviewers:
+ zea@chromium.org

[melandory, 2016-08-19 23:57:00]

Hey,

can you take a quick look and comment on approach, while I'm trying to figure
out how test it, so I don't write tests if you think approach is wrong.

Thanks!

[Nicolas Zea, 2016-08-20 00:23:43]

https://codereview.chromium.org/2260953002/diff/40001/components/sync/core/wr...
File components/sync/core/write_node.cc (right):

https://codereview.chromium.org/2260953002/diff/40001/components/sync/core/wr...
components/sync/core/write_node.cc:150: const
sync_pb::PasswordSpecificsMetadata& specifics) {}
I wonder if we really need this. Couldn't you set the metadata field as part of
SetPasswordSpecifics (you may need to do it within nigori_util.cc's
UpdateEntryWithEncryption in fact, which is eventually called from
SetPasswordSpecifics).  You should have access to the encryption state, and can
just clear/set the field as necessary there.

https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
File components/sync/core_impl/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
components/sync/core_impl/sync_encryption_handler_impl.cc:854:
child.SetPasswordSpecificsMetadata(
It occurs to me that I don't think you need this. SetPasswordSpecifics above
should automatically clear this as necessary (see my comment in write_node.cc).
You'll need to make some changes to nigori_util.cc to make that happen.

https://codereview.chromium.org/2260953002/diff/40001/components/sync/protoco...
File components/sync/protocol/password_specifics.proto (right):

https://codereview.chromium.org/2260953002/diff/40001/components/sync/protoco...
components/sync/protocol/password_specifics.proto:127: // should never be set
for full encryption and data is the explicitly cleared
nit: the last sentence is a bit tough to parse. How about:
"The field should never be set for full encryption users. If encryption is
enabled, this field must be cleared".

https://codereview.chromium.org/2260953002/diff/40001/components/sync/protoco...
components/sync/protocol/password_specifics.proto:129: optional
PasswordSpecificsMetadata unencrypted_metadata = 3;
Note that this also needs to be added to proto_value_conversions.cc (which will
help with debugging)

[commit-bot: I haz the power, 2016-08-20 03:40:37]

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

[commit-bot: I haz the power, 2016-08-20 03:40:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[melandory, 2016-08-22 22:08:41]

https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
File components/sync/core_impl/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
components/sync/core_impl/sync_encryption_handler_impl.cc:854:
child.SetPasswordSpecificsMetadata(
On 2016/08/20 00:23:43, Nicolas Zea wrote:
> It occurs to me that I don't think you need this. SetPasswordSpecifics above
> should automatically clear this as necessary (see my comment in
write_node.cc).
> You'll need to make some changes to nigori_util.cc to make that happen.

Disclaimer. Text below is written with an assumption of following setup: 
* data is cleared in nigori_util, UpdateEntryWithEncryption
* in order to get encryption state we query NigoriHandler::GetEncryptedTypes and
if encrypted types equal to EncryptableUserTypes, then it is custom passphrase
user .

So there is one thing which bothers me in proposed approach (or at least about
implementation I described).

Information that if user has custom_passphrase when "encrypt everything" is true
is external to the UpdateEntryWithEncryption and I think currently there is no
tests which check that this hypothesis is always true.

I see several ways to solve this:
1. Pipe passphrase_type_ flag to UpdateEntryWithEncryption.
2. Keep clearing of data in sync_encryption_handler_impl.cc, where we have
explicit signal about passphrase type.
3. Do implementation as written in the paragraph above and try to come up with a
solution for testing that custom passphrase state == encrypt everything state.

WDYT?

[Nicolas Zea, 2016-08-22 23:50:05]

On 2016/08/22 22:08:41, melandory wrote:
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> File components/sync/core_impl/sync_encryption_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> components/sync/core_impl/sync_encryption_handler_impl.cc:854:
> child.SetPasswordSpecificsMetadata(
> On 2016/08/20 00:23:43, Nicolas Zea wrote:
> > It occurs to me that I don't think you need this. SetPasswordSpecifics above
> > should automatically clear this as necessary (see my comment in
> write_node.cc).
> > You'll need to make some changes to nigori_util.cc to make that happen.
> 
> Disclaimer. Text below is written with an assumption of following setup: 
> * data is cleared in nigori_util, UpdateEntryWithEncryption
> * in order to get encryption state we query NigoriHandler::GetEncryptedTypes
and
> if encrypted types equal to EncryptableUserTypes, then it is custom passphrase
> user .
> 
> So there is one thing which bothers me in proposed approach (or at least about
> implementation I described).
> 
> Information that if user has custom_passphrase when "encrypt everything" is
true
> is external to the UpdateEntryWithEncryption and I think currently there is no
> tests which check that this hypothesis is always true.
> 
> I see several ways to solve this:
> 1. Pipe passphrase_type_ flag to UpdateEntryWithEncryption.
> 2. Keep clearing of data in sync_encryption_handler_impl.cc, where we have
> explicit signal about passphrase type.
> 3. Do implementation as written in the paragraph above and try to come up with
a
> solution for testing that custom passphrase state == encrypt everything state.
> 
> WDYT?

Good question. In general this is a bit of an issue in the codebase. Passphrase
type state and encrypted type state, although from a user perspective are
coupled, within the code they're independent.

I think I favor plumbing the passphrase type in, and just having DCHECKs. That
makes things more explicit, although might involve touching a number of
callsites and tests.

[melandory, 2016-08-23 23:32:34]

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

[commit-bot: I haz the power, 2016-08-23 23:33:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-08-24 00:31:55]

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

[commit-bot: I haz the power, 2016-08-24 00:32:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-08-24 00:39:15]

Patchset #5 (id:80001) has been deleted

[melandory, 2016-08-24 00:41:56]

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

[melandory, 2016-08-24 00:41:59]

On 2016/08/22 23:50:05, Nicolas Zea wrote:
> On 2016/08/22 22:08:41, melandory wrote:
> >
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> > File components/sync/core_impl/sync_encryption_handler_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> > components/sync/core_impl/sync_encryption_handler_impl.cc:854:
> > child.SetPasswordSpecificsMetadata(
> > On 2016/08/20 00:23:43, Nicolas Zea wrote:
> > > It occurs to me that I don't think you need this. SetPasswordSpecifics
above
> > > should automatically clear this as necessary (see my comment in
> > write_node.cc).
> > > You'll need to make some changes to nigori_util.cc to make that happen.
> > 
> > Disclaimer. Text below is written with an assumption of following setup: 
> > * data is cleared in nigori_util, UpdateEntryWithEncryption
> > * in order to get encryption state we query NigoriHandler::GetEncryptedTypes
> and
> > if encrypted types equal to EncryptableUserTypes, then it is custom
passphrase
> > user .
> > 
> > So there is one thing which bothers me in proposed approach (or at least
about
> > implementation I described).
> > 
> > Information that if user has custom_passphrase when "encrypt everything" is
> true
> > is external to the UpdateEntryWithEncryption and I think currently there is
no
> > tests which check that this hypothesis is always true.
> > 
> > I see several ways to solve this:
> > 1. Pipe passphrase_type_ flag to UpdateEntryWithEncryption.
> > 2. Keep clearing of data in sync_encryption_handler_impl.cc, where we have
> > explicit signal about passphrase type.
> > 3. Do implementation as written in the paragraph above and try to come up
with
> a
> > solution for testing that custom passphrase state == encrypt everything
state.
> > 
> > WDYT?
> 
> Good question. In general this is a bit of an issue in the codebase.
Passphrase
> type state and encrypted type state, although from a user perspective are
> coupled, within the code they're independent.
> 
> I think I favor plumbing the passphrase type in, and just having DCHECKs. That
> makes things more explicit, although might involve touching a number of
> callsites and tests.

PTAL, if approach looks fine I'll continue with plumbing.

[commit-bot: I haz the power, 2016-08-24 00:42:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nicolas Zea, 2016-08-24 01:27:12]

On 2016/08/24 00:41:59, melandory wrote:
> On 2016/08/22 23:50:05, Nicolas Zea wrote:
> > On 2016/08/22 22:08:41, melandory wrote:
> > >
> >
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> > > File components/sync/core_impl/sync_encryption_handler_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> > > components/sync/core_impl/sync_encryption_handler_impl.cc:854:
> > > child.SetPasswordSpecificsMetadata(
> > > On 2016/08/20 00:23:43, Nicolas Zea wrote:
> > > > It occurs to me that I don't think you need this. SetPasswordSpecifics
> above
> > > > should automatically clear this as necessary (see my comment in
> > > write_node.cc).
> > > > You'll need to make some changes to nigori_util.cc to make that happen.
> > > 
> > > Disclaimer. Text below is written with an assumption of following setup: 
> > > * data is cleared in nigori_util, UpdateEntryWithEncryption
> > > * in order to get encryption state we query
NigoriHandler::GetEncryptedTypes
> > and
> > > if encrypted types equal to EncryptableUserTypes, then it is custom
> passphrase
> > > user .
> > > 
> > > So there is one thing which bothers me in proposed approach (or at least
> about
> > > implementation I described).
> > > 
> > > Information that if user has custom_passphrase when "encrypt everything"
is
> > true
> > > is external to the UpdateEntryWithEncryption and I think currently there
is
> no
> > > tests which check that this hypothesis is always true.
> > > 
> > > I see several ways to solve this:
> > > 1. Pipe passphrase_type_ flag to UpdateEntryWithEncryption.
> > > 2. Keep clearing of data in sync_encryption_handler_impl.cc, where we have
> > > explicit signal about passphrase type.
> > > 3. Do implementation as written in the paragraph above and try to come up
> with
> > a
> > > solution for testing that custom passphrase state == encrypt everything
> state.
> > > 
> > > WDYT?
> > 
> > Good question. In general this is a bit of an issue in the codebase.
> Passphrase
> > type state and encrypted type state, although from a user perspective are
> > coupled, within the code they're independent.
> > 
> > I think I favor plumbing the passphrase type in, and just having DCHECKs.
That
> > makes things more explicit, although might involve touching a number of
> > callsites and tests.
> 
> PTAL, if approach looks fine I'll continue with plumbing.

Looking at this, I think this might be trickier than I initially thought. In
particular, the GenericChangeProcessor, which can be called from any thread,
does not have direct access to the passphrase type. It can only get access to
the set of encrypted types via the NigoriHandler. There is no thread-safe way to
get the passphrase type right now. And, the set of encrypted types can actually
be more than just the sensitive types if for some reason a new version of Chrome
were to redefine what the sensitive types were, so just looking at the encrypted
types isn't a good way to go.

Fundamentally, what we need to do is either make a new thread-safe version of
PassphraseType accessible (similar to the GetEncryptedTypes method in
NigoriHandler), or ensure this work of setting/clearing the metadata field only
happens on the sync thread (e.g. the same thread RencryptEverything or our
communication with the sync server happens on). Of these two options, I prefer
exposing a thread-safe version, which then ensures that what we persist into the
sync directory reflects what we send to the server (and any of the idempotency
checks we have in the code don't get confused).

I recommend taking a look at the Vault structure in SyncEncryptionHandlerImpl.
Maybe we should have the passphrase type in there?

[commit-bot: I haz the power, 2016-08-24 03:10:08]

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

[commit-bot: I haz the power, 2016-08-24 03:10:08]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-08-25 01:58:48]

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

[commit-bot: I haz the power, 2016-08-25 01:59:58]

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, 2016-08-25 03:14:22]

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

[commit-bot: I haz the power, 2016-08-25 03:14:23]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-08-26 16:01:04]

On 2016/08/24 01:27:12, Nicolas Zea wrote:
> On 2016/08/24 00:41:59, melandory wrote:
> > On 2016/08/22 23:50:05, Nicolas Zea wrote:
> > > On 2016/08/22 22:08:41, melandory wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> > > > File components/sync/core_impl/sync_encryption_handler_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2260953002/diff/40001/components/sync/core_im...
> > > > components/sync/core_impl/sync_encryption_handler_impl.cc:854:
> > > > child.SetPasswordSpecificsMetadata(
> > > > On 2016/08/20 00:23:43, Nicolas Zea wrote:
> > > > > It occurs to me that I don't think you need this. SetPasswordSpecifics
> > above
> > > > > should automatically clear this as necessary (see my comment in
> > > > write_node.cc).
> > > > > You'll need to make some changes to nigori_util.cc to make that
happen.
> > > > 
> > > > Disclaimer. Text below is written with an assumption of following setup:

> > > > * data is cleared in nigori_util, UpdateEntryWithEncryption
> > > > * in order to get encryption state we query
> NigoriHandler::GetEncryptedTypes
> > > and
> > > > if encrypted types equal to EncryptableUserTypes, then it is custom
> > passphrase
> > > > user .
> > > > 
> > > > So there is one thing which bothers me in proposed approach (or at least
> > about
> > > > implementation I described).
> > > > 
> > > > Information that if user has custom_passphrase when "encrypt everything"
> is
> > > true
> > > > is external to the UpdateEntryWithEncryption and I think currently there
> is
> > no
> > > > tests which check that this hypothesis is always true.
> > > > 
> > > > I see several ways to solve this:
> > > > 1. Pipe passphrase_type_ flag to UpdateEntryWithEncryption.
> > > > 2. Keep clearing of data in sync_encryption_handler_impl.cc, where we
have
> > > > explicit signal about passphrase type.
> > > > 3. Do implementation as written in the paragraph above and try to come
up
> > with
> > > a
> > > > solution for testing that custom passphrase state == encrypt everything
> > state.
> > > > 
> > > > WDYT?
> > > 
> > > Good question. In general this is a bit of an issue in the codebase.
> > Passphrase
> > > type state and encrypted type state, although from a user perspective are
> > > coupled, within the code they're independent.
> > > 
> > > I think I favor plumbing the passphrase type in, and just having DCHECKs.
> That
> > > makes things more explicit, although might involve touching a number of
> > > callsites and tests.
> > 
> > PTAL, if approach looks fine I'll continue with plumbing.
> 
> Looking at this, I think this might be trickier than I initially thought. In
> particular, the GenericChangeProcessor, which can be called from any thread,
> does not have direct access to the passphrase type. It can only get access to
> the set of encrypted types via the NigoriHandler. There is no thread-safe way
to
> get the passphrase type right now. And, the set of encrypted types can
actually
> be more than just the sensitive types if for some reason a new version of
Chrome
> were to redefine what the sensitive types were, so just looking at the
encrypted
> types isn't a good way to go.
> 
> Fundamentally, what we need to do is either make a new thread-safe version of
> PassphraseType accessible (similar to the GetEncryptedTypes method in
> NigoriHandler), or ensure this work of setting/clearing the metadata field
only
> happens on the sync thread (e.g. the same thread RencryptEverything or our
> communication with the sync server happens on). Of these two options, I prefer
> exposing a thread-safe version, which then ensures that what we persist into
the
> sync directory reflects what we send to the server (and any of the idempotency
> checks we have in the code don't get confused).
> 
> I recommend taking a look at the Vault structure in SyncEncryptionHandlerImpl.
> Maybe we should have the passphrase type in there?

CL becomes too big, so here is the split:

* Mostly mechanical change which makes "enum" an "enum class":
https://codereview.chromium.org/2279713002/
* Moving PassphraseType to the separate header:
https://codereview.chromium.org/2279753003/
* Adding passphrase_type to Vault: https://codereview.chromium.org/2278043003/
* Introducing new field in proto, clearing on ReEncryption event:
https://codereview.chromium.org/2278333002
* Passing correct parameters in GenericChangeProessor: still WIP,
https://codereview.chromium.org/2285753002