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

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

The prefs:kEnableDRM switch will never be disabled by the new logic.
Only the per-domain setting will be affected by user responses.  Also
changed the setting key to be a web origin instead of just a host.

BUG=chromium:309939
TEST=unit, manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232398

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

https://codereview.chromium.org/31043008/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/31043008/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:303: url =
web_contents->GetLastCommittedURL();
nit: you don't need braces or 1-line if-else. or, use a conditional operator?

https://codereview.chromium.org/31043008/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:370:
pref_service->GetDictionary(prefs::kRAConsentDomains);
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 :)

[pastarmovj, 2013-10-24 09:56:35]

LGTM with a nit.

https://codereview.chromium.org/31043008/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/31043008/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:171:
delegate_->ShowConsentPrompt(web_contents,
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:51]

This CL has changed significantly - PTAL

+jochen for *content_settings*

https://chromiumcodereview.appspot.com/31043008/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://chromiumcodereview.appspot.com/31043008/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:171:
delegate_->ShowConsentPrompt(web_contents,
On 2013/10/24 09:56:36, pastarmovj wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/31043008/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:303: url =
web_contents->GetLastCommittedURL();
On 2013/10/23 20:26:12, Jun Mukai wrote:
> nit: you don't need braces or 1-line if-else. or, use a conditional operator?

Done.

https://chromiumcodereview.appspot.com/31043008/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:370:
pref_service->GetDictionary(prefs::kRAConsentDomains);
On 2013/10/23 20:26:12, Jun Mukai wrote:
> 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 :)

I've reused the content setting used in clank.

[Jun Mukai, 2013-10-29 00:26:50]

lgtm with nits.

https://chromiumcodereview.appspot.com/31043008/diff/60001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://chromiumcodereview.appspot.com/31043008/diff/60001/chrome/browser/chr...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:304: return
web_contents->GetVisibleURL();
Why is this condition necessary?

https://chromiumcodereview.appspot.com/31043008/diff/60001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/platform_verification_flow.h (right):

https://chromiumcodereview.appspot.com/31043008/diff/60001/chrome/browser/chr...
chrome/browser/chromeos/attestation/platform_verification_flow.h:139: }
Could this be private?

[pastarmovj, 2013-10-29 10:37:59]

still lgtm

[jochen (gone - plz use gerrit), 2013-10-29 14:15:32]

content settings lgtm

[Darren Krahn, 2013-10-29 17:37:29]

+dalecurtis - see question about committed URLs

https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:304: return
web_contents->GetVisibleURL();
On 2013/10/29 00:26:51, Jun Mukai wrote:
> Why is this condition necessary?

When I was manually testing I came across the condition where web_contents has a
visible URL but not a committed URL -- I guess when it's still loading.  I'm not
sure if we need to handle this case in practice.

+dalecurtis, who knows more about how this is called

https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/platform_verification_flow.h (right):

https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/platform_verification_flow.h:139: }
On 2013/10/29 00:26:51, Jun Mukai wrote:
> Could this be private?

Actually, given I needed to 'friend' the test fixture all these methods can be
private.  Done.

[Darren Krahn, 2013-10-30 00:14:40]

On 2013/10/29 17:37:29, Darren Krahn wrote:
> +dalecurtis - see question about committed URLs
> 
>
https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
> File chrome/browser/chromeos/attestation/platform_verification_flow.cc
(right):
> 
>
https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
> chrome/browser/chromeos/attestation/platform_verification_flow.cc:304: return
> web_contents->GetVisibleURL();
> On 2013/10/29 00:26:51, Jun Mukai wrote:
> > Why is this condition necessary?
> 
> When I was manually testing I came across the condition where web_contents has
a
> visible URL but not a committed URL -- I guess when it's still loading.  I'm
not
> sure if we need to handle this case in practice.
> 
> +dalecurtis, who knows more about how this is called
> 
>
https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
> File chrome/browser/chromeos/attestation/platform_verification_flow.h (right):
> 
>
https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
> chrome/browser/chromeos/attestation/platform_verification_flow.h:139: }
> On 2013/10/29 00:26:51, Jun Mukai wrote:
> > Could this be private?
> 
> Actually, given I needed to 'friend' the test fixture all these methods can be
> private.  Done.

Per offline discussion with mukai@, we can discuss whether we need to fallback
to visible URLs separately.

