From 2d01e016c25f12724229c477f346b0499a10075c Mon Sep 17 00:00:00 2001
From: philenius <philenius@users.noreply.github.com>
Date: Tue, 24 Nov 2020 23:33:57 +0100
Subject: [PATCH] improve error handling in bucketExists()

Added call of validate() in bucketExists(). Reason: if the request for
checking whether a bucket exists contains invalid credentials (see unit
test), then S3 does not return an XML response. Instead the HTTP
response body is empty. This in turn caused the validate() function
to throw an XML parsing exception which in turn shadowed the original
error. This behavior is different compared to when we list buckets. In
this case, the response contains an HTTP body with a detailed error
message encoded as XML.
---
 lib/src/minio.dart         | 10 +++++---
 lib/src/minio_helpers.dart | 14 +++++++---
 test/minio_dart_test.dart  | 52 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/lib/src/minio.dart b/lib/src/minio.dart
index b203ff3..a1d1f6e 100644
--- a/lib/src/minio.dart
+++ b/lib/src/minio.dart
@@ -75,10 +75,13 @@ class Minio {
     MinioInvalidBucketNameError.check(bucket);
     try {
       final response = await _client.request(method: 'HEAD', bucket: bucket);
+      validate(response);
       return (response.statusCode == 200 || response.statusCode == 403);
     } on MinioS3Error catch (e) {
       final code = e.error.code;
-      if (code == 'NoSuchBucket' || code == 'NotFound') return false;
+      if (code == 'NoSuchBucket' || code == 'NotFound' || code == 'Not Found') {
+        return false;
+      }
       rethrow;
     } on StateError catch (e) {
       // Insight from testing: in most cases, AWS S3 returns the HTTP status code
@@ -87,11 +90,11 @@ class Minio {
       // status code 404 as officially documented. Then, this redirect response
       // lacks the HTTP header `location` which causes this exception in Dart's
       // HTTP library (`http_impl.dart`).
-      if (e.message == 'Response has no Location header for redirect')
+      if (e.message == 'Response has no Location header for redirect') {
         return false;
+      }
       rethrow;
     }
-    return true;
   }
 
   int _calculatePartSize(int size) {
@@ -456,6 +459,7 @@ class Minio {
       method: 'GET',
       region: region ?? 'us-east-1',
     );
+    validate(resp);
     final bucketsNode =
         xml.XmlDocument.parse(resp.body).findAllElements('Buckets').first;
     return bucketsNode.children.map((n) => Bucket.fromXml(n)).toList();
diff --git a/lib/src/minio_helpers.dart b/lib/src/minio_helpers.dart
index b8d2e38..4cd2fdc 100644
--- a/lib/src/minio_helpers.dart
+++ b/lib/src/minio_helpers.dart
@@ -217,9 +217,17 @@ Future<void> validateStreamed(
 
 void validate(Response response, {int expect}) {
   if (response.statusCode >= 400) {
-    final body = xml.XmlDocument.parse(response.body);
-    final error = Error.fromXml(body.rootElement);
-    throw MinioS3Error(error.message, error, response);
+    var error;
+
+    // Parse HTTP response body as XML only when not empty
+    if (response.body == null || response.body.isEmpty) {
+      error = Error(response.reasonPhrase, null, response.reasonPhrase, null);
+    } else {
+      final body = xml.XmlDocument.parse(response.body);
+      error = Error.fromXml(body.rootElement);
+    }
+
+    throw MinioS3Error(error?.message, error, response);
   }
 
   if (expect != null && response.statusCode != expect) {
diff --git a/test/minio_dart_test.dart b/test/minio_dart_test.dart
index cc066cf..e8626bc 100644
--- a/test/minio_dart_test.dart
+++ b/test/minio_dart_test.dart
@@ -39,6 +39,58 @@ void main() {
       );
     });
   });
+
+  group('bucketExists', () {
+    final bucketName = DateTime.now().millisecondsSinceEpoch.toString();
+
+    setUpAll(() async {
+      final minio = _getClient();
+      await minio.makeBucket(bucketName);
+    });
+
+    tearDownAll(() async {
+      final minio = _getClient();
+      await minio.removeBucket(bucketName);
+    });
+
+    test('bucketExists() returns true for an existing bucket', () async {
+      final minio = _getClient();
+      expect(await minio.bucketExists(bucketName), equals(true));
+    });
+
+    test('bucketExists() returns false for a non-existent bucket', () async {
+      final minio = _getClient();
+      expect(await minio.bucketExists('non-existing-bucket-name'), equals(false));
+    });
+
+    test('bucketExists() fails due to wrong access key', () async {
+      final minio = _getClient(accessKey: 'incorrect-access-key');
+      expect(
+        () async => await minio.bucketExists(bucketName),
+        throwsA(
+          isA<MinioError>().having(
+            (e) => e.message,
+            'message',
+            'Forbidden',
+          ),
+        ),
+      );
+    });
+
+    test('bucketExists() fails due to wrong secret key', () async {
+      final minio = _getClient(secretKey: 'incorrect-secret-key');
+      expect(
+        () async => await minio.bucketExists(bucketName),
+        throwsA(
+          isA<MinioError>().having(
+            (e) => e.message,
+            'message',
+            'Forbidden',
+          ),
+        ),
+      );
+    });
+  });
 }
 
 /// Initializes an instance of [Minio] with per default valid configuration.
-- 
GitLab