Skip to content
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

ReservedPackage + admin action. #8105

Merged
merged 4 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions app/lib/admin/actions/actions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import 'moderation_case_list.dart';
import 'moderation_case_resolve.dart';
import 'moderation_case_update.dart';
import 'package_info.dart';
import 'package_reserve.dart';
import 'package_version_info.dart';
import 'package_version_retraction.dart';
import 'publisher_block.dart';
Expand Down Expand Up @@ -98,6 +99,7 @@ final class AdminAction {
moderationCaseResolve,
moderationCaseUpdate,
packageInfo,
packageReserve,
packageVersionInfo,
packageVersionRetraction,
publisherBlock,
Expand Down
61 changes: 61 additions & 0 deletions app/lib/admin/actions/package_reserve.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// 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 'package:pub_dev/package/backend.dart';
import 'package:pub_dev/package/models.dart';
import 'package:pub_dev/shared/datastore.dart';

import 'actions.dart';

final packageReserve = AdminAction(
name: 'package-reserve',
summary: 'Creates a ReservedPackage entity.',
description: '''
Reserves a package name that can be claimed by @google.com accounts or
the allowed list of email addresses.

The action can be re-run with the same package name. In such cases the provided
email list will be added to the existing ReservedPackage entity.
''',
options: {
'package': 'The package name to be reserved.',
'emails': 'The list of email addresses, separated by comma.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an option to delete the entity? Maybe it is not worth it...

Copy link
Member

Choose a reason for hiding this comment

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

We could just add:

  • package-reserve-list
  • package-reserve-delete

Life is easier, if we split them into multiple actions. We can still put them all in the same file if want that.

We could even consider naming them:
package-reservation-create
package-reservation-list
package-reservation-delete

naming is a nit, but it's probably more intuitive to have separate options. And it's probably easier to read the code too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like a --delete flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - probably --delete=true as we didn't implement real flags for actions,

Copy link
Member

Choose a reason for hiding this comment

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

a separate command is MUCH easier to do.

Copy link
Member

Choose a reason for hiding this comment

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

it's really just something like:

final package = options['package'];
InvalidInputException.check(
  package != null && package.isNotEmpty,
  '`package` must be given',
);

await withRetryTransaction(dbService, (tx) async {
  final existing = await tx.lookupOrNull<ReservedPackage>(
      dbService.emptyKey.append(ReservedPackage, id: package));
      
  if (exiting == null) {
    return {'message': 'ReservedPackage for "$package" does not exist!'};
  }
  
  tx.delete(existing.key);
});

return {'message': 'ReservedPackage for "$package" deleted.'};

A --delete=true flag requires a lot more explanation in the documentation, it's not very intuitive.

},
invoke: (options) async {
final package = options['package'];
InvalidInputException.check(
package != null && package.isNotEmpty,
'`package` must be given',
);
final emails = options['emails']?.split(',');

final p = await packageBackend.lookupPackage(package!);
if (p != null) {
throw InvalidInputException('Package `$package` exists.');
}
final mp = await packageBackend.lookupModeratedPackage(package);
if (mp != null) {
throw InvalidInputException('ModeratedPackage `$package` exists.');
}

final entry = await withRetryTransaction(dbService, (tx) async {
final existing = await tx.lookupOrNull<ReservedPackage>(
dbService.emptyKey.append(ReservedPackage, id: package));
final entry = existing ?? ReservedPackage.init(package);
if (emails != null) {
entry.emails = <String>{...entry.emails, ...emails}.toList();
isoos marked this conversation as resolved.
Show resolved Hide resolved
}
tx.insert(entry);
return entry;
});

return {
'ReservedPackage': {
'name': entry.name,
'created': entry.created.toIso8601String(),
'emails': entry.emails,
},
};
},
);
37 changes: 31 additions & 6 deletions app/lib/package/backend.dart
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,14 @@ class PackageBackend {
return await db.lookupOrNull<ModeratedPackage>(packageKey);
}

/// Looks up a reserved package by name.
///
/// Returns `null` if the package doesn't exist.
Future<ReservedPackage?> lookupReservedPackage(String packageName) async {
final packageKey = db.emptyKey.append(ReservedPackage, id: packageName);
return await db.lookupOrNull<ReservedPackage>(packageKey);
}

/// Looks up a package by name.
Future<List<Package>> lookupPackages(Iterable<String> packageNames) async {
return (await db.lookup(packageNames
Expand Down Expand Up @@ -1014,10 +1022,19 @@ class PackageBackend {
required String name,
required AuthenticatedAgent agent,
}) async {
final isGoogleComUser =
agent is AuthenticatedUser && agent.user.email!.endsWith('@google.com');
final isReservedName = matchesReservedPackageName(name);
final isExempted = isGoogleComUser && isReservedName;
final reservedPackage = await lookupReservedPackage(name);
final reservedEmails = reservedPackage?.emails ?? const <String>[];

bool isAllowedUser = false;
if (agent is AuthenticatedUser) {
final email = agent.user.email;
isAllowedUser = email != null &&
(email.endsWith('@google.com') || reservedEmails.contains(email));
isoos marked this conversation as resolved.
Show resolved Hide resolved
}

final isReservedName =
reservedPackage != null || matchesReservedPackageName(name);
final isExempted = isReservedName && isAllowedUser;

final conflictingName = await nameTracker.accept(name);
if (conflictingName != null && !isExempted) {
Expand All @@ -1039,8 +1056,8 @@ class PackageBackend {
throw PackageRejectedException(newNameIssues.first.message);
}

// reserved package names for the Dart team
if (isReservedName && !isGoogleComUser) {
// reserved package names for the Dart team or allowlisted users
if (isReservedName && !isAllowedUser) {
throw PackageRejectedException.nameReserved(name);
}
}
Expand Down Expand Up @@ -1125,6 +1142,14 @@ class PackageBackend {
throw PackageRejectedException.nameReserved(newVersion.package);
}

if (isNew) {
final reservedPackage = await tx.lookupOrNull<ReservedPackage>(
db.emptyKey.append(ReservedPackage, id: newVersion.package));
if (reservedPackage != null) {
tx.delete(reservedPackage.key);
}
}

// If the version already exists, we fail.
if (version != null) {
_logger.info('Version ${version.version} of package '
Expand Down
22 changes: 22 additions & 0 deletions app/lib/package/models.dart
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,28 @@ class ModeratedPackage extends db.ExpandoModel<String> {
List<String>? versions;
}

/// Entity representing a reserved package: the name is available only
/// for a subset of the users (`@google.com` + list of [emails]).
@db.Kind(name: 'ReservedPackage', idType: db.IdType.String)
class ReservedPackage extends db.ExpandoModel<String> {
@db.StringProperty(required: true)
String? name;

@db.DateTimeProperty()
late DateTime created;

/// List of email addresses that are allowed to claim this package name.
/// This is on top of the `@google.com` email addresses.
@db.StringListProperty()
List<String> emails = <String>[];

ReservedPackage();
ReservedPackage.init(this.name) {
id = name;
created = clock.now().toUtc();
}
}

/// An identifier to point to a specific [package] and [version].
class QualifiedVersionKey {
final String? package;
Expand Down
102 changes: 102 additions & 0 deletions app/test/admin/package_reserve_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// 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 'package:_pub_shared/data/admin_api.dart';
import 'package:clock/clock.dart';
import 'package:pub_dev/fake/backend/fake_auth_provider.dart';
import 'package:pub_dev/package/backend.dart';
import 'package:pub_dev/package/models.dart';
import 'package:pub_dev/shared/datastore.dart';
import 'package:test/test.dart';

import '../package/backend_test_utils.dart';
import '../shared/handlers_test_utils.dart';
import '../shared/test_models.dart';
import '../shared/test_services.dart';

void main() {
group('Reserve package', () {
Future<void> _reserve(
String package, {
List<String>? emails,
}) async {
final api = createPubApiClient(authToken: siteAdminToken);
await api.adminInvokeAction(
'package-reserve',
AdminInvokeActionArguments(arguments: {
'package': package,
if (emails != null) 'emails': emails.join(','),
}),
);
}

testWithProfile('cannot reserve existing package', fn: () async {
await expectApiException(
_reserve('oxygen'),
code: 'InvalidInput',
status: 400,
message: 'Package `oxygen` exists.',
);
});

testWithProfile('cannot reserve ModeratedPackage', fn: () async {
await dbService.commit(inserts: [
ModeratedPackage()
..id = 'pkg'
..name = 'pkg'
..moderated = clock.now()
..uploaders = <String>[]
..versions = <String>['1.0.0']
]);
await expectApiException(
_reserve('pkg'),
code: 'InvalidInput',
status: 400,
message: 'ModeratedPackage `pkg` exists.',
);
});

testWithProfile('prevents non-whitelisted publishing', fn: () async {
await _reserve('pkg');

final pubspecContent = generatePubspecYaml('pkg', '1.0.0');
final bytes = await packageArchiveBytes(pubspecContent: pubspecContent);
await expectApiException(
createPubApiClient(authToken: adminClientToken)
.uploadPackageBytes(bytes),
code: 'PackageRejected',
status: 400,
message: 'Package name pkg is reserved.',
);
});

testWithProfile('allows whitelisted publishing', fn: () async {
await _reserve('pkg');
// update email addresses in a second request
await _reserve('pkg', emails: ['[email protected]']);

final pubspecContent = generatePubspecYaml('pkg', '1.0.0');
final bytes = await packageArchiveBytes(pubspecContent: pubspecContent);
await createPubApiClient(authToken: adminClientToken)
.uploadPackageBytes(bytes);

final rp = await packageBackend.lookupReservedPackage('pkg');
expect(rp, isNull);
});

testWithProfile('allows Dart-team publishing', fn: () async {
await _reserve('pkg');

final pubspecContent = generatePubspecYaml('pkg', '1.0.0');
final bytes = await packageArchiveBytes(pubspecContent: pubspecContent);
await createPubApiClient(
authToken: createFakeAuthTokenForEmail('[email protected]',
audience: 'fake-client-audience'))
.uploadPackageBytes(bytes);

final rp = await packageBackend.lookupReservedPackage('pkg');
expect(rp, isNull);
});
});
}
Loading