Skip to content

Honor legacy opt out status #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkgs/unified_analytics/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## 1.1.0

- Added a `okToSend` getter so that clients can easily and accurately check the state of the consent mechanism.
- Initialize the config file with user opted out if user was opted out in legacy Flutter and Dart analytics

## 1.0.1

Expand Down
1 change: 1 addition & 0 deletions pkgs/unified_analytics/analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ include: package:lints/recommended.yaml
linter:
rules:
- always_declare_return_types
- avoid_catches_without_on_clauses
- camel_case_types
- prefer_single_quotes
- unawaited_futures
Expand Down
11 changes: 9 additions & 2 deletions pkgs/unified_analytics/lib/src/analytics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,15 @@ class AnalyticsImpl implements Analytics {
}

@override
String get getConsentMessage =>
kToolsMessage.replaceAll('[tool name]', tool.description);
String get getConsentMessage {
// The command to swap in the consent message
final String commandString =
tool == DashTool.flutterTool ? 'flutter' : 'dart';

return kToolsMessage
.replaceAll('[tool name]', tool.description)
.replaceAll('[dart|flutter]', commandString);
}
Comment on lines +379 to +387
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been added to address issue #82 to change the consent message's command based on which tool is using it


/// Checking the [telemetryEnabled] boolean reflects what the
/// config file reflects
Expand Down
15 changes: 14 additions & 1 deletion pkgs/unified_analytics/lib/src/ga_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,28 @@ class GAClient {

/// Receive the payload in Map form and parse
/// into JSON to send to GA
///
/// The [Response] returned from this method can be
/// checked to ensure that events have been sent. A response
/// status code of `2xx` indicates a successful send event.
/// A response status code of `500` indicates an error occured on the send
/// can the error message can be found in the [Response.body]
Future<http.Response> sendData(Map<String, Object?> body) async {
final Uri uri = Uri.parse(postUrl);

/// Using a try catch all since post method can result in several
/// errors; clients using this method can check the awaited status
/// code to get a specific error message if the status code returned
/// is a 500 error status code
try {
return await _client.post(
Uri.parse(postUrl),
uri,
headers: <String, String>{
'Content-Type': 'application/json; charset=UTF-8',
},
body: jsonEncode(body),
);
// ignore: avoid_catches_without_on_clauses
} catch (error) {
return Future<http.Response>.value(http.Response(error.toString(), 500));
}
Expand Down
11 changes: 10 additions & 1 deletion pkgs/unified_analytics/lib/src/initializer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,16 @@ class Initializer {
required int toolsMessageVersion,
}) {
configFile.createSync(recursive: true);
configFile.writeAsStringSync(kConfigString);

// If the user was previously opted out, then we will
// replace the line that assumes automatic opt in with
// an opt out from the start
if (legacyOptOut(fs: fs, home: homeDirectory)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm reading this package's code wrong, it looks like this will be platform.environment['UserProfile'] on windows (https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/utils.dart#L78), when the tool will be writing to platform.environment['AppData'] on Windows https://github.com/dart-lang/usage/blob/master/lib/src/usage_impl_io.dart#L89

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a very good catch. In the PDD, it says all files will be written to the user's home directory, am I correct to assume the home directory on windows is stored in UserProfile?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding is that "home directory" is an imprecise term, and especially on Windows there is not one "correct" answer for where a user's home directory is across different Windows versions and also application usages. In other words:

  1. for the purposes of this PR, I think we need to check for legacy opt out from where package:usage is writing it, which is the AppData env var if present (not sure if it's ever not present, or what package:usage does in that case).
  2. for the purposes of fulfilling the PDD, I'm not sure what we should do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The home directory as the value of the UserProfile environment variable seems correct to me and fulfills the PDD. The most common definitions of "home directory" under Windows align with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should adjust the PDD to allow writing the files in a more Windows-friendly location like AppData.

Copy link
Contributor Author

@eliasyishak eliasyishak Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would be okay if we changed the env variable for home on windows to point to APPDATA then? That way we can continue what was done in the past and address @christopherfujino 's item 1 above?

configFile.writeAsStringSync(
kConfigString.replaceAll('reporting=1', 'reporting=0'));
} else {
configFile.writeAsStringSync(kConfigString);
}
}

/// Creates that log file that will store the record formatted
Expand Down
80 changes: 79 additions & 1 deletion pkgs/unified_analytics/lib/src/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:convert';
import 'dart:io' as io;
import 'dart:math' show Random;

import 'package:file/file.dart';
import 'package:path/path.dart' as p;

import 'enums.dart';
import 'user_property.dart';
Expand Down Expand Up @@ -75,12 +77,88 @@ Directory getHomeDirectory(FileSystem fs) {
} else if (io.Platform.isLinux) {
home = envVars['HOME'];
} else if (io.Platform.isWindows) {
home = envVars['UserProfile'];
home = envVars['AppData'];
}

return fs.directory(home!);
}

/// Returns `true` if user has opted out of legacy analytics in Dart or Flutter
///
/// Checks legacy opt-out status for the Flutter
/// and Dart in the following locations
///
/// Dart: `$HOME/.dart/dartdev.json`
///
/// Flutter: `$HOME/.flutter`
bool legacyOptOut({
required FileSystem fs,
required Directory home,
}) {
final File dartLegacyConfigFile =
fs.file(p.join(home.path, '.dart', 'dartdev.json'));
final File flutterLegacyConfigFile = fs.file(p.join(home.path, '.flutter'));

// Example of what the file looks like for dart
//
// {
// "firstRun": false,
// "enabled": false, <-- THIS USER HAS OPTED OUT
// "disclosureShown": true,
// "clientId": "52710e60-7c70-4335-b3a4-9d922630f12a"
// }
if (dartLegacyConfigFile.existsSync()) {
try {
// Read in the json object into a Map and check for
// the enabled key being set to false; this means the user
// has opted out of analytics for dart
final Map<String, Object?> dartObj =
jsonDecode(dartLegacyConfigFile.readAsStringSync());
if (dartObj.containsKey('enabled') && dartObj['enabled'] == false) {
return true;
}
} on FormatException {
// In the case of an error when parsing the json file, return true
// which will result in the user being opted out of unified_analytics
//
// A corrupted file could mean they opted out previously but for some
// reason, the file was written incorrectly
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should delete the file if we couldn't parse it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should delete the file if we couldn't parse it.

I am against such drastic action; if there is a bug in how we are parsing the file we could interfere with the legacy analytics consent system. Better to just leave it behind, as this is after all an optional feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, once legacy analytics is removed, we can add something to delete legacy analytics files if we see them.

} on FileSystemException {
return true;
}
}

// Example of what the file looks like for flutter
//
// {
// "firstRun": false,
// "clientId": "4c3a3d1e-e545-47e7-b4f8-10129f6ab169",
// "enabled": false <-- THIS USER HAS OPTED OUT
// }
if (flutterLegacyConfigFile.existsSync()) {
try {
// Same process as above for dart
final Map<String, Object?> flutterObj =
jsonDecode(dartLegacyConfigFile.readAsStringSync());
if (flutterObj.containsKey('enabled') && flutterObj['enabled'] == false) {
return true;
}
} on FormatException {
// In the case of an error when parsing the json file, return true
// which will result in the user being opted out of unified_analytics
//
// A corrupted file could mean they opted out previously but for some
// reason, the file was written incorrectly
return true;
} on FileSystemException {
return true;
}
}

return false;
}

/// A UUID generator.
///
/// This will generate unique IDs in the format:
Expand Down
Loading