Fixes #105: separates log messages from user messages (#106)

* Fixes #105: separates log messages from user messages

Adds logNote and logLevel param to renderHTML() and uses them in most calls.

bin/dev-conf.json

@@ -10,6 +10,6 @@
   "logging": {
     "log_dir": "./dev-log",
     "stdout": ["debug"],
-    "log_files": ["error"]
+    "log_files": ["notice"]
   }
 }

lib/armadietto.js

@@ -202,7 +202,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 {
@@ -216,7 +216,8 @@ class Armadietto {
         getLogger().error('Storage Error:', e);
       }
     }
-    new Assets(this, req, res).errorPage(404, pathname + ' Not found');
+    const locals = { title: 'Not found', message: `“${pathname}” doesn't exist` };
+    new Assets(this, req, res).errorPage(404, locals, `no handler for path '${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.errorPage(404, locals, `no file '${path.join(assetDir, filename)}'`);
     }
   }
 }

lib/controllers/base.js

@@ -102,12 +102,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 {string} [locals.title] - external page title
+   * @param {string} [locals.message] - User-friendly text pointing user toward action to resolve issue
+   * @param {Error} [locals.error]
+   * @param {string} [locals.username]
+   * @param {string} [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',
@@ -132,11 +146,25 @@ 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);
+    logRequest(this.request, username, status, html.length, logNote || locals.error || locals.message, logLevel);
   }
 
-  errorPage (status, message) {
-    this.renderHTML(status, 'error.html', { title: 'Error', status, message });
+  /**
+   * Renders error page with message or locals
+   * @param {number} status - HTTP status
+   * @param {string|Object} [messageOrLocals] - A string is the `message` field for template. An object is multiple fields.
+   * @param {string} [messageOrLocals.title] - external page title
+   * @param {string} [messageOrLocals.message] - User-friendly text pointing user toward action to resolve issue
+   * @param {string} [messageOrLocals.username]
+   * @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
+   */
+  errorPage (status, messageOrLocals, logNote = '', logLevel = undefined) {
+    const locals = typeof messageOrLocals === 'string' ? { message: messageOrLocals } : messageOrLocals;
+    if (!locals.title) {
+      locals.title = 'Error';
+    }
+    this.renderHTML(status, 'error.html', locals, logNote, logLevel);
   }
 }
 

lib/controllers/storage.js

@@ -84,7 +84,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();
@@ -139,7 +139,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;
     }
 
@@ -159,9 +159,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 calling methods; it's not an OAUTH error
+      getLogger().crit('Bad store.permissions implementation?', { error: e.message });
+      this.unauthorized(400, 'invalid_request', e);
       return false;
     }
 
@@ -181,7 +181,7 @@ class Storage extends Controller {
         return true;
       }
     }
-    this.unauthorized(403, 'insufficient_scope');
+    this.unauthorized(403, 'insufficient_scope', `user has permissions '${JSON.stringify(permissions)}' but lacks '${permission}'`);
     return false;
   }
 
@@ -197,12 +197,18 @@ class Storage extends Controller {
     this._headers.ETag = '"' + timestamp.toString() + '"';
   }
 
-  unauthorized (status, errMsg) {
+  /**
+   * Renders error response
+   * @param {number} status - HTTP status
+   * @param {string} errMsg - OAUTH code: invalid_request, access_denied, invalid_scope, etc.
+   * @param {string|Error} [logMsg] - should concisely give details & be distinct from other calls
+   */
+  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.errorPage(403, 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.errorPage(403, DISABLED_LOCALS, DISABLED_LOG_NOTE);
     if (this.blockUnsecureRequest()) return;
 
     try {
@@ -17,14 +19,14 @@ 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',
         params: this.params,
         error,
         message: error.message
-      });
+      }, `while signing up: ${error.message || error.code || error.name || error.toString()}`);
     }
   }
 
@@ -83,7 +85,8 @@ class Users extends Controller {
           : []
       });
     } catch (error) {
-      this.renderHTML(409, 'login.html', { params: this.params, error });
+      this.renderHTML(409, 'login.html', { params: this.params, error },
+        `while showing account: ${error.message || error.code || error.name || error.toString()}`);
     }
   }
 }

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;

lib/views/login.html

@@ -11,11 +11,11 @@ <h2>Log In</h2>
     <table>
       <tr>
         <th scope="row"><label for="username">Username</label></th>
-        <td><input type="text" id="username" name="username" value="<%= params.username || '' %>"></td>
+        <td><input type="text" id="username" name="username" value="<%= params.username || '' %>" autocomplete="username"></td>
       </tr>
       <tr>
         <th scope="row"><label for="password">Password</label></th>
-        <td><input type="password" id="password" name="password" value=""></td>
+        <td><input type="password" id="password" name="password" value="" autocomplete="current-password"></td>
       </tr>
     </table>
     <button type="submit" name="login" value="Go">Login</button>

notes/protocol.md

@@ -116,7 +116,7 @@ Response codes:
 * `304`: `GET` where precondition fails
 * `401`: tokens with insufficient permissions
 * `404`: `GET` or `DELETE` to non-existing documents
-* `409`: `PUT` or `DELTE` where precondition fails
+* `409`: `PUT` or `DELETE` where precondition fails
 * `420`: client violates rate limit or behaves maliciously
 * `500`: internal server error
 

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");
     });
   });