From eb4d8223946a96a2d787ee05faa234a88ebbf0b3 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Mon, 3 Apr 2023 12:26:37 -0400 Subject: [PATCH] feat: Check that all AndroidManifest.xml files have the INTERNET permission (#219) Co-authored-by: Felix Angelov --- .../lib/src/commands/doctor_command.dart | 48 ++++--- .../lib/src/doctor/doctor_validator.dart | 71 ++++++++++ ...android_internet_permission_validator.dart | 69 +++++++++ .../shorebird_version_validator.dart | 39 ++++++ .../lib/src/doctor/validators/validators.dart | 2 + packages/shorebird_cli/pubspec.yaml | 1 + ...id_internet_permission_validator_test.dart | 131 ++++++++++++++++++ .../shorebird_version_validator_test.dart | 90 ++++++++++++ .../src/commands/doctor_command_test.dart | 118 ++++++++++------ 9 files changed, 508 insertions(+), 61 deletions(-) create mode 100644 packages/shorebird_cli/lib/src/doctor/doctor_validator.dart create mode 100644 packages/shorebird_cli/lib/src/doctor/validators/android_internet_permission_validator.dart create mode 100644 packages/shorebird_cli/lib/src/doctor/validators/shorebird_version_validator.dart create mode 100644 packages/shorebird_cli/lib/src/doctor/validators/validators.dart create mode 100644 packages/shorebird_cli/test/src/commands/doctor/validators/android_internet_permission_validator_test.dart create mode 100644 packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_version_validator_test.dart diff --git a/packages/shorebird_cli/lib/src/commands/doctor_command.dart b/packages/shorebird_cli/lib/src/commands/doctor_command.dart index 1885a546..10ad34d6 100644 --- a/packages/shorebird_cli/lib/src/commands/doctor_command.dart +++ b/packages/shorebird_cli/lib/src/commands/doctor_command.dart @@ -1,8 +1,7 @@ -import 'dart:io'; - import 'package:mason_logger/mason_logger.dart'; -import 'package:path/path.dart' as p; import 'package:shorebird_cli/src/command.dart'; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:shorebird_cli/src/doctor/validators/validators.dart'; import 'package:shorebird_cli/src/shorebird_version_mixin.dart'; import 'package:shorebird_cli/src/version.dart'; @@ -14,7 +13,21 @@ import 'package:shorebird_cli/src/version.dart'; /// {@endtemplate} class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin { /// {@macro doctor_command} - DoctorCommand({required super.logger, super.runProcess}); + DoctorCommand({ + required super.logger, + List? validators, + super.runProcess, + }) { + this.validators = validators ?? + [ + ShorebirdVersionValidator( + isShorebirdVersionCurrent: isShorebirdVersionCurrent, + ), + AndroidInternetPermissionValidator(), + ]; + } + + late final List validators; @override String get name => 'doctor'; @@ -24,26 +37,29 @@ class DoctorCommand extends ShorebirdCommand with ShorebirdVersionMixin { @override Future run() async { - var numIssues = 0; - final workingDirectory = p.dirname(Platform.script.toFilePath()); logger.info(''' -Doctor summary Shorebird v$packageVersion '''); - final isShorebirdUpToDate = await isShorebirdVersionCurrent( - workingDirectory: workingDirectory, - ); + var numIssues = 0; + for (final validator in validators) { + final progress = logger.progress(validator.description); + final issues = await validator.validate(); + numIssues += issues.length; + if (issues.isEmpty) { + progress.complete(); + } else { + progress.fail(); - if (!isShorebirdUpToDate) { - numIssues += 1; - logger.info(''' -A new version of shorebird is available! -Run `shorebird upgrade` to upgrade. -'''); + for (final issue in issues) { + logger.info(' ${issue.displayMessage}'); + } + } } + logger.info(''); + if (numIssues == 0) { logger.info('No issues detected!'); } else { diff --git a/packages/shorebird_cli/lib/src/doctor/doctor_validator.dart b/packages/shorebird_cli/lib/src/doctor/doctor_validator.dart new file mode 100644 index 00000000..9e199735 --- /dev/null +++ b/packages/shorebird_cli/lib/src/doctor/doctor_validator.dart @@ -0,0 +1,71 @@ +import 'package:mason_logger/mason_logger.dart'; +import 'package:meta/meta.dart'; + +/// Severity level of a [ValidationIssue]. +/// +/// [error]s should be fixed before continuing development. +/// [warning]s should be fixed before releasing your app, but are not as urgent. +enum ValidationIssueSeverity { + error, + warning, +} + +/// Display helpers for printing [ValidationIssue]s. +extension Display on ValidationIssueSeverity { + String get leading { + switch (this) { + case ValidationIssueSeverity.error: + return red.wrap('[✗]')!; + case ValidationIssueSeverity.warning: + return yellow.wrap('[!]')!; + } + } +} + +/// A (potential) problem with the current Shorebird installation or project. +@immutable +class ValidationIssue { + const ValidationIssue({required this.severity, required this.message}); + + /// How important it is to fix this issue. + final ValidationIssueSeverity severity; + + /// A description of the issue. + final String message; + + /// A console-friendly description of this issue. + String? get displayMessage { + return '${severity.leading} $message'; + } + + // coverage:ignore-start + @override + String toString() => '$severity $message'; + // coverage:ignore-end + + @override + bool operator ==(Object other) { + if (identical(this, other)) return true; + + return other is ValidationIssue && + other.severity == severity && + other.message == message; + } + + // coverage:ignore-start + @override + int get hashCode => Object.hashAll([severity, message]); + // coverage:ignore-end +} + +/// Checks for a specific issue with either the Shorebird installation or the +/// current Shorebird project. +abstract class DoctorValidator { + /// A one-sentence explanation of what this validator is checking. + String get description; + + /// Checks for [ValidationIssue]s. + /// + /// Returns an empty list if no issues are found. + Future> validate(); +} diff --git a/packages/shorebird_cli/lib/src/doctor/validators/android_internet_permission_validator.dart b/packages/shorebird_cli/lib/src/doctor/validators/android_internet_permission_validator.dart new file mode 100644 index 00000000..4cf2a213 --- /dev/null +++ b/packages/shorebird_cli/lib/src/doctor/validators/android_internet_permission_validator.dart @@ -0,0 +1,69 @@ +import 'dart:io'; + +import 'package:path/path.dart' as p; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:xml/xml.dart'; + +/// Checks that all AndroidManifest.xml files in android/app/src/{flavor}/ +/// contain the INTERNET permission, which is required for Shorebird to work. +/// +/// See https://github.com/shorebirdtech/shorebird/issues/160. +class AndroidInternetPermissionValidator extends DoctorValidator { + // coverage:ignore-start + @override + String get description => + 'AndroidManifest.xml files contain INTERNET permission'; + // coverage:ignore-end + + @override + Future> validate() async { + const manifestFileName = 'AndroidManifest.xml'; + final androidSrcDir = Directory( + p.join( + Directory.current.path, + 'android', + 'app', + 'src', + ), + ); + final manifestsWithoutInternetPermission = androidSrcDir + .listSync() + .whereType() + .where((dir) { + return dir.listSync().whereType().any( + (file) => p.basename(file.path) == 'AndroidManifest.xml', + ); + }) + .map((e) => p.join(e.path, manifestFileName)) + .where((manifest) => !_androidManifestHasInternetPermission(manifest)); + + if (manifestsWithoutInternetPermission.isNotEmpty) { + return manifestsWithoutInternetPermission + .map( + (String manifestPath) => ValidationIssue( + severity: ValidationIssueSeverity.error, + message: '$manifestPath is missing the INTERNET permission.', + ), + ) + .toList(); + } + + return []; + } + + bool _androidManifestHasInternetPermission(String path) { + final xmlDocument = XmlDocument.parse(File(path).readAsStringSync()); + return xmlDocument.rootElement.childElements + .any(_isInternetPermissionElement); + } + + bool _isInternetPermissionElement(XmlElement element) { + if (element.localName != 'uses-permission') { + return false; + } + + final attribute = element.attributes.first; + return attribute.qualifiedName == 'android:name' && + attribute.value == 'android.permission.INTERNET'; + } +} diff --git a/packages/shorebird_cli/lib/src/doctor/validators/shorebird_version_validator.dart b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_version_validator.dart new file mode 100644 index 00000000..bf9e10eb --- /dev/null +++ b/packages/shorebird_cli/lib/src/doctor/validators/shorebird_version_validator.dart @@ -0,0 +1,39 @@ +import 'dart:io'; + +import 'package:path/path.dart' as p; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; + +/// Verifies that the currently installed version of Shorebird is the latest. +class ShorebirdVersionValidator extends DoctorValidator { + ShorebirdVersionValidator({required this.isShorebirdVersionCurrent}); + + final Future Function({required String workingDirectory}) + isShorebirdVersionCurrent; + + // coverage:ignore-start + @override + String get description => 'Shorebird is up-to-date'; + // coverage:ignore-end + + @override + Future> validate() async { + final workingDirectory = p.dirname(Platform.script.toFilePath()); + final isShorebirdUpToDate = await isShorebirdVersionCurrent( + workingDirectory: workingDirectory, + ); + + if (!isShorebirdUpToDate) { + return [ + const ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: ''' +A new version of shorebird is available! +Run `shorebird upgrade` to upgrade. +''', + ) + ]; + } + + return []; + } +} diff --git a/packages/shorebird_cli/lib/src/doctor/validators/validators.dart b/packages/shorebird_cli/lib/src/doctor/validators/validators.dart new file mode 100644 index 00000000..af3bbb52 --- /dev/null +++ b/packages/shorebird_cli/lib/src/doctor/validators/validators.dart @@ -0,0 +1,2 @@ +export 'android_internet_permission_validator.dart'; +export 'shorebird_version_validator.dart'; diff --git a/packages/shorebird_cli/pubspec.yaml b/packages/shorebird_cli/pubspec.yaml index a872b7b9..6b3108ba 100644 --- a/packages/shorebird_cli/pubspec.yaml +++ b/packages/shorebird_cli/pubspec.yaml @@ -25,6 +25,7 @@ dependencies: pubspec_parse: ^1.2.2 shorebird_code_push_client: path: ../shorebird_code_push_client + xml: ^6.2.2 yaml: ^3.1.1 yaml_edit: ^2.1.0 diff --git a/packages/shorebird_cli/test/src/commands/doctor/validators/android_internet_permission_validator_test.dart b/packages/shorebird_cli/test/src/commands/doctor/validators/android_internet_permission_validator_test.dart new file mode 100644 index 00000000..dab7765c --- /dev/null +++ b/packages/shorebird_cli/test/src/commands/doctor/validators/android_internet_permission_validator_test.dart @@ -0,0 +1,131 @@ +import 'dart:io'; + +import 'package:collection/collection.dart'; +import 'package:path/path.dart' as p; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:shorebird_cli/src/doctor/validators/validators.dart'; +import 'package:test/test.dart'; + +void main() { + const manifestWithInternetPermission = ''' + + + +'''; + + const manifestWithCommentedOutInternetPermission = ''' + + + +'''; + + const manifestWithNonInternetPermissions = ''' + + + +'''; + + const manifestWithNoPermissions = ''' + + +'''; + + group('AndroidInternetPermissionValidator', () { + Directory createTempDir() => Directory.systemTemp.createTempSync(); + + void writeManifestToPath(String manifestContents, String path) { + Directory(path).createSync(recursive: true); + File(p.join(path, 'AndroidManifest.xml')) + .writeAsStringSync(manifestContents); + } + + test( + 'returns successful result if all AndroidManifest.xml files have the ' + 'INTERNET permission', + () async { + final tempDirectory = createTempDir(); + writeManifestToPath( + manifestWithInternetPermission, + p.join(tempDirectory.path, 'android/app/src/debug'), + ); + writeManifestToPath( + manifestWithInternetPermission, + p.join(tempDirectory.path, 'android/app/src/main'), + ); + + final results = await IOOverrides.runZoned( + () => AndroidInternetPermissionValidator().validate(), + getCurrentDirectory: () => tempDirectory, + ); + + expect(results.map((res) => res.severity), isEmpty); + }, + ); + + test( + 'returns separate errors for all AndroidManifest.xml files without the ' + 'INTERNET permission', + () async { + final tempDirectory = createTempDir(); + final manifestPaths = [ + 'internet_permission', + 'debug', + 'main', + 'profile', + ] + .map( + (dir) => p.join( + tempDirectory.path, + 'android', + 'app', + 'src', + dir, + ), + ) + .toList(); + final badManifestPaths = manifestPaths.slice(1); + + writeManifestToPath( + manifestWithInternetPermission, + manifestPaths[0], + ); + writeManifestToPath( + manifestWithCommentedOutInternetPermission, + manifestPaths[1], + ); + writeManifestToPath( + manifestWithNonInternetPermissions, + manifestPaths[2], + ); + writeManifestToPath( + manifestWithNoPermissions, + manifestPaths[3], + ); + + final results = await IOOverrides.runZoned( + () => AndroidInternetPermissionValidator().validate(), + getCurrentDirectory: () => tempDirectory, + ); + + expect(results, hasLength(3)); + + expect( + results, + containsAll( + badManifestPaths.map( + (path) => ValidationIssue( + severity: ValidationIssueSeverity.error, + message: + '$path/AndroidManifest.xml is missing the INTERNET permission.', + ), + ), + ), + ); + }, + ); + }); +} diff --git a/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_version_validator_test.dart b/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_version_validator_test.dart new file mode 100644 index 00000000..396ec4f1 --- /dev/null +++ b/packages/shorebird_cli/test/src/commands/doctor/validators/shorebird_version_validator_test.dart @@ -0,0 +1,90 @@ +import 'dart:io'; + +import 'package:mason_logger/mason_logger.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:shorebird_cli/src/commands/doctor_command.dart'; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:shorebird_cli/src/doctor/validators/shorebird_version_validator.dart'; +import 'package:test/test.dart'; + +class _MockLogger extends Mock implements Logger {} + +class _MockProcessResult extends Mock implements ProcessResult {} + +void main() { + const currentShorebirdRevision = 'revision-1'; + const newerShorebirdRevision = 'revision-2'; + + group('ShorebirdVersionValidator', () { + late ShorebirdVersionValidator validator; + late Logger logger; + late DoctorCommand command; + late ProcessResult fetchCurrentVersionResult; + late ProcessResult fetchLatestVersionResult; + + setUp(() { + logger = _MockLogger(); + fetchCurrentVersionResult = _MockProcessResult(); + fetchLatestVersionResult = _MockProcessResult(); + + command = DoctorCommand( + logger: logger, + runProcess: ( + executable, + arguments, { + bool runInShell = false, + workingDirectory, + }) async { + if (executable == 'git') { + const revParseHead = ['rev-parse', '--verify', 'HEAD']; + if (arguments.every((arg) => revParseHead.contains(arg))) { + return fetchCurrentVersionResult; + } + + const revParseUpstream = ['rev-parse', '--verify', '@{upstream}']; + if (arguments.every((arg) => revParseUpstream.contains(arg))) { + return fetchLatestVersionResult; + } + } + return _MockProcessResult(); + }, + ); + + validator = ShorebirdVersionValidator( + isShorebirdVersionCurrent: command.isShorebirdVersionCurrent, + ); + + when( + () => fetchCurrentVersionResult.exitCode, + ).thenReturn(ExitCode.success.code); + when( + () => fetchCurrentVersionResult.stdout, + ).thenReturn(currentShorebirdRevision); + when( + () => fetchLatestVersionResult.exitCode, + ).thenReturn(ExitCode.success.code); + when( + () => fetchLatestVersionResult.stdout, + ).thenReturn(currentShorebirdRevision); + }); + + test('returns no issues when shorebird is up-to-date', () async { + final results = await validator.validate(); + expect(results, isEmpty); + }); + + test('returns a warning when a newer shorebird is available', () async { + when( + () => fetchLatestVersionResult.stdout, + ).thenReturn(newerShorebirdRevision); + + final results = await validator.validate(); + expect(results, hasLength(1)); + expect(results.first.severity, ValidationIssueSeverity.warning); + expect( + results.first.message, + contains('A new version of shorebird is available!'), + ); + }); + }); +} diff --git a/packages/shorebird_cli/test/src/commands/doctor_command_test.dart b/packages/shorebird_cli/test/src/commands/doctor_command_test.dart index a549cac1..fe8c3d80 100644 --- a/packages/shorebird_cli/test/src/commands/doctor_command_test.dart +++ b/packages/shorebird_cli/test/src/commands/doctor_command_test.dart @@ -1,83 +1,111 @@ -import 'dart:io'; - import 'package:mason_logger/mason_logger.dart'; import 'package:mocktail/mocktail.dart'; import 'package:shorebird_cli/src/commands/commands.dart'; +import 'package:shorebird_cli/src/doctor/doctor_validator.dart'; +import 'package:shorebird_cli/src/doctor/validators/validators.dart'; import 'package:test/test.dart'; +class _MockShorebirdVersionValidator extends Mock + implements ShorebirdVersionValidator {} + +class _MockAndroidInternetPermissionValidator extends Mock + implements AndroidInternetPermissionValidator {} + class _MockLogger extends Mock implements Logger {} -class _MockProcessResult extends Mock implements ProcessResult {} +class _MockProgress extends Mock implements Progress {} void main() { - const currentShorebirdRevision = 'revision-1'; - const newerShorebirdRevision = 'revision-2'; - group('doctor', () { late Logger logger; + late Progress progress; late DoctorCommand command; - late ProcessResult fetchCurrentVersionResult; - late ProcessResult fetchLatestVersionResult; + late AndroidInternetPermissionValidator androidInternetPermissionValidator; + late ShorebirdVersionValidator shorebirdVersionValidator; setUp(() { logger = _MockLogger(); - fetchCurrentVersionResult = _MockProcessResult(); - fetchLatestVersionResult = _MockProcessResult(); + progress = _MockProgress(); + + androidInternetPermissionValidator = + _MockAndroidInternetPermissionValidator(); + shorebirdVersionValidator = _MockShorebirdVersionValidator(); command = DoctorCommand( logger: logger, - runProcess: ( - executable, - arguments, { - bool runInShell = false, - workingDirectory, - }) async { - if (executable == 'git') { - const revParseHead = ['rev-parse', '--verify', 'HEAD']; - if (arguments.every((arg) => revParseHead.contains(arg))) { - return fetchCurrentVersionResult; - } - - const revParseUpstream = ['rev-parse', '--verify', '@{upstream}']; - if (arguments.every((arg) => revParseUpstream.contains(arg))) { - return fetchLatestVersionResult; - } - } - return _MockProcessResult(); - }, + validators: [ + androidInternetPermissionValidator, + shorebirdVersionValidator, + ], ); - when( - () => fetchCurrentVersionResult.exitCode, - ).thenReturn(ExitCode.success.code); - when( - () => fetchCurrentVersionResult.stdout, - ).thenReturn(currentShorebirdRevision); - when( - () => fetchLatestVersionResult.exitCode, - ).thenReturn(ExitCode.success.code); - when( - () => fetchLatestVersionResult.stdout, - ).thenReturn(currentShorebirdRevision); + when(() => logger.progress(any())).thenReturn(progress); + + when(() => logger.info(any())).thenReturn(null); + + when(() => androidInternetPermissionValidator.description) + .thenReturn('Android'); + when(() => androidInternetPermissionValidator.validate()) + .thenAnswer((_) async => []); + + when(() => shorebirdVersionValidator.description) + .thenReturn('Shorebird Version'); + when(() => shorebirdVersionValidator.validate()) + .thenAnswer((_) async => []); }); test('prints "no issues" when everything is OK', () async { await command.run(); + for (final validator in command.validators) { + verify(validator.validate).called(1); + } verify( () => logger.info(captureAny(that: contains('No issues detected'))), ).called(1); }); - test('prints that an upgrade is available', () async { + test('prints messages when warnings or errors found', () async { when( - () => fetchLatestVersionResult.stdout, - ).thenReturn(newerShorebirdRevision); + () => androidInternetPermissionValidator.validate(), + ).thenAnswer( + (_) async => [ + const ValidationIssue( + severity: ValidationIssueSeverity.warning, + message: 'oh no!', + ), + const ValidationIssue( + severity: ValidationIssueSeverity.error, + message: 'OH NO!', + ), + ], + ); await command.run(); + + for (final validator in command.validators) { + verify(validator.validate).called(1); + } + verify( () => logger.info( captureAny( - that: contains('A new version of shorebird is available!'), + that: contains('[!] oh no!'), + ), + ), + ).called(1); + + verify( + () => logger.info( + captureAny( + that: contains('[✗] OH NO!'), + ), + ), + ).called(1); + + verify( + () => logger.info( + captureAny( + that: contains('2 issues detected.'), ), ), ).called(1);