New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 820049 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

Sync data and metadata can, in rare cases, leak across accounts on shared devices

Project Member Reported by mastiz@chromium.org, Mar 8 2018

Issue description

Multiple sync datatypes handle the disable-sync event by deleting data and/or metadata from disk.

As per data is concerned: datatypes like user consents are on a per-gaia-id basis and shouldn't be kept around when sync is disabled (or at least when the user signs out). 

As per sync metadata is concerned, it has been historically decided that it's most sensible privacy-wise to wipe it out, and reconstruct again when sync is reenabled. 

In the current implementation, we handle the [meta]data wipeout when the user disables sync (or signs out). If it crashes during that process, we don't have the ability to detect that on the next run (and hence consents would be attributed to the next user enabling sync).

A more robust implementation could involve extending our proto ModelTypeState (part of metadata) with a field that represents the owner's identity. If we find a mismatch when sync is starting, all metadata (and in some cases like user consents) should be thrown away.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c38383dc5c8277199514abb4d1d30f897989a582

commit c38383dc5c8277199514abb4d1d30f897989a582
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Mar 12 11:33:32 2018

Improve deletion of [meta]data during DisableSync

We introduce a dedicated method in ModelTypeStore to avoid round trips
across threads when a datatype's desire is to delete it all (data and
metadata). Benefits include:
- Simpler code: bridge's don't need to read+delete.
- (Hopefully) more robust against crashes during DisableSync().
- Less error-prone: we've had examples in the past where overriding
  DisableSync() forgot to call the base class's implementation, leading
  to metadata staying around.

Bug:  819233 ,  820049 
Change-Id: If6b7b04ee73b8c6a28aef7380a7eb91b866f1567
Reviewed-on: https://chromium-review.googlesource.com/955624
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542461}
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/device_info/device_info_sync_bridge.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/device_info/device_info_sync_bridge.h
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/device_info/device_info_sync_bridge_unittest.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model/model_type_store.h
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_backend.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_backend.h
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_backend_unittest.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_impl.h
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_impl_unittest.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/user_events/user_event_sync_bridge_unittest.cc

Cc: s...@chromium.org
 Issue 584503  has been merged into this issue.

Comment 3 by jkrcal@chromium.org, Apr 11 2018

Cc: jkrcal@chromium.org
Labels: -Restrict-View-Google
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f28c2bc895daf8d39951249b2fbdc1555acd182

commit 0f28c2bc895daf8d39951249b2fbdc1555acd182
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Jun 12 20:22:25 2018

Propagate more context information in OnSyncStarting()

We rename ActivationContext-->DataTypeActivationResponse to avoid
confusion, and introduce a new struct (DataTypeActivationRequest) that
bundles context information that is relevant during activation of a
datatype.

In this implementation, ModelTypeController pulls those bits from
SyncService's public API, but it may make sense to instead explicitly
plumb them through DataTypeManager, in future patches.

Bug:  833483 , 820049 ,850428
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I91c5c3552dbd5db1c446dd6e00ef74389135af87
Reviewed-on: https://chromium-review.googlesource.com/1087062
Reviewed-by: Mike Pinkerton <pinkerton@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566550}
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/BUILD.gn
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/glue/sync_backend_host_impl.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/glue/sync_backend_host_impl.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/model_type_controller_unittest.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/sync_client_mock.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/sync_client_mock.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/driver/sync_service.h
[delete] https://crrev.com/ece8228cecf913285fec932ca62516fd1f305fd8/components/sync/engine/activation_context.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_request.cc
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_request.h
[add] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_response.cc
[rename] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/data_type_activation_response.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_model_type_connector.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_model_type_connector.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_sync_engine.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/fake_sync_engine.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/model_type_configurer.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine/model_type_connector.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_connector_proxy.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_connector_proxy.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_registry.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_registry.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/engine_impl/model_type_registry_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model/fake_model_type_controller_delegate.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model/fake_model_type_controller_delegate.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model/model_type_controller_delegate.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync_bookmarks/bookmark_model_type_processor.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync_bookmarks/bookmark_model_type_processor.h
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/0f28c2bc895daf8d39951249b2fbdc1555acd182/ios/chrome/test/app/sync_test_util.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f90b98db91ba39761b5998fa148f4f2746edfc14

