media: Refactor PlatformVerificationFlow.

chrome/browser/media/protected_media_identifier_permission_context.cc

Index: chrome/browser/media/protected_media_identifier_permission_context.cc
diff --git a/chrome/browser/media/protected_media_identifier_permission_context.cc b/chrome/browser/media/protected_media_identifier_permission_context.cc
index 5b95217e33d76a21d87bae31ab400f9d58592921..de7d3148c4927c68b2c8a50185a03df5c09c6f1f 100644
--- a/chrome/browser/media/protected_media_identifier_permission_context.cc
+++ b/chrome/browser/media/protected_media_identifier_permission_context.cc
@@ -10,6 +10,7 @@
 #include "chrome/common/pref_names.h"
 #include "components/content_settings/core/common/permission_request_id.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 
 #if defined(OS_CHROMEOS)
@@ -18,10 +19,11 @@
 #include "chrome/browser/chromeos/attestation/platform_verification_dialog.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/settings/cros_settings_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/user_prefs/user_prefs.h"
 #include "ui/views/widget/widget.h"
 
 using chromeos::attestation::PlatformVerificationDialog;
-using chromeos::attestation::PlatformVerificationFlow;
 #endif
 
 ProtectedMediaIdentifierPermissionContext::
@@ -39,6 +41,16 @@ ProtectedMediaIdentifierPermissionContext::
     ~ProtectedMediaIdentifierPermissionContext() {
 }
 