[commit-bot: I haz the power, 2013-10-30 00:23:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/150001

[commit-bot: I haz the power, 2013-10-30 04:12:07]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-30 17:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/150001

[DaleCurtis, 2013-10-30 18:13:44]

https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/31043008/diff/60001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:304: return
web_contents->GetVisibleURL();
On 2013/10/29 17:37:30, Darren Krahn wrote:
> On 2013/10/29 00:26:51, Jun Mukai wrote:
> > Why is this condition necessary?
> 
> When I was manually testing I came across the condition where web_contents has
a
> visible URL but not a committed URL -- I guess when it's still loading.  I'm
not
> sure if we need to handle this case in practice.
> 
> +dalecurtis, who knows more about how this is called

No idea about this.  It seems unlikely to be true by the time the CDM gets
around to invoking any of these calls.  I'm not familiar with this aspect of
WebContents though.

[commit-bot: I haz the power, 2013-10-30 21:06:03]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-10-30 21:16:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/150001

[commit-bot: I haz the power, 2013-10-30 21:16:40]

Failed to apply patch for
chrome/browser/chromeos/attestation/platform_verification_flow.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/attestation/platform_verification_flow.cc
  Hunk #1 FAILED at 13.
  1 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/attestation/platform_verification_flow.cc.rej

Patch:       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..a0b23db6847965275faadc236652aa3bff21d884
100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow.cc
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
@@ -13,8 +13,9 @@
 #include "chrome/browser/chromeos/login/user.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/content_settings_pattern.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/attestation/attestation_flow.h"
 #include "chromeos/cryptohome/async_method_caller.h"
@@ -26,9 +27,12 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/url_constants.h"
 
 namespace {
 
+const char kDefaultHttpsPort[] = "443";
+
 // A callback method to handle DBus errors.
 void DBusCallback(const base::Callback<void(bool)>& on_success,
                   const base::Closure& on_failure,
@@ -61,7 +65,6 @@ class DefaultDelegate : public
PlatformVerificationFlow::Delegate {
   virtual ~DefaultDelegate() {}
 
   virtual void ShowConsentPrompt(
-      PlatformVerificationFlow::ConsentType type,
       content::WebContents* web_contents,
       const PlatformVerificationFlow::Delegate::ConsentCallback& callback)
       OVERRIDE {
@@ -116,6 +119,11 @@ void PlatformVerificationFlow::ChallengePlatformKey(
     const std::string& challenge,
     const ChallengeCallback& callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (!GetURL(web_contents).is_valid()) {
+    LOG(WARNING) << "PlatformVerificationFlow: Invalid URL.";
+    ReportError(callback, INTERNAL_ERROR);
+    return;
+  }
   if (!IsAttestationEnabled(web_contents)) {
     LOG(INFO) << "PlatformVerificationFlow: Feature disabled.";
     ReportError(callback, POLICY_REJECTED);
@@ -138,12 +146,23 @@ 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(GetContentSettings(web_contents),
+                     GetURL(web_contents),
+                     NULL));
+
   Delegate::ConsentCallback consent_callback = base::Bind(
       &PlatformVerificationFlow::OnConsentResponse,
       weak_factory_.GetWeakPtr(),
@@ -151,14 +170,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)
+    delegate_->ShowConsentPrompt(web_contents, consent_callback);
+  else
     consent_callback.Run(CONSENT_RESPONSE_NONE);
-  } else {
-    delegate_->ShowConsentPrompt(consent_type,
-                                 web_contents,
-                                 consent_callback);
-  }
 }
 
 void PlatformVerificationFlow::RegisterProfilePrefs(
@@ -166,12 +182,6 @@ void PlatformVerificationFlow::RegisterProfilePrefs(
   prefs->RegisterBooleanPref(prefs::kRAConsentFirstTime,
                              false,
                             
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-  prefs->RegisterDictionaryPref(
-      prefs::kRAConsentDomains,
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kRAConsentAlways,
-                             false,
-                            
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 void PlatformVerificationFlow::OnConsentResponse(
@@ -179,16 +189,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;
     }
@@ -289,7 +299,10 @@ const GURL& PlatformVerificationFlow::GetURL(
     content::WebContents* web_contents) {
   if (!testing_url_.is_empty())
     return testing_url_;
-  return web_contents->GetLastCommittedURL();
+  const GURL& url = web_contents->GetLastCommittedURL();
+  if (!url.is_valid())
+    return web_contents->GetVisibleURL();
+  return url;
 }
 
 User* PlatformVerificationFlow::GetUser(content::WebContents* web_contents) {
@@ -299,6 +312,14 @@ User*
PlatformVerificationFlow::GetUser(content::WebContents* web_contents) {
       Profile::FromBrowserContext(web_contents->GetBrowserContext()));
 }
 
+HostContentSettingsMap* PlatformVerificationFlow::GetContentSettings(
+    content::WebContents* web_contents) {
+  if (testing_content_settings_)
+    return testing_content_settings_;
+  return Profile::FromBrowserContext(web_contents->GetBrowserContext())->
+      GetHostContentSettingsMap();
+}
+
 bool PlatformVerificationFlow::IsAttestationEnabled(
     content::WebContents* web_contents) {
   // Check the device policy for the feature.
@@ -322,92 +343,77 @@ 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(GetContentSettings(web_contents),
+                             GetURL(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(GetContentSettings(web_contents),
+                      GetURL(web_contents),
+                      (consent_response =…
(message too large)

[commit-bot: I haz the power, 2013-10-30 23:16:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/500001

[commit-bot: I haz the power, 2013-10-31 01:01:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-31 04:23:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/770001

[commit-bot: I haz the power, 2013-10-31 07:54:08]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-31 17:14:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/770001

[commit-bot: I haz the power, 2013-10-31 20:43:26]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-01 05:09:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@chromium.org/31043008/430002

[commit-bot: I haz the power, 2013-11-01 15:21:45]

Change committed as 232398