import: Use the correct author ID when using sessions

There are two different ways an author ID becomes associated with a
user: either bound to a token or bound to a session ID. (The token and
session ID come from the `token` and `sessionID` cookies, or, in the
case of socket.io messages, from the `token` and `sessionID` message
properties.) When `settings.requireSession` is true or the user is
accessing a group pad, the session ID should be used. Otherwise the
token should be used.

Before this change, the `/p/:pad/import` handler was always using the
token, even when `settings.requireSession` was true. This caused the
following error because a different author ID was bound to the token
versus the session ID:

> Unable to import file into ${pad}. Author ${authorID} exists but he
> never contributed to this pad

This bug was reported in issue #4006. PR #4012 worked around the
problem by binding the same author ID to the token as well as the
session ID.

This change does the following:
  * Modifies the import handler to use the session ID to obtain the
    author ID (when appropriate).
  * Expands the documentation for the SecurityManager checkAccess
    function.
  * Removes the workaround from PR #4012.
  * Cleans up the `bin/createUserSession.js` test script.

bin/createUserSession.js

@@ -2,77 +2,46 @@
  * A tool for generating a test user session which can be used for debugging configs
  * that require sessions.
  */
+const m = (f) => __dirname + '/../' + f;
 
-const request = require('../src/node_modules/request');
-const settings = require(__dirname+'/../tests/container/loadSettings').loadSettings();
-const supertest = require(__dirname+'/../src/node_modules/supertest');
-const api = supertest('http://'+settings.ip+":"+settings.port);
-const path = require('path');
 const fs = require('fs');
+const path = require('path');
+const querystring = require('querystring');
+const request = require(m('src/node_modules/request'));
+const settings = require(m('src/node/utils/Settings'));
+const supertest = require(m('src/node_modules/supertest'));
 
-// get the API Key
-var filePath = path.join(__dirname, '../APIKEY.txt');
-var apikey = fs.readFileSync(filePath,  {encoding: 'utf-8'});
+(async () => {
+  const api = supertest('http://'+settings.ip+':'+settings.port);
 
-// Set apiVersion to base value, we change this later.
-var apiVersion = 1;
+  const filePath = path.join(__dirname, '../APIKEY.txt');
+  const apikey = fs.readFileSync(filePath,  {encoding: 'utf-8'});
 
-// Update the apiVersion
-api.get('/api/')
-  .expect(function(res){
-    apiVersion = res.body.currentVersion;
-    if (!res.body.currentVersion) throw new Error("No version set in API");
-    return;
-  })
-  .end(function(err, res){
-    // Now we know the latest API version, let's create a group
-    var uri = '/api/'+apiVersion+'/createGroup?apikey='+apikey;
-    api.post(uri)
-      .expect(function(res){
-        if (res.body.code === 1){
-          console.error("Error creating group", res.body);
-        }else{
-          var groupID = res.body.data.groupID;
-          console.log("groupID", groupID);
+  let res;
 
-          // creating a group pad
-          api.post('/api/'+apiVersion+'/createGroupPad?apikey='+apikey+'&groupID='+groupID)
-          .expect(function(res){
-            if (res.body.code === 1){
-              console.error("Error creating author", res.body);
-            }else{
-              console.log("Test Pad ID ====> ", res.body.data.padID)
-            }
-          }).end(function(){})
+  res = await api.get('/api/');
+  const apiVersion = res.body.currentVersion;
+  if (!apiVersion) throw new Error('No version set in API');
+  const uri = (cmd, args) => `/api/${apiVersion}/${cmd}?${querystring.stringify(args)}`;
 
-          // create an author
-          api.post('/api/'+apiVersion+'/createAuthor?apikey='+apikey)
-          .expect(function(res){
-            if (res.body.code === 1){
-              console.error("Error creating author", res.body);
-            }else{
-              console.log("authorID", res.body.data.authorID)
-              var authorID = res.body.data.authorID;
-              // create a session for this authorID
-              var validUntil = Math.floor(new Date() / 1000) + 60000;
-              console.log("validUntil", validUntil)
-              api.post('/api/'+apiVersion+'/createSession?apikey='+apikey + '&groupID='+groupID+'&authorID='+authorID+'&validUntil='+validUntil)
-              .expect(function(res){
-                if (res.body.code === 1){
-                  console.error("Error creating author", res.body);
-                }else{
-                  console.log("Session made: ====> create a cookie named sessionID and set it's value to ", res.body.data.sessionID);
-                }
-              })
-              .end(function(){}) // I shouldn't have nested but here we are..  it's not too ugly :P
+  res = await api.post(uri('createGroup', {apikey}));
+  if (res.body.code === 1) throw new Error(`Error creating group: ${res.body}`);
+  const groupID = res.body.data.groupID;
+  console.log('groupID', groupID);
 
-            }
-          })
-          .end(function(){})
+  res = await api.post(uri('createGroupPad', {apikey, groupID}));
+  if (res.body.code === 1) throw new Error(`Error creating group pad: ${res.body}`);
+  console.log('Test Pad ID ====> ', res.body.data.padID);
 
-        }
-        return;
-      })
-      .end(function(){})
-  });
-// end
+  res = await api.post(uri('createAuthor', {apikey}));
+  if (res.body.code === 1) throw new Error(`Error creating author: ${res.body}`);
+  const authorID = res.body.data.authorID;
+  console.log('authorID', authorID);
+
+  const validUntil = Math.floor(new Date() / 1000) + 60000;
+  console.log('validUntil', validUntil);
+  res = await api.post(uri('createSession', {apikey, groupID, authorID, validUntil}));
+  if (res.body.code === 1) throw new Error(`Error creating session: ${res.body}`);
+  console.log('Session made: ====> create a cookie named sessionID and set the value to',
+              res.body.data.sessionID);
+})();

