Updated uploader checks. (#2642)

* Updated uploader checks.

* OperationForbiddenException

* AccountPkgOptions.isAdmin

* 403

* isPackageAdmin with fixed userId parameter.

Author: isoos

app/bin/tools/uploader.dart

@@ -72,7 +72,7 @@ Future addUploader(String packageName, String uploaderEmail) async {
         await accountBackend.getEmailsOfUserIds(package.uploaders);
     print('Current uploaders: $uploaderEmails');
     final user = await accountBackend.lookupOrCreateUserByEmail(uploaderEmail);
-    if (package.hasUploader(user.userId)) {
+    if (package.containsUploader(user.userId)) {
       throw Exception('Uploader $uploaderEmail already exists');
     }
     package.addUploader(user.userId);
@@ -104,7 +104,7 @@ Future removeUploader(String packageName, String uploaderEmail) async {
         await accountBackend.getEmailsOfUserIds(package.uploaders);
     print('Current uploaders: $uploaderEmails');
     final user = await accountBackend.lookupOrCreateUserByEmail(uploaderEmail);
-    if (!package.hasUploader(user.userId)) {
+    if (!package.containsUploader(user.userId)) {
       throw Exception('Uploader $uploaderEmail does not exist');
     }
     if (package.uploaderCount <= 1) {

app/lib/frontend/backend.dart

@@ -23,6 +23,7 @@ import 'package:uuid/uuid.dart';
 import '../account/backend.dart';
 import '../history/backend.dart';
 import '../history/models.dart';
+import '../publisher/models.dart';
 import '../shared/analyzer_client.dart';
 import '../shared/configuration.dart';
 import '../shared/dartdoc_client.dart';
@@ -283,9 +284,7 @@ class Backend {
           throw NotFoundException('Package $package does not exists.');
         }
         latestVersion = p.latestVersion;
-        if (!p.hasUploader(user.userId)) {
-          throw AuthorizationException.userIsNotAdminForPackage(package);
-        }
+        await checkPackageAdmin(p, user.userId);
         p.isDiscontinued = options.isDiscontinued ?? p.isDiscontinued;
         _logger.info('Updating $package options: '
             'isDiscontinued: ${p.isDiscontinued} '
@@ -297,6 +296,40 @@ class Backend {
       await analyzerClient.triggerAnalysis(package, latestVersion, <String>{});
     });
   }
+
+  /// Whether [userId] is a package admin (through direct uploaders list or
+  /// publisher admin).
+  ///
+  /// Returns false if the user is not an admin.
+  Future<bool> isPackageAdmin(models.Package p, String userId) async {
+    if (userId == null) {
+      return false;
+    }
+    if (p.publisherId == null) {
+      return p.containsUploader(userId);
+    } else {
+      final memberKey = db.emptyKey
+          .append(Publisher, id: p.publisherId)
+          .append(PublisherMember, id: userId);
+      final list = await db.lookup<PublisherMember>([memberKey]);
+      final member = list.single;
+      return member?.role == PublisherMemberRole.admin;
+    }
+  }
+
+  /// Whether the [userId] is a package admin (through direct uploaders list or
+  /// publisher admin).
+  ///
+  /// Throws AuthenticationException if the user is provided.
+  /// Throws AuthorizationException if the user is not an admin for the package.
+  Future checkPackageAdmin(models.Package package, String userId) async {
+    if (userId == null) {
+      throw AuthenticationException.authenticationRequired();
+    }
+    if (!await isPackageAdmin(package, userId)) {
+      throw AuthorizationException.userIsNotAdminForPackage(package.name);
+    }
+  }
 }
 
 /// Invalidate [cache] entries for given [package].
@@ -503,11 +536,7 @@ class GCloudPackageRepository extends PackageRepository {
       if (package == null) {
         _logger.info('New package uploaded. [new-package-uploaded]');
         package = _newPackageFromVersion(db, newVersion);
-      }
-
-      // Check if the uploader of the new version is allowed to upload to
-      // the package.
-      if (!package.hasUploader(user.userId)) {
+      } else if (!await backend.isPackageAdmin(package, user.userId)) {
         _logger.info('User ${user.userId} (${user.email}) is not an uploader '
             'for package ${package.name}, rolling transaction back.');
         await T.rollback();
@@ -649,15 +678,15 @@ class GCloudPackageRepository extends PackageRepository {
       final packageKey = db.emptyKey.append(models.Package, id: packageName);
       final package = (await db.lookup([packageKey])).first as models.Package;
 
-      _validatePackageUploader(packageName, package, user.userId);
+      await _validatePackageUploader(packageName, package, user.userId);
 
       if (!isValidEmail(uploaderEmail)) {
         throw GenericProcessingException(
             'Not a valid e-mail: `$uploaderEmail`.');
       }
 
       final uploader = await accountBackend.lookupUserByEmail(uploaderEmail);
-      if (uploader != null && package.hasUploader(uploader.userId)) {
+      if (uploader != null && package.containsUploader(uploader.userId)) {
         // The requested uploaderEmail is already part of the uploaders.
         return;
       }
@@ -709,13 +738,13 @@ class GCloudPackageRepository extends PackageRepository {
       final package = (await tx.lookup([packageKey])).first as models.Package;
 
       try {
-        _validatePackageUploader(packageName, package, fromUserId);
+        await _validatePackageUploader(packageName, package, fromUserId);
       } catch (_) {
         await tx.rollback();
         rethrow;
       }
 
-      if (package.hasUploader(uploader.userId)) {
+      if (package.containsUploader(uploader.userId)) {
         // The requested uploaderEmail is already part of the uploaders.
         await tx.rollback();
         return;
@@ -742,15 +771,15 @@ class GCloudPackageRepository extends PackageRepository {
     });
   }
 
-  void _validatePackageUploader(
-      String packageName, models.Package package, String userId) {
+  Future _validatePackageUploader(
+      String packageName, models.Package package, String userId) async {
     // Fail if package doesn't exist.
     if (package == null) {
       throw NotFoundException.resource(packageName);
     }
 
     // Fail if calling user doesn't have permission to change uploaders.
-    if (!package.hasUploader(userId)) {
+    if (!await backend.isPackageAdmin(package, userId)) {
       throw AuthorizationException.userCannotChangeUploaders(package.name);
     }
   }
@@ -763,22 +792,12 @@ class GCloudPackageRepository extends PackageRepository {
         final packageKey = db.emptyKey.append(models.Package, id: packageName);
         final package = (await T.lookup([packageKey])).first as models.Package;
 
-        // Fail if package doesn't exist.
-        if (package == null) {
-          await T.rollback();
-          throw NotFoundException.resource(packageName);
-        }
-
-        // Fail if calling user doesn't have permission to change uploaders.
-        if (!package.hasUploader(user.userId)) {
-          await T.rollback();
-          throw AuthorizationException.userCannotChangeUploaders(package.name);
-        }
+        await _validatePackageUploader(packageName, package, user.userId);
 
         final uploader =
             await accountBackend.lookupOrCreateUserByEmail(uploaderEmail);
         // Fail if the uploader we want to remove does not exist.
-        if (!package.hasUploader(uploader.userId)) {
+        if (!package.containsUploader(uploader.userId)) {
           await T.rollback();
           throw GenericProcessingException(
               'The uploader to remove does not exist.');

app/lib/frontend/handlers/account.dart

@@ -25,11 +25,13 @@ Future<shelf.Response> putAccountConsentHandler(
 /// Handles /api/account/options/packages/<package>
 Future<shelf.Response> accountPkgOptionsHandler(
     shelf.Request request, String package) async {
-  final p = await backend.lookupPackage(package);
-  if (p == null) {
-    return notFoundHandler(request);
-  }
-  final options =
-      AccountPkgOptions(isUploader: p.hasUploader(authenticatedUser.userId));
-  return jsonResponse(options.toJson());
+  return await withAuthenticatedUser((user) async {
+    final p = await backend.lookupPackage(package);
+    if (p == null) {
+      return notFoundHandler(request);
+    }
+    final options = AccountPkgOptions(
+        isAdmin: await backend.isPackageAdmin(p, user.userId));
+    return jsonResponse(options.toJson());
+  });
 }

app/lib/frontend/models.dart

@@ -11,6 +11,7 @@ import 'package:meta/meta.dart';
 import 'package:pub_semver/pub_semver.dart';
 
 import '../scorecard/models.dart';
+import '../shared/exceptions.dart';
 import '../shared/model_properties.dart';
 import '../shared/search_service.dart' show ApiPageRef;
 import '../shared/urls.dart' as urls;
@@ -80,23 +81,31 @@ class Package extends db.ExpandoModel {
     return shortDateFormat.format(updated);
   }
 
-  // Check if a user is an uploader for a package.
-  bool hasUploader(String uploaderId) {
-    return uploaderId != null && uploaders.contains(uploaderId);
+  // Check if a [userId] is in the list of [uploaders].
+  bool containsUploader(String userId) {
+    return userId != null && uploaders.contains(userId);
   }
 
   int get uploaderCount => uploaders.length;
 
-  /// Add the id to the list of uploaders.
-  void addUploader(String uploaderId) {
-    if (uploaderId != null && !uploaders.contains(uploaderId)) {
-      uploaders.add(uploaderId);
+  /// Add the [userId] to the list of [uploaders].
+  void addUploader(String userId) {
+    if (publisherId != null) {
+      throw OperationForbiddenException.publisherOwnedPackageNoUploader(
+          name, publisherId);
+    }
+    if (userId != null && !uploaders.contains(userId)) {
+      uploaders.add(userId);
     }
   }
 
-  // Remove the id from the list of uploaders.
-  void removeUploader(String uploaderId) {
-    uploaders.removeWhere((s) => s == uploaderId);
+  // Remove the [userId] from the list of [uploaders].
+  void removeUploader(String userId) {
+    if (publisherId != null) {
+      throw OperationForbiddenException.publisherOwnedPackageNoUploader(
+          name, publisherId);
+    }
+    uploaders.remove(userId);
   }
 
   /// Updates latest stable and dev version keys with the new version.

app/lib/shared/exceptions.dart

@@ -150,6 +150,20 @@ class PackageRejectedException extends ResponseException
             400, 'PackageRejected', 'Package archive exceeded $limit bytes.');
 }
 
+/// Thrown when the operation is rejected because of the internal state of a resource.
+class OperationForbiddenException extends ResponseException
+    implements GenericProcessingException {
+  /// The operation tried to update the list of uploaders, but it can't be done
+  /// while the package is owned by a publisher.
+  OperationForbiddenException.publisherOwnedPackageNoUploader(
+      String packageName, String publisherId)
+      : super._(
+            403,
+            'OperationForbidden',
+            'Package "$packageName" is owned by publisher "$publisherId". '
+                'Updating the uploaders is not permitted.');
+}
+
 /// Thrown when authentication failed, credentials is missing or invalid.
 class AuthenticationException extends ResponseException
     implements UnauthorizedAccessException {

app/test/frontend/backend_test.dart

@@ -171,21 +171,21 @@ void main() {
       testWithServices('not logged in', () async {
         final pkg = foobarPackage.name;
         final rs = backend.repository.addUploader(pkg, '[email protected]');
-        expectLater(rs, throwsA(isA<AuthenticationException>()));
+        await expectLater(rs, throwsA(isA<AuthenticationException>()));
       });
 
       testWithServices('not authorized', () async {
         final pkg = foobarPackage.name;
         registerAuthenticatedUser(
             AuthenticatedUser('uuid-foo-at-bar-dot-com', '[email protected]'));
         final rs = backend.repository.addUploader(pkg, '[email protected]');
-        expectLater(rs, throwsA(isA<AuthorizationException>()));
+        await expectLater(rs, throwsA(isA<AuthorizationException>()));
       });
 
       testWithServices('package does not exist', () async {
         registerAuthenticatedUser(hansAuthenticated);
         final rs = backend.repository.addUploader('no_package', '[email protected]');
-        expectLater(rs, throwsA(isA<NotFoundException>()));
+        await expectLater(rs, throwsA(isA<NotFoundException>()));
       });
 
       Future testAlreadyExists(
@@ -243,7 +243,7 @@ void main() {
       testWithServices('not logged in', () async {
         final rs =
             backend.repository.removeUploader('hydrogen', hansUser.email);
-        expectLater(rs, throwsA(isA<AuthenticationException>()));
+        await expectLater(rs, throwsA(isA<AuthenticationException>()));
       });
 
       testWithServices('not authorized', () async {
@@ -257,14 +257,14 @@ void main() {
         registerAuthenticatedUser(hansAuthenticated);
         final rs =
             backend.repository.removeUploader('hydrogen', hansUser.email);
-        expectLater(rs, throwsA(isA<AuthorizationException>()));
+        await expectLater(rs, throwsA(isA<AuthorizationException>()));
       });
 
       testWithServices('package does not exist', () async {
         registerAuthenticatedUser(hansAuthenticated);
         final rs =
             backend.repository.removeUploader('non_hydrogen', hansUser.email);
-        expectLater(rs, throwsA(isA<NotFoundException>()));
+        await expectLater(rs, throwsA(isA<NotFoundException>()));
       });
 
       testWithServices('cannot remove last uploader', () async {
@@ -368,7 +368,7 @@ void main() {
         testWithServices('no active user', () async {
           final rs = backend.repository
               .startAsyncUpload(Uri.parse('http://example.com/'));
-          expectLater(rs, throwsA(isA<AuthenticationException>()));
+          await expectLater(rs, throwsA(isA<AuthenticationException>()));
         });
 
         testWithServices('successful', () async {

pkg/client_data/lib/account_api.dart

@@ -10,10 +10,10 @@ part 'account_api.g.dart';
 /// Account-specific information about a package.
 @JsonSerializable()
 class AccountPkgOptions {
-  final bool isUploader;
+  final bool isAdmin;
 
   AccountPkgOptions({
-    @required this.isUploader,
+    @required this.isAdmin,
   });
 
   factory AccountPkgOptions.fromJson(Map<String, dynamic> json) =>

pkg/client_data/lib/account_api.g.dart

@@ -87,7 +87,7 @@ Future _updateOnCredChange() async {
             .get('/api/account/options/packages/${pageData.pkgData.package}');
         final map = json.decode(rs.body) as Map<String, dynamic>;
         final options = AccountPkgOptions.fromJson(map);
-        _isPkgUploader = options.isUploader ?? false;
+        _isPkgUploader = options.isAdmin ?? false;
         _updateUi();
       }
     } catch (e) {