Add 97% test coverage on DropBox connector

* Adds UT on Dropbox
* Fixes error handling
* Uses /create_folder_v2 and /dowload_v2
* Retrieve account when providing only the token
* Fixes batch upload (Closes ##114)
* Add some security check
* Normalize errors (as for #103)

.gitignore

@@ -4,3 +4,4 @@ npm-debug.log
 coverage
 *.sw*
 .nyc_output
+.vscode

lib/unifile-dropbox.js

@@ -1,10 +1,12 @@
 'use strict';
 
+const {PassThrough} = require('stream');
 const Promise = require('bluebird');
 const request = require('request');
 const Mime = require('mime');
 
 const Tools = require('unifile-common-tools');
+const {UnifileError, BatchError} = require('./error');
 
 const NAME = 'dropbox';
 const DB_OAUTH_URL = 'https://www.dropbox.com/oauth2';
@@ -20,10 +22,8 @@ const DB_OAUTH_URL = 'https://www.dropbox.com/oauth2';
  * @return {Promise} a Promise of the result send by server
  * @private
  */
-function callAPI(session, path, data, subdomain = 'api', isJson = true, headers = {}) {
-	let authorization;
-	if(session.basic) authorization = 'Basic ' + session.basic;
-	else authorization = 'Bearer ' + session.token;
+function callAPI(session, path, data, subdomain = 'api', isJson = true, headers = null) {
+	const authorization = 'Bearer ' + session.token;
 
 	const reqOptions = {
 		url: `https://${subdomain}.dropboxapi.com/2${path}`,
@@ -36,7 +36,7 @@ function callAPI(session, path, data, subdomain = 'api', isJson = true, headers
 		encoding: null
 	};
 
-	if(Object.keys(data).length !== 0) reqOptions.body = data;
+	if(data && Object.keys(data).length !== 0) reqOptions.body = data;
 
 	if(headers) {
 		for(const header in headers) {
@@ -47,19 +47,33 @@ function callAPI(session, path, data, subdomain = 'api', isJson = true, headers
 	return new Promise(function(resolve, reject) {
 		request(reqOptions, function(err, res, body) {
 			if(err) {
-				return reject(err);
-			}
-			if(res.statusCode >= 400) {
-				return reject({statusCode: res.statusCode, message: (() => {
-					console.error('Error in unifile', body.toString());
+				reject(err);
+			} else if(res.statusCode >= 400) {
+				let errorMessage = null;
+				// In case of users/get_current_account, Dropbox return a String with the error
+				// Since isJson = true, it gets parsed by request
+				if(Buffer.isBuffer(body)) {
 					try {
-						return JSON.parse(body.toString()).error_summary;
+						errorMessage = JSON.parse(body.toString()).error_summary;
 					} catch (e) {
-						return 'Unknown error from Dropbox.';
+						errorMessage = body.toString();
 					}
-				})()});
-			}
-			resolve(body);
+				} else {
+					errorMessage = (isJson ? body : JSON.parse(body)).error_summary;
+				}
+				// Dropbox only uses 409 for endpoints specific errors
+				if(errorMessage.includes('/not_found/')) {
+					reject(new UnifileError(UnifileError.ENOENT, 'Not Found'));
+				} else if(errorMessage.startsWith('path/conflict/')) {
+					reject(new UnifileError(UnifileError.EINVAL, 'Creation failed due to conflict'));
+				} else if(errorMessage.startsWith('path/not_file/')) {
+					reject(new UnifileError(UnifileError.EINVAL, 'Path is a directory'));
+				} else if(res.statusCode === 401) {
+					reject(new UnifileError(UnifileError.EACCES, errorMessage));
+				} else {
+					reject(new UnifileError(UnifileError.EIO, errorMessage));
+				}
+			} else resolve(body);
 		});
 	});
 }
@@ -132,8 +146,8 @@ class DropboxConnector {
 	 * @param {string} config.redirectUri - Dropbox application redirect URI
 	 * @param {string} config.clientId - Dropbox application client ID
 	 * @param {string} config.clientSecret - Dropbox application client secret
-	 * @param {string} [config.writeMode] - Write mode when files conflicts. Must be one of
-	 * 	'add'/'overwrite'/'update'. Default to 'overwrite'.
+	 * @param {string} [config.writeMode=overwrite] - Write mode when files conflicts. Must be one of
+	 * 	'add'/'overwrite'/'update'.
 	 * {@link https://www.dropbox.com/developers/documentation/http/documentation#files-upload|see Dropbox manual}
 	 * @param {ConnectorStaticInfos} [config.infos] - Connector infos to override
 	 */
@@ -148,7 +162,7 @@ class DropboxConnector {
 			name: NAME,
 			displayName: 'Dropbox',
 			icon: '../assets/dropbox.png',
-			description: 'Edit html files from your Dropbox.'
+			description: 'Edit files from your Dropbox.'
 		});
 
 		this.name = this.infos.name;
@@ -161,21 +175,36 @@ class DropboxConnector {
 		return Object.assign({
 			isLoggedIn: (session && 'token' in session),
 			isOAuth: true,
-			username: session.account ? session.account.name.display_name : ''
+			username: session.account ? session.account.name.display_name : undefined
 		}, this.infos);
 	}
 
 	setAccessToken(session, token) {
 		session.token = token;
+		const accountFields = [
+			'account_id',
+			'name',
+			'email'
+		];
+		const filterAccountInfos = (account) => {
+			return Object.keys(account).reduce((memo, key) => {
+				if(accountFields.includes(key)) memo[key] = account[key];
+				return memo;
+			}, {});
+		};
+		let accountPromised = null;
 		if(session.account && 'id' in session.account) {
-			return callAPI(session, '/users/get_account', {
+			accountPromised = callAPI(session, '/users/get_account', {
 				account_id: session.account.id
-			})
-			.then((account) => {
-				session.account = account;
-				return token;
 			});
-		} else return Promise.resolve(token);
+		} else {
+			accountPromised = callAPI(session, '/users/get_current_account');
+		}
+		return accountPromised.then((account) => {
+			session.account = filterAccountInfos(account);
+			return token;
+		})
+		.catch((err) => Promise.reject(new UnifileError(UnifileError.EACCES, 'Bad credentials')));
 	}
 
 	clearAccessToken(session) {
@@ -204,12 +233,9 @@ class DropboxConnector {
 			return resolve(body.access_token);
 		}
 
-		if(loginInfos.constructor === String) {
-			returnPromise = new Promise((resolve, reject) =>
-				request(loginInfos, processResponse.bind(this, resolve, reject)));
-		} else if(loginInfos.state !== session.state) {
-			return Promise.reject('Invalid request (cross-site request)');
-		} else {
+		if(typeof loginInfos === 'object' && 'state' in loginInfos && 'code' in loginInfos) {
+			if(loginInfos.state !== session.state)
+				return Promise.reject(new UnifileError(UnifileError.EACCES, 'Invalid request (cross-site request)'));
 			returnPromise = new Promise((resolve, reject) => {
 				request({
 					url: 'https://api.dropboxapi.com/oauth2/token',
@@ -224,6 +250,8 @@ class DropboxConnector {
 					json: true
 				}, processResponse.bind(this, resolve, reject));
 			});
+		} else {
+			return Promise.reject(new UnifileError(UnifileError.EACCES, 'Invalid credentials'));
 		}
 		return returnPromise.then((token) => {
 			return this.setAccessToken(session, token);
@@ -250,6 +278,8 @@ class DropboxConnector {
 	}
 
 	stat(session, path) {
+		if(!path) return Promise.reject(new UnifileError(UnifileError.EINVAL, 'You must provide a path to stat'));
+
 		return callAPI(session, '/files/get_metadata', {
 			path: makePathAbsolute(path)
 		})
@@ -265,7 +295,8 @@ class DropboxConnector {
 	}
 
 	mkdir(session, path) {
-		return callAPI(session, '/files/create_folder', {
+		if(!path) return Promise.reject(new UnifileError(UnifileError.EINVAL, 'Cannot create dir with an empty name.'));
+		return callAPI(session, '/files/create_folder_v2', {
 			path: makePathAbsolute(path)
 		});
 	}
@@ -285,18 +316,26 @@ class DropboxConnector {
 
 	createWriteStream(session, path) {
 
-		return request({
+		const writeStream = request({
 			url: 'https://content.dropboxapi.com/2/files/upload',
 			method: 'POST',
 			headers: {
 				'Authorization': 'Bearer ' + session.token,
 				'Content-Type': 'application/octet-stream',
 				'User-Agent': 'Unifile',
 				'Dropbox-API-Arg': JSON.stringify({
-					path: makePathAbsolute(path)
+					path: makePathAbsolute(path),
+					mode: this.writeMode
 				})
 			}
+		})
+		.on('response', (response) => {
+			if(response.statusCode === 409)
+				writeStream.emit('error', new UnifileError(UnifileError.EIO, 'Creation failed'));
+			else if(response.statusCode === 200) writeStream.emit('close');
 		});
+
+		return writeStream;
 	}
 
 	readFile(session, path) {
@@ -308,8 +347,8 @@ class DropboxConnector {
 	}
 
 	createReadStream(session, path) {
-
-		return request({
+		const readStream = new PassThrough();
+		const req = request({
 			url: 'https://content.dropboxapi.com/2/files/download',
 			method: 'POST',
 			headers: {
@@ -319,18 +358,47 @@ class DropboxConnector {
 					path: makePathAbsolute(path)
 				})
 			}
+		})
+		.on('response', (response) => {
+			if(response.statusCode === 200) req.pipe(readStream);
+
+			switch (response.statusCode) {
+				case 400: readStream.emit('error', new UnifileError(UnifileError.EINVAL, 'Invalid request'));
+					break;
+				case 409:
+					const chunks = [];
+					req.on('data', (data) => {
+						chunks.push(data);
+					});
+					req.on('end', () => {
+						const body = JSON.parse(Buffer.concat(chunks).toString());
+						if(body.error_summary.startsWith('path/not_found'))
+							readStream.emit('error', new UnifileError(UnifileError.ENOENT, 'Not Found'));
+						else if(body.error_summary.startsWith('path/not_file'))
+							readStream.emit('error', new UnifileError(UnifileError.EISDIR, 'Path is a directory'));
+						else
+							readStream.emit('error', new UnifileError(UnifileError.EIO, 'Unable to read file'));
+					});
+			}
 		});
+		return readStream;
 	}
 
 	rename(session, src, dest) {
+		if(!src)
+			return Promise.reject(new UnifileError(UnifileError.EINVAL, 'Cannot rename path with an empty name'));
+		if(!dest)
+			return Promise.reject(new UnifileError(UnifileError.EINVAL, 'Cannot rename path with an empty destination'));
 		return callAPI(session, '/files/move', {
 			from_path: makePathAbsolute(src),
 			to_path: makePathAbsolute(dest)
 		});
 	}
 
 	unlink(session, path) {
-		return callAPI(session, '/files/delete', {
+		if(!path)
+			return Promise.reject(new UnifileError(UnifileError.EINVAL, 'Cannot remove path with an empty name'));
+		return callAPI(session, '/files/delete_v2', {
 			path: makePathAbsolute(path)
 		});
 	}
@@ -401,7 +469,7 @@ class DropboxConnector {
 							},
 							commit: {
 								path: makePathAbsolute(action.path),
-								mode: {'.tag': this.writeMode}
+								mode: this.writeMode
 							}
 						};
 					});
@@ -413,6 +481,9 @@ class DropboxConnector {
 		}
 
 		for(const action of actions) {
+			if(!action.path)
+				return Promise.reject(new BatchError(UnifileError.EINVAL,
+					'Cannot execute batch action without a path'));
 			switch (action.name.toLowerCase()) {
 				case 'unlink':
 				case 'rmdir':
@@ -424,21 +495,33 @@ class DropboxConnector {
 				case 'rename':
 					closeBatchs(action.name.toLowerCase());
 					if(!action.destination)
-						return Promise.reject('Rename action must have a `destination` field');
+						return Promise.reject(new BatchError(
+							UnifileError.EINVAL,
+							'Rename actions should have a destination'));
 					moveEntries.push({
 						from_path: makePathAbsolute(action.path),
 						to_path: makePathAbsolute(action.destination)
 					});
 					break;
 				case 'writefile':
 					closeBatchs(action.name.toLowerCase());
+					if(!action.content)
+						return Promise.reject(new BatchError(
+							UnifileError.EINVAL,
+							'Write actions should have a content'));
 					uploadEntries.push(action);
 					break;
 				case 'mkdir':
 					closeBatchs(action.name.toLowerCase());
 					actionsChain = actionsChain.then(() => {
-						this.mkdir(session, action.path);
-					});
+						return this.mkdir(session, action.path);
+					})
+					.catch((err) => err.name !== 'BatchError',
+						(err) => {
+							throw new BatchError(
+								UnifileError.EINVAL,
+								`Could not complete action ${action.name}: ${err.message}`);
+						});
 					break;
 				default:
 					console.warn(`Unsupported batch action: ${action.name}`);

lib/unifile-github.js

@@ -599,7 +599,7 @@ class GitHubConnector {
 								`Could not complete action ${actionName}: Cannot create file here.`));
 						case 'rename':
 							if(!action.destination)
-								return new Promise.reject(new BatchError(
+								return Promise.reject(new BatchError(
 									UnifileError.EINVAL,
 									'Rename actions should have a destination'));
 							actionsChain = actionsChain.then(() => this.rename(session, action.path, action.destination))
@@ -907,8 +907,7 @@ class GitHubConnector {
 								default: return UnifileError.EIO;
 							}
 						})();
-						const error = new UnifileError(code, JSON.parse(body).message);
-						return reject(error);
+						return reject(new UnifileError(code, JSON.parse(body).message));
 					}
 					try {
 						const result = res.statusCode !== 204 ? JSON.parse(body) : null;