[sync] Properly handle case where sync is disabled by dasher admin

If a user signs in with a dasher account for which sync is disabled by
admin, SyncerProtoUtil::PostClientToServerMessage neglects to check if
sync is disabled, and incorrectly flags the server response as a
NOT_MY_BIRTHDAY error. This results in incorrect sync state showing up
in about:sync, and UI brokenness, where we mistakenly expose the "Set up
sync..."  button.

This patch updates SyncerProtoUtil::PostClientToServerMessage to first
check the server response to see if sync is disabled, prior to checking
for other sync protocol errors.

BUG=276030
TEST=Sign in with a dasher account for which sync is disabled. The "Set
up sync..." button must not be exposed in the UI.

Review URL: https://chromiumcodereview.appspot.com/23030008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@218906 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 34 insertions, 4 deletions

diff --git a/sync/engine/syncer_proto_util.cc b/sync/engine/syncer_proto_util.cc
index 690a837..085bed4 100644
--- a/sync/engine/syncer_proto_util.cc
+++ b/sync/engine/syncer_proto_util.cc
@@ -199,6 +199,7 @@ SyncProtocolError ConvertErrorPBToLocalType(
   return sync_protocol_error;
 }
 
+// static
 bool SyncerProtoUtil::VerifyResponseBirthday(
     const ClientToServerResponse& response,
     syncable::Directory* dir) {
@@ -231,6 +232,13 @@ bool SyncerProtoUtil::VerifyResponseBirthday(
 }
 
 // static
+bool SyncerProtoUtil::IsSyncDisabledByAdmin(
+    const sync_pb::ClientToServerResponse& response) {
+  return (response.has_error_code() &&
+          response.error_code() == sync_pb::SyncEnums::DISABLED_BY_ADMIN);
+}
+
+// static
 void SyncerProtoUtil::AddRequestBirthday(syncable::Directory* dir,
                                          ClientToServerMessage* msg) {
   if (!dir->store_birthday().empty())
@@ -380,11 +388,14 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
 
   SyncProtocolError sync_protocol_error;
 
-  // Birthday mismatch overrides any error that is sent by the server.
-  if (!VerifyResponseBirthday(*response, dir)) {
+  // The DISABLED_BY_ADMIN error overrides other errors sent by the server.
+  if (IsSyncDisabledByAdmin(*response)) {
+    sync_protocol_error.error_type = DISABLED_BY_ADMIN;
+    sync_protocol_error.action = STOP_SYNC_FOR_DISABLED_ACCOUNT;
+  } else if (!VerifyResponseBirthday(*response, dir)) {
+    // If sync isn't disabled, first check for a birthday mismatch error.
     sync_protocol_error.error_type = NOT_MY_BIRTHDAY;
-     sync_protocol_error.action =
-         DISABLE_SYNC_ON_CLIENT;
+    sync_protocol_error.action = DISABLE_SYNC_ON_CLIENT;
   } else if (response->has_error()) {
     // This is a new server. Just get the error from the protocol.
     sync_protocol_error = ConvertErrorPBToLocalType(response->error());
diff --git a/sync/engine/syncer_proto_util.h b/sync/engine/syncer_proto_util.h
index 4e627e0..d2468c0 100644
--- a/sync/engine/syncer_proto_util.h
+++ b/sync/engine/syncer_proto_util.h
@@ -129,6 +129,10 @@ class SYNC_EXPORT_PRIVATE SyncerProtoUtil {
       const sync_pb::ClientToServerResponse& response,
       syncable::Directory* dir);
 
+  // Returns true if sync is disabled by admin for a dasher account.
+  static bool IsSyncDisabledByAdmin(
+      const sync_pb::ClientToServerResponse& response);
+
   // Post the message using the scm, and do some processing on the returned
   // headers. Decode the server response.
   static bool PostAndProcessHeaders(ServerConnectionManager* scm,
@@ -142,6 +146,7 @@ class SYNC_EXPORT_PRIVATE SyncerProtoUtil {
   friend class SyncerProtoUtilTest;
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, AddRequestBirthday);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, PostAndProcessHeaders);
+  FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, VerifyDisabledByAdmin);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, VerifyResponseBirthday);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, HandleThrottlingNoDatatypes);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, HandleThrottlingWithDatatypes);
diff --git a/sync/engine/syncer_proto_util_unittest.cc b/sync/engine/syncer_proto_util_unittest.cc
index be49413..c288132 100644
--- a/sync/engine/syncer_proto_util_unittest.cc
+++ b/sync/engine/syncer_proto_util_unittest.cc
@@ -227,6 +227,20 @@ TEST_F(SyncerProtoUtilTest, VerifyResponseBirthday) {
   EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
 }
 
+TEST_F(SyncerProtoUtilTest, VerifyDisabledByAdmin) {
+  // No error code
+  sync_pb::ClientToServerResponse response;
+  EXPECT_FALSE(SyncerProtoUtil::IsSyncDisabledByAdmin(response));
+
+  // Has error code, but not disabled
+  response.set_error_code(sync_pb::SyncEnums::NOT_MY_BIRTHDAY);
+  EXPECT_FALSE(SyncerProtoUtil::IsSyncDisabledByAdmin(response));
+
+  // Has error code, and is disabled by admin
+  response.set_error_code(sync_pb::SyncEnums::DISABLED_BY_ADMIN);
+  EXPECT_TRUE(SyncerProtoUtil::IsSyncDisabledByAdmin(response));
+}
+
 TEST_F(SyncerProtoUtilTest, AddRequestBirthday) {
   EXPECT_TRUE(directory()->store_birthday().empty());
   ClientToServerMessage msg;