Repository: rickbergfalk/sqlpad (js)

Issue:
Use `userId` for user references instead of email address
Now that the backing database has moved from nosql to relational, I'd like to update the implementation to use a more traditional id foreign key reference for user.

`queries.created_by` and `queries.updated_by` are currently strings of the email address of the user. `query_acl.user_email` can be used for setting access. 

In the case of `query_acl`, the option to store user email can be removed. From an API/seed data standpoint, user_email can be provided, but it will translate to user id prior to being stored.

For queries, `created_by`/`updated_by` can either be updated to store user id, or another field can be added in its place. 

Pull request:
Use userId for queries user references
Fixes #745 

Updates `queries` and `query_acl` tables to use user id to store user references instead of user email. 

An overview of changes involved:

* `queries.created_by` now stores `user.id` value. During migration, if original `created_by` value was not found, a disabled user was added to the `users` table for reference
* `queries.updated_by` was updated in same fashion `created_by` was
* `query_acl.user_email` was migrated to populate `user_id` column instead with the appropriate user id value for the corresponding email address. Again if email was not found, a disabled user was added.
* Seed query import follows the same process as migration, ensuring email address references are corrected accordingly
* Seed query import is not strict with userId/userEmail/groupId references, in that only 1 may be specified per object. If more than 1 reference is provided per object, an error is thrown.
* `api/queries` now return a `createdByUser` object, corresponding to summary information about the userId in `created_by` column. `updatedByUser` is returned similarly for single user query API operations (create, update, get). These objects have user information `{ id, name, email }`.

Patch:
diff --git a/client/src/queries/QueryPreview.js b/client/src/queries/QueryPreview.js
index 0e2a50f9..716d9cf0 100644
--- a/client/src/queries/QueryPreview.js
+++ b/client/src/queries/QueryPreview.js
@@ -10,12 +10,19 @@ function QueryPreview({ query }) {
     return null;
   }
 
