Don't count host permission changes for v2 apps as a permission increase.

Don't count host permission changes for v2 apps as a permission increase.

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

[Jeffrey Yasskin, 2013-07-09 00:32:57]

lgtm assuming apps warn about the same API permissions as extensions.

https://codereview.chromium.org/18399007/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/permission_set.cc (right):

https://codereview.chromium.org/18399007/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/permission_set.cc:460: if
(HasLessAPIPrivilegesThan(permissions))
Mike, are there other API privileges that we display for Extensions but not
Apps?

[Yoyo Zhou, 2013-07-09 00:34:14]

I'm going by the exception for platform apps in
PermissionSet::GetPermissionMessages.

[Jeffrey Yasskin, 2013-07-09 00:37:24]

Can you see any way to unify the list of non-warned permissions between these
two use sites? If not, please comment that both sites need to stay in sync with
each other.

[Yoyo Zhou, 2013-07-09 01:53:52]

I did some refactoring to make this flow better, but unfortunately there wasn't
an easy way to unify the decision. I added comments as you suggested.

[miket_OOO, 2013-07-09 17:12:08]

https://codereview.chromium.org/18399007/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/permission_set.cc (right):

https://codereview.chromium.org/18399007/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/permission_set.cc:460: if
(HasLessAPIPrivilegesThan(permissions))
On 2013/07/09 00:32:57, Jeffrey Yasskin wrote:
> Mike, are there other API privileges that we display for Extensions but not
> Apps?

None come to mind. I ran through chrome_api_permissions.cc, permission_set.cc,
permission_set_unittest.cc, and _permission_features.json.

https://codereview.chromium.org/18399007/diff/5002/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.h (right):

https://codereview.chromium.org/18399007/diff/5002/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.h:181: // Returns true if
|permissions| has more host permissions compared to this
Just a drive-by readability observation here. This method is much smarter than
this comment suggests. It's not a simple set comparison.

The comment is probably OK as-is if you think that every person using this
method will understand it's a high-level, semantic comparison. But if someone
tries something crazy like adding it into an equality test, it might lead to a
bad place.

[Yoyo Zhou, 2013-07-09 17:16:59]

https://codereview.chromium.org/18399007/diff/5002/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.h (right):

https://codereview.chromium.org/18399007/diff/5002/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.h:181: // Returns true if
|permissions| has more host permissions compared to this
On 2013/07/09 17:12:09, miket wrote:
> Just a drive-by readability observation here. This method is much smarter than
> this comment suggests. It's not a simple set comparison.
> 
> The comment is probably OK as-is if you think that every person using this
> method will understand it's a high-level, semantic comparison. But if someone
> tries something crazy like adding it into an equality test, it might lead to a
> bad place.

Yeah, it can lead to surprising results, but I don't feel so bad since it's a
private method.

[miket_OOO, 2013-07-09 17:25:49]

LGTM either way. Thanks!

[commit-bot: I haz the power, 2013-07-09 17:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/18399007/5002

[commit-bot: I haz the power, 2013-07-09 17:33:48]

Failed to apply patch for
chrome/common/extensions/permissions/permission_set.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/permissions/permission_set.cc
  Hunk #2 FAILED at 265.
  Hunk #3 succeeded at 437 (offset -9 lines).
  Hunk #4 succeeded at 447 (offset -9 lines).
  Hunk #5 succeeded at 540 (offset -9 lines).
  Hunk #6 succeeded at 554 (offset -9 lines).
  Hunk #7 succeeded at 608 (offset -9 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
chrome/common/extensions/permissions/permission_set.cc.rej

Patch:       chrome/common/extensions/permissions/permission_set.cc
Index: chrome/common/extensions/permissions/permission_set.cc
diff --git a/chrome/common/extensions/permissions/permission_set.cc
b/chrome/common/extensions/permissions/permission_set.cc
index
a2980c503d02f7c276b3c19fbd30f77a9d02003a..b711c7f17986ef755fe70183166cb50d4597b03f
100644
--- a/chrome/common/extensions/permissions/permission_set.cc
+++ b/chrome/common/extensions/permissions/permission_set.cc
@@ -94,6 +94,8 @@ std::string GetPermissionName(const std::string&
function_name) {
     return function_name;
 }
 
+
+
 }  // namespace
 
 namespace extensions {
@@ -263,36 +265,11 @@ PermissionMessages PermissionSet::GetPermissionMessages(
     return messages;
   }
 
-  // Since platform apps always use isolated storage, they can't (silently)
-  // access user data on other domains, so there's no need to prompt.
-  if (extension_type != Manifest::TYPE_PLATFORM_APP) {
-    if (HasEffectiveAccessToAllHosts()) {
-      messages.push_back(PermissionMessage(
-          PermissionMessage::kHostsAll,
-          l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_ALL_HOSTS)));
-    } else {
-      for (URLPatternSet::const_iterator i = effective_hosts_.begin();
-           i != effective_hosts_.end(); ++i) {
-        if (i->scheme() != chrome::kChromeUIScheme)
-          continue;
-        // chrome://favicon is the only URL for chrome:// scheme that we
-        // want to support. We want to deprecate the "chrome" scheme.
-        // We should not add any additional "host" here.
-        if (GURL(chrome::kChromeUIFaviconURL).host() != i->host())
-          continue;
-        messages.push_back(PermissionMessage(
-            PermissionMessage::kFavicon,
-            l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_FAVICON)));
-      }
-      std::set<std::string> hosts = GetDistinctHostsForDisplay();
-      if (!hosts.empty())
-        messages.push_back(PermissionMessage::CreateFromHostList(hosts));
-    }
-  }
-
-  std::set<PermissionMessage> simple_msgs =
-      GetSimplePermissionMessages();
-  messages.insert(messages.end(), simple_msgs.begin(), simple_msgs.end());
+  std::set<PermissionMessage> host_msgs =
+      GetHostPermissionMessages(extension_type);
+  std::set<PermissionMessage> api_msgs = GetAPIPermissionMessages();
+  messages.insert(messages.end(), host_msgs.begin(), host_msgs.end());
+  messages.insert(messages.end(), api_msgs.begin(), api_msgs.end());
 
   return messages;
 }
