Add protocol buffer for reporting permission actions to Safe Browsing servers

chrome/common/safe_browsing/permission_extension.proto

Index: chrome/common/safe_browsing/permission_extension.proto
diff --git a/chrome/common/safe_browsing/permission_extension.proto b/chrome/common/safe_browsing/permission_extension.proto
new file mode 100644
index 0000000000000000000000000000000000000000..9ed298b8dbbb17e3c37e343f72d3ff089e1339fd
--- /dev/null
+++ b/chrome/common/safe_browsing/permission_extension.proto
@@ -0,0 +1,89 @@
+// Copyright 2016 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.
+
+// Protocol buffer for permission reports sent to the safe browsing client-side
+// detection (CSD) frontends.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+
+package safe_browsing;
+
+// A single Permission Report sent to Safe Browsing client-side detection
+// frontends.
+message PermissionReportExtension {

[kcarattini, 2016/05/25 00:21:28]

This isn't actually an extension in this context. What about just
"PermissionsReport"?

+  // The origin (scheme/host/port) of the site requesting the permission.
+  optional string origin = 1;
+
+  // The permission being requested/acted upon.
+  optional PermissionType permission = 2;
+
+  // The platform

[raymes, 2016/05/24 03:01:45]

nit: "." at the end of a comment

[stefanocs, 2016/05/24 07:07:41]

Done.

+  optional PlatformType platform_type = 3;

[raymes, 2016/05/24 03:01:45]

Should this be optional?

[kcarattini, 2016/05/24 06:01:01]

I tried to keep these optional in-line with proto3 guidelines. Making something
required can cause crashes, so it's better to define them optional and handle
failures gracefully.

+
+  // When the request was initiated.
+  repeated RequestTrigger request_trigger = 4;
+
+  // The action the user took. Required.
+  optional Action action = 5;

[raymes, 2016/05/24 03:01:45]

This says "required" in the comment but is marked as optional?

[kcarattini, 2016/05/24 06:01:01]

See above.

+
+  // The UI used to complete the action.
+  optional SourceUI source_ui = 6;

[raymes, 2016/05/24 03:01:45]

Should this be optional?

[kcarattini, 2016/05/24 06:01:01]

See above.

+
+  // The relevant field trials enabled for this report.
+  repeated string field_trials = 7;
+
+  // Platform
+  enum PlatformType {
+    PLATFORM_TYPE_UNSPECIFIED = 0;

[raymes, 2016/05/24 03:01:45]

I know nothing about protocol buffers :P Is _UNSPECIFIED commonly used as a
pattern for some reason? Would we ever expect the platform not to be specified?
Would we want an _UNSPECIFIED for Action too? 

[kcarattini, 2016/05/24 06:01:01]

It was suggested to have an unspecified value for anything that had the
possibility of being unset to avoid confusion with an actual 0 value. This
matches the report logs proto design.

+    DESKTOP = 1;
+    ANDROID = 2;
+  }
+
+  // Various request triggers we want to record, e.g. on page load.
+  enum RequestTrigger {
+    REQUEST_TRIGGER_UNSPECIFIED = 0;
+    ONLOAD = 1;
+    AFTER_GESTURE = 2;
+  }
+
+  // User Permission Actions
+  enum Action {
+    GRANTED = 0;
+    DENIED = 1;
+    DISMISSED = 2;
+    IGNORED = 3;
+    REVOKED = 4;
+  }
+
+  // Places in the UI that a permission change can occur.
+  enum SourceUI {
+    SOURCE_UI_UNSPECIFIED = 0;
+    PROMPT = 1;
+    OIB = 2;
+    CONTENT_SETTINGS = 3;

[raymes, 2016/05/24 03:01:45]

Could we remove this and just use SITE_SETTINGS? Conceptually they are the same
surface (content settings will be going away soon). We will be able to
distinguish from the platform type anyway. Kendra - wdyt?

[kcarattini, 2016/05/24 06:01:01]

I'm fine with that. We'll need to change the logs proto to match. Stefano, can
you put a TODO in the comments to remember to change the logs proto?

[stefanocs, 2016/05/24 07:07:41]

Done.

[stefanocs, 2016/05/24 07:07:41]

Done.

+    SITE_SETTINGS = 4;
+  }
+
+  // The various types of permissions.

[raymes, 2016/05/24 03:01:45]

nit: fill 80 chars

+  // This should stay in sync with //src/content/public/browser/permission_type.h
+  // in the Chromium repository:
+  // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/browser/permission_type.h

[kcarattini, 2016/05/23 05:55:52]

Is this already in sync with the existing enum? You can just use it directly
instead of defining a new one here.

[stefanocs, 2016/05/23 06:47:06]

It seems it is not. How do I use it directly here?

[kcarattini, 2016/05/24 01:36:13]

Actually, Raymes, I'm curious about the enum in permission_type.h. Why does it
start from 1? Is the 0 value ever used?

[raymes, 2016/05/24 03:01:45]

I'm not sure - I believe 0 is never used. 

[kcarattini, 2016/05/24 06:01:01]

I spoke with Raymes offline, and we decided to keep the two enums separate. Just
change the comment to say "This should stay in sync with the corresponding Safe
Browsing logs enum" or something to that effect.

[stefanocs, 2016/05/24 07:07:41]

Done.

+  enum PermissionType {
+    // Unknown/invalid API. The zero-index is used in practice to represent
+    // disabling of all APIs for a given full hash.
+    UNKNOWN_PERMISSION = 0;
+
+    MIDI_SYSEX = 1;
+    PUSH_MESSAGING = 2;
+    NOTIFICATIONS = 3;
+    GEOLOCATION = 4;
+    PROTECTED_MEDIA_IDENTIFIER = 5;
+    MIDI = 6;
+    DURABLE_STORAGE = 7;
+    AUDIO_CAPTURE = 8;
+    VIDEO_CAPTURE = 9;
+  }
+}