+  let userReference = query.createdBy;
+  if (query.createdByUser) {
+    userReference = query.createdByUser.name || query.createdByUser.email;
+  }
+
+  const connectionName = query.connection && query.connection.name;
+
   return (
     <div className={styles.preview}>
       <div>
         <div className={styles.previewQueryName}>{query.name}</div>
-        <div>Connection {query.connectionName}</div>
-        <div>By {query.createdBy}</div>
+        <div>Connection {connectionName}</div>
+        <div>By {userReference}</div>
         <div>
           {query.tags && query.tags.map((tag) => <Tag key={tag}>{tag}</Tag>)}
         </div>
diff --git a/docs/seed-data.md b/docs/seed-data.md
index e28ff1b5..a38f238a 100644
--- a/docs/seed-data.md
+++ b/docs/seed-data.md
@@ -47,9 +47,11 @@ Example seed connection JSON file:
 
 Queries are created or replaced matching on query id. At this time the query ACL implementation controls whether queries may be updated within SQLPad. It is entirely possible for these to be loaded, altered in the UI, then have those changes lost on next server start.
 
-At this point SQLPad does not enforce referential integrity, so queries may be created with a `createdBy` containing an email address for a user that does not exist.
+`createdBy` may contain either a user's email address, or the id of the user that owns the query. If a user does not exist in SQLPad with either that email address or id, a disabled user record will be added for reference. The `created_by` column in the `queries` table will be populated with the found or created user record.
 
-Example seed query JSON file (comments only added for doc purposes):
+The `acl` entries work in a similar way, but are explicit about field names. If providing the email address of a user that has access to the query, use `userEmail`. Reference the SQLPad userId in the `userId` field.
+
+Example seed query JSON file (comments only added for documentaion purposes):
 
 ```js
 {
@@ -57,6 +59,7 @@ Example seed query JSON file (comments only added for doc purposes):
   "name": "Seed query 1",
   "connectionId": "seed-connection-1",
   "queryText": "SELECT * FROM seed_table",
+  // email address or user id (preferred)
   "createdBy": "admin@sqlpad.com",
   "acl": [
     // an ACL entry with write=false allows that user to read
@@ -67,7 +70,8 @@ Example seed query JSON file (comments only added for doc purposes):
       "write": false
     },
     // ACL entry can also be specified with a users email address.
-    // The user does not need to exist in SQLPad at this point
+    // SQLPad will translate this email address to the corresponding userId stored in SQLPad
+    // If no user is found for this email address, a disabled user account will be added
     {
       "userEmail": "someone@sqlpad.com",
       "write": true
@@ -80,3 +84,22 @@ Example seed query JSON file (comments only added for doc purposes):
   ]
 }
 ```
+
+When providing `acl` values, do not provide more than one `userId`, `userEmail`, or `groupId` reference per `acl` object. This will throw an error:
+
+```js
+{
+  "id": "seed-query-1",
+  "name": "Seed query 1",
+  "connectionId": "seed-connection-1",
+  "queryText": "SELECT * FROM seed_table",
+  "createdBy": "admin@sqlpad.com",
+  "acl": [
+    {
+      "userId": "some-userId-in-sqlpad",
+      "userEmail": "someone@sqlpad.com",
+      "write": false
+    }
+  ]
+}
+```
diff --git a/server/lib/decorate-query-user-access.js b/server/lib/decorate-query-user-access.js
index bce58738..c762d1b9 100644
--- a/server/lib/decorate-query-user-access.js
+++ b/server/lib/decorate-query-user-access.js
@@ -13,7 +13,7 @@ function decorateQueryUserAccess(query, user) {
   clone.canWrite = false;
   clone.canDelete = false;
 
-  if (user.role === 'admin' || user.email === clone.createdBy) {
+  if (user.role === 'admin' || user.id === clone.createdBy) {
     clone.canRead = true;
     clone.canWrite = true;
     clone.canDelete = true;
@@ -21,10 +21,7 @@ function decorateQueryUserAccess(query, user) {
     const writeAcl = clone.acl
       // filter acl records that match for this user
       .filter(
-        (acl) =>
-          acl.groupId === consts.EVERYONE_ID ||
-          acl.userId === user.id ||
-          acl.userEmail === user.email
+        (acl) => acl.groupId === consts.EVERYONE_ID || acl.userId === user.id
       )
       // and return first one that has write
       .find((a) => a.write === true);
@@ -32,10 +29,7 @@ function decorateQueryUserAccess(query, user) {
 
     // A record in ACL allows read permissions
     const canRead = query.acl.find(
-      (acl) =>
-        acl.groupId === consts.EVERYONE_ID ||
-        acl.userId === user.id ||
-        acl.userEmail === user.email
+      (acl) => acl.groupId === consts.EVERYONE_ID || acl.userId === user.id
     );
     clone.canRead = Boolean(canRead);
   }
diff --git a/server/lib/load-seed-data.js b/server/lib/load-seed-data.js
index 811bd6d2..9173e655 100644
--- a/server/lib/load-seed-data.js
+++ b/server/lib/load-seed-data.js
@@ -35,6 +35,31 @@ async function getItemDirectoryData(appLog, seedDataPath, dir) {
   return items;
 }
 
+/**
+ * User references from seed data can be email or user id
+ * This tries to find the user referenced, and creates a disabled user if one is not found
+ * @param {import('../models')} models
+ * @param {string} emailOrId
+ */
+async function findOrCreateUserReference(models, emailOrId) {
+  let user = await models.users.findOneByEmail(emailOrId.toLowerCase());
+  if (!user) {
+    user = await models.users.findOneById(emailOrId);
+  }
+
+  // If user isn't found, we will add disabled user to reference
+  if (!user) {
+    user = await models.users.create({
+      id: emailOrId.trim(),
+      name: emailOrId.split('@')[0],
+      email: emailOrId.toLowerCase().trim(),
+      role: 'editor',
+      disabled: true,
+    });
+  }
+  return user;
+}
+
 /**
  * @param {*} appLog
  * @param {*} config
@@ -53,7 +78,57 @@ async function loadSeedData(appLog, config, models) {
   const queries = await getItemDirectoryData(appLog, seedDataPath, 'queries');
   for (const query of queries) {
     const existing = await models.queries.findOneById(query.id);
-    const data = { ...existing, ...query };
+
+    // Find actual createdBy user.
+    // query.createdBy could be email or user id
+    let createdByUser = await findOrCreateUserReference(
+      models,
+      query.createdBy
+    );
+    let updatedByUser = createdByUser;
+    if (query.updatedBy) {
+      updatedByUser = await findOrCreateUserReference(models, query.updatedBy);
+    }
+
+    if (query.acl) {
+      for (const acl of query.acl) {
+        let fieldCount = 0;
+        if (acl.userId) {
+          fieldCount += 1;
+        }
+        if (acl.userEmail) {
+          fieldCount += 1;
+        }
+        if (acl.groupId) {
+          fieldCount += 1;
+        }
+        if (fieldCount > 1) {
+          throw new Error(
+            'Query seed import - only specify one of userId, userEmail, or groupId'
+          );
+        }
+
+        if (acl.userEmail) {
+          const aclUser = await findOrCreateUserReference(
+            models,
+            acl.userEmail
+          );
+          delete acl.userEmail;
+          acl.userId = aclUser.id;
+        }
+      }
+    }
+
+    const data = {
+      ...existing,
+      ...query,
+      createdBy: createdByUser.id,
+      updatedBy: updatedByUser.id,
+    };
+
+    // TODO - only update query if query data is different
+    // Aways updating is causing updatedAt to change, which is bringing these results to the top of UI
+    // This will require taking a subset of query data and performing a deep equal
     await models.upsertQuery(data);
   }
 
diff --git a/server/lib/push-query-to-slack.js b/server/lib/push-query-to-slack.js
index 84c4d92b..3ab4eb38 100644
--- a/server/lib/push-query-to-slack.js
+++ b/server/lib/push-query-to-slack.js
@@ -1,7 +1,7 @@
 const appLog = require('./app-log');
 const request = require('request');
 
-function pushQueryToSlack(config, query) {
+function pushQueryToSlack(config, query, user) {
   const SLACK_WEBHOOK = config.get('slackWebhook');
   if (SLACK_WEBHOOK) {
     const PUBLIC_URL = config.get('publicUrl');
@@ -13,7 +13,7 @@ function pushQueryToSlack(config, query) {
         text: `New Query <${PUBLIC_URL}${BASE_URL}/queries/${query.id}|${
           query.name
         }> 
-saved by ${query.updatedBy || query.createdBy} on SQLPad 
+saved by ${user.name || user.email} on SQLPad 
 ${'```sql'}
 ${query.queryText}
 ${'```'}`,
diff --git a/server/migrations/04-00900-query-user-id.js b/server/migrations/04-00900-query-user-id.js
new file mode 100644
index 00000000..8ad1254b
--- /dev/null
+++ b/server/migrations/04-00900-query-user-id.js
@@ -0,0 +1,219 @@
+/* eslint-disable no-await-in-loop */
+const Sequelize = require('sequelize');
+const _ = require('lodash');
+const { v4: uuidv4 } = require('uuid');
+const migrationUtils = require('../lib/migration-utils');
+
+/**
+ * @param {import('sequelize').QueryInterface} queryInterface
+ * @param {import('../lib/config')} config
+ * @param {import('../lib/logger')} appLog
+ * @param {object} nedb - collection of nedb objects created in /lib/db.js
+ * @param {object} sequelizeDb - sequelize instance
+ */
+// eslint-disable-next-line no-unused-vars
+async function up(queryInterface, config, appLog, nedb, sequelizeDb) {
+  // Get distinct list of emails used for userids
+  // Trim and lower case them
+  // For each email, see if a user exists for it
+  // If user does not exist, create a user record, disabled for that lower cased email
+  const rows = await sequelizeDb.query(
+    `
+      SELECT DISTINCT created_by AS email FROM queries WHERE created_by IS NOT NULL
+      UNION
+      SELECT DISTINCT updated_by AS email FROM queries WHERE updated_by IS NOT NULL
+      UNION 
+      SELECT DISTINCT user_email AS email FROM query_acl WHERE user_email IS NOT NULL
+    `,
+    {
+      type: Sequelize.QueryTypes.SELECT,
+    }
+  );
+
+  // lowercase and trim emails
+  let emails = rows.map((row) => {
+    return (row.email || '').trim().toLowerCase();
+  });
+  emails = _.uniq(emails);
+
+  const usersToCreate = [];
+
+  for (const email of emails) {
+    const existingUsers = await sequelizeDb.query(
+      `SELECT id FROM users WHERE email = :email`,
+      {
+        replacements: { email },
+        type: Sequelize.QueryTypes.SELECT,
+      }
+    );
+
+    if (existingUsers.length === 0) {
+      usersToCreate.push({
+        id: uuidv4(),
+        name: email.split('@')[0],
+        email,
+        role: 'editor',
+        disabled: true,
+        created_at: new Date(),
+        updated_at: new Date(),
+      });
+    }
+  }
+
+  if (usersToCreate.length > 0) {
+    await queryInterface.bulkInsert('users', usersToCreate);
+  }
+
+  // At this point get all users, and create a map converting email -> id
+  // All email addresses in the system should map to some sort of user account
+  let users = await sequelizeDb.query(`SELECT id, email FROM users`, {
+    type: Sequelize.QueryTypes.SELECT,
+  });
+
+  const emailId = {};
+  users.forEach((user) => {
+    emailId[user.email] = user.id;
+  });
+
+  const transaction = await sequelizeDb.transaction();
+  try {
+    /**
+     * Migrate created_by & updated_by for queries
+     */
+    const queries = await sequelizeDb.query(
+      `SELECT id, created_by, updated_by FROM queries`,
+      {
+        type: Sequelize.QueryTypes.SELECT,
+        transaction,
+      }
+    );
+
+    for (const query of queries) {
+      const replacements = {
+        id: query.id,
+        createdBy: emailId[query.created_by.trim().toLowerCase()],
+        updatedBy: null,
+      };
+      if (query.updated_by) {
+        replacements.updatedBy = emailId[query.updated_by.trim().toLowerCase()];
+      }
+
+      await sequelizeDb.query(
+        `
+          UPDATE queries 
+          SET 
+            created_by = :createdBy, 
+            updated_by = :updatedBy 
+          WHERE
+            id = :id
+        `,
+        {
+          type: Sequelize.QueryTypes.UPDATE,
+          replacements,
+          transaction,
+        }
+      );
+    }
+
+    /**
+     * Migrate user_email for query_acl
+     */
+    const acls = await sequelizeDb.query(
+      `
+        SELECT id, user_id, user_email
+        FROM query_acl
+        WHERE user_email IS NOT NULL
+      `,
+      {
+        type: Sequelize.QueryTypes.SELECT,
+        transaction,
+      }
+    );
+
+    for (const acl of acls) {
+      const replacements = {
+        id: acl.id,
+        userId: emailId[acl.user_email.trim().toLowerCase()],
+      };
+      await sequelizeDb.query(
+        `
+          UPDATE query_acl 
+          SET 
+            user_id = :userId
+          WHERE
+            id = :id
+        `,
+        {
+          type: Sequelize.QueryTypes.UPDATE,
+          replacements,
+          transaction,
+        }
+      );
+    }
+
+    await transaction.commit();
+  } catch (error) {
+    await transaction.rollback();
+    throw error;
+  }
+
+  // Remove user_email column
+  // Each DB behaves differently here.
+  // These constraint/index removes are wrapped in try/catches and ignored to handle this.
+  // MySQL needs this constraint removed and is fine otherwise
+  // SQL Server needs the index dropped
+  // SQLite has no clue. It seems to rebuild the table when a column is removed and indexes need to be added back
+  try {
+    await queryInterface.removeConstraint(
+      'query_acl',
+      'query_acl_user_email_query_id_key'
+    );
+  } catch (error) {
+    // ignore error. constraint may not exist depending on backend database
+  }
+  try {
+    await queryInterface.removeIndex(
+      'query_acl',
+      'query_acl_user_email_query_id_key'
+    );
+  } catch (error) {
+    // ignore error. constraint may not exist depending on backend database
+  }
+  await queryInterface.removeColumn('query_acl', 'user_email');
+
+  // For SQLite constraints appear to be lost when a column is removed.
+  // Re-add the index if necessary
+  await migrationUtils.addOrReplaceIndex(
+    queryInterface,
+    'query_acl',
+    'query_acl_group_id_query_id_key',
+    ['group_id', 'query_id'],
+    {
+      unique: true,
+      where: {
+        group_id: {
+          [Sequelize.Op.ne]: null,
+        },
+      },
+    }
+  );
+
+  await migrationUtils.addOrReplaceIndex(
+    queryInterface,
+    'query_acl',
+    'query_acl_user_id_query_id_key',
+    ['user_id', 'query_id'],
+    {
+      unique: true,
+      where: {
+        user_id: {
+          [Sequelize.Op.ne]: null,
+        },
+      },
+    }
+  );
+}
+
+module.exports = {
+  up,
+};
diff --git a/server/models/index.js b/server/models/index.js
index 6e2c1a29..d0abd6b1 100644
--- a/server/models/index.js
+++ b/server/models/index.js
@@ -72,6 +72,28 @@ class Models {
     }
     query.acl = await this.queryAcl.findAllByQueryId(id);
     query.acl = query.acl.map((acl) => acl.toJSON());
+
+    // TODO this method returns more info that regular users should have
+    // Should there be a smaller users find one?
+    // Maybe just select the fields needed now that an ORM is in place
+    // Not everything neatly maps to access patterns needed
+    const createdBy = await this.users.findOneById(query.createdBy);
+    const updatedBy = await this.users.findOneById(query.updatedBy);
+
+    query.createdByUser = {
+      id: createdBy.id,
+      name: createdBy.name,
+      email: createdBy.email,
+    };
+
+    if (updatedBy) {
+      query.updatedByUser = {
+        id: updatedBy.id,
+        name: createdBy.name,
+        email: createdBy.email,
+      };
+    }
+
     return query;
   }
 
diff --git a/server/routes/queries.js b/server/routes/queries.js
index 0313d996..8ca6e6d2 100644
--- a/server/routes/queries.js
+++ b/server/routes/queries.js
@@ -79,13 +79,16 @@ async function listQueries(req, res) {
       queries.name,
       queries.chart,
       queries.query_text,
-      queries.created_by,
       queries.connection_id,
       connections.name AS connection_name,
-      connections.driver AS connection_driver
+      connections.driver AS connection_driver,
+      queries.created_by AS created_by_user_id,
+      users.name AS created_by_user_name,
+      users.email AS created_by_user_email
     FROM
       queries
       LEFT JOIN connections ON queries.connection_id = connections.id
+      LEFT JOIN users ON queries.created_by = users.id
   `;
   const whereSqls = [];
   const params = {};
@@ -100,11 +103,11 @@ async function listQueries(req, res) {
 
   if (typeof ownedByUser === 'string') {
     if (ownedByUser === 'true') {
-      whereSqls.push('queries.created_by = :userEmail');
+      whereSqls.push('queries.created_by = :userId');
     } else if (ownedByUser === 'false') {
-      whereSqls.push('queries.created_by != :userEmail');
+      whereSqls.push('queries.created_by != :userId');
     }
-    params.userEmail = user.email;
+    params.userId = user.id;
   }
 
   if (tags) {
@@ -125,17 +128,15 @@ async function listQueries(req, res) {
   // User can see queries they've created, or queries they have access to via ACL
   if (user.role !== 'admin') {
     whereSqls.push(`
-      queries.created_by = :userEmail
+      queries.created_by = :userId
       OR queries.id IN ( 
         SELECT qa.query_id 
         FROM query_acl qa 
         WHERE 
           qa.group_id = '__EVERYONE__' 
-          OR user_id = :userId 
-          OR user_email = :userEmail 
+          OR user_id = :userId
       )
     `);
-    params.userEmail = user.email;
     params.userId = user.id;
   }
 
@@ -203,12 +204,17 @@ async function listQueries(req, res) {
       name: query.name,
       chart: typeof query.chart === 'string' ? JSON.parse(query.chart) : null,
       queryText: query.query_text,
-      createdBy: query.created_by,
       connection: {
         id: query.connection_id,
         name: query.connection_name,
         driver: query.connection_driver,
       },
+      createdBy: query.created_by_user_id,
+      createdByUser: {
+        id: query.created_by_user_id,
+        name: query.created_by_user_name,
+        email: query.created_by_user_email,
+      },
     };
   });
   const queryIds = queries.map((query) => query.id);
@@ -295,7 +301,6 @@ router.get('/api/queries/:id', mustBeAuthenticated, wrap(getQuery));
 async function createQuery(req, res) {
   const { models, body, user } = req;
   const { name, tags, connectionId, queryText, chart, acl } = body;
-  const { email } = user;
 
   const query = {
     name: name || 'No Name Query',
@@ -303,15 +308,15 @@ async function createQuery(req, res) {
     connectionId,
     queryText,
     chart,
-    createdBy: email,
-    updatedBy: email,
+    createdBy: user.id,
+    updatedBy: user.id,
     acl,
   };
 
   const newQuery = await models.upsertQuery(query);
 
   // This is async, but save operation doesn't care about when/if finished
-  pushQueryToSlack(req.config, newQuery);
+  pushQueryToSlack(req.config, newQuery, user);
 
   return res.utils.data(decorateQueryUserAccess(newQuery, user));
 }
@@ -344,7 +349,7 @@ async function updateQuery(req, res) {
     connectionId,
     queryText,
     chart,
-    updatedBy: user.email,
+    updatedBy: user.id,
     acl,
   });
 
diff --git a/server/routes/users.js b/server/routes/users.js
index 29127981..1c3dca70 100644
--- a/server/routes/users.js
+++ b/server/routes/users.js
@@ -117,6 +117,7 @@ async function deleteUser(req, res) {
 
 router.get('/api/users', mustBeAuthenticated, wrap(listUsers));
 router.post('/api/users', mustBeAdmin, wrap(createUser));
+// TODO allow regular users to use getUser API, but restrict data returned
 router.get('/api/users/:id', mustBeAdmin, wrap(getUser));
 router.put('/api/users/:id', mustBeAdmin, wrap(updateUser));
 router.delete('/api/users/:id', mustBeAdmin, wrap(deleteUser));
diff --git a/server/sequelize-db/queries.js b/server/sequelize-db/queries.js
index a0f77803..b7f291e9 100644
--- a/server/sequelize-db/queries.js
+++ b/server/sequelize-db/queries.js
@@ -39,14 +39,12 @@ module.exports = function (sequelize) {
           },
         },
       },
-      // createdBy is an email address
-      // (possibly weird, but user ids may not be known ahead of time
-      // email is human friendly too
+      // createdBy used to be email address, but now stores userId as of v5
       createdBy: {
         type: DataTypes.STRING,
         allowNull: false,
       },
-      // updatedBy is also an email address.
+      // updatedBy used to be email address, but now stores userId as of v5
       updatedBy: {
         type: DataTypes.STRING,
       },
diff --git a/server/sequelize-db/query-acl.js b/server/sequelize-db/query-acl.js
index 43e0e109..daae1d06 100644
--- a/server/sequelize-db/query-acl.js
+++ b/server/sequelize-db/query-acl.js
@@ -20,11 +20,6 @@ module.exports = function (sequelize) {
         type: DataTypes.STRING,
         allowNull: true,
       },
-      // Email address can also be specified if userId is not known
-      userEmail: {
-        type: DataTypes.STRING,
-        allowNull: true,
-      },
       // The "Group" data model does not exist yet today but some day maybe will
       // It is intended to be a generic grouping mechanism
       // For now it'll contain special group values like "__EVERYONE__" found in consts.EVERYONE_ID


Test patch:
diff --git a/server/test/api/queries.js b/server/test/api/queries.js
index 41f86aa5..94a66d13 100644
--- a/server/test/api/queries.js
+++ b/server/test/api/queries.js
@@ -47,6 +47,9 @@ describe('api/queries', function () {
     assert(query.canRead);
     assert(query.canWrite);
     assert(query.canDelete);
+    assert.equal(query.createdByUser.id, query.createdBy);
+    assert(query.createdByUser.hasOwnProperty('email'));
+    assert(query.createdByUser.hasOwnProperty('name'));
 
     assert.equal(body.length, 1, '1 length');
   });
@@ -83,7 +86,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
       ],
     });
@@ -97,7 +99,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { userId: utils.users.editor2.id, write: false },
       ],
@@ -114,7 +115,6 @@ describe('api/queries', function () {
         ...createQueryBody,
         acl: [
           { userId: 'fakeUser', write: true },
-          { userEmail: 'fakeEmail', write: true },
           { groupId: 'fakeGroup', write: true },
           { groupId: '__EVERYONE__', write: false },
           { userId: utils.users.editor2.id, write: false },
@@ -129,7 +129,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { groupId: '__EVERYONE__', write: false },
         { userId: utils.users.editor2.id, write: true },
@@ -143,7 +142,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { groupId: '__EVERYONE__', write: false },
         { userId: utils.users.editor2.id, write: false },
@@ -162,7 +160,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { groupId: '__EVERYONE__', write: true },
         { userId: utils.users.editor2.id, write: false },
@@ -178,7 +175,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { userId: utils.users.editor2.id, write: false },
       ],
@@ -193,7 +189,6 @@ describe('api/queries', function () {
         ...createQueryBody,
         acl: [
           { userId: 'fakeUser', write: true },
-          { userEmail: 'fakeEmail', write: true },
           { groupId: 'fakeGroup', write: true },
           { groupId: '__EVERYONE__', write: false },
           { userId: utils.users.editor2.id, write: false },
@@ -208,7 +203,6 @@ describe('api/queries', function () {
       name: 'test query2',
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { userId: utils.users.editor2.id, write: true },
       ],
@@ -219,7 +213,6 @@ describe('api/queries', function () {
       ...createQueryBody,
       acl: [
         { userId: 'fakeUser', write: true },
-        { userEmail: 'fakeEmail', write: true },
         { groupId: 'fakeGroup', write: true },
         { userId: utils.users.editor2.id, write: false },
       ],
@@ -245,10 +238,10 @@ describe('api/queries', function () {
     });
   });
 
-  it('ACL userEmail gives access like expected', async function () {
+  it('ACL userId gives access like expected', async function () {
     const body1 = await utils.put('editor', `/api/queries/${query.id}`, {
       ...createQueryBody,
-      acl: [{ userEmail: 'editor2@test.com', write: true }],
+      acl: [{ userId: utils.users.editor2.id, write: true }],
     });
     // can<Action> represents what user that made API call can do
     assert.strictEqual(body1.canRead, true);
@@ -263,7 +256,7 @@ describe('api/queries', function () {
 
     const body3 = await utils.put('editor2', `/api/queries/${query.id}`, {
       ...createQueryBody,
-      acl: [{ userEmail: 'editor2@test.com', write: false }],
+      acl: [{ userId: utils.users.editor2.id, write: false }],
     });
     assert.strictEqual(body3.canRead, true);
     assert.strictEqual(body3.canWrite, false);
@@ -422,7 +415,7 @@ describe('api/queries', function () {
 
     // createdBy
     params = queryString.stringify(
-      { createdBy: utils.users.editor2.email },
+      { createdBy: utils.users.editor2.id },
       { arrayFormat: 'bracket' }
     );
     body = await utils.get('editor', `/api/queries?${params}`);
diff --git a/server/test/fixtures/seed-data-bad-query-2/queries/seed-query-1.json b/server/test/fixtures/seed-data-bad-query-2/queries/seed-query-1.json
new file mode 100644
index 00000000..f0ba7ecd
--- /dev/null
+++ b/server/test/fixtures/seed-data-bad-query-2/queries/seed-query-1.json
@@ -0,0 +1,14 @@
+{
+  "id": "seed-query-1",
+  "name": "Seed query 1",
+  "connectionId": "seed-connection-1",
+  "queryText": "SELECT * FROM seed_table",
+  "createdBy": "admin@sqlpad.com",
+  "acl": [
+    {
+      "userId": "some-userId-in-sqlpad",
+      "userEmail": "someone@sqlpad.com",
+      "write": false
+    }
+  ]
+}
diff --git a/server/test/lib/seed-data-path.js b/server/test/lib/seed-data-path.js
index f6aa4ff6..ac80c908 100644
--- a/server/test/lib/seed-data-path.js
+++ b/server/test/lib/seed-data-path.js
@@ -9,7 +9,8 @@ describe('seedDataPath', function () {
     await utils.init();
     const queries = await utils.models.queries.findAll();
     assert.strictEqual(queries.length, 2);
-    assert(queries.find((q) => q.id === 'seed-query-1'));
+    const query1 = queries.find((q) => q.id === 'seed-query-1');
+    assert(query1);
     const queryAcls = await utils.models.queryAcl.findAllByQueryId(
       'seed-query-1'
     );
@@ -17,6 +18,11 @@ describe('seedDataPath', function () {
     const connections = await utils.models.connections.findAll();
     assert.equal(connections.length, 1);
     assert.strictEqual(connections[0].id, 'seed-connection-1');
+
+    // Users were auto created
+    const u1 = await utils.models.users.findOneByEmail('someone@sqlpad.com');
+    assert(u1);
+    assert(queryAcls.find((acl) => acl.userId === u1.id));
   });
 
   it('Handles child directories with no valid files', async function () {
@@ -45,4 +51,13 @@ describe('seedDataPath', function () {
     const queries = await utils.models.queries.findAll();
     assert.strictEqual(queries.length, 0);
   });
+
+  it('throws for multiple acl user specifiers', async function () {
+    const utils = new TestUtils({
+      seedDataPath: './test/fixtures/seed-data-bad-query-2',
+    });
+    await assert.rejects(() => utils.init());
+    const queries = await utils.models.queries.findAll();
+    assert.strictEqual(queries.length, 0);
+  });
 });
diff --git a/server/test/migrations/nedb-to-sqlite.js b/server/test/migrations/nedb-to-sqlite.js
index dd8036df..90d6f22e 100644
--- a/server/test/migrations/nedb-to-sqlite.js
+++ b/server/test/migrations/nedb-to-sqlite.js
@@ -1,3 +1,4 @@
+/* eslint-disable no-await-in-loop */
 const assert = require('assert');
 const path = require('path');
 const Sequelize = require('sequelize');
@@ -27,7 +28,7 @@ function copyDbFiles(source, destination) {
   });
 }
 
-describe('v4-to-v5', function () {
+describe('nedb-to-sqlite', function () {
   /**
    * @type {TestUtils}
    */
@@ -79,6 +80,11 @@ describe('v4-to-v5', function () {
     for (const original of originalQueries) {
       const query = queries.find((item) => item.id === original._id);
 
+      // createdBy and updatedBy were converted to userId instead of email
+      const createdUser = await utils.sequelizeDb.Users.findOne({
+        where: { email: original.createdBy },
+      });
+
       assert.equal(query.id, original._id);
       assert.equal(query.name, original.name);
       assert.equal(query.connectionId, original.connectionId, 'connectionId');
@@ -89,8 +95,7 @@ describe('v4-to-v5', function () {
         }
         assert.deepEqual(query.chart, original.chartConfiguration);
       }
-      assert.equal(query.createdBy, original.createdBy, 'createdBy');
-      assert.equal(query.updatedBy, original.modifiedBy, 'updatedBy');
+      assert.equal(query.createdBy, createdUser.id, 'createdBy');
       assert.equal(
         new Date(query.updatedAt).toISOString().replace(/\.\d+Z/, ''),
         new Date(original.modifiedDate).toISOString().replace(/\.\d+Z/, ''),
diff --git a/server/test/sequelize/query-acl.js b/server/test/sequelize/query-acl.js
index aafbc0dc..c0525460 100644
--- a/server/test/sequelize/query-acl.js
+++ b/server/test/sequelize/query-acl.js
@@ -17,16 +17,6 @@ describe('QueryAcl', function () {
     assert.strictEqual(qa1.userId, 'test');
     assert.strictEqual(qa1.write, false);
 
-    const qa2 = await sdb.QueryAcl.create({
-      queryId,
-      userEmail: 'test@sqlpad.com',
-    });
-    assert.strictEqual(qa2.queryId, queryId);
-    assert(!qa2.userId);
-    assert.strictEqual(qa2.userEmail, 'test@sqlpad.com');
-    assert(!qa2.groupId);
-    assert.strictEqual(qa2.write, false);
-
     const qa3 = await sdb.QueryAcl.create({
       queryId,
       groupId: 'group',
@@ -54,17 +44,6 @@ describe('QueryAcl', function () {
       });
     });
 
-    await assert.rejects(async () => {
-      await utils.models.queryAcl.create({
-        queryId: 'q1',
-        userEmail: 'e1',
-      });
-      await utils.models.queryAcl.create({
-        queryId: 'q1',
-        userEmail: 'e1',
-      });
-    });
-
     await assert.rejects(async () => {
       await utils.models.queryAcl.create({
         queryId: 'q1',