src/node/db/AuthorManager.js

@@ -77,17 +77,6 @@ exports.createAuthorIfNotExistsFor = async function(authorMapper, name)
   return author;
 };
 
-/**
- * Sets the token <> AuthorID relationship.
- * Discussion at https://github.com/ether/etherpad-lite/issues/4006
- * @param {String} token The token (generated by a client)
- * @param {String} authorID The authorID (returned by the Security Manager)
- */
-exports.setToken2Author = async function(token, authorID)
-{
-  await db.set("token2author:"+token, authorID);
-}
-
 /**
  * Returns the AuthorID for a mapper. We can map using a mapperkey,
  * so far this is token2author and mapper2author

src/node/db/SecurityManager.js

@@ -31,15 +31,25 @@ const WRONG_PASSWORD = Object.freeze({accessStatus: 'wrongPassword'});
 const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'});
 
 /**
- * This function controlls the access to a pad, it checks if the user can access a pad.
- * @param padID the pad the user wants to access
- * @param sessionCookie the session the user has (set via api)
- * @param token a random token representing the author, of the form
- *              t.randomstring_of_lenght_20. The random string is generated by
- *              the client.
- *              Used for every pad in the web UI. Not used for the HTTP API.
- * @param password the password the user has given to access this pad, can be null
- * @return {accessStatus: grant|deny|wrongPassword|needPassword, authorID: a.xxxxxx})
+ * Determines whether the user can access a pad.
+ *
+ * @param padID identifies the pad the user wants to access.
+ * @param sessionCookie identifies the sessions the user created via the HTTP API, if any.
+ *     Note: The term "session" used here is unrelated to express-session.
+ * @param token is a random token of the form t.randomstring_of_length_20 generated by the client
+ *     when using the web UI (not the HTTP API). This token is only used if settings.requireSession
+ *     is false and the user is accessing a public pad. If there is not an author already associated
+ *     with this token then a new author object is created (including generating an author ID) and
+ *     associated with this token.
+ * @param password is the password the user has given to access this pad. It can be null.
+ * @return {accessStatus: grant|deny|wrongPassword|needPassword, authorID: a.xxxxxx}. The caller
+ *     must use the author ID returned in this object when making any changes associated with the
+ *     author.
+ *
+ * WARNING: Tokens and session IDs MUST be kept secret, otherwise users will be able to impersonate
+ * each other (which might allow them to gain privileges).
+ *
+ * TODO: Add a hook so that plugins can make access decisions.
  */
 exports.checkAccess = async function(padID, sessionCookie, token, password)
 {

src/node/handler/PadMessageHandler.js

@@ -36,6 +36,7 @@ var hooks = require("ep_etherpad-lite/static/js/pluginfw/hooks.js");
 var channels = require("channels");
 var stats = require('../stats');
 var remoteAddress = require("../utils/RemoteAddress").remoteAddress;
+const assert = require('assert').strict;
 const nodeify = require("nodeify");
 const { RateLimiterMemory } = require('rate-limiter-flexible');
 
@@ -260,20 +261,11 @@ exports.handleMessage = async function(client, message)
 
   let dropMessage = await handleMessageHook();
   if (!dropMessage) {
-
-    // check permissions
-
     if (message.type == "CLIENT_READY") {
       // client tried to auth for the first time (first msg from the client)
       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
-
     // the session may have been dropped during earlier processing
     if (!sessioninfos[client.id]) {
       messageLogger.warn("Dropping message from a connection that has gone away.")
@@ -901,12 +893,6 @@ async function handleClientReady(client, message)
   // Get ro/rw id:s
   let padIds = await readOnlyManager.getIds(message.padId);
 
-  // check permissions
-
-  // Note: message.sessionID is an entierly 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
   let statusObject = await securityManager.checkAccess(padIds.padId, message.sessionID, message.token, message.password);
   let accessStatus = statusObject.accessStatus;
@@ -920,19 +906,11 @@ async function handleClientReady(client, message)
   let author = statusObject.authorID;
 
   // get all authordata of this new user
+  assert(author);
   let value = await authorManager.getAuthor(author);
   let authorColorId = value.colorId;
   let authorName = value.name;
 
-  /*
-  * Here we know authorID, token and session.  We should ?always? store it..
-  * TODO: I fear that this might allow a user to pass a token for an authorID
-  * meaning that they could in theory "imitate" another author?
-  * Perhaps the fix to this is check to see if it exists first and if it
-  * does then abort..  Details: https://github.com/ether/etherpad-lite/issues/4006
-  */
-  await authorManager.setToken2Author(message.token, statusObject.authorID)
-
   // load the pad-object from the database
   let pad = await padManager.getPad(padIds.padId);
 

src/node/hooks/express/importexport.js

@@ -1,3 +1,4 @@
+const assert = require('assert').strict;
 var hasPadAccess = require("../../padaccess");
 var settings = require('../../utils/Settings');
 var exportHandler = require('../../handler/ExportHandler');
@@ -5,6 +6,7 @@ var importHandler = require('../../handler/ImportHandler');
 var padManager = require("../../db/PadManager");
 var authorManager = require("../../db/AuthorManager");
 const rateLimit = require("express-rate-limit");
+const securityManager = require("../../db/SecurityManager");
 
 settings.importExportRateLimiting.onLimitReached = function(req, res, options) {
   // when the rate limiter triggers, write a warning in the logs
@@ -51,57 +53,41 @@ exports.expressCreateServer = function (hook_name, args, cb) {
   // handle import requests
   args.app.use('/p/:pad/import', limiter);
   args.app.post('/p/:pad/import', async function(req, res, next) {
-    if (await hasPadAccess(req, res)) {
-      let exists = await padManager.doesPadExists(req.params.pad);
-      if (!exists) {
-        console.warn(`Someone tried to import into a pad that doesn't exist (${req.params.pad})`);
-        return next();
-      }
-
-      /*
-       * Starting from Etherpad 1.8.3 onwards, importing into a pad is allowed
-       * only if a user has his browser opened and connected to the pad (i.e. a
-       * Socket.IO session is estabilished for him) and he has already
-       * contributed to that specific pad.
-       *
-       * Note that this does not have anything to do with the "session", used
-       * for logging into "group pads". That kind of session is not needed here.
-       *
-       * This behaviour does not apply to API requests, only to /p/$PAD$/import
-       *
-       * See: https://github.com/ether/etherpad-lite/pull/3833#discussion_r407490205
-       */
-      if (!req.cookies && !settings.allowAnyoneToImport) {
-        console.warn(`Unable to import file into "${req.params.pad}". No cookies included in request`);
-        return next();
-      }
-
-      if (!req.cookies.token && !settings.allowAnyoneToImport) {
-        console.warn(`Unable to import file into "${req.params.pad}". No token in the cookies`);
-        return next();
-      }
-
-      let author = await authorManager.getAuthor4Token(req.cookies.token);
-      // author is of the form: "a.g2droBYw1prY7HW9"
-      if (!author && !settings.allowAnyoneToImport) {
-        console.warn(`Unable to import file into "${req.params.pad}". No Author found for token ${req.cookies.token}`);
-
-        return next();
-      }
-
-      let authorsPads = await authorManager.listPadsOfAuthor(author);
-      if (!authorsPads && !settings.allowAnyoneToImport) {
-        console.warn(`Unable to import file into "${req.params.pad}". Author "${author}" exists but he never contributed to any pad`);
-        return next();
-      }
-
-      let authorsPadIDs = authorsPads.padIDs;
-      if ( (authorsPadIDs.indexOf(req.params.pad) === -1) && !settings.allowAnyoneToImport) {
-        console.warn(`Unable to import file into "${req.params.pad}". Author "${author}" exists but he never contributed to this pad`);
-        return next();
-      }
-
-      importHandler.doImport(req, res, req.params.pad);
+    if (!(await padManager.doesPadExists(req.params.pad))) {
+      console.warn(`Someone tried to import into a pad that doesn't exist (${req.params.pad})`);
+      return next();
     }
+
+    const {accessStatus, authorID} = await securityManager.checkAccess(
+      req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password);
+    if (accessStatus !== 'grant') return res.status(403).send('Forbidden');
+    assert(authorID);
+
+    /*
+     * Starting from Etherpad 1.8.3 onwards, importing into a pad is allowed
+     * only if a user has his browser opened and connected to the pad (i.e. a
+     * Socket.IO session is estabilished for him) and he has already
+     * contributed to that specific pad.
+     *
+     * Note that this does not have anything to do with the "session", used
+     * for logging into "group pads". That kind of session is not needed here.
+     *
+     * This behaviour does not apply to API requests, only to /p/$PAD$/import
+     *
+     * See: https://github.com/ether/etherpad-lite/pull/3833#discussion_r407490205
+     */
+    if (!settings.allowAnyoneToImport) {
+      const authorsPads = await authorManager.listPadsOfAuthor(authorID);
+      if (!authorsPads) {
+        console.warn(`Unable to import file into "${req.params.pad}". Author "${authorID}" exists but he never contributed to any pad`);
+        return next();
+      }
+      if (authorsPads.padIDs.indexOf(req.params.pad) === -1) {
+        console.warn(`Unable to import file into "${req.params.pad}". Author "${authorID}" exists but he never contributed to this pad`);
+        return next();
+      }
+    }
+
+    importHandler.doImport(req, res, req.params.pad);
   });
 }