Simplify PlatformVerificationFlow::CheckConsent() logic.

BUG=455262
TEST=Fixed unittest and manually tested various scenarios.

Review URL: https://codereview.chromium.org/979273003

Cr-Commit-Position: refs/heads/master@{#319511}

5 files changed, 55 insertions, 56 deletions

diff --git a/chrome/browser/chromeos/attestation/platform_verification_flow.cc b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
index 6330430..5e03617 100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow.cc
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
@@ -216,23 +216,33 @@ void PlatformVerificationFlow::CheckEnrollment(const ChallengeContext& context,
 }
 
 void PlatformVerificationFlow::CheckConsent(const ChallengeContext& context,
-                                            bool attestation_enrolled) {
-  PrefService* pref_service = delegate_->GetPrefs(context.web_contents);
+                                            bool /* attestation_enrolled */) {
+  content::WebContents* web_contents = context.web_contents;
+
+  bool enabled_for_origin = false;
+  bool found =
+      GetOriginPref(delegate_->GetContentSettings(web_contents),
+                    delegate_->GetURL(web_contents), &enabled_for_origin);
+  if (found && !enabled_for_origin) {
+    VLOG(1) << "Platform verification denied because the origin has been "
+            << "blocked by the user.";
+    ReportError(context.callback, USER_REJECTED);
+    return;
+  }
+
+  PrefService* pref_service = delegate_->GetPrefs(web_contents);
   if (!pref_service) {
     LOG(ERROR) << "Failed to get user prefs.";
     ReportError(context.callback, INTERNAL_ERROR);
     return;
   }
-  bool consent_required = (
-      // Consent required if attestation has never been enrolled on this device.
-      !attestation_enrolled ||
-      // Consent required if this is the first use of attestation for content
-      // protection on this device.
-      !pref_service->GetBoolean(prefs::kRAConsentFirstTime) ||
-      // Consent required if consent has never been given for this domain.
-      !GetDomainPref(delegate_->GetContentSettings(context.web_contents),
-                     delegate_->GetURL(context.web_contents),
-                     NULL));
+
+  // Consent required if user has never given consent for this origin, or if
+  // user has never given consent to attestation for content protection on this
+  // device.
+  bool consent_required =
+      !found || !pref_service->GetBoolean(prefs::kRAConsentGranted);
+
   Delegate::ConsentCallback consent_callback = base::Bind(
       &PlatformVerificationFlow::OnConsentResponse,
       this,
@@ -252,8 +262,8 @@ void PlatformVerificationFlow::CheckConsent(const ChallengeContext& context,
 
 void PlatformVerificationFlow::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* prefs) {
-  prefs->RegisterBooleanPref(prefs::kRAConsentFirstTime,
-                             false,
+  prefs->RegisterBooleanPref(prefs::kRAConsentGranted,
+                             false,  // Default value.
                              user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
@@ -419,13 +429,13 @@ bool PlatformVerificationFlow::IsAttestationEnabled(
     return false;
   }
 
-  // Check the user preference for this domain.
-  bool enabled_for_domain = false;
-  bool found = GetDomainPref(delegate_->GetContentSettings(web_contents),
-                             delegate_->GetURL(web_contents),
-                             &enabled_for_domain);
-  if (found && !enabled_for_domain) {
-    VLOG(1) << "Platform verification denied because the domain has been "
+  // Check the user preference for this origin.
+  bool enabled_for_origin = false;
+  bool found =
+      GetOriginPref(delegate_->GetContentSettings(web_contents),
+                    delegate_->GetURL(web_contents), &enabled_for_origin);
+  if (found && !enabled_for_origin) {
+    VLOG(1) << "Platform verification denied because the origin has been "
             << "blocked by the user.";
     return false;
   }
@@ -440,14 +450,18 @@ bool PlatformVerificationFlow::UpdateSettings(
     LOG(ERROR) << "Failed to get user prefs.";
     return false;
   }
-  pref_service->SetBoolean(prefs::kRAConsentFirstTime, true);
-  RecordDomainConsent(delegate_->GetContentSettings(web_contents),
+
+  if (consent_response == CONSENT_RESPONSE_ALLOW) {
+    pref_service->SetBoolean(prefs::kRAConsentGranted, true);
+  }
+
+  RecordOriginConsent(delegate_->GetContentSettings(web_contents),
                       delegate_->GetURL(web_contents),
                       (consent_response == CONSENT_RESPONSE_ALLOW));
   return true;
 }
 
-bool PlatformVerificationFlow::GetDomainPref(
+bool PlatformVerificationFlow::GetOriginPref(
     HostContentSettingsMap* content_settings,
     const GURL& url,
     bool* pref_value) {
@@ -465,10 +479,10 @@ bool PlatformVerificationFlow::GetDomainPref(
   return true;
 }
 
-void PlatformVerificationFlow::RecordDomainConsent(
+void PlatformVerificationFlow::RecordOriginConsent(
     HostContentSettingsMap* content_settings,
     const GURL& url,
-    bool allow_domain) {
+    bool allow_origin) {
   CHECK(content_settings);
   CHECK(url.is_valid());
   // Build a pattern to represent scheme and host.
@@ -486,8 +500,8 @@ void PlatformVerificationFlow::RecordDomainConsent(
     builder->WithPortWildcard();
   ContentSettingsPattern pattern = builder->Build();
   if (pattern.IsValid()) {
-    ContentSetting setting = allow_domain ? CONTENT_SETTING_ALLOW
-                                          : CONTENT_SETTING_BLOCK;
+    ContentSetting setting =
+        allow_origin ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
     content_settings->SetContentSetting(
         pattern,
         pattern,
diff --git a/chrome/browser/chromeos/attestation/platform_verification_flow.h b/chrome/browser/chromeos/attestation/platform_verification_flow.h
index 09d9887..3d3b0aa 100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow.h
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow.h
@@ -257,18 +257,18 @@ class PlatformVerificationFlow
   bool UpdateSettings(content::WebContents* web_contents,
                       ConsentResponse consent_response);
 
-  // Finds the domain-specific consent pref in |content_settings| for |url|.  If
-  // a pref exists for the domain, returns true and sets |pref_value| if it is
+  // Finds the origin-specific consent pref in |content_settings| for |url|.  If
+  // a pref exists for the origin, returns true and sets |pref_value| if it is
   // not NULL.
-  bool GetDomainPref(HostContentSettingsMap* content_settings,
+  bool GetOriginPref(HostContentSettingsMap* content_settings,
                      const GURL& url,
                      bool* pref_value);
 
-  // Records the domain-specific consent pref in |content_settings| for |url|.
-  // The pref will be set to |allow_domain|.
-  void RecordDomainConsent(HostContentSettingsMap* content_settings,
+  // Records the origin-specific consent pref in |content_settings| for |url|.
+  // The pref will be set to |allow_origin|.
+  void RecordOriginConsent(HostContentSettingsMap* content_settings,
                            const GURL& url,
-                           bool allow_domain);
+                           bool allow_origin);
 
   // Returns true iff |certificate| is an expired X.509 certificate.
   bool IsExpired(const std::string& certificate);
diff --git a/chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc b/chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc
index 36a8181..c984e2c 100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc
@@ -207,9 +207,8 @@ class PlatformVerificationFlowTest : public ::testing::Test {
     cros_settings->SetBoolean(kAttestationForContentProtectionEnabled, true);
 
     // Start with the first-time setting set since most tests want this.
-    fake_delegate_.pref_service().SetUserPref(prefs::kRAConsentFirstTime,
+    fake_delegate_.pref_service().SetUserPref(prefs::kRAConsentGranted,
                                               new base::FundamentalValue(true));
-
   }
 
   void TearDown() {
@@ -244,8 +243,7 @@ class PlatformVerificationFlowTest : public ::testing::Test {
   }
 
   void SetUserConsent(const GURL& url, bool allow) {
-    verifier_->RecordDomainConsent(fake_delegate_.GetContentSettings(NULL),
-                                   url,
+    verifier_->RecordOriginConsent(fake_delegate_.GetContentSettings(NULL), url,
                                    allow);
   }
 
@@ -332,22 +330,9 @@ TEST_F(PlatformVerificationFlowTest, SuccessNoConsent) {
   EXPECT_EQ(0, fake_delegate_.num_consent_calls());
 }
 
-TEST_F(PlatformVerificationFlowTest, SuccessWithAttestationConsent) {
-  SetUserConsent(GURL(kTestURL), true);
-  fake_cryptohome_client_.set_attestation_enrolled(false);
-  ExpectAttestationFlow();
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
-  EXPECT_EQ(kTestSignedData, challenge_salt_);
-  EXPECT_EQ(kTestSignature, challenge_signature_);
-  EXPECT_EQ(kTestCertificate, certificate_);
-  EXPECT_EQ(1, fake_delegate_.num_consent_calls());
-}
-
-TEST_F(PlatformVerificationFlowTest, SuccessWithFirstTimeConsent) {
+TEST_F(PlatformVerificationFlowTest, SuccessWithConsent) {
   SetUserConsent(GURL(kTestURL), true);
-  fake_delegate_.pref_service().SetUserPref(prefs::kRAConsentFirstTime,
+  fake_delegate_.pref_service().SetUserPref(prefs::kRAConsentGranted,
                                             new base::FundamentalValue(false));
   ExpectAttestationFlow();
   verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index e49fdc3..934c107 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -751,7 +751,7 @@ const char kOpenNetworkConfiguration[] = "onc";
 
 // A boolean pref that tracks whether the user has already given consent for
 // enabling remote attestation for content protection.
-const char kRAConsentFirstTime[] = "settings.privacy.ra_consent";
+const char kRAConsentGranted[] = "settings.privacy.ra_consent_granted";
 
 // A boolean pref recording whether user has dismissed the multiprofile
 // itroduction dialog show.
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index 4e462cf..8a4c013 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -256,7 +256,7 @@ extern const char kAttestationEnabled[];
 extern const char kAttestationExtensionWhitelist[];
 extern const char kTouchHudProjectionEnabled[];
 extern const char kOpenNetworkConfiguration[];
-extern const char kRAConsentFirstTime[];
+extern const char kRAConsentGranted[];
 extern const char kMultiProfileNeverShowIntro[];
 extern const char kMultiProfileWarningShowDismissed[];
 extern const char kMultiProfileUserBehavior[];