From d6ca74ccf75b717d1c0517c596253712aed2efa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Istv=C3=A1n=20So=C3=B3s?= Date: Fri, 9 Aug 2019 17:33:25 +0200 Subject: [PATCH] Updated uploader checks. (#2642) * Updated uploader checks. * OperationForbiddenException * AccountPkgOptions.isAdmin * 403 * isPackageAdmin with fixed userId parameter. --- app/bin/tools/uploader.dart | 4 +- app/lib/frontend/backend.dart | 73 ++++++++++++++++---------- app/lib/frontend/handlers/account.dart | 16 +++--- app/lib/frontend/models.dart | 29 ++++++---- app/lib/shared/exceptions.dart | 14 +++++ app/test/frontend/backend_test.dart | 14 ++--- pkg/client_data/lib/account_api.dart | 4 +- pkg/client_data/lib/account_api.g.dart | 4 +- pkg/web_app/lib/src/account.dart | 2 +- 9 files changed, 102 insertions(+), 58 deletions(-) diff --git a/app/bin/tools/uploader.dart b/app/bin/tools/uploader.dart index 51ed45bba3..696cd00f51 100644 --- a/app/bin/tools/uploader.dart +++ b/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) { diff --git a/app/lib/frontend/backend.dart b/app/lib/frontend/backend.dart index 5d9edb9342..ae4b9b061d 100644 --- a/app/lib/frontend/backend.dart +++ b/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, {}); }); } + + /// Whether [userId] is a package admin (through direct uploaders list or + /// publisher admin). + /// + /// Returns false if the user is not an admin. + Future 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([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,7 +678,7 @@ 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( @@ -657,7 +686,7 @@ class GCloudPackageRepository extends PackageRepository { } 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.'); diff --git a/app/lib/frontend/handlers/account.dart b/app/lib/frontend/handlers/account.dart index 85f61bd32a..23fc1ea259 100644 --- a/app/lib/frontend/handlers/account.dart +++ b/app/lib/frontend/handlers/account.dart @@ -25,11 +25,13 @@ Future putAccountConsentHandler( /// Handles /api/account/options/packages/ Future 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()); + }); } diff --git a/app/lib/frontend/models.dart b/app/lib/frontend/models.dart index d634903ea7..d4d8b4552b 100644 --- a/app/lib/frontend/models.dart +++ b/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. diff --git a/app/lib/shared/exceptions.dart b/app/lib/shared/exceptions.dart index 6cfa918734..9342086dac 100644 --- a/app/lib/shared/exceptions.dart +++ b/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 { diff --git a/app/test/frontend/backend_test.dart b/app/test/frontend/backend_test.dart index 8e32bf4092..2ea6083b41 100644 --- a/app/test/frontend/backend_test.dart +++ b/app/test/frontend/backend_test.dart @@ -171,7 +171,7 @@ void main() { testWithServices('not logged in', () async { final pkg = foobarPackage.name; final rs = backend.repository.addUploader(pkg, 'a@b.com'); - expectLater(rs, throwsA(isA())); + await expectLater(rs, throwsA(isA())); }); testWithServices('not authorized', () async { @@ -179,13 +179,13 @@ void main() { registerAuthenticatedUser( AuthenticatedUser('uuid-foo-at-bar-dot-com', 'foo@bar.com')); final rs = backend.repository.addUploader(pkg, 'a@b.com'); - expectLater(rs, throwsA(isA())); + await expectLater(rs, throwsA(isA())); }); testWithServices('package does not exist', () async { registerAuthenticatedUser(hansAuthenticated); final rs = backend.repository.addUploader('no_package', 'a@b.com'); - expectLater(rs, throwsA(isA())); + await expectLater(rs, throwsA(isA())); }); 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())); + await expectLater(rs, throwsA(isA())); }); testWithServices('not authorized', () async { @@ -257,14 +257,14 @@ void main() { registerAuthenticatedUser(hansAuthenticated); final rs = backend.repository.removeUploader('hydrogen', hansUser.email); - expectLater(rs, throwsA(isA())); + await expectLater(rs, throwsA(isA())); }); testWithServices('package does not exist', () async { registerAuthenticatedUser(hansAuthenticated); final rs = backend.repository.removeUploader('non_hydrogen', hansUser.email); - expectLater(rs, throwsA(isA())); + await expectLater(rs, throwsA(isA())); }); 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())); + await expectLater(rs, throwsA(isA())); }); testWithServices('successful', () async { diff --git a/pkg/client_data/lib/account_api.dart b/pkg/client_data/lib/account_api.dart index 4f3caa6f57..4653215fd9 100644 --- a/pkg/client_data/lib/account_api.dart +++ b/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 json) => diff --git a/pkg/client_data/lib/account_api.g.dart b/pkg/client_data/lib/account_api.g.dart index 5511ea55d9..5805d5fc20 100644 --- a/pkg/client_data/lib/account_api.g.dart +++ b/pkg/client_data/lib/account_api.g.dart @@ -8,13 +8,13 @@ part of 'account_api.dart'; AccountPkgOptions _$AccountPkgOptionsFromJson(Map json) { return AccountPkgOptions( - isUploader: json['isUploader'] as bool, + isAdmin: json['isAdmin'] as bool, ); } Map _$AccountPkgOptionsToJson(AccountPkgOptions instance) => { - 'isUploader': instance.isUploader, + 'isAdmin': instance.isAdmin, }; Consent _$ConsentFromJson(Map json) { diff --git a/pkg/web_app/lib/src/account.dart b/pkg/web_app/lib/src/account.dart index 1c1ec2157b..10fbc83dac 100644 --- a/pkg/web_app/lib/src/account.dart +++ b/pkg/web_app/lib/src/account.dart @@ -87,7 +87,7 @@ Future _updateOnCredChange() async { .get('/api/account/options/packages/${pageData.pkgData.package}'); final map = json.decode(rs.body) as Map; final options = AccountPkgOptions.fromJson(map); - _isPkgUploader = options.isUploader ?? false; + _isPkgUploader = options.isAdmin ?? false; _updateUi(); } } catch (e) {