Add checks for Feature Policy to the mojo Permission Service

content/browser/permissions/permission_service_impl.cc

Index: content/browser/permissions/permission_service_impl.cc
diff --git a/content/browser/permissions/permission_service_impl.cc b/content/browser/permissions/permission_service_impl.cc
index 50f13c585def91fab0832f7983efcc2d75e9ccff..ec638d915c5410da89f7e61f0fb2ec693d91315c 100644
--- a/content/browser/permissions/permission_service_impl.cc
+++ b/content/browser/permissions/permission_service_impl.cc
@@ -10,10 +10,14 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/permission_manager.h"
 #include "content/public/browser/permission_type.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/common/content_features.h"
+#include "third_party/WebKit/public/platform/WebFeaturePolicy.h"
 
 using blink::mojom::PermissionDescriptorPtr;
 using blink::mojom::PermissionName;
@@ -56,8 +60,56 @@ PermissionType PermissionDescriptorToPermissionType(
   return PermissionType::NUM;
 }
 
-// This function allows the usage of the the multiple request map
-// with single requests.
+blink::WebFeaturePolicyFeature PermissionTypeToFeaturePolicyFeature(
+    PermissionType type) {
+  switch (type) {
+    case PermissionType::MIDI:
+    case PermissionType::MIDI_SYSEX:
+      return blink::WebFeaturePolicyFeature::kMidiFeature;
+    case PermissionType::GEOLOCATION:
+      return blink::WebFeaturePolicyFeature::kGeolocation;
+    case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
+      return blink::WebFeaturePolicyFeature::kEme;
+    case PermissionType::AUDIO_CAPTURE:
+      return blink::WebFeaturePolicyFeature::kMicrophone;
+    case PermissionType::VIDEO_CAPTURE:
+      return blink::WebFeaturePolicyFeature::kCamera;
+    case PermissionType::PUSH_MESSAGING:
+    case PermissionType::NOTIFICATIONS:
+    case PermissionType::DURABLE_STORAGE:
+    case PermissionType::BACKGROUND_SYNC:
+    case PermissionType::FLASH:
+    case PermissionType::NUM:
+      // These aren't exposed by feature policy.
+      return blink::WebFeaturePolicyFeature::kNotFound;
+  }
+
+  NOTREACHED();
+  return blink::WebFeaturePolicyFeature::kNotFound;
+}
+
+bool AllowedByFeaturePolicy(RenderFrameHost* rfh, PermissionType type) {
+  if (!base::FeatureList::IsEnabled(
+          features::kUseFeaturePolicyForPermissions)) {
+    // Default to ignoring the feature policy.
+    return true;
+  }
+
+  blink::WebFeaturePolicyFeature feature_policy_feature =
+      PermissionTypeToFeaturePolicyFeature(type);
+  if (feature_policy_feature == blink::WebFeaturePolicyFeature::kNotFound)
+    return true;
+
+  // If there is no frame, there is no policy, so disable the feature for
+  // safety.
+  if (!rfh)
+    return false;
+
+  return rfh->IsFeatureEnabled(feature_policy_feature);
+}
+
+// This function allows the usage of the the multiple request map with single
+// requests.
 void PermissionRequestResponseCallbackWrapper(
     const base::Callback<void(PermissionStatus)>& callback,
     const std::vector<PermissionStatus>& vector) {
@@ -67,20 +119,56 @@ void PermissionRequestResponseCallbackWrapper(
 
 } // anonymous namespace
 
-PermissionServiceImpl::PendingRequest::PendingRequest(
-    const RequestPermissionsCallback& callback,
-    int request_count)
-    : callback(callback),
-      request_count(request_count) {
-}
+class PermissionServiceImpl::PendingRequest {
+ public:
+  PendingRequest(std::vector<PermissionType> types,
+                 const RequestPermissionsCallback& callback)
+      : types_(types),
+        callback_(callback),
+        has_result_been_set_(types.size(), false),
+        results_(types.size(), PermissionStatus::DENIED) {}
+
+  ~PendingRequest() {
+    if (callback_.is_null())
+      return;
 
-PermissionServiceImpl::PendingRequest::~PendingRequest() {
-  if (callback.is_null())
-    return;
+    std::vector<PermissionStatus> result(types_.size(),
+                                         PermissionStatus::DENIED);
+    callback_.Run(result);
+  }
 
-  std::vector<PermissionStatus> result(request_count, PermissionStatus::DENIED);
-  callback.Run(result);
-}
+  int id() { return id_; }

[mlamouri (slow - plz ping), 2017/05/30 09:38:22]

nit: `const`

[raymes, 2017/05/31 05:32:00]

Done.

+  void set_id(int id) { id_ = id; }
+
+  size_t RequestSize() { return types_.size(); }

[mlamouri (slow - plz ping), 2017/05/30 09:38:22]

ditto

[raymes, 2017/05/31 05:32:00]

Done.

+
+  void SetResult(int index, PermissionStatus result) {
+    DCHECK_EQ(false, has_result_been_set_[index]);
+    has_result_been_set_[index] = true;
+    results_[index] = result;
+  }
+
+  bool HasResultBeenSet(size_t index) const {
+    return has_result_been_set_[index];
+  }
+
+  void RunCallback() {
+    // Check that all results have been set.
+    DCHECK(std::find(has_result_been_set_.begin(), has_result_been_set_.end(),
+                     false) == has_result_been_set_.end());
+    callback_.Run(results_);
+    callback_.Reset();
+  }
+
+ private:
+  // Request ID received from the PermissionManager.
+  int id_;
+  std::vector<PermissionType> types_;
+  RequestPermissionsCallback callback_;
+
+  std::vector<bool> has_result_been_set_;
+  std::vector<PermissionStatus> results_;

[mlamouri (slow - plz ping), 2017/05/30 09:38:22]

Did you consider:
`std::vector<base::Optional<PermissionStatus>>`. I think it would be cleaner
because you wouldn't need to keep track of whether things have been set.

[raymes, 2017/05/31 05:32:00]

Good idea. I tried this but it meant having to transform the array at the end
into a non-optional version to run the callback with. That didn't seem ideal, so
I just left it as-is.

+};
 
 PermissionServiceImpl::PermissionServiceImpl(PermissionServiceContext* context)
     : context_(context), weak_factory_(this) {}
@@ -96,7 +184,7 @@ PermissionServiceImpl::~PermissionServiceImpl() {
   // Cancel pending requests.
   for (RequestsMap::Iterator<PendingRequest> it(&pending_requests_);
        !it.IsAtEnd(); it.Advance()) {
-    permission_manager->CancelPermissionRequest(it.GetCurrentValue()->id);
+    permission_manager->CancelPermissionRequest(it.GetCurrentValue()->id());
   }
   pending_requests_.Clear();
 }
@@ -173,30 +261,50 @@ void PermissionServiceImpl::RequestPermissions(
   for (size_t i = 0; i < types.size(); ++i)
     types[i] = PermissionDescriptorToPermissionType(permissions[i]);
 
-  int pending_request_id = pending_requests_.Add(
-      base::MakeUnique<PendingRequest>(callback, permissions.size()));
+  std::unique_ptr<PendingRequest> pending_request =
+      base::MakeUnique<PendingRequest>(types, callback);
+  std::vector<PermissionType> request_types;
+  for (size_t i = 0; i < types.size(); ++i) {
+    // Check feature policy.
+    if (!AllowedByFeaturePolicy(context_->render_frame_host(), types[i]))
+      pending_request->SetResult(i, PermissionStatus::DENIED);
+    else
+      request_types.push_back(types[i]);
+  }
+
+  int pending_request_id = pending_requests_.Add(std::move(pending_request));
   int id = browser_context->GetPermissionManager()->RequestPermissions(
-      types, context_->render_frame_host(), origin.GetURL(), user_gesture,
+      request_types, context_->render_frame_host(), origin.GetURL(),
+      user_gesture,
       base::Bind(&PermissionServiceImpl::OnRequestPermissionsResponse,
                  weak_factory_.GetWeakPtr(), pending_request_id));
 
   // Check if the request still exists. It may have been removed by the
   // the response callback.
-  PendingRequest* pending_request = pending_requests_.Lookup(
-      pending_request_id);
-  if (!pending_request)
-      return;
-  pending_request->id = id;
+  PendingRequest* in_progress_request =
+      pending_requests_.Lookup(pending_request_id);
+  if (!in_progress_request)
+    return;
+  in_progress_request->set_id(id);
 }
 
 void PermissionServiceImpl::OnRequestPermissionsResponse(
     int pending_request_id,
-    const std::vector<PermissionStatus>& result) {
+    const std::vector<PermissionStatus>& partial_result) {
   PendingRequest* request = pending_requests_.Lookup(pending_request_id);
-  RequestPermissionsCallback callback(request->callback);
-  request->callback.Reset();
+  auto partial_result_it = partial_result.begin();
+  for (size_t i = 0; i < request->RequestSize(); ++i) {

[mlamouri (slow - plz ping), 2017/05/30 09:38:22]

Can you add a comment on top of this loop explaining what you are doing. Maybe
move the comment inside it too. It took me longer than I wished to understand
how you were able to avoid re-writing the same indexes. It can be easy to miss
that you are looping on the `request`.

[raymes, 2017/05/31 05:32:00]

Thanks - I tried to make the comment clearer.

+    // We fill in the unset results with those from the call to
+    // RequestPermissions.
+    if (!request->HasResultBeenSet(i)) {
+      request->SetResult(i, *partial_result_it);
+      ++partial_result_it;
+    }
+  }
+  DCHECK(partial_result.end() == partial_result_it);

[mlamouri (slow - plz ping), 2017/05/30 09:38:22]

Would DCHECK_EQ work here?

[raymes, 2017/05/31 05:32:00]

Unfortunately it fails to compile I think because the iterators don't translate
to strings:
error: no matching function for call to 'MakeCheckOpValueString'

+
+  request->RunCallback();
   pending_requests_.Remove(pending_request_id);
-  callback.Run(result);
 }
 
 void PermissionServiceImpl::HasPermission(
@@ -254,8 +362,10 @@ PermissionStatus PermissionServiceImpl::GetPermissionStatusFromType(
     const url::Origin& origin) {
   BrowserContext* browser_context = context_->GetBrowserContext();
   DCHECK(browser_context);
-  if (!browser_context->GetPermissionManager())
+  if (!browser_context->GetPermissionManager() ||
+      !AllowedByFeaturePolicy(context_->render_frame_host(), type)) {
     return PermissionStatus::DENIED;
+  }
 
   GURL requesting_origin(origin.Serialize());
   // If the embedding_origin is empty we'll use |origin| instead.