Simplify PlatformVerificationFlow::CheckConsent() logic.

chrome/browser/chromeos/attestation/platform_verification_flow.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/attestation/platform_verification_flow.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 198 matching lines @@
     return;
   }
   BoolDBusMethodCallback dbus_callback = base::Bind(
       &DBusCallback,
       base::Bind(&PlatformVerificationFlow::CheckConsent, this, context),
       base::Bind(&ReportError, context.callback, INTERNAL_ERROR));
   cryptohome_client_->TpmAttestationIsEnrolled(dbus_callback);
 }
 
 void PlatformVerificationFlow::CheckConsent(const ChallengeContext& context,
-                                            bool attestation_enrolled) {
-  PrefService* pref_service = delegate_->GetPrefs(context.web_contents);
+                                            bool /* attestation_enrolled */) {

[xhwang, 2015/03/06 17:15:09]

We can do a lot of cleanup around this. But since this is still hacky and we'll
do more clean up after the content setting is not synced, I'd like to keep this
CL small and do that later.

+  content::WebContents* web_contents = context.web_contents;
+
+  bool enabled_for_domain = false;

[ddorwin, 2015/03/06 17:46:11]

s/domain/origin/
The function would need to be renamed too.

[xhwang, 2015/03/06 18:40:10]

Done.

+  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 "
+            << "blocked by the user.";
+    ReportError(context.callback, USER_REJECTED);

[Darren Krahn, 2015/03/06 17:28:35]

It seems this is the most useful when the content settings *are* synced. Useful
in the sense that the user will see potentially fewer dialogs.

[xhwang, 2015/03/06 18:40:10]

The settings are still synced. Actually this is already checked due to l.182,
but the setting could be changed since then (due to user action or syncing), so
we are checking again for the integrity of this function. All of these could be
simplified when we stop syncing the content setting.

+    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 to attestation for content
+  // protection on this device, or if user has never given consent for this

[ddorwin, 2015/03/06 17:46:11]

you probably want to reverse this if you do the comment below

[xhwang, 2015/03/06 18:40:10]

Done.

+  // domain.
+  bool consent_required =
+      !pref_service->GetBoolean(prefs::kRAConsentGranted) || !found;

[ddorwin, 2015/03/06 17:46:11]

nit: Swap to short circuit with the bool check

[xhwang, 2015/03/06 18:40:10]

Done.

+
   Delegate::ConsentCallback consent_callback = base::Bind(
       &PlatformVerificationFlow::OnConsentResponse,
       this,
       context,
       consent_required);
   if (consent_required) {
     // TODO(xhwang): Using delegate_->GetURL() here is not right. The consent
     // may be requested by a frame from a different origin. This will be solved
     // when http://crbug.com/454847 is fixed.
     delegate_->ShowConsentPrompt(
         context.web_contents,
         delegate_->GetURL(context.web_contents).GetOrigin(), consent_callback);
   } else {
     consent_callback.Run(CONSENT_RESPONSE_NONE);
   }
 }
 
 void PlatformVerificationFlow::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* prefs) {
-  prefs->RegisterBooleanPref(prefs::kRAConsentFirstTime,
-                             false,
+  prefs->RegisterBooleanPref(prefs::kRAConsentGranted, false,

[ddorwin, 2015/03/06 17:46:11]

nit: false,  // Default.

[xhwang, 2015/03/06 18:40:10]

Done.

                              user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 void PlatformVerificationFlow::OnConsentResponse(
     const ChallengeContext& context,
     bool consent_required,
     ConsentResponse consent_response) {
   if (consent_required) {
     if (consent_response == CONSENT_RESPONSE_NONE) {
       // No user response - do not proceed and do not modify any settings.
@@ skipping 166 matching lines @@
 }
 
 bool PlatformVerificationFlow::UpdateSettings(
     content::WebContents* web_contents,
     ConsentResponse consent_response) {
   PrefService* pref_service = delegate_->GetPrefs(web_contents);
   if (!pref_service) {
     LOG(ERROR) << "Failed to get user prefs.";
     return false;
   }
-  pref_service->SetBoolean(prefs::kRAConsentFirstTime, true);
+
+  if (consent_response == CONSENT_RESPONSE_ALLOW)

[Darren Krahn, 2015/03/06 17:28:35]

optional nit: Since the 'goto fail' bug I've started to add {} to even
single-line conditions to help catch bad merges.

[xhwang, 2015/03/06 18:40:10]

Done.

+    pref_service->SetBoolean(prefs::kRAConsentGranted, true);
+
   RecordDomainConsent(delegate_->GetContentSettings(web_contents),
                       delegate_->GetURL(web_contents),
                       (consent_response == CONSENT_RESPONSE_ALLOW));
   return true;
 }
 
 bool PlatformVerificationFlow::GetDomainPref(
     HostContentSettingsMap* content_settings,
     const GURL& url,
     bool* pref_value) {
@@ skipping 51 matching lines @@
                                             certificate.length()));
   if (!x509.get() || x509->valid_expiry().is_null()) {
     LOG(WARNING) << "Failed to parse certificate, cannot check expiry.";
     return false;
   }
   return (base::Time::Now() > x509->valid_expiry());
 }
 
 }  // namespace attestation
 }  // namespace chromeos