1529406 - Record user’s logged-in/out status and track insiders with Google Analytics

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, task)

Description

[Kohei Yoshino [:kohei]]

Our current GA implementation is not tracking insiders, which means dozens of power users (employees) are not being tracked even if DNT is disabled. I think we can ease this restriction while prevent private bugs from being tracked.

Also, I’d like to add a custom dimension to see how many users are logged in or out like this:

// Custom Dimension: logged in (true) or out (false)
ga('set', 'dimension1', !!BUGZILLA.user.login);

Thoughts?

Comment 1

[Sylvestre Ledru [:Sylvestre]]

As long as we don't have access to any personal information, I am fine!

Comment 2

[Kohei Yoshino [:kohei]]

Once more thing: currently we are dropping URL query params and some page titles, and logging internal template names instead of URLs. This measure aimed at preventing any sensitive info from being logged, but the collected data is limited. I’m particularly interested in search queries so bug lists’ page title can be generated from the params. Emma is also interested in bug searches according to her comment on Slack.

So, I’d like to start logging actual URLs including params. Any sensitive params, like token used for links to delete a saved search, can be dropped client side. Dylan, do we have anything else we should omit?

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Bugzilla_api_token, Bugzilla_api_key, token, api_key, token, state, secret, github_secret... the list is quite long.
Better: remove all query parameters that are not in an approved list.

We will continue not loading GA on secure bugs, correct?

Comment 4

[Kohei Yoshino [:kohei]]

API keys and tokens are only used for background API calls, right? github_secret is sent via <form method="post"> so it shouldn’t be part of GET params if I read the code correctly. I think It’s hard to create a whitelist for this. Of course, private bugs will continue to be excluded.

Comment 5

[Kohei Yoshino [:kohei]]

We should also be dropping list_id on buglist.cgi, which is just an integer but assigned to individual queries. It’s stored in Bugzilla’s profile_search table along with a user ID, so personally identifiable information.

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Added task to identify the list of safe query parameters.

Comment 7

[Kohei Yoshino [:kohei]]

That’s a really hard task because we’ll be logging params on any page. Even search params can be complex if you use Custom Search criteria. Using a blacklist should be okay and faster. Only token, list_id and perhaps a few more can appear in the URL of user facing pages.

Comment 8

[Kohei Yoshino [:kohei]]

• Params sent via POST request won’t be logged
• API calls, Bugzilla_api_token, Bugzilla_api_key, api_key and Bugzilla_token params in particular, won’t be logged
• Both token and list_id are going away in Bug 1529368 and Bug 1496879 respectively. (Correction: token is used for several more features other than saved searches so it takes time to remove it entirely)

Comment 9

[Kohei Yoshino [:kohei]]

I’d like to move this forward by simply blacklisting token and list_id which are being exposed in the URL of user-facing pages. @dylan: Are you okay with this?

Comment 10

[Kohei Yoshino [:kohei]]

Attachment: GitHub Pull Request

I have already sent a draft PR.

Comment 11

[Kohei Yoshino [:kohei]]

I’ve just found another one called t: https://bugzilla.mozilla.org/token.cgi?t=[token]&a=request_new_account

Comment 12

[Dylan Hardison [:dylan] (he/him)]

This needs to be a whitelist of allowed parameters, not a blacklist of safe ones.

Comment 13

[Kohei Yoshino [:kohei]]

I’d like to move this forward by separating the URL logging.

Comment 14

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

If I understand this so far, we need a list of safe parameters to keep?

Comment 15

[Kohei Yoshino [:kohei]]

Because I’ve changed the scope of the bug, this doesn’t need a whitelist. We need it for Bug 1529704.

Comment 16

[David Lawrence [:dkl]]

Merged to master.