From 324929ca2d70446d9d1c2cca3c73d89a6d3c5734 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 00:57:28 +0200 Subject: [PATCH 01/30] PadMessageHandler: early return to reduce code depth. Get rid of an else branch to simplify code layout. No functional changes at all. ============== This series is an attempt to reduce the control structure depth of the code base, maintaining at the same time its exact same behaviour, bugs included. It is, in a sense, an initial attempt at a refactoring in the spirit of its original definition [0]. The idea beyond this refactoring is that reducing the code depth and, sometimes, inverting some conditions, bugs and logic errors may become easier to spot, and the code easier to read. When looked at ignoring whitespace changes, all of these diffs should appear trivial. [0] https://refactoring.com/ --- src/node/handler/PadMessageHandler.js | 51 ++++++++++++++------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index d316faf01..767348d2a 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -265,33 +265,34 @@ exports.handleMessage = function(client, message) if(!sessioninfos[client.id].auth){ console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") return; - }else{ - var auth = sessioninfos[client.id].auth; - var checkAccessCallback = function(err, statusObject) - { - if(ERR(err, callback)) return; + } - //access was granted - if(statusObject.accessStatus == "grant") - { - callback(); - } - //no access, send the client a message that tell him why - else - { - client.json.send({accessStatus: statusObject.accessStatus}) - } - }; - //check if pad is requested via readOnly - if (auth.padID.indexOf("r.") === 0) { - //Pad is readOnly, first get the real Pad ID - readOnlyManager.getPadId(auth.padID, function(err, value) { - ERR(err); - securityManager.checkAccess(value, auth.sessionID, auth.token, auth.password, checkAccessCallback); - }); - } else { - securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback); + var auth = sessioninfos[client.id].auth; + var checkAccessCallback = function(err, statusObject) + { + if(ERR(err, callback)) return; + + //access was granted + if(statusObject.accessStatus == "grant") + { + callback(); } + //no access, send the client a message that tell him why + else + { + client.json.send({accessStatus: statusObject.accessStatus}) + } + }; + + //check if pad is requested via readOnly + if (auth.padID.indexOf("r.") === 0) { + //Pad is readOnly, first get the real Pad ID + readOnlyManager.getPadId(auth.padID, function(err, value) { + ERR(err); + securityManager.checkAccess(value, auth.sessionID, auth.token, auth.password, checkAccessCallback); + }); + } else { + securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback); } }, finalHandler From b60c0b122ce1a3413cc4c974c267f7619364e82b Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:23:38 +0200 Subject: [PATCH 02/30] PadMessageHandler: reversed condition to make core logic evident. No behavioural changes. This one replaces a big "if (message)" negating its truthy condition. Being lame, I erred on the safe side and wrote a super ugly statement that is guaranteed to respect the original logic. In the hope that eventual logic errors become more evident now. See: https://stackoverflow.com/questions/36661748/what-is-the-exact-negation-of-ifvariable-in-javascript#36661843 --- src/node/handler/PadMessageHandler.js | 117 ++++++++++++++------------ 1 file changed, 64 insertions(+), 53 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 767348d2a..c52565de3 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -244,60 +244,71 @@ exports.handleMessage = function(client, message) } }; - if (message) { - async.series([ - handleMessageHook, - //check permissions - function(callback) - { - // client tried to auth for the first time (first msg from the client) - if(message.type == "CLIENT_READY") { - createSessionInfo(client, message); - } - - // Note: message.sessionID is an entirely different kind of - // session from the sessions we use here! Beware! - // FIXME: Call our "sessions" "connections". - // FIXME: Use a hook instead - // FIXME: Allow to override readwrite access with readonly - - // Simulate using the load testing tool - if(!sessioninfos[client.id].auth){ - console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") - return; - } - - var auth = sessioninfos[client.id].auth; - var checkAccessCallback = function(err, statusObject) - { - if(ERR(err, callback)) return; - - //access was granted - if(statusObject.accessStatus == "grant") - { - callback(); - } - //no access, send the client a message that tell him why - else - { - client.json.send({accessStatus: statusObject.accessStatus}) - } - }; - - //check if pad is requested via readOnly - if (auth.padID.indexOf("r.") === 0) { - //Pad is readOnly, first get the real Pad ID - readOnlyManager.getPadId(auth.padID, function(err, value) { - ERR(err); - securityManager.checkAccess(value, auth.sessionID, auth.token, auth.password, checkAccessCallback); - }); - } else { - securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback); - } - }, - finalHandler - ]); + /* + * In a previous version of this code, an "if (message)" wrapped the + * following async.series(). + * This ugly "!Boolean(message)" is a lame way to exactly negate the truthy + * condition and replace it with an early return, while being sure to leave + * the original behaviour unchanged. + * + * A shallower code could maybe make more evident latent logic errors. + */ + if (!Boolean(message)) { + return; } + + async.series([ + handleMessageHook, + //check permissions + function(callback) + { + // client tried to auth for the first time (first msg from the client) + if(message.type == "CLIENT_READY") { + createSessionInfo(client, message); + } + + // Note: message.sessionID is an entirely different kind of + // session from the sessions we use here! Beware! + // FIXME: Call our "sessions" "connections". + // FIXME: Use a hook instead + // FIXME: Allow to override readwrite access with readonly + + // Simulate using the load testing tool + if(!sessioninfos[client.id].auth){ + console.error("Auth was never applied to a session. If you are using the stress-test tool then restart Etherpad and the Stress test tool.") + return; + } + + var auth = sessioninfos[client.id].auth; + var checkAccessCallback = function(err, statusObject) + { + if(ERR(err, callback)) return; + + //access was granted + if(statusObject.accessStatus == "grant") + { + callback(); + } + //no access, send the client a message that tell him why + else + { + client.json.send({accessStatus: statusObject.accessStatus}) + } + }; + + //check if pad is requested via readOnly + if (auth.padID.indexOf("r.") === 0) { + //Pad is readOnly, first get the real Pad ID + readOnlyManager.getPadId(auth.padID, function(err, value) { + ERR(err); + securityManager.checkAccess(value, auth.sessionID, auth.token, auth.password, checkAccessCallback); + }); + } else { + securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback); + } + }, + finalHandler + ]); } From d19436d04494724777adfd829078b0ac66c1781e Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:34:45 +0200 Subject: [PATCH 03/30] adminsettings: early return, no functional changes. --- src/node/hooks/express/adminsettings.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/node/hooks/express/adminsettings.js b/src/node/hooks/express/adminsettings.js index 73691837a..baa0bb70a 100644 --- a/src/node/hooks/express/adminsettings.js +++ b/src/node/hooks/express/adminsettings.js @@ -28,15 +28,13 @@ exports.socketio = function (hook_name, args, cb) { if (err) { return console.log(err); } - else - { - //if showSettingsInAdminPage is set to false, then return NOT_ALLOWED in the result - if(settings.showSettingsInAdminPage === false) { - socket.emit("settings", {results:'NOT_ALLOWED'}); - } - else { - socket.emit("settings", {results: data}); - } + + //if showSettingsInAdminPage is set to false, then return NOT_ALLOWED in the result + if(settings.showSettingsInAdminPage === false) { + socket.emit("settings", {results:'NOT_ALLOWED'}); + } + else { + socket.emit("settings", {results: data}); } }); }); From 391bd79e03ea088cee79905d619f985619735d68 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:38:55 +0200 Subject: [PATCH 04/30] padurlsanitize: early return, no functional changes --- src/node/hooks/express/padurlsanitize.js | 39 ++++++++++++------------ 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/node/hooks/express/padurlsanitize.js b/src/node/hooks/express/padurlsanitize.js index a9972220b..be3ffb1b4 100644 --- a/src/node/hooks/express/padurlsanitize.js +++ b/src/node/hooks/express/padurlsanitize.js @@ -8,26 +8,25 @@ exports.expressCreateServer = function (hook_name, args, cb) { if(!padManager.isValidPadId(padId) || /\/$/.test(req.url)) { res.status(404).send('Such a padname is forbidden'); + return; } - else - { - padManager.sanitizePadId(padId, function(sanitizedPadId) { - //the pad id was sanitized, so we redirect to the sanitized version - if(sanitizedPadId != padId) - { - var real_url = sanitizedPadId; - real_url = encodeURIComponent(real_url); - var query = url.parse(req.url).query; - if ( query ) real_url += '?' + query; - res.header('Location', real_url); - res.status(302).send('You should be redirected to ' + real_url + ''); - } - //the pad id was fine, so just render it - else - { - next(); - } - }); - } + + padManager.sanitizePadId(padId, function(sanitizedPadId) { + //the pad id was sanitized, so we redirect to the sanitized version + if(sanitizedPadId != padId) + { + var real_url = sanitizedPadId; + real_url = encodeURIComponent(real_url); + var query = url.parse(req.url).query; + if ( query ) real_url += '?' + query; + res.header('Location', real_url); + res.status(302).send('You should be redirected to ' + real_url + ''); + } + //the pad id was fine, so just render it + else + { + next(); + } + }); }); } From 12f224ae723ac7ba15531f4778732cd220ee5d9f Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:44:13 +0200 Subject: [PATCH 05/30] db/PadManager: early return, no functional changes --- src/node/db/PadManager.js | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/node/db/PadManager.js b/src/node/db/PadManager.js index 2ecd6e274..ae908e5c0 100644 --- a/src/node/db/PadManager.js +++ b/src/node/db/PadManager.js @@ -159,21 +159,20 @@ exports.getPad = function(id, text, callback) if(pad != null) { callback(null, pad); + return; } - //try to load pad - else - { - pad = new Pad(id); - //initalize the pad - pad.init(text, function(err) - { - if(ERR(err, callback)) return; - globalPads.set(id, pad); - padList.addPad(id); - callback(null, pad); - }); - } + //try to load pad + pad = new Pad(id); + + //initalize the pad + pad.init(text, function(err) + { + if(ERR(err, callback)) return; + globalPads.set(id, pad); + padList.addPad(id); + callback(null, pad); + }); } exports.listAllPads = function(cb) From ecb0c41d29b9824f1ae592db9d9ed378e92aa747 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:46:18 +0200 Subject: [PATCH 06/30] db/PadManager: early return, no functional changes --- src/node/db/PadManager.js | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/node/db/PadManager.js b/src/node/db/PadManager.js index ae908e5c0..12fa227a3 100644 --- a/src/node/db/PadManager.js +++ b/src/node/db/PadManager.js @@ -205,30 +205,29 @@ exports.sanitizePadId = function(padId, callback) { if(transform_index >= padIdTransforms.length) { callback(padId); + return; } + //check if padId exists - else + exports.doesPadExists(padId, function(junk, exists) { - exports.doesPadExists(padId, function(junk, exists) + if(exists) { - if(exists) + callback(padId); + } + else + { + //get the next transformation *that's different* + var transformedPadId = padId; + while(transformedPadId == padId && transform_index < padIdTransforms.length) { - callback(padId); + transformedPadId = padId.replace(padIdTransforms[transform_index][0], padIdTransforms[transform_index][1]); + transform_index += 1; } - else - { - //get the next transformation *that's different* - var transformedPadId = padId; - while(transformedPadId == padId && transform_index < padIdTransforms.length) - { - transformedPadId = padId.replace(padIdTransforms[transform_index][0], padIdTransforms[transform_index][1]); - transform_index += 1; - } - //check the next transform - exports.sanitizePadId(transformedPadId, callback, transform_index); - } - }); - } + //check the next transform + exports.sanitizePadId(transformedPadId, callback, transform_index); + } + }); } exports.isValidPadId = function(padId) From 4728736dd8283f2a26a66405dea5da1151436752 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:47:38 +0200 Subject: [PATCH 07/30] db/PadManager: early return, no functional changes --- src/node/db/PadManager.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/node/db/PadManager.js b/src/node/db/PadManager.js index 12fa227a3..035ef3e5e 100644 --- a/src/node/db/PadManager.js +++ b/src/node/db/PadManager.js @@ -214,19 +214,18 @@ exports.sanitizePadId = function(padId, callback) { if(exists) { callback(padId); + return; } - else + + //get the next transformation *that's different* + var transformedPadId = padId; + while(transformedPadId == padId && transform_index < padIdTransforms.length) { - //get the next transformation *that's different* - var transformedPadId = padId; - while(transformedPadId == padId && transform_index < padIdTransforms.length) - { - transformedPadId = padId.replace(padIdTransforms[transform_index][0], padIdTransforms[transform_index][1]); - transform_index += 1; - } - //check the next transform - exports.sanitizePadId(transformedPadId, callback, transform_index); + transformedPadId = padId.replace(padIdTransforms[transform_index][0], padIdTransforms[transform_index][1]); + transform_index += 1; } + //check the next transform + exports.sanitizePadId(transformedPadId, callback, transform_index); }); } From 30d814d8ed9d455560251fd8492d4e8880a27d20 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:57:28 +0200 Subject: [PATCH 08/30] db/API.js: early return, no functional changes --- src/node/db/API.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index be3e73486..8c8e515cd 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -184,17 +184,17 @@ exports.getRevisionChangeset = function(padID, rev, callback) callback(null, changeset); }) - } - //the client wants the latest changeset, lets return it to him - else - { - pad.getRevisionChangeset(pad.getHeadRevisionNumber(), function(err, changeset) - { - if(ERR(err, callback)) return; - callback(null, changeset); - }) + return; } + + //the client wants the latest changeset, lets return it to him + pad.getRevisionChangeset(pad.getHeadRevisionNumber(), function(err, changeset) + { + if(ERR(err, callback)) return; + + callback(null, changeset); + }) }); } From 1d45a63864a7d1a7f71f5541b003de522873a6ab Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 01:57:00 +0200 Subject: [PATCH 09/30] db/API.js: early return, no functional changes --- src/node/db/API.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index 8c8e515cd..7c6b95117 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -268,13 +268,13 @@ exports.getText = function(padID, rev, callback) callback(null, data); }) + + return; } + //the client wants the latest text, lets return it to him - else - { - var padText = exportTxt.getTXTFromAtext(pad, pad.atext); - callback(null, {"text": padText}); - } + var padText = exportTxt.getTXTFromAtext(pad, pad.atext); + callback(null, {"text": padText}); }); } From 05a33f15331bac551010585c0890a40e6421771f Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:08:05 +0200 Subject: [PATCH 10/30] db/API.js, SessionManager: lot of copied & pasted code in integer parsing Replaced with an early return, no functional changes. --- src/node/db/API.js | 56 +++++++++++++---------------------- src/node/db/SessionManager.js | 8 ++--- 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index 7c6b95117..c420b9994 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -137,15 +137,13 @@ exports.getRevisionChangeset = function(padID, rev, callback) if (rev !== undefined && typeof rev !== "number") { // try to parse the number - if (!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if (isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } // ensure this is not a negative number @@ -219,15 +217,13 @@ exports.getText = function(padID, rev, callback) if(rev !== undefined && typeof rev != "number") { //try to parse the number - if(!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if(isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } //ensure this is not a negativ number @@ -359,15 +355,13 @@ exports.getHTML = function(padID, rev, callback) if (rev !== undefined && typeof rev != "number") { - if (!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if (isNaN(parseInt(rev))) { callback(new customError("rev is not a number","apierror")); return; } + + rev = parseInt(rev); } if(rev !== undefined && rev < 0) @@ -641,15 +635,13 @@ exports.saveRevision = function(padID, rev, callback) if(rev !== undefined && typeof rev != "number") { //try to parse the number - if(!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if(isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } //ensure this is not a negativ number @@ -782,15 +774,13 @@ exports.restoreRevision = function (padID, rev, callback) if (rev !== undefined && typeof rev != "number") { //try to parse the number - if (!isNaN(parseInt(rev))) - { - rev = parseInt(rev); - } - else + if (isNaN(parseInt(rev))) { callback(new customError("rev is not a number", "apierror")); return; } + + rev = parseInt(rev); } //ensure this is not a negativ number @@ -1177,30 +1167,26 @@ exports.createDiffHTML = function(padID, startRev, endRev, callback){ if(startRev !== undefined && typeof startRev != "number") { //try to parse the number - if(!isNaN(parseInt(startRev))) - { - startRev = parseInt(startRev, 10); - } - else + if(isNaN(parseInt(startRev))) { callback({stop: "startRev is not a number"}); return; } + + startRev = parseInt(startRev, 10); } //check if rev is a number if(endRev !== undefined && typeof endRev != "number") { //try to parse the number - if(!isNaN(parseInt(endRev))) - { - endRev = parseInt(endRev, 10); - } - else + if(isNaN(parseInt(endRev))) { callback({stop: "endRev is not a number"}); return; } + + endRev = parseInt(endRev, 10); } //get the pad diff --git a/src/node/db/SessionManager.js b/src/node/db/SessionManager.js index 518c70c7a..954203758 100644 --- a/src/node/db/SessionManager.js +++ b/src/node/db/SessionManager.js @@ -90,15 +90,13 @@ exports.createSession = function(groupID, authorID, validUntil, callback) if(typeof validUntil != "number") { //try to parse the number - if(!isNaN(parseInt(validUntil))) - { - validUntil = parseInt(validUntil); - } - else + if(isNaN(parseInt(validUntil))) { callback(new customError("validUntil is not a number","apierror")); return; } + + validUntil = parseInt(validUntil); } //ensure this is not a negativ number From 42bc0a59e13d333eb5d0f462cec82173f2ccd5fb Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:09:33 +0200 Subject: [PATCH 11/30] db/API.js: early return, no functional changes --- src/node/db/API.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index c420b9994..b47c860fe 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -442,11 +442,10 @@ exports.setHTML = function(padID, html, callback) if(e){ callback(new customError("HTML is malformed","apierror")); return; - }else{ - //update the clients on the pad - padMessageHandler.updatePadClients(pad, callback); - return; } + + //update the clients on the pad + padMessageHandler.updatePadClients(pad, callback); }); }); } From fef57efd463a329199831a7bd8976325e255b2d6 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:10:45 +0200 Subject: [PATCH 12/30] db/API.js: early return, no functional changes --- src/node/db/API.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index b47c860fe..f477b0e9c 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -1116,9 +1116,9 @@ exports.sendClientsMessage = function (padID, msg, callback) { getPadSafe(padID, true, function (err, pad) { if (ERR(err, callback)) { return; - } else { - padMessageHandler.handleCustomMessage(padID, msg, callback); } + + padMessageHandler.handleCustomMessage(padID, msg, callback); } ); } From 610a6db8c8c379beca9cdb11cb197c17bb744595 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:13:06 +0200 Subject: [PATCH 13/30] db/API.js: early return, no functional changes --- src/node/db/API.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index f477b0e9c..fa1fd3056 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -399,19 +399,19 @@ exports.getHTML = function(padID, rev, callback) var data = {html: html}; callback(null, data); }); + + return; } + //the client wants the latest text, lets return it to him - else + exportHtml.getPadHTML(pad, undefined, function (err, html) { - exportHtml.getPadHTML(pad, undefined, function (err, html) - { - if(ERR(err, callback)) return; - html = "" +html; // adds HTML head - html += ""; - var data = {html: html}; - callback(null, data); - }); - } + if(ERR(err, callback)) return; + html = "" +html; // adds HTML head + html += ""; + var data = {html: html}; + callback(null, data); + }); }); } From 67ce19eddb72f9e611ce466939d3707639f77dcf Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:16:24 +0200 Subject: [PATCH 14/30] db/API.js: removed unuseful else clause, no functional changes --- src/node/db/API.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index fa1fd3056..d2ff92062 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -723,8 +723,9 @@ exports.createPad = function(padID, text, callback) callback(new customError("createPad can't create group pads","apierror")); return; } + //check for url special characters - else if(padID.match(/(\/|\?|&|#)/)) + if(padID.match(/(\/|\?|&|#)/)) { callback(new customError("malformed padID: Remove special characters","apierror")); return; From b59818676ec2f086d064c5b75d6961b8a31dd445 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:18:32 +0200 Subject: [PATCH 15/30] db/API.js: early return to make error handling evident. No functional changes --- src/node/db/API.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node/db/API.js b/src/node/db/API.js index d2ff92062..21c958809 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -949,11 +949,10 @@ exports.getPadID = function(roID, callback) if(retrievedPadID == null) { callback(new customError("padID does not exist","apierror")); + return; } - else - { - callback(null, {padID: retrievedPadID}); - } + + callback(null, {padID: retrievedPadID}); }); } From 2b8646a855626283758a3e19ccdcd6a9a32f9d17 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:28:35 +0200 Subject: [PATCH 16/30] db/AuthorManager: early return, no functional changes --- src/node/db/AuthorManager.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/node/db/AuthorManager.js b/src/node/db/AuthorManager.js index 1f2a736be..5f48e19d8 100644 --- a/src/node/db/AuthorManager.js +++ b/src/node/db/AuthorManager.js @@ -104,16 +104,16 @@ function mapAuthorWithDBKey (mapperkey, mapper, callback) //return the author callback(null, author); }); - } - //there is a author with this mapper - else - { - //update the timestamp of this author - db.setSub("globalAuthor:" + author, ["timestamp"], new Date().getTime()); - //return the author - callback(null, {authorID: author}); + return; } + + //there is a author with this mapper + //update the timestamp of this author + db.setSub("globalAuthor:" + author, ["timestamp"], new Date().getTime()); + + //return the author + callback(null, {authorID: author}); }); } From 61823e7689fcdec71a5119a6c246eada9f5ed450 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:28:40 +0200 Subject: [PATCH 17/30] db/AuthorManager: early return, no functional changes --- src/node/db/AuthorManager.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/node/db/AuthorManager.js b/src/node/db/AuthorManager.js index 5f48e19d8..c7ebf47f4 100644 --- a/src/node/db/AuthorManager.js +++ b/src/node/db/AuthorManager.js @@ -209,20 +209,19 @@ exports.listPadsOfAuthor = function (authorID, callback) if(author == null) { callback(new customError("authorID does not exist","apierror")) + return; } + //everything is fine, return the pad IDs - else + var pads = []; + if(author.padIDs != null) { - var pads = []; - if(author.padIDs != null) + for (var padId in author.padIDs) { - for (var padId in author.padIDs) - { - pads.push(padId); - } + pads.push(padId); } - callback(null, {padIDs: pads}); } + callback(null, {padIDs: pads}); }); } From 6af419a88e3c71a84ec425bbbe35c99afcbfc9f5 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:33:29 +0200 Subject: [PATCH 18/30] SecurityManager.js: early return, no functional changes --- src/node/db/SecurityManager.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node/db/SecurityManager.js b/src/node/db/SecurityManager.js index 98feafb3a..f930b9618 100644 --- a/src/node/db/SecurityManager.js +++ b/src/node/db/SecurityManager.js @@ -90,13 +90,13 @@ exports.checkAccess = function (padID, sessionCookie, token, password, callback) // grant or deny access, with author of token callback(null, statusObject); }); + + return; } + // user may create new pads - no need to check anything - else - { - // grant access, with author of token - callback(null, statusObject); - } + // grant access, with author of token + callback(null, statusObject); }); //don't continue From c85bcf06140e9cd10f75be29f0395752a6c77a38 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:36:25 +0200 Subject: [PATCH 19/30] db/GroupManager: move inner function on top. No functional change This is to make easier on the eye the next change. --- src/node/db/GroupManager.js | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index 82c14c39d..44bda74f2 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -213,7 +213,19 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback) //try to get a group for this mapper db.get("mapper2group:"+groupMapper, function(err, groupID) { - if(ERR(err, callback)) return; + function createGroupForMapper(cb) { + exports.createGroup(function(err, responseObj) + { + if(ERR(err, cb)) return; + + //create the mapper entry for this group + db.set("mapper2group:"+groupMapper, responseObj.groupID); + + cb(null, responseObj); + }); + } + + if(ERR(err, callback)) return; // there is a group for this mapper if(groupID) { @@ -229,18 +241,6 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback) else { createGroupForMapper(callback) } - - function createGroupForMapper(cb) { - exports.createGroup(function(err, responseObj) - { - if(ERR(err, cb)) return; - - //create the mapper entry for this group - db.set("mapper2group:"+groupMapper, responseObj.groupID); - - cb(null, responseObj); - }); - } }); } From 604952bc97bc31e1af05cfa08268fd5c0cd89d8c Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:38:09 +0200 Subject: [PATCH 20/30] db/GroupManager: fix indentation This is to make easier on the eye the next change. --- src/node/db/GroupManager.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index 44bda74f2..f8255ba36 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -227,20 +227,20 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback) if(ERR(err, callback)) return; - // there is a group for this mapper - if(groupID) { - exports.doesGroupExist(groupID, function(err, exists) { - if(ERR(err, callback)) return; - if(exists) return callback(null, {groupID: groupID}); - - // hah, the returned group doesn't exist, let's create one - createGroupForMapper(callback) - }) - } - //there is no group for this mapper, let's create a group - else { - createGroupForMapper(callback) - } + // there is a group for this mapper + if(groupID) { + exports.doesGroupExist(groupID, function(err, exists) { + if(ERR(err, callback)) return; + if(exists) return callback(null, {groupID: groupID}); + + // hah, the returned group doesn't exist, let's create one + createGroupForMapper(callback) + }) + } + //there is no group for this mapper, let's create a group + else { + createGroupForMapper(callback) + } }); } From f7254a47eaa17f0a703de05a6e4f55a2d60d2913 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:39:05 +0200 Subject: [PATCH 21/30] db/GroupManager: early return, no functional changes --- src/node/db/GroupManager.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index f8255ba36..e65c0dc46 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -236,11 +236,12 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback) // hah, the returned group doesn't exist, let's create one createGroupForMapper(callback) }) + + return; } + //there is no group for this mapper, let's create a group - else { - createGroupForMapper(callback) - } + createGroupForMapper(callback) }); } From da8faa1aa90c7b477b20d8d6022bd818d85d0294 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:40:14 +0200 Subject: [PATCH 22/30] db/GroupManager: early return, no functional changes --- src/node/db/GroupManager.js | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index e65c0dc46..118c044bb 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -321,19 +321,18 @@ exports.listPads = function(groupID, callback) if(exists == false) { callback(new customError("groupID does not exist","apierror")); + return; } + //group exists, let's get the pads - else + db.getSub("group:" + groupID, ["pads"], function(err, result) { - db.getSub("group:" + groupID, ["pads"], function(err, result) - { - if(ERR(err, callback)) return; - var pads = []; - for ( var padId in result ) { - pads.push(padId); - } - callback(null, {padIDs: pads}); - }); - } + if(ERR(err, callback)) return; + var pads = []; + for ( var padId in result ) { + pads.push(padId); + } + callback(null, {padIDs: pads}); + }); }); } From 9ed7608421c0ea242be5955fd253a1e1d7c482b3 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:41:14 +0200 Subject: [PATCH 23/30] db/GroupManager: early return, no functional changes --- src/node/db/GroupManager.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index 118c044bb..c80d8c06e 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -62,13 +62,12 @@ exports.deleteGroup = function(groupID, callback) if(_group == null) { callback(new customError("groupID does not exist","apierror")); + return; } + //group exists, everything is fine - else - { - group = _group; - callback(); - } + group = _group; + callback(); }); }, //iterate trough all pads of this groups and delete them From a1d21c0cd25f6aa7685785b57169619dfa192ef2 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:41:53 +0200 Subject: [PATCH 24/30] db/GroupManager: early return, no functional changes --- src/node/db/GroupManager.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index c80d8c06e..741fe6e5a 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -261,12 +261,11 @@ exports.createGroupPad = function(groupID, padName, text, callback) if(exists == false) { callback(new customError("groupID does not exist","apierror")); + return; } + //group exists, everything is fine - else - { - callback(); - } + callback(); }); }, //ensure pad does not exists From e90487c3e29ec97abfa293ef99e4e366ca8aeba3 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:42:29 +0200 Subject: [PATCH 25/30] db/GroupManager: early return, no functional changes --- src/node/db/GroupManager.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js index 741fe6e5a..0c9be1221 100644 --- a/src/node/db/GroupManager.js +++ b/src/node/db/GroupManager.js @@ -279,12 +279,11 @@ exports.createGroupPad = function(groupID, padName, text, callback) if(exists == true) { callback(new customError("padName does already exist","apierror")); + return; } + //pad does not exist, everything is fine - else - { - callback(); - } + callback(); }); }, //create the pad From 049f5f2859700185e4f94251ba10c1edbf5c0245 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:44:51 +0200 Subject: [PATCH 26/30] db/Pad: removed unuseful else clause, no functional changes --- src/node/db/Pad.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index 0cb01cace..a3920cc8a 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -489,11 +489,9 @@ Pad.prototype.copy = function copy(destinationID, force, callback) { callback(new customError("groupID does not exist for destinationID","apierror")); return; } + //everything is fine, continue - else - { - callback(); - } + callback(); }); } else From 0e8789863c5f2ef83dff941af1f1cf31038e76b9 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:46:08 +0200 Subject: [PATCH 27/30] db/Pad: removed unuseful else clause, no functional changes --- src/node/db/Pad.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index a3920cc8a..6386e8b58 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -513,13 +513,12 @@ Pad.prototype.copy = function copy(destinationID, force, callback) { console.error("erroring out without force - after"); return; } - else // exists and forcing - { - padManager.getPad(destinationID, function(err, pad) { - if (ERR(err, callback)) return; - pad.remove(callback); - }); - } + + // exists and forcing + padManager.getPad(destinationID, function(err, pad) { + if (ERR(err, callback)) return; + pad.remove(callback); + }); } else { From d931a700b442eab0b9042e5acf7e7eacee71a493 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:49:40 +0200 Subject: [PATCH 28/30] db/Pad: reversed condition to make error handling evident. No functional changes Here it was legal to replace a lax comparison with a strict one, since we are using indexOf(), whose return value is known. --- src/node/db/Pad.js | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index 6386e8b58..f5d65e6ff 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -476,26 +476,27 @@ Pad.prototype.copy = function copy(destinationID, force, callback) { // if it's a group pad, let's make sure the group exists. function(callback) { - if (destinationID.indexOf("$") != -1) + if (destinationID.indexOf("$") === -1) { - destGroupID = destinationID.split("$")[0] - groupManager.doesGroupExist(destGroupID, function (err, exists) - { - if(ERR(err, callback)) return; - - //group does not exist - if(exists == false) - { - callback(new customError("groupID does not exist for destinationID","apierror")); - return; - } - - //everything is fine, continue - callback(); - }); - } - else callback(); + return; + } + + destGroupID = destinationID.split("$")[0] + groupManager.doesGroupExist(destGroupID, function (err, exists) + { + if(ERR(err, callback)) return; + + //group does not exist + if(exists == false) + { + callback(new customError("groupID does not exist for destinationID","apierror")); + return; + } + + //everything is fine, continue + callback(); + }); }, // if the pad exists, we should abort, unless forced. function(callback) From 69e1bf28aa95e23bebc4cc92868b959a70582cb7 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 02:52:26 +0200 Subject: [PATCH 29/30] db/Pad: reversed condition to make core logic evident. No functional changes Here it was legal to replace a lax comparison with a strict one, since we are using indexOf(), whose return value is known. --- src/node/db/Pad.js | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index f5d65e6ff..66f9aa598 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -620,29 +620,28 @@ Pad.prototype.remove = function remove(callback) { //is it a group pad? -> delete the entry of this pad in the group function(callback) { - //is it a group pad? - if(padID.indexOf("$")!=-1) - { - var groupID = padID.substring(0,padID.indexOf("$")); - - db.get("group:" + groupID, function (err, group) - { - if(ERR(err, callback)) return; - - //remove the pad entry - delete group.pads[padID]; - - //set the new value - db.set("group:" + groupID, group); - - callback(); - }); - } - //its no group pad, nothing to do here - else + if(padID.indexOf("$") === -1) { + // it isn't a group pad, nothing to do here callback(); + return; } + + // it is a group pad + var groupID = padID.substring(0,padID.indexOf("$")); + + db.get("group:" + groupID, function (err, group) + { + if(ERR(err, callback)) return; + + //remove the pad entry + delete group.pads[padID]; + + //set the new value + db.set("group:" + groupID, group); + + callback(); + }); }, //remove the readonly entries function(callback) From 1a93ab4eb53557e0d67306f71ce7985593092647 Mon Sep 17 00:00:00 2001 From: muxator Date: Wed, 29 Aug 2018 03:03:34 +0200 Subject: [PATCH 30/30] db/Pad: reversed truthy condition to make core logic evident Since the original comparison compared for truthy and not for "===", and it's 3 AM now, I blindly negated it, in order to show how fragile it was in the first instance. No functional changes. This is the final commit of this refactoring series. --- src/node/db/Pad.js | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index 66f9aa598..91ab7f792 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -505,26 +505,29 @@ Pad.prototype.copy = function copy(destinationID, force, callback) { { if(ERR(err, callback)) return; - if(exists == true) - { - if (!force) - { - console.error("erroring out without force"); - callback(new customError("destinationID already exists","apierror")); - console.error("erroring out without force - after"); - return; - } - - // exists and forcing - padManager.getPad(destinationID, function(err, pad) { - if (ERR(err, callback)) return; - pad.remove(callback); - }); - } - else + /* + * this is the negation of a truthy comparison. Has been left in this + * wonky state to keep the old (possibly buggy) behaviour + */ + if (!(exists == true)) { callback(); + return; } + + if (!force) + { + console.error("erroring out without force"); + callback(new customError("destinationID already exists","apierror")); + console.error("erroring out without force - after"); + return; + } + + // exists and forcing + padManager.getPad(destinationID, function(err, pad) { + if (ERR(err, callback)) return; + pad.remove(callback); + }); }); }, // copy the 'pad' entry