Bug: UA indicates we should show consent message even after user has already opted out

[kenzieschmoll]

Intended behavior per the PDD: "If the reporting control flag ("reporting") is set to opt-out of reporting, then no notices should be shown in any tool."

Current behavior:

The current logic sets _showMessage to true for the first run of a Dash tool: https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/analytics.dart#L447. When a message is shown, the tool showing the message will set analytics enabled to true.

However, this doesn't take into account if the user has already run the command to disable analytics, which seems like a bug? Imagine this scenario:

1. User opens tool A and sees the analytics prompt. The user will be automatically opted into analytics collection for the second run of tool A.
2. User decides to disable analytics by running the command. For the subsequent runs of tool A, analytics will not be collected.
3. User opens tool B and sees the analytics prompt. User ignores prompt, and now the user will be opted into analytics again for the second run of tool B (and for usage of tool A since now analytics enabled = true).

This seems incorrect. If the user just opted out of analytics, we should not be prompting then again.

The code for shouldShowMessage just returns the value of _showMessage, which doesn't take the enabled state into account:
https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/analytics.dart#L516.

Should this be modified to return _showMessage && !_telemetrySuppressed? Not sure whether we should check _telemetrySuppressed or the value of telemetryEnabled, or both.

@christopherfujino

[christopherfujino]

@andrewkolos can you work on this bug? It has pretty serious correctness implications for how we use analytics.

[andrewkolos]

• User opens tool A and sees the analytics prompt. The user will be automatically opted into analytics collection for the second run of tool A.
• User decides to disable analytics by running the command. For the subsequent runs of tool A, analytics will not be collected.
• User opens tool B and sees the analytics prompt. User ignores prompt, and now the user will be opted into analytics again for the second run of tool B (and for usage of tool A since now analytics enabled = true).

Could you clarify what the expected behavior is here? My personal interpretation is that the user should be opted into telemetry for tool B but not for tool A, as these are two separate tools. However, the issue description seems to imply that disabling telemetry for tool A should disable telemetry for any other tool that is run in the future.

I do agree that tool A's telemetry should not be re-enabled without the user explicitly enabling it. This is definitely a bug.

[andrewkolos]

I do agree that tool A's telemetry should not be re-enabled without the user explicitly enabling it. This is definitely a bug.

Actually, I didn't verify that this is even the case, so I am not even sure there is a bug here.

[kenzieschmoll]

Could you clarify what the expected behavior is here? My personal interpretation is that the user should be opted into telemetry for tool B but not for tool A, as these are two separate tools. However, the issue description seems to imply that disabling telemetry for tool A should disable telemetry for any other tool that is run in the future.

The intent of unified_analytics is that there would be a single opt-in / opt-out state for analytics that is shared across all Dash tools. When we show the analytics notice, we also set the reporting flag to true, and then a user is expected to manually opt out if they do not want analytics to be collected. The bug is coming from when we decide to show the notice and set the opt-in to true.

As the PDD currently stands "If the reporting control flag ("reporting") is set to opt-out of reporting, then no notices should be shown in any tool." This is what we are currently violating by showing the analytics notice in tool B in the example above.

[andrewkolos]

The intent of unified_analytics is that there would be a single opt-in / opt-out state for analytics that is shared across all Dash tools.

This was the part that surprised me to the point of me questioning whether I was reading this issue correctly. Thanks for clarifying. I'll get back to fixing this.

[andrewkolos]

From reading the code and doing some testing on a fresh VM, I don't think the prompt on the second tool re-enables telemetry.

I wrote a test to verify this:

tools/pkgs/unified_analytics/test/unified_analytics_test.dart

Lines 1301 to 1303 in 5997ca9

	test(
	'running a second tool for the first time does not re-enable analytics',
	() async {

I also ported this test to the 5.8.8 branch on my local checkout, and it still passed.

So I think the only bug here is that Analytics::shouldShowMessage will be true whenever you are running some tool for the first time¹, when it should only be true the first time you are running any tool that uses unified_analytics.

Footnotes

1. Or to be more technically precise, Analytics::clientShowedMessage hasn't been called. ↩

[andrewkolos]

@andrewkolos to 1) take a second pass over the unified_analytics code to be confident that the bug described in this issue is not present and then 2) sync up with kenzieschmoll

[christopherfujino]

@andrewkolos To 1) take a second pass over the unified_analytics code to be confident that the bug described in this issue is not present and then 2) sync up with kenzieschmoll

Andrew and I looked at the code, and we believe that:

1. showing the consent message does not actually mutate the state of whether the user is opted in or out of unified analytics. It does mutate whether or not we will show the consent message the next time this tool is run.
2. per our current PDD, we believe that this is working as intended that we show the consent message once per tool (e.g. flutter_tools, dartdev, etc). Once we update our PDD, we can improve this to just show the consent message once per client.
3. the only time that a user who has opted out of analytics then gets opted in is if they explicitly choose to do so, via flutter --enable-analytics or dart --enable-analytics etc.