1515697 - [Telemetry] The number of new permissions in add-on updates are incorrectly calculated (too high).

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Rob Wu [:robwu]]

A telemetry event may be recorded at the "permissions_prompt" step when an add-on is updated [1].
This includes two keys "num_perms" and "num_origins", and its meaning is documented at [2]:

> addonsManager:
>  install:
>    ...
>    extra_keys:
>      ...
>      num_perms: The number of permissions shown to the user in the extension permission doorhanger
>      num_origins: The number of origins shown to the user in the extension permission doorhanger

The original design document also suggests that these counters should reflect the permissions as shown in the permission doorhanger [3].

The actual implementation counts the number of permissions and the number of origins that aren't covered by the previous version. If the set is empty, no telemetry event is sent. [3][4]
This permission-based counting deviates from the intended / documented behavior, in the following ways:

1. Some API permissions have no warnings, and the doorhanger may be skipped if the set of warnings generated from the permissions is empty [5][6]. Still, a telemetry event is recorded.
   Expected: num_perms should be the size of the set of permissions that cause warning messages.
             If the set is empty, the doorhanger is not shown and the "permissions_prompt" step should not be recorded.
   Actual: num_perms counts all permissions, including warningless permissions, **even if the permission doorhanger is not shown**.

2. Some host permissions may overlap with each other. E.g. "http://example.com/", "http://example.com/path/", "https://example.com/" all map to one "example.com" warning.
   Expected: num_origins should count the unique number of origins and ignore duplicates.
   Actual: num_origins counts all host permissions, including duplicates/overlapping origins.



[1] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/toolkit/mozapps/extensions/AddonManager.jsm#3532-3534,3543-3545
[2] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/toolkit/components/telemetry/Events.yaml#92-93
[2] https://docs.google.com/document/d/1QVelIRhn1MqIZTF0uxfr1XOxMURFpbeMVgjocyHpT88/edit#heading=h.qauhyohmd7dx

[3] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/mozapps/extensions/content/extensions.js#646-651 
[4] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/mozapps/extensions/AddonManager.jsm#1226-1231

[5] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/browser/modules/ExtensionsUI.jsm#208,211-217
[6] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/browser/modules/ExtensionsUI.jsm#158,177-183

Comment 1

[Rob Wu [:robwu]]

This report shows a mismatch in the documented meaning of the num_perms and num_origins keys of extension update flows.

I think that we should generate the counters based on the list of unique permission warnings (at [1]), instead of the list of permissions.

Ben, the proposed change will affect the reported values of num_perms and num_origins in telemetry.
Does this report (in particular Expected vs Actual) and the proposed (high-level) fix look good to you?


[7] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/Extension.jsm#1092

Comment 2

[Ben Miroglio [:bmiroglio]]

Adding Luca to this as he'll have more context.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've been discussing about this with Rob over the last couple of days, and I agree that we should apply the fixes needed to make the "permissions prompt" telemetry event as representative as possible to what the user is actually experiencing, 
in particular:

- when the permission prompt is not shown:
  - the event for the permission prompt shouldn't be sent 
  - or the event should include some extra vars that makes us able to know when the permission prompt has been shown 
    and when it has been skipped

- when the number of permissions listed in the manifest is different from the number shown to the user:
  - the existing extra vars (num_perms and num_origins) should be representative of the number of entries 
    we actually show to the user
  - or the event should include some additional "extra vars" that makes us able to know how many permission were actually
    listed in the manifest and how many we have been actually shown to the user

If "the differences between the actual and shown permissions" is not something that we are interesting in (from a data analysis point of view), then we may just change the value that we set to the two existing extra vars (num_perms and num_origins) and be sure to not record the telemetry event when their value is 0.

Alternatively we could include two additional extra vars:

- shown_perms, as number of permissions actually listed in the prompt 
  (while num_perms would be the number of new permissions listed in the manifest)
- shown_origins, as number of the origin actually listed in the prompt 
  (while num_origins would be the number of new origins listed in the manifest)

Then, when both these new extra vars are set to 0, it means that there were some new permissions in the manifest, but none of them was going to be shown and the prompt has been skipped.

(Using two new extra vars may also make it a bit easier to know when the data collected is using the new format/meaning)

I asked Rob to file the issue explicitly on Bugzilla and ensure that Ben is aware of these proposed changes because 
(once we apply any of these changes) the data collected (and its actual meaning) will be affected accordingly, 
and it could be quite surprising if not pointed out upfront.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1515697 - Replace addonsManager telemetry events num_origin and num_perms extras with num_strings.

This patch includes the following changes:

• added a new "num_strings" extra key to the "addonsManager install" and "addonsManager manage"
  telemetry events, where "num_strings" represents the "number of permissions actually visible
  in the extension permission doorhanger"

• do not record a telemetry event for the "permission_prompt" (or "sideload_prompt") if the
  permissions_prompt is not going to be shown

• add num_strings in the test assertions from the existing tests

• added some additional assertions to test in automation that we don't record the telemetry
  event for "permission_prompt" when no permission prompt is being shown for an
  extension update (as part of the browser_extension_update_background_noprompt.js test)

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Ben,
the attached patch includes a draft of the changes briefly described in comment 4,
in particular:

