From 25a6b06975ff8cb709a638081f71be38a3cc55f5 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Tue, 2 Sep 2014 18:34:09 -0400 Subject: [PATCH] storage: fix credentials issue. Previously, if a user wanted to generate a signed URL, there was no guarantee a credentials object would have already been generated. By itself, generating a signed URL doesn't require a network connection, thus the uncertainty that we would have already fetched a token. This change requires a valid credentials object to exist before returning the `credentials` object. --- lib/common/connection.js | 24 ++++++++++++- lib/storage/index.js | 41 +++++++++++++--------- regression/storage.js | 58 +++++++++++++++++-------------- test/common/connection.js | 29 ++++++++++++++++ test/storage/index.js | 15 +++++++- test/testdata/privateKeyFile.json | 2 +- 6 files changed, 123 insertions(+), 46 deletions(-) diff --git a/lib/common/connection.js b/lib/common/connection.js index 8bf5aaa4a228..41470241cbae 100644 --- a/lib/common/connection.js +++ b/lib/common/connection.js @@ -95,9 +95,10 @@ module.exports.Token = Token; function Connection(opts) { events.EventEmitter.call(this); - this.opts = opts || {}; + opts = opts || {}; this.credentials = null; + this.opts = opts; this.scopes = opts.scopes || []; this.token = null; // existing access token, if exists @@ -308,4 +309,25 @@ Connection.prototype.authorizeReq = function(requestOptions) { return requestOptions; }; +/** + * Get the connection's credentials. If a token hasn't been fetched yet, one + * will be triggered now. + * + * @return {object} + */ +Connection.prototype.getCredentials = function(callback) { + var that = this; + if (this.credentials) { + setImmediate(callback, null, that.credentials); + return; + } + this.fetchToken(function(err) { + if (err) { + callback(err); + return; + } + callback(null, that.credentials); + }); +}; + module.exports.Connection = Connection; diff --git a/lib/storage/index.js b/lib/storage/index.js index 28fc21a3922d..b40342e4d14d 100644 --- a/lib/storage/index.js +++ b/lib/storage/index.js @@ -263,10 +263,10 @@ Bucket.prototype.remove = function(name, callback) { * action: 'read', * expires: Math.round(Date.now() / 1000) + (60 * 60 * 24 * 14), // 2 weeks. * resource: 'my-dog.jpg' - * }); + * }, function(err, url) {}); * ``` */ -Bucket.prototype.getSignedUrl = function(options) { +Bucket.prototype.getSignedUrl = function(options, callback) { options.action = { read: 'GET', write: 'PUT', @@ -278,22 +278,29 @@ Bucket.prototype.getSignedUrl = function(options) { resource: options.resource }); - var sign = crypto.createSign('RSA-SHA256'); - sign.update([ - options.action, - (options.contentMd5 || ''), - (options.contentType || ''), - options.expires, - (options.extensionHeaders || '') + options.resource - ].join('\n')); - var signature = sign.sign(this.conn.credentials.private_key, 'base64'); + this.conn.getCredentials(function(err, credentials) { + if (err) { + callback(err); + return; + } + + var sign = crypto.createSign('RSA-SHA256'); + sign.update([ + options.action, + (options.contentMd5 || ''), + (options.contentType || ''), + options.expires, + (options.extensionHeaders || '') + options.resource + ].join('\n')); + var signature = sign.sign(credentials.private_key, 'base64'); - return [ - 'http://storage.googleapis.com' + options.resource, - '?GoogleAccessId=' + this.conn.credentials.client_email, - '&Expires=' + options.expires, - '&Signature=' + encodeURIComponent(signature) - ].join(''); + callback(null, [ + 'http://storage.googleapis.com' + options.resource, + '?GoogleAccessId=' + credentials.client_email, + '&Expires=' + options.expires, + '&Signature=' + encodeURIComponent(signature) + ].join('')); + }); }; /** diff --git a/regression/storage.js b/regression/storage.js index 03e41830981d..6789f2d666a5 100644 --- a/regression/storage.js +++ b/regression/storage.js @@ -185,6 +185,7 @@ describe('storage', function() { describe('sign urls', function() { var filename = 'LogoToSign.jpg'; + var localFile = fs.readFileSync(files.logo.path); beforeEach(function(done) { fs.createReadStream(files.logo.path) @@ -194,49 +195,54 @@ describe('storage', function() { }); it('should create a signed read url', function(done) { - var signedReadUrl = bucket.getSignedUrl({ + bucket.getSignedUrl({ action: 'read', expires: Math.round(Date.now() / 1000) + 5, resource: filename + }, function(err, signedReadUrl) { + assert.ifError(err); + request.get(signedReadUrl, function(err, resp, body) { + assert.equal(body, localFile); + bucket.remove(filename, done); + }); }); - var localFile = fs.readFileSync(files.logo.path); - request.get(signedReadUrl, function(err, resp, body) { - assert.equal(body, localFile); - bucket.remove(filename, done); - }); }); it('should create a signed delete url', function(done) { - var signedDeleteUrl = bucket.getSignedUrl({ + bucket.getSignedUrl({ action: 'delete', expires: Math.round(Date.now() / 1000) + 5, resource: filename + }, function(err, signedDeleteUrl) { + assert.ifError(err); + request.del(signedDeleteUrl, function(err, resp) { + assert.equal(resp.statusCode, 204); + bucket.stat(filename, function(err) { + assert.equal(err.code, 404); + done(); + }); + }); }); - request.del(signedDeleteUrl, function(err, resp) { - assert.equal(resp.statusCode, 204); - bucket.stat(filename, function(err) { - assert.equal(err.code, 404); - done(); - }); - }); }); it('should allow control of expiration', function(done) { - var OFFSET_SECONDS = 5; - var signedReadUrl = bucket.getSignedUrl({ + var offsetSeconds = 5; + bucket.getSignedUrl({ action: 'read', - expires: Math.round(Date.now() / 1000) + OFFSET_SECONDS, + expires: Math.round(Date.now() / 1000) + offsetSeconds, resource: filename - }); - request.get(signedReadUrl, function(err, resp, body) { - assert.equal(body, fs.readFileSync(files.logo.path)); - setTimeout(function() { - request.get(signedReadUrl, function(err, resp) { - assert.equal(resp.statusCode, 400); - bucket.remove(filename, done); + }, function(err, signedReadUrl) { + assert.ifError(err); + request.get(signedReadUrl, function(err, resp, body) { + assert.equal(body, localFile); }); - }, (OFFSET_SECONDS + 5) * 1000); - }); + setTimeout(function() { + request.get(signedReadUrl, function(err, resp) { + assert.equal(resp.statusCode, 400); + bucket.remove(filename, done); + }); + }, (offsetSeconds + 1) * 1000); + }); }); }); }); diff --git a/test/common/connection.js b/test/common/connection.js index 388a3304bdcc..97740ff8bc8b 100644 --- a/test/common/connection.js +++ b/test/common/connection.js @@ -49,6 +49,35 @@ describe('Connection', function() { }); describe('credentials object', function() { + describe('getCredentials', function() { + it('should expose a method to retrieve credentials', function() { + var conn = new connection.Connection(); + assert.equal(typeof conn.getCredentials, 'function'); + }); + + it('should return credentials object if provided', function(done) { + var conn = new connection.Connection({ + credentials: privateKeyFileJson + }); + conn.getCredentials(function(err, credentials) { + assert.deepEqual(credentials, privateKeyFileJson); + done(); + }); + }); + + it('should fetch token if missing credentials object', function(done) { + var conn = new connection.Connection(); + conn.fetchToken = function(cb) { + conn.credentials = privateKeyFileJson; + cb(); + }; + conn.getCredentials(function(err, credentials) { + assert.deepEqual(credentials, privateKeyFileJson); + done(); + }); + }); + }); + it('should accept and assign a complete credentials object', function() { var credConnection = new connection.Connection({ credentials: privateKeyFileJson diff --git a/test/storage/index.js b/test/storage/index.js index 91701470039b..d09b14820661 100644 --- a/test/storage/index.js +++ b/test/storage/index.js @@ -27,7 +27,7 @@ function createBucket() { return new storage.Bucket({ bucketName: 'bucket-name', email: 'xxx@email.com', - pemFilePath: '/some/path' + credentials: require('../testdata/privateKeyFile.json') }); } @@ -130,4 +130,17 @@ describe('Bucket', function() { }; bucket.remove('file-name'); }); + + it('should create a signed url', function(done) { + var bucket = createBucket(); + bucket.getSignedUrl({ + action: 'read', + resource: 'filename', + expires: Date.now() / 1000 + }, function(err, url) { + assert.ifError(err); + assert.equal(typeof url, 'string'); + done(); + }); + }); }); diff --git a/test/testdata/privateKeyFile.json b/test/testdata/privateKeyFile.json index c8f4f80e71f8..3a71d60b4089 100644 --- a/test/testdata/privateKeyFile.json +++ b/test/testdata/privateKeyFile.json @@ -1,6 +1,6 @@ { "private_key_id": "7", - "private_key": "-----BEGIN PRIVATE KEY-----\n\n-----END PRIVATE KEY-----\n", + "private_key": "-----BEGIN RSA PRIVATE KEY-----\nMIIBOgIBAAJBAK8Q+ToR4tWGshaKYRHKJ3ZmMUF6jjwCS/u1A8v1tFbQiVpBlxYB\npaNcT2ENEXBGdmWqr8VwSl0NBIKyq4p0rhsCAQMCQHS1+3wL7I5ZzA8G62Exb6RE\nINZRtCgBh/0jV91OeDnfQUc07SE6vs31J8m7qw/rxeB3E9h6oGi9IVRebVO+9zsC\nIQDWb//KAzrSOo0P0yktnY57UF9Q3Y26rulWI6LqpsxZDwIhAND/cmlg7rUz34Pf\nSmM61lJEmMEjKp8RB/xgghzmCeI1AiEAjvVVMVd8jCcItTdwyRO0UjWU4JOz0cnw\n5BfB8cSIO18CIQCLVPbw60nOIpUClNxCJzmMLbsrbMcUtgVS6wFomVvsIwIhAK+A\nYqT6WwsMW2On5l9di+RPzhDT1QdGyTI5eFNS+GxY\n-----END RSA PRIVATE KEY-----", "client_email": "firstpart@secondpart.com", "client_id": "8", "type": "service_account"