Changed platform verification user consent logic to be per-domain.

chrome/browser/chromeos/attestation/platform_verification_flow.cc

Index: chrome/browser/chromeos/attestation/platform_verification_flow.cc
diff --git a/chrome/browser/chromeos/attestation/platform_verification_flow.cc b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
index d6ef3ace1ee2847e7048b4176e3ec1fea8cf49fb..c51fea02a9f144625fbf9c5a908ddd3d50dba1b1 100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow.cc
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
@@ -61,7 +61,6 @@ class DefaultDelegate : public PlatformVerificationFlow::Delegate {
   virtual ~DefaultDelegate() {}
 
   virtual void ShowConsentPrompt(
-      PlatformVerificationFlow::ConsentType type,
       content::WebContents* web_contents,
       const PlatformVerificationFlow::Delegate::ConsentCallback& callback)
       OVERRIDE {
@@ -121,6 +120,11 @@ void PlatformVerificationFlow::ChallengePlatformKey(
     ReportError(callback, POLICY_REJECTED);
     return;
   }
+  if (GetURLSpec(web_contents).empty()) {
+    LOG(WARNING) << "PlatformVerificationFlow: Invalid URL.";
+    ReportError(callback, INTERNAL_ERROR);
+    return;
+  }
   BoolDBusMethodCallback dbus_callback = base::Bind(
       &DBusCallback,
       base::Bind(&PlatformVerificationFlow::CheckConsent,
@@ -138,12 +142,21 @@ void PlatformVerificationFlow::CheckConsent(content::WebContents* web_contents,
                                             const std::string& challenge,
                                             const ChallengeCallback& callback,
                                             bool attestation_enrolled) {
-  ConsentType consent_type = CONSENT_TYPE_NONE;
-  if (!attestation_enrolled || IsFirstUse(web_contents)) {
-    consent_type = CONSENT_TYPE_ATTESTATION;
-  } else if (IsAlwaysAskRequired(web_contents)) {
-    consent_type = CONSENT_TYPE_ALWAYS;
+  PrefService* pref_service = GetPrefs(web_contents);
+  if (!pref_service) {
+    LOG(ERROR) << "Failed to get user prefs.";
+    ReportError(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(pref_service, GetURLSpec(web_contents), NULL));
+
   Delegate::ConsentCallback consent_callback = base::Bind(
       &PlatformVerificationFlow::OnConsentResponse,
       weak_factory_.GetWeakPtr(),
@@ -151,12 +164,11 @@ void PlatformVerificationFlow::CheckConsent(content::WebContents* web_contents,
       service_id,
       challenge,
       callback,
-      consent_type);
-  if (consent_type == CONSENT_TYPE_NONE) {
+      consent_required);
+  if (!consent_required) {
     consent_callback.Run(CONSENT_RESPONSE_NONE);
   } else {
-    delegate_->ShowConsentPrompt(consent_type,
-                                 web_contents,
+    delegate_->ShowConsentPrompt(web_contents,

[pastarmovj, 2013/10/24 09:56:36]

nit: This should fit on one line and then you will get rid of the curlies as
well :)
Also you can swap the if and else and then the logic will be even more
straightforward IMHO.

[Darren Krahn, 2013/10/28 23:49:52]

Done.

                                  consent_callback);
   }
 }
@@ -169,9 +181,6 @@ void PlatformVerificationFlow::RegisterProfilePrefs(
   prefs->RegisterDictionaryPref(
       prefs::kRAConsentDomains,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kRAConsentAlways,
-                             false,
-                             user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 void PlatformVerificationFlow::OnConsentResponse(
@@ -179,16 +188,16 @@ void PlatformVerificationFlow::OnConsentResponse(
     const std::string& service_id,
     const std::string& challenge,
     const ChallengeCallback& callback,
-    ConsentType consent_type,
+    bool consent_required,
     ConsentResponse consent_response) {
-  if (consent_type != CONSENT_TYPE_NONE) {
+  if (consent_required) {
     if (consent_response == CONSENT_RESPONSE_NONE) {
       // No user response - do not proceed and do not modify any settings.
       LOG(WARNING) << "PlatformVerificationFlow: No response from user.";
       ReportError(callback, USER_REJECTED);
       return;
     }
-    if (!UpdateSettings(web_contents, consent_type, consent_response)) {
+    if (!UpdateSettings(web_contents, consent_response)) {
       ReportError(callback, INTERNAL_ERROR);
       return;
     }
@@ -285,11 +294,20 @@ PrefService* PlatformVerificationFlow::GetPrefs(
   return user_prefs::UserPrefs::Get(web_contents->GetBrowserContext());
 }
 
-const GURL& PlatformVerificationFlow::GetURL(
+std::string PlatformVerificationFlow::GetURLSpec(
     content::WebContents* web_contents) {
-  if (!testing_url_.is_empty())
-    return testing_url_;
-  return web_contents->GetLastCommittedURL();
+  GURL url;
+  if (!testing_url_.is_empty()) {
+    url = testing_url_;
+  } else {
+    url = web_contents->GetLastCommittedURL();

[Jun Mukai, 2013/10/23 20:26:12]

nit: you don't need braces or 1-line if-else. or, use a conditional operator?

[Darren Krahn, 2013/10/28 23:49:52]

Done.

+  }
+  if (!url.is_valid())
+    return std::string();
+  GURL origin = url.GetOrigin();
+  if (!origin.is_valid())
+    return std::string();
+  return origin.spec();
 }
 
 User* PlatformVerificationFlow::GetUser(content::WebContents* web_contents) {
@@ -322,92 +340,49 @@ bool PlatformVerificationFlow::IsAttestationEnabled(
 
   // Check the user preference for this domain.
   bool enabled_for_domain = false;
-  bool found = GetDomainPref(web_contents, &enabled_for_domain);
+  bool found = GetDomainPref(pref_service,
+                             GetURLSpec(web_contents),
+                             &enabled_for_domain);
   return (!found || enabled_for_domain);
 }
 
-bool PlatformVerificationFlow::IsFirstUse(content::WebContents* web_contents) {
-  PrefService* pref_service = GetPrefs(web_contents);
-  if (!pref_service) {
-    LOG(ERROR) << "Failed to get user prefs.";
-    return true;
-  }
-  return !pref_service->GetBoolean(prefs::kRAConsentFirstTime);
-}
-
-bool PlatformVerificationFlow::IsAlwaysAskRequired(
-    content::WebContents* web_contents) {
-  PrefService* pref_service = GetPrefs(web_contents);
-  if (!pref_service) {
-    LOG(ERROR) << "Failed to get user prefs.";
-    return true;
-  }
-  if (!pref_service->GetBoolean(prefs::kRAConsentAlways))
-    return false;
-  // Show the consent UI if the user has not already explicitly allowed or
-  // denied for this domain.
-  return !GetDomainPref(web_contents, NULL);
-}
-
 bool PlatformVerificationFlow::UpdateSettings(
     content::WebContents* web_contents,
-    ConsentType consent_type,
     ConsentResponse consent_response) {
   PrefService* pref_service = GetPrefs(web_contents);
   if (!pref_service) {
     LOG(ERROR) << "Failed to get user prefs.";
     return false;
   }
-  if (consent_type == CONSENT_TYPE_ATTESTATION) {
-    if (consent_response == CONSENT_RESPONSE_DENY) {
-      pref_service->SetBoolean(prefs::kEnableDRM, false);
-    } else if (consent_response == CONSENT_RESPONSE_ALLOW) {
-      pref_service->SetBoolean(prefs::kRAConsentFirstTime, true);
-      RecordDomainConsent(web_contents, true);
-    } else if (consent_response == CONSENT_RESPONSE_ALWAYS_ASK) {
-      pref_service->SetBoolean(prefs::kRAConsentFirstTime, true);
-      pref_service->SetBoolean(prefs::kRAConsentAlways, true);
-      RecordDomainConsent(web_contents, true);
-    }
-  } else if (consent_type == CONSENT_TYPE_ALWAYS) {
-    bool allowed = (consent_response == CONSENT_RESPONSE_ALLOW ||
-                    consent_response == CONSENT_RESPONSE_ALWAYS_ASK);
-    RecordDomainConsent(web_contents, allowed);
-  }
+  pref_service->SetBoolean(prefs::kRAConsentFirstTime, true);
+  RecordDomainConsent(pref_service,
+                      GetURLSpec(web_contents),
+                      (consent_response == CONSENT_RESPONSE_ALLOW));
   return true;
 }
 
-bool PlatformVerificationFlow::GetDomainPref(
-    content::WebContents* web_contents,
-    bool* pref_value) {
-  PrefService* pref_service = GetPrefs(web_contents);
+bool PlatformVerificationFlow::GetDomainPref(PrefService* pref_service,
+                                             const std::string& url_spec,
+                                             bool* pref_value) {
   CHECK(pref_service);
-  base::DictionaryValue::Iterator iter(
-      *pref_service->GetDictionary(prefs::kRAConsentDomains));
-  const GURL& url = GetURL(web_contents);
-  while (!iter.IsAtEnd()) {
-    if (url.DomainIs(iter.key().c_str())) {
-      if (pref_value) {
-        if (!iter.value().GetAsBoolean(pref_value)) {
-          LOG(ERROR) << "Unexpected pref type.";
-          *pref_value = false;
-        }
-      }
-      return true;
-    }
-    iter.Advance();
-  }
-  return false;
+  CHECK(!url_spec.empty());
+  const base::DictionaryValue* values =
+      pref_service->GetDictionary(prefs::kRAConsentDomains);

[Jun Mukai, 2013/10/23 20:26:12]

Can we use kContentSettings and chrome/common/content_settings* here?
Then, I can easily integrate the pref into the content-settings UI in
chrome://settings page :)

[Darren Krahn, 2013/10/28 23:49:52]

I've reused the content setting used in clank.

+  CHECK(values);
+  bool tmp = false;
+  bool result = values->GetBoolean(url_spec, &tmp);
+  if (pref_value)
+    *pref_value = tmp;
+  return result;
 }
 
-void PlatformVerificationFlow::RecordDomainConsent(
-    content::WebContents* web_contents,
-    bool allow_domain) {
-  PrefService* pref_service = GetPrefs(web_contents);
+void PlatformVerificationFlow::RecordDomainConsent(PrefService* pref_service,
+                                                   const std::string& url_spec,
+                                                   bool allow_domain) {
   CHECK(pref_service);
+  CHECK(!url_spec.empty());
   DictionaryPrefUpdate updater(pref_service, prefs::kRAConsentDomains);
-  const GURL& url = GetURL(web_contents);
-  updater->SetBoolean(url.host(), allow_domain);
+  updater->SetBoolean(url_spec, allow_domain);
 }
 
 }  // namespace attestation