commit f90b98db91ba39761b5998fa148f4f2746edfc14
Author: Mohamed Amir Yosef <mamir@chromium.org>
Date: Mon Jun 25 18:39:45 2018

[Sync::USS] Add cache_guid to model_type_state.proto

In preparation for detecting a mismatch between the running sync client
cache guid, and the one persisted in the metadata, a new field is
introduced in model_type_state.proto such that the cache_guid is
persisted togethter with other model type state fields.

Mismatch detection will be used to prevent leak of sync data between
different accounts syncing on the same device.

Bug:  820049 
Change-Id: If537753f0e3011148692c27bd3b63a0742872a28
Reviewed-on: https://chromium-review.googlesource.com/1113338
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570104}
[modify] https://crrev.com/f90b98db91ba39761b5998fa148f4f2746edfc14/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/f90b98db91ba39761b5998fa148f4f2746edfc14/components/sync/protocol/model_type_state.proto
[modify] https://crrev.com/f90b98db91ba39761b5998fa148f4f2746edfc14/components/sync_bookmarks/bookmark_model_type_processor.cc
[modify] https://crrev.com/f90b98db91ba39761b5998fa148f4f2746edfc14/components/sync_bookmarks/bookmark_model_type_processor.h
[modify] https://crrev.com/f90b98db91ba39761b5998fa148f4f2746edfc14/components/sync_bookmarks/synced_bookmark_tracker.h

Labels: sync-fixit-2018q3
The remaining work here includes:
1. Detecting a cache_guid mismatches in ClientTagBasedModelTypeProcessor::ConnectIfReady().
2. Ignore sync metadata in that case, and tell the bridge to delete it (treat as DisableSync/EnableSync).
Owner: mamir@chromium.org
Status: Assigned (was: Available)
Assigning to mamir@ as per our offline discussion, mostly because it's an interesting scenario for bookmarks as well.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98953818dd8ce902b5ee6a7f965871f0d067de96

commit 98953818dd8ce902b5ee6a7f965871f0d067de96
Author: Mohamed Amir Yosef <mamir@chromium.org>
Date: Wed Jul 11 08:15:17 2018

[Sync::USS] Delete metadata upon a cacheGUID mismatch

This CL addresses the remaining work to fix the attached bug
which includes:

1. Detecting a cache_guid mismatches in
ClientTagBasedModelTypeProcessor::ConnectIfReady().

2. Ignore sync metadata in that case, and tell the bridge to delete it
 (treat as DisableSync/EnableSync).


Change-Id: Ibf2f4c0b89872884f1c09a3a2e8fec1136014b93

Bug:  820049 
Change-Id: Ibf2f4c0b89872884f1c09a3a2e8fec1136014b93
Reviewed-on: https://chromium-review.googlesource.com/1130527
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574107}
[modify] https://crrev.com/98953818dd8ce902b5ee6a7f965871f0d067de96/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/98953818dd8ce902b5ee6a7f965871f0d067de96/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/98953818dd8ce902b5ee6a7f965871f0d067de96/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

Status: Fixed (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Yesterday (29 hours ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b3df3ef06b31337442e932fc41a3b30e84e3b70

commit 7b3df3ef06b31337442e932fc41a3b30e84e3b70
Author: Mohamed Amir Yosef <mamir@chromium.org>
Date: Tue Jan 22 08:45:55 2019

[Sync::USS] Fix for cache guid mismatch when loading bookmarks

Currently, if there is a mistmatch in the cache guid between the persisted
one and the one coming from sync, the bookmark model will stay in
the MODEL_STARTING state and effectively block sync machinery.

This patch makes sure that in such case, the BookmarkModelTypeProcessor
will consider the persisted data corrupted, and start clean.

Bug: 516866, 820049 , 816723
Change-Id: I06605b20f3357016d1b2d5a86009ce7abe0b392d
Reviewed-on: https://chromium-review.googlesource.com/c/1425501
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624747}
[modify] https://crrev.com/7b3df3ef06b31337442e932fc41a3b30e84e3b70/components/sync_bookmarks/bookmark_model_type_processor.cc

Sign in to add a comment