Enable chrome.mdns for Chrome Apps

chrome/browser/extensions/api/mdns/mdns_api.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/extensions/api/mdns/mdns_api.h"
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/profiler/scoped_profile.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/common/extensions/api/mdns.h"
+#include "extensions/browser/extension_registry.h"
 
 namespace extensions {
 
 namespace mdns = api::mdns;
 
 namespace {
 
 // Whitelisted mDNS service types.
 const char kCastServiceType[] = "_googlecast._tcp.local";
 const char kPrivetServiceType[] = "_privet._tcp.local";
 const char kTestServiceType[] = "_testing._tcp.local";
 
-bool IsServiceTypeWhitelisted(const std::string& service_type) {
+bool IsServiceTypeWhitelisted(const std::string& service_type,
+                              bool is_extension) {
+  // The mdns API is available to apps without restrictions.  A whitelisted set
+  // of extensions is allowed to use the mdns API, but only to connect to a
+  // whitelisted set of service types.

[not at google - send to devlin, 2014/12/10 18:23:04]

Note that a more correct check here would be is_platform_app, not !is_extension,
because there are things that aren't platform apps (like hosted apps, and V1
apps) that shouldn't be whitelisted (at least, not as part of this change - we
can think about that later).

[Red Daly, 2015/01/26 21:55:33]

Done, but in UpdateMDnsListeners.

+  if (!is_extension) {

[not at google - send to devlin, 2014/12/10 18:23:05]

I actually think that doing all of this work inside UpdateMdnsListeners, then
just passing a bool here and checking it once, looks weird. If you:

- made IsServiceTypeWhitelisted a member rather than static function, it could
grab the ExtensionRegistry itself, find the extension, etc - which would require
fewer changes at the call site and make more sense to me.

- just do what Mark suggests and don't change this function at all, and put
*all* logic inside UpdateMdnsListeners.

[Red Daly, 2015/01/26 21:55:33]

I took the second approach (kept function static and with original signature).

+    return true;
+  }
   return service_type == kCastServiceType ||
          service_type == kPrivetServiceType ||
          service_type == kTestServiceType;
 }
 
 }  // namespace
 
 MDnsAPI::MDnsAPI(content::BrowserContext* context) : browser_context_(context) {
   DCHECK(browser_context_);
   EventRouter::Get(context)
@@ skipping 66 matching lines @@
     new_service_types.insert(filter_value);
   }
 
   // Find all the added and removed service types since last update.
   std::set<std::string> added_service_types =
       base::STLSetDifference<std::set<std::string> >(
           new_service_types, service_types_);
   std::set<std::string> removed_service_types =
       base::STLSetDifference<std::set<std::string> >(
           service_types_, new_service_types);
+  service_types_ = new_service_types;

[mark a. foltz, 2014/12/09 23:56:34]

If we update |service_types_| here for a disabled extension, a later call may
not get all of its service types registered.  I think this should go at the end
of the method.

[not at google - send to devlin, 2014/12/10 18:23:05]

This shouldn't happen because disabled extensions can't have listened for new
service types, they shouldn't be executing code. However, disabling them
presumably would clear *all* service types.

 
   // Update the registry.
   DnsSdRegistry* registry = dns_sd_registry();

[mark a. foltz, 2014/12/09 23:56:34]

This doesn't need to be done until after |extension| is known to be not null.

[Red Daly, 2015/01/26 21:55:33]

The control flow changed a little.  At this point, the registry is needed even
when extension is null because that is the case for a disabled extension
(registry->UnregisterDnsSdListener should be called for every listener of a
disabled extension).

+
+  const Extension* extension = ExtensionRegistry::Get(browser_context_)->
+      GetExtensionById(details.extension_id, ExtensionRegistry::ENABLED);

[mark a. foltz, 2014/12/09 23:56:34]

Are only ENABLED extensions allowed to register event listeners?  Or should we
allow DISABLED extensions/apps to follow this code path?

[not at google - send to devlin, 2014/12/10 18:23:04]

Yes I think that EVERYTHING was correct, whoops, because this is called for
listener removal as well (I didn't notice that before), which could be called
for extensions that were just disabled (or uninstalled, or terminated). Etc.
Assuming they actually are in that state when OnListenerRemoved is called - I'm
not sure. Can't hurt to check, not that expensive.

With that in mind (either the extension might be there, or it might not be),
returning early looks wrong to me, if the behaviour is that uninstalling an
extension does actually make it disappear from all extension sets:
1. App is installed with service type kSomeServiceType.
2. App is uninstalled.
3. |removed_service_types| will now contain kSomeServiceType.
4. The code never actually gets to the reregistration stage because this is an
early-return.
5. |registry| is still watching for kSomeServiceType.

Maybe that can't happen. However, returning early here.... just doesn't feel
right.

I see that Mark is making similar but different comments.

[Red Daly, 2015/01/26 21:55:33]

I checked and this is called when an extension is disabled that was listening
for mDNS services.  I changed the flow to correct the erroneous behavior
described above.

+  if (!extension) {
+    return;
+  }
+

[mark a. foltz, 2014/12/09 23:56:34]

Slight preference for declaring a bool here and leaving IsServiceTypeWhitelisted
alone:

bool all_service_types_allowed = !extension.is_extension()

if (all_service_types_allowed || IsServiceTypeWhitelisted(*it)) {
  ...
}

[Red Daly, 2015/01/26 21:55:33]

Obsolete (moved IsServiceTypeWhitelisted call to loop above).

   for (std::set<std::string>::iterator it = added_service_types.begin();
        it != added_service_types.end(); ++it) {
-    if (IsServiceTypeWhitelisted(*it))
+    if (IsServiceTypeWhitelisted(*it, extension->is_extension()))
       registry->RegisterDnsSdListener(*it);
   }
   for (std::set<std::string>::iterator it = removed_service_types.begin();
        it != removed_service_types.end(); ++it) {
-    if (IsServiceTypeWhitelisted(*it))
+    if (IsServiceTypeWhitelisted(*it, extension->is_extension()))
       registry->UnregisterDnsSdListener(*it);
   }
-
-  service_types_ = new_service_types;
 }
 
 void MDnsAPI::OnDnsSdEvent(const std::string& service_type,
                            const DnsSdRegistry::DnsSdServiceList& services) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   std::vector<linked_ptr<mdns::MDnsService> > args;
   for (DnsSdRegistry::DnsSdServiceList::const_iterator it = services.begin();
        it != services.end(); ++it) {
     linked_ptr<mdns::MDnsService> mdns_service =
@@ skipping 12 matching lines @@
   event->filter_info.SetServiceType(service_type);
 
   VLOG(1) << "Broadcasting OnServiceList event: " << event.get();
 
   // TODO(justinlin): To avoid having listeners without filters getting all
   // events, modify API to have this event require filters.
   extensions::EventRouter::Get(browser_context_)->BroadcastEvent(event.Pass());
 }
 
 }  // namespace extensions