From cadb6d6b5833cd3c4285530b2e65a946c3ef6b38 Mon Sep 17 00:00:00 2001
From: Stephen Sawchuk <sawchuk@gmail.com>
Date: Wed, 11 Mar 2015 12:50:38 -0400
Subject: [PATCH] always update predefinedAcl metadata

---
 lib/storage/bucket.js  | 94 ++++++++++++++++++++++++++++--------------
 test/storage/bucket.js | 25 ++++++-----
 2 files changed, 77 insertions(+), 42 deletions(-)

diff --git a/lib/storage/bucket.js b/lib/storage/bucket.js
index 101909765830..1d3462a4a0f0 100644
--- a/lib/storage/bucket.js
+++ b/lib/storage/bucket.js
@@ -479,24 +479,35 @@ Bucket.prototype.makePublic = function(options, callback) {
     options = {};
   }
 
-  // Allow public to read bucket contents while preserving original permissions.
-  this.acl.add({
-    entity: 'allUsers',
-    role: 'READER'
-  }, function(err) {
-    if (err) {
-      callback(err);
-      return;
-    }
+  options = options || {};
+  options.public = true;
+
+  async.parallel([
+    setPredefinedAcl,
+    addAclPermissions,
+    makeFilesPublic
+  ], callback);
 
-    if (options.includeFiles) {
-      options.public = true;
-      self.makeAllFilesPublicPrivate_(options, callback);
+  function setPredefinedAcl(done) {
+    self.setPredefinedAcl_(options, done);
+  }
+
+  function addAclPermissions(done) {
+    // Allow reading bucket contents while preserving original permissions.
+    self.acl.add({
+      entity: 'allUsers',
+      role: 'READER'
+    }, done);
+  }
+
+  function makeFilesPublic(done) {
+    if (!options.includeFiles) {
+      done();
       return;
     }
 
-    callback();
-  });
+    self.makeAllFilesPublicPrivate_(options, done);
+  }
 };
 
 /**
@@ -568,30 +579,23 @@ Bucket.prototype.makePrivate = function(options, callback) {
     options = {};
   }
 
-  var query = {
-    predefinedAcl: 'projectPrivate'
-  };
+  options = options || {};
+  options.private = true;
 
-  // You aren't allowed to set both predefinedAcl & acl properties on a bucket
-  // so acl must explicitly be nullified.
-  var metadata = { acl: null };
+  async.parallel([setPredefinedAcl, makeFilesPrivate], callback);
 
-  this.makeReq_('PATCH', '', query, metadata, function(err, resp) {
-    if (err) {
-      callback(err);
-      return;
-    }
-
-    self.metadata = resp;
+  function setPredefinedAcl(done) {
+    self.setPredefinedAcl_(options, done);
+  }
 
-    if (options.includeFiles) {
-      options.private = true;
-      self.makeAllFilesPublicPrivate_(options, callback);
+  function makeFilesPrivate(done) {
+    if (!options.includeFiles) {
+      done();
       return;
     }
 
-    callback();
-  });
+    self.makeAllFilesPublicPrivate_(options, done);
+  }
 };
 
 /**
@@ -827,6 +831,32 @@ Bucket.prototype.makeAllFilesPublicPrivate_ = function(options, callback) {
   }
 };
 
+Bucket.prototype.setPredefinedAcl_ = function(options, callback) {
+  var self = this;
+
+  var predefinedValue = options.public ? 'publicRead' : 'projectPrivate';
+
+  var query = {
+    predefinedAcl: predefinedValue,
+    predefinedDefaultObjectAcl: predefinedValue
+  };
+
+  // You aren't allowed to set both predefinedAcl & acl properties on a bucket
+  // so acl must explicitly be nullified.
+  var metadata = { acl: null };
+
+  this.makeReq_('PATCH', '', query, metadata, function(err, resp) {
+    if (err) {
+      callback(err);
+      return;
+    }
+
+    self.metadata = resp;
+
+    callback();
+  });
+};
+
 /**
  * Make a new request object from the provided arguments and wrap the callback
  * to intercept non-successful responses.
diff --git a/test/storage/bucket.js b/test/storage/bucket.js
index b691bc859c5d..dcb853e0b8dc 100644
--- a/test/storage/bucket.js
+++ b/test/storage/bucket.js
@@ -440,6 +440,12 @@ describe('Bucket', function() {
   });
 
   describe('makePublic', function() {
+    beforeEach(function() {
+      bucket.makeReq_ = function(method, path, query, body, callback) {
+        callback();
+      };
+    });
+
     it('should add correct entity and role to the ACL', function(done) {
       bucket.acl.add = function(opts) {
         assert.equal(opts.entity, 'allUsers');
@@ -484,21 +490,19 @@ describe('Bucket', function() {
         bucket.makePublic({ includeFiles: true }, assert.ifError);
       });
 
-      it('should pass correct options and callback', function(done) {
+      it('should pass correct options', function(done) {
         bucket.acl.add = function (opts, callback) {
           callback();
         };
 
-        bucket.makeAllFilesPublicPrivate_ = function(opts, callback) {
+        bucket.makeAllFilesPublicPrivate_ = function(opts) {
           assert.strictEqual(opts.public, true);
           assert.strictEqual(opts.force, true);
 
-          assert.equal(callback, done);
-
           done();
         };
 
-        bucket.makePublic({ includeFiles: true, force: true }, done);
+        bucket.makePublic({ includeFiles: true, force: true }, assert.ifError);
       });
     });
   });
@@ -508,7 +512,10 @@ describe('Bucket', function() {
       bucket.makeReq_ = function(method, path, query, body) {
         assert.equal(method, 'PATCH');
         assert.equal(path, '');
-        assert.deepEqual(query, { predefinedAcl: 'projectPrivate' });
+        assert.deepEqual(query, {
+          predefinedAcl: 'projectPrivate',
+          predefinedDefaultObjectAcl: 'projectPrivate'
+        });
         assert.deepEqual(body, { acl: null });
         done();
       };
@@ -566,17 +573,15 @@ describe('Bucket', function() {
         bucket.makePrivate({ includeFiles: true }, assert.ifError);
       });
 
-      it('should pass correct options and callback', function(done) {
+      it('should pass correct options', function(done) {
         bucket.makeReq_ = function(method, path, query, body, callback) {
           callback();
         };
 
-        bucket.makeAllFilesPublicPrivate_ = function(opts, callback) {
+        bucket.makeAllFilesPublicPrivate_ = function(opts) {
           assert.strictEqual(opts.private, true);
           assert.strictEqual(opts.force, true);
 
-          assert.equal(callback, done);
-
           done();
         };