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..817952909b8b0708036ab6dad46dbc6ad79a390a 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,23 @@ 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 granted if user has given consent for this origin, and if user has
+  // given consent to attestation for content protection on this device.
+  bool consent_granted =
+      content_setting == CONTENT_SETTING_ALLOW &&
+      profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted);

[ddorwin, 2015/03/12 21:38:54]

I don't think we need this anymore because this will always cause ASK. You can
DCHECK it at line 90 if you want I guess.

[xhwang, 2015/03/13 00:54:42]

I am not sure I follow your comment. Can you elaborate?

[ddorwin, 2015/03/13 17:03:10]

I was saying that we don't need the RHS of the && because GetPermissionStatus()
would never return ALLOW without kRAConsentGranted being set. However, I now see
that we're calling the base class's GetPermissionStatus(), which wouldn't have
that check.

Is there a reason we call the base class?

[xhwang, 2015/03/13 21:31:15]

Good point. Done.

 
-  if (content_setting == CONTENT_SETTING_ALLOW ||
-      content_setting == CONTENT_SETTING_BLOCK) {
+  if (consent_granted) {
     NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
-                        false /* persist */, content_setting);
+                        false /* persist */, CONTENT_SETTING_ALLOW);
     return;
   }
 
@@ -80,10 +100,14 @@ 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 show the existing platform 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 +121,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

[ddorwin, 2015/03/12 21:38:53]

"Block" is no longer correct?

[xhwang, 2015/03/13 00:54:42]

Done. Also I restructured this function a bit to make it more clear and more
consistent with RequestPermission().

+  // user dismissed the dialog triggered by RequestPermission() and the content
+  // setting is set to "allow" by server sync. In this case, we should ask.

[ddorwin, 2015/03/12 21:38:53]

It's not clear where all this is called. (I think it could be used anywhere
since it's accessible via at least two interfaces.) I guess this is okay, as
long as the platform verification code, etc. consider ASK to be BLOCK.

[xhwang, 2015/03/13 00:54:42]

Acknowledged.

+  if (!profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted)) {
+    DVLOG(1) << "RA consent never granted on this device.";
+    return CONTENT_SETTING_ASK;
+  }
+#endif
+
   return PermissionContextBase::GetPermissionStatus(requesting_origin,
                                                     embedding_origin);
 }
@@ -114,7 +151,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 +182,56 @@ void ProtectedMediaIdentifierPermissionContext::UpdateTabContext(
 // across platforms.
 bool ProtectedMediaIdentifierPermissionContext::
     IsProtectedMediaIdentifierEnabled() const {
-  bool enabled = false;
-
 #if defined(OS_ANDROID)
-  enabled = profile()->GetPrefs()->GetBoolean(
-      prefs::kProtectedMediaIdentifierEnabled);
-#endif
+  if (!profile()->GetPrefs()->GetBoolean(
+          prefs::kProtectedMediaIdentifierEnabled)) {
+    DVLOG(1) << "Protected media identifier disabled by a user master switch.";
+    return false;
+  }
+#elif defined(OS_CHROMEOS)
+  // 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;
+  }
 
-#if defined(OS_CHROMEOS)
-  // This could be disabled by the device policy.
+  // This could be disabled by the device policy or by user's master switch.
   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 +243,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:

[ddorwin, 2015/03/12 21:38:54]

Let's please bottom out on the histogram decision and file a bug if necessary.

[xhwang, 2015/03/13 00:54:42]

+Darren.

       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"));
+      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;