@@ -444,7 +421,8 @@ bool PermissionSet::HasEffectiveFullAccess() const {
 }
 
 bool PermissionSet::HasLessPrivilegesThan(
-    const PermissionSet* permissions) const {
+    const PermissionSet* permissions,
+    Manifest::Type extension_type) const {
   // Things can't get worse than native code access.
   if (HasEffectiveFullAccess())
     return false;
@@ -453,7 +431,7 @@ bool PermissionSet::HasLessPrivilegesThan(
   if (permissions->HasEffectiveFullAccess())
     return true;
 
-  if (HasLessHostPrivilegesThan(permissions))
+  if (HasLessHostPrivilegesThan(permissions, extension_type))
     return true;
 
   if (HasLessAPIPrivilegesThan(permissions))
@@ -546,8 +524,7 @@ void PermissionSet::InitEffectiveHosts() {
       explicit_hosts(), scriptable_hosts(), &effective_hosts_);
 }
 
-std::set<PermissionMessage>
-    PermissionSet::GetSimplePermissionMessages() const {
+std::set<PermissionMessage> PermissionSet::GetAPIPermissionMessages() const {
   std::set<PermissionMessage> messages;
   for (APIPermissionSet::const_iterator permission_it = apis_.begin();
        permission_it != apis_.end(); ++permission_it) {
@@ -561,15 +538,50 @@ std::set<PermissionMessage>
   return messages;
 }
 
+std::set<PermissionMessage> PermissionSet::GetHostPermissionMessages(
+    Manifest::Type extension_type) const {
+  // Since platform apps always use isolated storage, they can't (silently)
+  // access user data on other domains, so there's no need to prompt.
+  // Note: this must remain consistent with HasLessHostPrivilegesThan.
+  // See crbug.com/255229.
+  std::set<PermissionMessage> messages;
+  if (extension_type == Manifest::TYPE_PLATFORM_APP)
+    return messages;
+
+  if (HasEffectiveAccessToAllHosts()) {
+    messages.insert(PermissionMessage(
+        PermissionMessage::kHostsAll,
+        l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_ALL_HOSTS)));
+  } else {
+    for (URLPatternSet::const_iterator i = effective_hosts_.begin();
+         i != effective_hosts_.end(); ++i) {
+      if (i->scheme() != chrome::kChromeUIScheme)
+        continue;
+      // chrome://favicon is the only URL for chrome:// scheme that we
+      // want to support. We want to deprecate the "chrome" scheme.
+      // We should not add any additional "host" here.
+      if (GURL(chrome::kChromeUIFaviconURL).host() != i->host())
+        continue;
+      messages.insert(PermissionMessage(
+          PermissionMessage::kFavicon,
+          l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_FAVICON)));
+    }
+    std::set<std::string> hosts = GetDistinctHostsForDisplay();
+    if (!hosts.empty())
+      messages.insert(PermissionMessage::CreateFromHostList(hosts));
+  }
+  return messages;
+}
+
 bool PermissionSet::HasLessAPIPrivilegesThan(
     const PermissionSet* permissions) const {
   if (permissions == NULL)
     return false;
 
   std::set<PermissionMessage> current_warnings =
-      GetSimplePermissionMessages();
+      GetAPIPermissionMessages();
   std::set<PermissionMessage> new_warnings =
-      permissions->GetSimplePermissionMessages();
+      permissions->GetAPIPermissionMessages();
   std::set<PermissionMessage> delta_warnings;
   std::set_difference(new_warnings.begin(), new_warnings.end(),
                       current_warnings.begin(), current_warnings.end(),
@@ -580,7 +592,13 @@ bool PermissionSet::HasLessAPIPrivilegesThan(
 }
 
 bool PermissionSet::HasLessHostPrivilegesThan(
-    const PermissionSet* permissions) const {
+    const PermissionSet* permissions,
+    Manifest::Type extension_type) const {
+  // Platform apps host permission changes do not count as privilege increases.
+  // Note: this must remain consistent with GetHostPermissionMessages.
+  if (extension_type == Manifest::TYPE_PLATFORM_APP)
+    return false;
+
   // If this permission set can access any host, then it can't be elevated.
   if (HasEffectiveAccessToAllHosts())
     return false;

[commit-bot: I haz the power, 2013-07-09 18:59:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/18399007/14001

[commit-bot: I haz the power, 2013-07-10 01:42:45]

Change committed as 210704