Fixes #105: separates log messages from user messages

lib/armadietto.js

@@ -204,7 +204,7 @@ class Armadietto {
       if (!Storage.VALID_NAME.test(username) || !Storage.VALID_PATH.test(path)) {
         res.writeHead(400, { 'Access-Control-Allow-Origin': req.headers.origin || '*' });
         res.end();
-        return logRequest(req, username, 400, 0, `user “${username}” or path “${path}” invalid`);
+        return logRequest(req, username, 400, 0, `user '${username}' or path '${path}' invalid`);
       }
 
       try {
@@ -218,7 +218,8 @@ class Armadietto {
         getLogger().error('Storage Error:', e);
       }
     }
-    new Assets(this, req, res).errorPage(404, uri.pathname + ' Not found');
+    const locals = { title: 'Not found', message: `“${uri.pathname}” doesn't exist` };
+    new Assets(this, req, res).renderHTML(404, 'error.html', locals, `no handler for path '${uri.pathname}'`);
   }
 
   isSecureRequest (r) {

lib/controllers/assets.js

@@ -24,7 +24,8 @@ class Assets extends Controller {
       this.response.end(content);
       logRequest(this.request, '-', 200, content.length);
     } else {
-      this.errorPage(404, filename + ' Not found');
+      const locals = { title: 'Not found', message: `“${path.join('assets', filename)}” doesn't exist` };
+      this.renderHTML(404, 'error.html', locals, `no file '${path.join(assetDir, filename)}'`);
     }
   }
 }

lib/controllers/base.js

@@ -103,12 +103,26 @@ class Controller {
     logRequest(this.request, '-', 200, body.length);
   }
 