+#if defined(OS_CHROMEOS)
+// static
+void ProtectedMediaIdentifierPermissionContext::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* prefs) {
+  prefs->RegisterBooleanPref(prefs::kRAConsentGranted,
+                             false,  // Default value.
+                             user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+}
+#endif
+
 void ProtectedMediaIdentifierPermissionContext::RequestPermission(
     content::WebContents* web_contents,
     const PermissionRequestID& id,
@@ -49,6 +61,9 @@ void ProtectedMediaIdentifierPermissionContext::RequestPermission(
 
   GURL embedding_origin = web_contents->GetLastCommittedURL().GetOrigin();
 
+  DVLOG(1) << __FUNCTION__ << ": (" << requesting_origin.spec() << ", "
+           << embedding_origin.spec() << ")";
+
   if (!requesting_origin.is_valid() || !embedding_origin.is_valid() ||
       !IsProtectedMediaIdentifierEnabled()) {
     NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
@@ -57,18 +72,24 @@ void ProtectedMediaIdentifierPermissionContext::RequestPermission(
   }
 
 #if defined(OS_CHROMEOS)
-  // On ChromeOS, we don't use PermissionContextBase::RequestPermission() which
-  // uses the standard permission infobar/bubble UI. See http://crbug.com/454847
-  // Instead, we check the content setting and show the existing platform
-  // verification UI.
-  // TODO(xhwang): Remove when http://crbug.com/454847 is fixed.
-  ContentSetting content_setting =
-      GetPermissionStatus(requesting_origin, embedding_origin);
+  ContentSetting content_setting = PermissionContextBase::GetPermissionStatus(
+      requesting_origin, embedding_origin);
+  if (content_setting == CONTENT_SETTING_BLOCK) {
+    NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
+                        false /* persist */, CONTENT_SETTING_BLOCK);
+    return;
+  }
+
+  // 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 =
+      content_setting != CONTENT_SETTING_ALLOW ||
+      !profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted);
 
-  if (content_setting == CONTENT_SETTING_ALLOW ||
-      content_setting == CONTENT_SETTING_BLOCK) {
+  if (!consent_required) {

[ddorwin, 2015/03/12 03:58:12]

nit: All the negative logic is a bit confusing. Perhaps invert it to
full_consent_given or something.

[xhwang, 2015/03/12 17:35:26]

Done.

     NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
-                        false /* persist */, content_setting);
+                        false /* persist */, CONTENT_SETTING_ALLOW);
     return;
   }
 
@@ -80,10 +101,15 @@ void ProtectedMediaIdentifierPermissionContext::RequestPermission(
     return;
   }
 
+  // On ChromeOS, we don't use PermissionContextBase::RequestPermission() which
+  // uses the standard permission infobar/bubble UI. See http://crbug.com/454847
+  // Instead, we check the content setting and show the existing platform

[ddorwin, 2015/03/12 03:58:12]

... setting (above) and ... ?

[xhwang, 2015/03/12 17:35:27]

Done.

+  // verification UI.
+  // TODO(xhwang): Remove when http://crbug.com/454847 is fixed.
   views::Widget* widget = PlatformVerificationDialog::ShowDialog(
       web_contents, requesting_origin,
       base::Bind(&ProtectedMediaIdentifierPermissionContext::
-                     OnPlatformVerificationResult,
+                     OnPlatformVerificationConsentResponse,
                  weak_factory_.GetWeakPtr(), web_contents, id,
                  requesting_origin, embedding_origin, callback));
   pending_requests_.insert(
@@ -97,9 +123,22 @@ void ProtectedMediaIdentifierPermissionContext::RequestPermission(
 ContentSetting ProtectedMediaIdentifierPermissionContext::GetPermissionStatus(
       const GURL& requesting_origin,
       const GURL& embedding_origin) const {
+  DVLOG(1) << __FUNCTION__ << ": (" << requesting_origin.spec() << ", "
+           << embedding_origin.spec() << ")";
+
   if (!IsProtectedMediaIdentifierEnabled())
     return CONTENT_SETTING_BLOCK;
 
+#if defined(OS_CHROMEOS)
+  // Block if user never granted RA consent on this device. It's possible that
+  // user dismissed the dialog triggered by RequestPermission() and the content
+  // setting is set to "allow" by server sync. In this case, we should block.
+  if (!profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted)) {
+    DVLOG(1) << "RA consent never granted on this device.";
+    return CONTENT_SETTING_BLOCK;

[ddorwin, 2015/03/12 03:58:12]

Why do we block in this case rather than return NONE?

Also, we seem to handle this in two locations - here and line 88.

[xhwang, 2015/03/12 17:35:26]

Good point. Changed to ASK.
 
Yes, this is the tricky part which is explained in the comment above. For
RequestPermission, we'll show the UI when kRAConsentGranted is false. For
GetPermissionStatus, we'll return ASK. Since the behavior is different, it's
hard to put this check in one single place.

+  }
+#endif
+
   return PermissionContextBase::GetPermissionStatus(requesting_origin,
                                                     embedding_origin);
 }
@@ -114,7 +153,7 @@ void ProtectedMediaIdentifierPermissionContext::CancelPermissionRequest(
   if (request == pending_requests_.end() || !request->second.second.Equals(id))
     return;
 
-  // Close the |widget_|. OnPlatformVerificationResult() will be fired
+  // Close the |widget_|. OnPlatformVerificationConsentResponse() will be fired
   // during this process, but since |web_contents| is removed from
   // |pending_requests_|, the callback will simply be dropped.
   views::Widget* widget = request->second.first;
@@ -145,36 +184,58 @@ void ProtectedMediaIdentifierPermissionContext::UpdateTabContext(
 // across platforms.
 bool ProtectedMediaIdentifierPermissionContext::
     IsProtectedMediaIdentifierEnabled() const {
-  bool enabled = false;
-
 #if defined(OS_ANDROID)
-  enabled = profile()->GetPrefs()->GetBoolean(
-      prefs::kProtectedMediaIdentifierEnabled);
+  if (!profile()->GetPrefs()->GetBoolean(
+          prefs::kProtectedMediaIdentifierEnabled)) {
+    DVLOG(1) << "Protected media identifier disabled by a user master switch.";
+    return false;
+  }
 #endif
 
 #if defined(OS_CHROMEOS)

[ddorwin, 2015/03/12 03:58:12]

Should this be #elif?

[xhwang, 2015/03/12 17:35:26]

Done.

-  // This could be disabled by the device policy.
+  // Platform verification is not allowed in incognito or guest mode.
+  if (profile()->IsOffTheRecord() || profile()->IsGuestSession()) {
+    DVLOG(1) << "Protected media identifier disabled in incognito or guest "
+                "mode.";
+    return false;
+  }
+
+  // This could be disabled by user's master switch or by the device policy.

[ddorwin, 2015/03/12 03:58:12]

It appears we check in the opposite order. If so, please update.

[xhwang, 2015/03/12 17:35:26]

Done.

   bool enabled_for_device = false;
-  enabled = chromeos::CrosSettings::Get()->GetBoolean(
-                chromeos::kAttestationForContentProtectionEnabled,
-                &enabled_for_device) &&
-            enabled_for_device &&
-            profile()->GetPrefs()->GetBoolean(prefs::kEnableDRM);
+  if (!chromeos::CrosSettings::Get()->GetBoolean(
+          chromeos::kAttestationForContentProtectionEnabled,
+          &enabled_for_device) ||
+      !enabled_for_device ||
+      !profile()->GetPrefs()->GetBoolean(prefs::kEnableDRM)) {
+    DVLOG(1) << "Protected media identifier disabled by the user or by device "
+                "policy.";
+    return false;
+  }
 #endif
 
-  DVLOG_IF(1, !enabled)
-      << "Protected media identifier disabled by the user or by device policy.";
-  return enabled;
+  return true;
 }
 
 #if defined(OS_CHROMEOS)
-void ProtectedMediaIdentifierPermissionContext::OnPlatformVerificationResult(
-    content::WebContents* web_contents,
-    const PermissionRequestID& id,
-    const GURL& requesting_origin,
-    const GURL& embedding_origin,
-    const BrowserPermissionCallback& callback,
-    chromeos::attestation::PlatformVerificationFlow::ConsentResponse response) {
+static void RecordRAConsentGranted(content::WebContents* web_contents) {
+  PrefService* pref_service =
+      user_prefs::UserPrefs::Get(web_contents->GetBrowserContext());
+  if (!pref_service) {
+    LOG(ERROR) << "Failed to get user prefs.";
+    return;
+  }
+  pref_service->SetBoolean(prefs::kRAConsentGranted, true);
+}
+
+void ProtectedMediaIdentifierPermissionContext::
+    OnPlatformVerificationConsentResponse(
+        content::WebContents* web_contents,
+        const PermissionRequestID& id,
+        const GURL& requesting_origin,
+        const GURL& embedding_origin,
+        const BrowserPermissionCallback& callback,
+        chromeos::attestation::PlatformVerificationDialog::ConsentResponse
+            response) {
   // The request may have been canceled. Drop the callback in that case.
   PendingRequestMap::iterator request = pending_requests_.find(web_contents);
   if (request == pending_requests_.end())
@@ -186,15 +247,22 @@ void ProtectedMediaIdentifierPermissionContext::OnPlatformVerificationResult(
   ContentSetting content_setting = CONTENT_SETTING_DEFAULT;
   bool persist = false; // Whether the ContentSetting should be saved.
   switch (response) {
-    case PlatformVerificationFlow::CONSENT_RESPONSE_NONE:
+    case PlatformVerificationDialog::CONSENT_RESPONSE_NONE:
       content_setting = CONTENT_SETTING_DEFAULT;
       persist = false;
       break;
-    case PlatformVerificationFlow::CONSENT_RESPONSE_ALLOW:
+    case PlatformVerificationDialog::CONSENT_RESPONSE_ALLOW:
+      VLOG(1) << "Platform verification accepted by user.";
+      content::RecordAction(
+          base::UserMetricsAction("PlatformVerificationAccepted"));

[ddorwin, 2015/03/12 03:58:12]

Should we record None too? Would be interesting to know how many hit 'X'/"esc".

[xhwang, 2015/03/12 17:35:26]

This and PlatformVerificationRejected are actually in Counts, not in Histograms.
I also feel a Histogram (with a NONE entry) would be more useful. +Darren.

[Darren Krahn, 2015/03/13 22:47:11]

I don't care either way. IIUC, user actions are typically measured with 'action'
metrics -- we could just add a 'cancel' action. A histogram works too, though.

+      RecordRAConsentGranted(web_contents);
       content_setting = CONTENT_SETTING_ALLOW;
       persist = true;
       break;
-    case PlatformVerificationFlow::CONSENT_RESPONSE_DENY:
+    case PlatformVerificationDialog::CONSENT_RESPONSE_DENY:
+      VLOG(1) << "Platform verification denied by user.";
+      content::RecordAction(
+          base::UserMetricsAction("PlatformVerificationRejected"));
       content_setting = CONTENT_SETTING_BLOCK;
       persist = true;
       break;