• an additional "num_strings" extra key (as the representation of the number
  of the API permission and origin permissions that are actually going to be
  shown to the user) added to the telemetry events

• telemetry events for the "permission_prompt" only recorded if the permission
  prompt is being shown

if these changes sound good to you from a "data analysis" point of view,
I'm going to proceed with the data and phabricator review requests.

Comment 6

[Ben Miroglio [:bmiroglio]]

These all SGTM! Thanks Luca

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: request-data-review-bug-1515697-num-permissions-strings.md

Hi :chutten,
the attached markdown file contains the data review request for the changes we are going to apply to the existing addonsManager.install and addonsManager.manage telemetry events.

(These telemetry events have been added as part of Bug 1433335 and the original data review request is attachment 8989190 [details]).

Comment 8

[Chris H-C :chutten]

Comment on attachment 9038068 [details]
request-data-review-bug-1515697-num-permissions-strings.md

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. These collections are Telemetry so are documented in their definitions file ([Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml)) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. These collections are Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

N/A, these collections expire in Firefox 68.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction. (Case could be made that it's Category 1)

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :rpl is responsible for renewing/removing this collection before it expires in Firefox 68.

---
Result: datareview+

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks :chutten for the super-quick review on the data review request.

Should I add you or :janerik as a blocking reviewer on the phabricator revision for an explicit sign-off on the small changes applied to Events.yaml file?

Comment 10

[Chris H-C :chutten]

From my point-of-view it shouldn't be necessary, but if you insist I say :janerik since I did the Data Review.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Chris H-C :chutten from comment #10)

From my point-of-view it shouldn't be necessary, but if you insist I say :janerik since I did the Data Review.

That's fine ;-), I asked to be sure that it wasn't necessary from your point of view.

Thanks!

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Ben,
as part of the review of the attached patch, we were wondering:

do we still need the existing num_perms and num_origins extra vars along with the new num_strings extra?

if from your point of view the new num_strings extra is enough, then we would be happy to remove the other two num_perms and num_origins as part of the same patch.

(and I would likely rename num_strings to perm_strings, if we agree on proceeding with the removal of the other two).

Comment 13

[Ben Miroglio [:bmiroglio]]

What information are we losing about the install flow if we remove num_perms and num_origins and only have num_strings (if any)?

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Ben Miroglio [:bmiroglio] from comment #13)

What information are we losing about the install flow if we remove num_perms and num_origins and only have num_strings (if any)?

In short, from num_strings you could not know exactly how many API permissions and how many host permissions there were originally in the extension manifest (or, in case of update, how many new hosts and/or new API permissions there were in the manifest of an updating extension)

num_strings represents the subset of both APIs and host permissions that are actually going to be listed in the permission doorhanger, and so if we are only interested into how many "permissions" have been shown to the user in the permission prompt, it sounds like num_strings may be the only thing that we would need to collect.

(as an example, if the <all_urls> host permission is requested by the extension, the permission prompt will not show any of the
other host permissions to the user, because it is going to just show "Access your data for all websites" which include every possible host permission).

Comment 15

[Ben Miroglio [:bmiroglio]]

I see. Is there a way for me to find the extension's manifest (or just the associated API/host permissions) programmatically? For example, "Show me all permissions for extension X"?

It seems the user flow data is still in tact with only num_strings, however we are working on ways (Bug 1520939) that can help us measure the security level of the add-ons ecosystem, and those permissions could play a big role. If I can access them already, then I'm fine with going with num_strings only. Otherwise I'd opt to keep them.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Ben Miroglio [:bmiroglio] from comment #15)

I see. Is there a way for me to find the extension's manifest (or just the associated API/host permissions) programmatically? For example, "Show me all permissions for extension X"?

It seems the user flow data is still in tact with only num_strings, however we are working on ways (Bug 1520939) that can help us measure the security level of the add-ons ecosystem, and those permissions could play a big role. If I can access them already, then I'm fine with going with num_strings only. Otherwise I'd opt to keep them.

Given the extension id, the extension permissions can also be retrieved by querying the AMO DB from STMO,
and those details should be actually more precise of the details that just num_perms and num_origins could provide.

does the approach described above fit for the plans we have for Bug 1520939?

Comment 17

[Ben Miroglio [:bmiroglio]]

Makes sense (its been awhile since I tinkered with that DB!). That is fine for the other bug, and you have my sign off for removing the num_perms and num_origins, replacing them with num_strings).

Thanks for the clarifying discussion.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32767a757c6c
Replace addonsManager telemetry events num_origin and num_perms extras with num_strings. r=aswan

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/32767a757c6c

Comment 20

[Victor Carciu]

Verified as fixed using Firefox 67.0a1 (2019-03-12) and following the steps from https://irccloud.mozilla.com/pastebin/L3TjlbsC/bug_1515697_str.md.

I will attach post fix screenshots for install and update with extra permissions actions. I can also confirm that no "addonsManager install" telemetry event has been collected for the extra.step "permissions prompt" if there are no additional permissions in the updated version.

Comment 21

[Victor Carciu]

Attachment: Postifx screenshot for install with permissions

Comment 22

[Victor Carciu]

Attachment: Postfix screenshot for update with extra permissions added