-  renderHTML (status, file, locals = {}) {
+  /**
+   * Combines EJS Embedded JavaScript template & locals. Sends HTML to client.
+   * @param {number} status - HTTP status
+   * @param {string} file - path to EJS template
+   * @param {Object} [locals] - fields are substituted into template. Messages should be user-friendly
+   * @param {Object} [locals.title] - external page title
+   * @param {Object} [locals.message] - User-friendly text pointing user toward action to resolve issue
+   * @param {Object} [locals.error]
+   * @param {Object} [locals.username]
+   * @param {Object} [locals.basePath]
+   * @param {string|Error} [logNote] - should concisely give details & be distinct from other calls to renderHTML
+   * @param {string} [logLevel] - emerg, alert, crit, error, warning, notice, info or debug
+   */
+  renderHTML (status, file, locals = {}, logNote = '', logLevel = undefined) {
     const response = this.response;
     const layout = this.readFile(path.join(viewDir, '/layout.html')).toString();
     const body = this.readFile(path.join(viewDir, file)).toString();
 
     locals.basePath = this.server._basePath;
+    locals.status = status;
 
     const globals = {
       scheme: this.request.secure ? 'https' : 'http',
@@ -133,11 +147,7 @@ class Controller {
     response.writeHead(status, headers);
     response.end(html);
     const username = locals.username || (locals.params && locals.params.username) || '-';
-    logRequest(this.request, username, status, html.length, locals.message || locals.error);
-  }
-
-  errorPage (status, message) {
-    this.renderHTML(status, 'error.html', { title: 'Error', status, message });
+    logRequest(this.request, username, status, html.length, logNote || locals.error || locals.message, logLevel);
   }
 }
 

lib/controllers/storage.js

@@ -82,7 +82,7 @@ class Storage extends Controller {
     const type = this.request.headers['content-type'] || '';
     const range = this.request.headers['content-range'] || false;
     if (range) {
-      this.unauthorized(400, 'Content-Range in PUT');
+      this.unauthorized(400, 'invalid_request', 'Content-Range in PUT');
       return false;
     }
     const version = this.getVersion();
@@ -137,7 +137,7 @@ class Storage extends Controller {
   async checkToken (permission) {
     if (this.server._forceSSL && !this.request.secure) {
       await this.server._store.revokeAccess(this._username, this._token);
-      this.unauthorized(400, 'invalid_request');
+      this.unauthorized(400, 'invalid_request', 'HTTPS required');
       return false;
     }
 
@@ -157,9 +157,9 @@ class Storage extends Controller {
         this.unauthorized(401, 'invalid_token');
         return false;
       }
-    } catch (e) {
-      getLogger().error('Bad store.permissions implementation?', { error: e.message });
-      this.unauthorized(400, 'bad_store_implementation');
+    } catch (e) { // TODO: catch this in callers; it's not an OAUTH error
+      getLogger().crit('Bad store.permissions implementation?', { error: e.message });
+      this.unauthorized(400, 'invalid_request', e);
       return false;
     }
 
@@ -179,7 +179,7 @@ class Storage extends Controller {
         return true;
       }
     }
-    this.unauthorized(403, 'insufficient_scope');
+    this.unauthorized(403, 'insufficient_scope', `user '${this._username}' lacks scope for '${this._path}'`);
     return false;
   }
 
@@ -195,12 +195,12 @@ class Storage extends Controller {
     this._headers.ETag = '"' + timestamp.toString() + '"';
   }
 
-  unauthorized (status, errMsg) {
+  unauthorized (status, errMsg, logMsg) {
     const realm = this.getHost();
     this._headers['WWW-Authenticate'] = `Bearer realm="${realm}" error="${errMsg}"`;
     this.response.writeHead(status, this._headers);
     this.response.end();
-    logRequest(this.request, this._username, status, 0, errMsg);
+    logRequest(this.request, this._username, status, 0, logMsg || errMsg);
   }
 }
 

lib/controllers/users.js

@@ -1,14 +1,16 @@
+const DISABLED_LOCALS = { title: 'Forbidden', message: 'Signing up is not allowed currently' };
+const DISABLED_LOG_NOTE = 'signups disabled';
 const Controller = require('./base');
 
 class Users extends Controller {
   showForm () {
-    if (!this.server._allow.signup) return this.errorPage(403, 'Forbidden');
+    if (!this.server._allow.signup) return this.renderHTML(403, 'error.html', DISABLED_LOCALS, DISABLED_LOG_NOTE);
     if (this.redirectToSSL()) return;
     this.renderHTML(200, 'signup.html', { title: 'Signup', params: this.params, error: null });
   }
 
   async register () {
-    if (!this.server._allow.signup) return this.errorPage(403, 'Forbidden');
+    if (!this.server._allow.signup) return this.renderHTML(403, 'error.html', DISABLED_LOCALS, DISABLED_LOG_NOTE);
     if (this.blockUnsecureRequest()) return;
 
     try {
@@ -17,7 +19,7 @@ class Users extends Controller {
         title: 'Signup Success',
         params: this.params,
         host: this.getHost()
-      });
+      }, `created user '${this.params.username}' w/ email '${this.params.email}'`, 'notice');
     } catch (error) {
       this.renderHTML(409, 'signup.html', {
         title: 'Signup Failure',

lib/logger.js

@@ -95,7 +95,7 @@ function logRequest (req, username, status, numBytesWritten, logNote, logLevel)
     level = logLevel;
   } else if (!status || status >= 500) { // presumably a coding error
     level = 'crit';
-  } else if ([401].includes(status)) { // user authentication is routine
+  } else if ([401].includes(status)) { // user authentication is notable
     level = 'notice';
   } else if (status >= 400) { // client error
     level = 'warning';

lib/stores/file_tree.js

@@ -93,7 +93,7 @@ class FileTree {
   async authenticate (params) {
     const username = params.username || '';
     const user = await this.readAuth(username);
-    if (!user) throw new Error('Username not found');
+    if (!user) throw new Error(`Username '${params.username}' not found`);
     const key = user.password.key;
     const hash = await core.hashPassword(params.password, user.password);
     if (hash.key === key) return true;

spec/store_spec.js

@@ -60,7 +60,7 @@ sharedExamplesFor('Stores', (store) => {
 
     it('returns an error if the user does not exist', () => {
       get.params.username = 'zeb';
-      return expect(get.authenticate).to.be.rejectedWith('Username not found');
+      return expect(get.authenticate).to.be.rejectedWith("Username 'zeb' not found");
     });
   });