diff --git a/app/lib/admin/actions/create_moderation_case.dart b/app/lib/admin/actions/create_moderation_case.dart index 9302c822d3..eba93c9163 100644 --- a/app/lib/admin/actions/create_moderation_case.dart +++ b/app/lib/admin/actions/create_moderation_case.dart @@ -23,7 +23,6 @@ Returns the fields on the newly created moderation case. 'kind': 'The kind of the moderation case. (default value: `notification`)', 'source': 'The source of the moderation case. (default value: `internal-notification`)', - 'status': 'The status of the moderation case. (default value: `pending`)', 'subject': 'The subject of the moderation case.', 'url': 'The url of the moderation case (optional).' }, @@ -42,10 +41,6 @@ Returns the fields on the newly created moderation case. InvalidInputException.check( ModerationSource.isValidSource(source), 'invalid source'); - final status = options['status'] ?? ModerationStatus.pending; - InvalidInputException.check( - ModerationStatus.isValidStatus(status), 'invalid status'); - final subject = options['subject']; InvalidInputException.check( subject != null && subject.isNotEmpty, @@ -63,7 +58,7 @@ Returns the fields on the newly created moderation case. reporterEmail: reporterEmail, source: source, kind: kind, - status: status, + status: ModerationStatus.pending, subject: subject, isSubjectOwner: false, url: url, diff --git a/app/lib/admin/actions/resolve_moderation_case.dart b/app/lib/admin/actions/resolve_moderation_case.dart index 0f9b3ba653..557257465a 100644 --- a/app/lib/admin/actions/resolve_moderation_case.dart +++ b/app/lib/admin/actions/resolve_moderation_case.dart @@ -16,6 +16,8 @@ Closes the moderation case and updates the status based on the actions logged on ''', options: { 'case': 'The caseId to be closed.', + 'status': + 'The resolved status of the case. (optional, will be automatically inferred if absent)' }, invoke: (options) async { final caseId = options['case']; @@ -24,6 +26,14 @@ Closes the moderation case and updates the status based on the actions logged on 'case must be given', ); + var status = options['status']; + InvalidInputException.check( + status == null || + (status != ModerationStatus.pending && + ModerationStatus.isValidStatus(status)), + 'invalid status', + ); + final mc = await withRetryTransaction(dbService, (tx) async { final mc = await tx.lookupOrNull( dbService.emptyKey.append(ModerationCase, id: caseId!)); @@ -38,28 +48,32 @@ Closes the moderation case and updates the status based on the actions logged on final hasModeratedAction = mc.getActionLog().hasModeratedAction(); - if (mc.kind == ModerationKind.notification) { - mc.status = hasModeratedAction - ? ModerationStatus.moderationApplied - : ModerationStatus.noAction; - } else if (mc.kind == ModerationKind.appeal) { - final appealedCase = await tx.lookupValue( - dbService.emptyKey.append(ModerationCase, id: mc.appealedCaseId!)); - final appealHadModeratedAction = - appealedCase.getActionLog().hasModeratedAction(); - if (appealHadModeratedAction) { - mc.status = hasModeratedAction - ? ModerationStatus.moderationReverted - : ModerationStatus.moderationUpheld; + if (status == null) { + if (mc.kind == ModerationKind.notification) { + status = hasModeratedAction + ? ModerationStatus.moderationApplied + : ModerationStatus.noAction; + } else if (mc.kind == ModerationKind.appeal) { + final appealedCase = await tx.lookupValue(dbService + .emptyKey + .append(ModerationCase, id: mc.appealedCaseId!)); + final appealHadModeratedAction = + appealedCase.getActionLog().hasModeratedAction(); + if (appealHadModeratedAction) { + status = hasModeratedAction + ? ModerationStatus.moderationReverted + : ModerationStatus.moderationUpheld; + } else { + status = hasModeratedAction + ? ModerationStatus.noActionReverted + : ModerationStatus.noActionUpheld; + } } else { - mc.status = hasModeratedAction - ? ModerationStatus.noActionReverted - : ModerationStatus.noActionUpheld; + throw UnimplementedError('Kind "${mc.kind}" is not implemented.'); } - } else { - throw UnimplementedError('Kind "${mc.kind}" is not implemented.'); } + mc.status = status!; tx.insert(mc); return mc; }); diff --git a/app/lib/admin/actions/update_moderation_case.dart b/app/lib/admin/actions/update_moderation_case.dart index f0590a45c7..247af43ecc 100644 --- a/app/lib/admin/actions/update_moderation_case.dart +++ b/app/lib/admin/actions/update_moderation_case.dart @@ -67,6 +67,12 @@ Returns the fields of the updated moderation case. mc.source = source ?? mc.source; mc.status = status ?? mc.status; + InvalidInputException.check( + (mc.resolved != null && mc.status != ModerationStatus.pending) || + (mc.resolved == null && mc.status == ModerationStatus.pending), + 'resolved timestamp in conflict with status', + ); + tx.insert(mc); }); final mc = await adminBackend.lookupModerationCase(caseId); diff --git a/app/lib/shared/integrity.dart b/app/lib/shared/integrity.dart index aec97e8d42..43cc33713f 100644 --- a/app/lib/shared/integrity.dart +++ b/app/lib/shared/integrity.dart @@ -13,21 +13,22 @@ import 'package:gcloud/storage.dart'; import 'package:http/http.dart' as http; import 'package:logging/logging.dart'; import 'package:pool/pool.dart'; -import 'package:pub_dev/publisher/backend.dart'; -import 'package:pub_dev/shared/env_config.dart'; import 'package:retry/retry.dart'; import '../account/agent.dart'; import '../account/models.dart'; +import '../admin/models.dart'; import '../audit/models.dart'; import '../package/backend.dart'; import '../package/model_properties.dart'; import '../package/models.dart'; +import '../publisher/backend.dart'; import '../publisher/models.dart'; +import '../shared/env_config.dart'; import 'configuration.dart'; import 'datastore.dart'; -import 'email.dart' show looksLikeEmail; +import 'email.dart' show isValidEmail, looksLikeEmail; import 'storage.dart'; import 'urls.dart' as urls; import 'utils.dart' show canonicalizeVersion, ByteArrayEqualsExt; @@ -91,6 +92,7 @@ class IntegrityChecker { yield* _checkLikes(); yield* _checkModeratedPackages(); yield* _checkAuditLogs(); + yield* _checkModerationCases(); yield* _reportPubspecVersionIssues(); // TODO: report unmapped properties } finally { @@ -851,6 +853,50 @@ class IntegrityChecker { Future _packageMissing(String packageName) async => !(await _packageExists(packageName)); + Stream _checkModerationCases() async* { + _logger.info('Scanning ModerationCases...'); + yield* _queryWithPool(_checkModerationCase); + } + + Stream _checkModerationCase(ModerationCase mc) async* { + if (!isValidEmail(mc.reporterEmail)) { + yield 'ModerationCase "${mc.caseId}" has invalid `reporterEmail`.'; + } + if (mc.resolved != null && mc.status == ModerationStatus.pending) { + yield 'ModerationCase "${mc.caseId}" is resolved but `status` is "pending".'; + } + if (mc.status != ModerationStatus.pending && mc.resolved == null) { + yield 'ModerationCase "${mc.caseId}" has non-pending `status` but `resolved` is null.'; + } + final subject = ModerationSubject.tryParse(mc.subject); + if (subject == null) { + yield 'ModerationCase "${mc.caseId}" has invalid `subject`.'; + } + if (mc.url != null && Uri.tryParse(mc.url!) == null) { + yield 'ModerationCase "${mc.caseId}" has invalid `url`.'; + } + for (final entry in mc.getActionLog().entries) { + if (entry.timestamp.isBefore(mc.opened)) { + yield 'ModerationCase "${mc.caseId}" has action logged before it was opened.'; + } + if (mc.resolved != null && entry.timestamp.isAfter(mc.resolved!)) { + yield 'ModerationCase "${mc.caseId}" has action logged after it was resolved.'; + } + if (ModerationSubject.tryParse(entry.subject) == null) { + yield 'ModerationCase "${mc.caseId}" has action logged with invalid `subject`.'; + } + } + // TODO: verify fields once the other PR lands + + if (mc.appealedCaseId != null) { + final appealed = await dbService.lookupOrNull( + dbService.emptyKey.append(ModerationCase, id: mc.appealedCaseId!)); + if (appealed == null) { + yield 'ModerationCase "${mc.caseId}" references an appealed case that does not exists.'; + } + } + } + Stream _reportPubspecVersionIssues() async* { for (final String package in _badVersionInPubspec.keys) { final values = _badVersionInPubspec[package]!.toList()..sort(); diff --git a/app/test/admin/moderate_package_test.dart b/app/test/admin/moderate_package_test.dart index 73e934a28b..27e6643a8f 100644 --- a/app/test/admin/moderate_package_test.dart +++ b/app/test/admin/moderate_package_test.dart @@ -354,7 +354,11 @@ void main() { testWithProfile('status already closed', fn: () async { final mc = await _report('oxygen'); - await dbService.commit(inserts: [mc..status = ModerationStatus.noAction]); + await dbService.commit(inserts: [ + mc + ..resolved = clock.now() + ..status = ModerationStatus.noAction + ]); await expectApiException( _moderate('oxygen', state: true, caseId: mc.caseId), diff --git a/app/test/admin/moderate_publisher_test.dart b/app/test/admin/moderate_publisher_test.dart index 96522e54c9..8545b0f672 100644 --- a/app/test/admin/moderate_publisher_test.dart +++ b/app/test/admin/moderate_publisher_test.dart @@ -192,7 +192,11 @@ void main() { testWithProfile('status already closed', fn: () async { final mc = await _report('example.com'); - await dbService.commit(inserts: [mc..status = ModerationStatus.noAction]); + await dbService.commit(inserts: [ + mc + ..resolved = clock.now() + ..status = ModerationStatus.noAction + ]); await expectApiException( _moderate('example.com', state: true, caseId: mc.caseId), diff --git a/app/test/admin/moderation_case_test.dart b/app/test/admin/moderation_case_test.dart index 3426cd4637..841522b355 100644 --- a/app/test/admin/moderation_case_test.dart +++ b/app/test/admin/moderation_case_test.dart @@ -45,7 +45,6 @@ void main() { 'subject': 'package:oxygen', 'kind': 'notification', 'source': 'external-notification', - 'status': 'no-action', }), ); expect(rs.output, { @@ -55,7 +54,7 @@ void main() { 'opened': isNotEmpty, 'resolved': null, 'source': 'external-notification', - 'status': 'no-action', + 'status': 'pending', 'subject': 'package:oxygen', 'isSubjectOwner': false, 'url': null, @@ -101,7 +100,6 @@ void main() { final values = { 'kind': 'kind', 'source': 'source', - 'status': 'status', 'reporter-email': 'non-email', }; @@ -135,6 +133,14 @@ void main() { testWithProfile('success', fn: () async { final caseId = await _create(); final api = createPubApiClient(authToken: siteAdminToken); + // close case first to change status + await api.adminInvokeAction( + 'resolve-moderation-case', + AdminInvokeActionArguments(arguments: { + 'case': caseId, + }), + ); + final rs = await api.adminInvokeAction( 'update-moderation-case', AdminInvokeActionArguments(arguments: { @@ -150,7 +156,7 @@ void main() { 'reporterEmail': 'user@pub.dev', 'kind': 'notification', 'opened': isNotEmpty, - 'resolved': null, + 'resolved': isNotEmpty, 'source': 'external-notification', 'status': 'no-action', 'subject': 'package:oxygen', diff --git a/app/test/frontend/handlers/report_test.dart b/app/test/frontend/handlers/report_test.dart index 5c7d637119..7fae0e59ac 100644 --- a/app/test/frontend/handlers/report_test.dart +++ b/app/test/frontend/handlers/report_test.dart @@ -3,6 +3,7 @@ // BSD-style license that can be found in the LICENSE file. import 'package:_pub_shared/data/account_api.dart'; +import 'package:clock/clock.dart'; import 'package:pub_dev/admin/models.dart'; import 'package:pub_dev/fake/backend/fake_auth_provider.dart'; import 'package:pub_dev/fake/backend/fake_email_sender.dart'; @@ -322,6 +323,9 @@ void main() { if (logSubject != null) { mc.addActionLogEntry(logSubject, ModerationAction.apply, null); } + if (mc.status != ModerationStatus.pending) { + mc.resolved = clock.now(); + } await dbService.commit(inserts: [mc]); }