Repository: rickbergfalk/sqlpad (js)

Issue:
Allow users to be enabled/disabled
In prep for #745, there are cases where we need to have user accounts for reference, but they should not permit a user to log in.

Some examples: 
* A user could be deleted after creating queries, leaving a reference to a user that does not exist
* A seed query could be added for an email address that does not exist

To address this I'm going to add an `disabled` flag or similar to the `users` table, and allow it to be toggled via admin. Email addresses referenced in seed queries will eventually convert to user accounts that are disabled on creation.

Admins can then go enable the user in the user management section of the application.

Another benefit of all this allowing users to be disabled, but open authentication to remain open for auto-creating users.

This is probably not needed for this application, but if user auto-creation was turned on (depending on auth mechanism), and a user was deleted to prevent them from accessing the application, they could log in again by re-authenticating, which would recreate the user record that was deleted. With a disabled flag set to true, that user record persists, and the user is no longer allowed to log in on future attempts. This is probably beyond the scope of what people are using SQLPad for but potentially nice to have?

Pull request description:
Allow users to be enabled/disabled
Adds the ability to disable a user account as opposed to deleting them. 

Server:
* Adds a `disabled` flag on `users` table. It is nullable and null by default.
* Only an admin may update the users
* Adds `disabled` field to user list API
* Removes `data` from user list API
* Adds user get API (`api/users/<userId>`), that returns `passwordResetId` and `data` fields
* Adds utility function to clean user API return, ensuring `passhash` is never returned for any user API (even for admins)

UI:
* Adds button to toggle user enable/disable in UI

The UI is pretty basic for now but functional.

![sqlpad-user-enable-disable-ui](https://user-images.githubusercontent.com/303966/86053097-9da6e580-ba1d-11ea-8e27-e27fbd8b7212.jpg)


Fixes #746 

Patch:
diff --git a/client/src/users/EditUserForm.js b/client/src/users/EditUserForm.js
index 4a5d6714..d2aa9a0b 100644
--- a/client/src/users/EditUserForm.js
+++ b/client/src/users/EditUserForm.js
@@ -1,4 +1,4 @@
-import React, { useState } from 'react';
+import React, { useState, useEffect } from 'react';
 import { Link } from 'react-router-dom';
 import { v4 as uuidv4 } from 'uuid';
 import Button from '../common/Button';
@@ -7,19 +7,35 @@ import message from '../common/message';
 import Select from '../common/Select';
 import Spacer from '../common/Spacer';
 import { api } from '../utilities/fetch-json.js';
+import useSWR, { mutate } from 'swr';
+
+function EditUserForm({ userId }) {
+  let { data } = useSWR(`/api/users/${userId}`);
+  const user = data || {};
 
-function EditUserForm({ user }) {
   const [role, setRole] = useState(user.role);
   const [passwordResetId, setPasswordResetId] = useState(user.passwordResetId);
 
+  useEffect(() => {
+    setRole(user.role);
+    setPasswordResetId(user.passwordResetId);
+  }, [user.role, user.passwordResetId]);
+
+  // If still loading hide form
+  if (!data) {
+    return null;
+  }
+
   const handleRoleChange = async (event) => {
     setRole(event.target.value);
-    const json = await api.put('/api/users/' + user.id, {
+    const json = await api.put(`/api/users/${user.id}`, {
       role: event.target.value,
     });
     if (json.error) {
       return message.error('Update failed: ' + json.error);
     }
+    mutate('api/users');
+    mutate(`/api/users/${user.id}`);
   };
 
   const generatePasswordResetLink = async () => {
@@ -31,6 +47,8 @@ function EditUserForm({ user }) {
       return message.error('Update failed: ' + json.error);
     }
     setPasswordResetId(passwordResetId);
+    mutate('api/users');
+    mutate(`/api/users/${user.id}`);
   };
 
   const removePasswordResetLink = async () => {
diff --git a/client/src/users/UserList.js b/client/src/users/UserList.js
index a0bb6a94..4362d5df 100644
--- a/client/src/users/UserList.js
+++ b/client/src/users/UserList.js
@@ -15,6 +15,7 @@ function UserList() {
   const { currentUser } = useAppContext();
   const [showAddUser, setShowAddUser] = useState(false);
   const [editUser, setEditUser] = useState(null);
+  const [toggling, setToggling] = useState(false);
 
   const { data: usersData, error, mutate } = useSWR('/api/users');
   const users = (usersData || []).map((user) => ({ ...user, key: user.id }));
@@ -25,6 +26,24 @@ function UserList() {
     }
   }, [error]);
 
+  const handleDisableToggle = async (user) => {
+    setToggling(true);
+    const json = await api.put(`/api/users/${user.id}`, {
+      disabled: !user.disabled,
+    });
+    setToggling(false);
+    if (json.error) {
+      return message.error('Disable toggle failed: ' + json.error);
+    }
+    const mutatedUsers = users.map((u) => {
+      if (u.id === user.id) {
+        return json.data;
+      }
+      return u;
+    });
+    mutate(mutatedUsers);
+  };
+
   const handleDelete = async (user) => {
     const json = await api.delete(`/api/users/${user.id}`);
     if (json.error) {
@@ -60,7 +79,17 @@ function UserList() {
               style={{ marginLeft: 8 }}
               onClick={() => setEditUser(user)}
             >
-              edit
+              Edit
+            </Button>
+          );
+          actions.push(
+            <Button
+              key="toggle-disable"
+              style={{ marginLeft: 8 }}
+              disabled={toggling}
+              onClick={() => handleDisableToggle(user)}
+            >
+              {user.disabled ? 'Enable' : 'Disable'}
             </Button>
           );
           actions.push(
@@ -75,11 +104,12 @@ function UserList() {
           );
         }
 
-        const userSignupInfo = !user.signupAt ? (
-          <em> - not signed up yet</em>
-        ) : (
-          ''
-        );
+        let additionalUserInfo = '';
+        if (user.disabled) {
+          additionalUserInfo = <em> - disabled</em>;
+        } else if (!user.signupAt) {
+          additionalUserInfo = <em> - not signed up yet</em>;
+        }
 
         return (
           <ListItem key={user.id}>
@@ -87,7 +117,7 @@ function UserList() {
               {user.email}
               <br />
               <Text type="secondary">
-                {user.role} {userSignupInfo}
+                {user.role} {additionalUserInfo}
               </Text>
             </div>
             {actions}
@@ -113,7 +143,7 @@ function UserList() {
           setEditUser(null);
         }}
       >
-        <EditUserForm user={editUser} />
+        <EditUserForm userId={editUser && editUser.id} />
       </Modal>
     </>
   );
diff --git a/server/auth-strategies/auth-proxy.js b/server/auth-strategies/auth-proxy.js
index b690133e..371c8b2a 100644
--- a/server/auth-strategies/auth-proxy.js
+++ b/server/auth-strategies/auth-proxy.js
@@ -8,7 +8,7 @@ const appLog = require('../lib/app-log');
 
 /**
  * An auth-proxy custom strategy
- * If enabled, iterate over headers and map the values to a user object
+ * Iterate over headers and map the values to a user object
  * Look up that user and perform usual auth validations
  * @param {Req} req
  * @param {function} done
@@ -50,6 +50,10 @@ async function authProxyStrategy(req, done) {
     // If existing user is found, see if there are changes and update it
     // Only perform update if actual changes though
     if (existingUser) {
+      if (existingUser.disabled) {
+        return done(null, false);
+      }
+
       const existingId = existingUser.id;
       // Get a subset of existing user to use for comparison
       const existingForCompare = {};
@@ -73,7 +77,9 @@ async function authProxyStrategy(req, done) {
       }
 
       // Auto create the user
-      const newUser = await models.users.create(headerUser);
+      const newUser = await models.users.create({
+        ...headerUser,
+      });
       return done(null, newUser);
     }
 
diff --git a/server/auth-strategies/basic.js b/server/auth-strategies/basic.js
index 7eec045e..36e9a5fa 100644
--- a/server/auth-strategies/basic.js
+++ b/server/auth-strategies/basic.js
@@ -21,6 +21,9 @@ function enableBasic(config) {
           if (!user) {
             return callback(null, false);
           }
+          if (user.disabled) {
+            return callback(null, false);
+          }
           const isMatch = await passhash.comparePassword(
             password,
             user.passhash
diff --git a/server/auth-strategies/google.js b/server/auth-strategies/google.js
index 3473e34d..e2787e5a 100644
--- a/server/auth-strategies/google.js
+++ b/server/auth-strategies/google.js
@@ -26,6 +26,9 @@ async function passportGoogleStrategyHandler(
     ]);
 
     if (user) {
+      if (user.disabled) {
+        return done(null, false);
+      }
       user.signupAt = new Date();
       const newUser = await models.users.update(user.id, {
         signupAt: new Date(),
diff --git a/server/auth-strategies/index.js b/server/auth-strategies/index.js
index e5f64544..486c9907 100644
--- a/server/auth-strategies/index.js
+++ b/server/auth-strategies/index.js
@@ -23,7 +23,7 @@ passport.deserializeUser(async function (req, id, done) {
   const { models } = req;
   try {
     const user = await models.users.findOneById(id);
-    if (user) {
+    if (user && !user.disabled) {
       return done(null, user);
     }
     done(null, false);
diff --git a/server/auth-strategies/ldap.js b/server/auth-strategies/ldap.js
index a1a1b331..75cfa8bf 100644
--- a/server/auth-strategies/ldap.js
+++ b/server/auth-strategies/ldap.js
@@ -32,6 +32,9 @@ function enableLdap(config) {
               message: 'wrong LDAP username or password',
             });
           }
+          if (user.disabled) {
+            return done(null, false);
+          }
           return done(null, {
             id: user.id,
             role: user.role,
diff --git a/server/auth-strategies/local.js b/server/auth-strategies/local.js
index d4e90e66..8cee5454 100644
--- a/server/auth-strategies/local.js
+++ b/server/auth-strategies/local.js
@@ -22,6 +22,9 @@ function enableLocal(config) {
           if (!user) {
             return done(null, false, { message: 'wrong email or password' });
           }
+          if (user.disabled) {
+            return done(null, false);
+          }
           const isMatch = await passhash.comparePassword(
             password,
             user.passhash
diff --git a/server/auth-strategies/saml.js b/server/auth-strategies/saml.js
index 306397a4..67e0ad80 100644
--- a/server/auth-strategies/saml.js
+++ b/server/auth-strategies/saml.js
@@ -42,6 +42,9 @@ function enableSaml(config) {
           ]);
 
           if (user) {
+            if (user.disabled) {
+              return done(null, false);
+            }
             return done(null, {
               id: user.id,
               role: user.role,
diff --git a/server/lib/get-header-user.js b/server/lib/get-header-user.js
index 3647642d..0b8d357f 100644
--- a/server/lib/get-header-user.js
+++ b/server/lib/get-header-user.js
@@ -8,7 +8,6 @@ require('../typedefs');
 function getHeaderUser(req) {
   const { config } = req;
 
-  // If auth proxy is not enabled don't even try
   if (!config.get('authProxyEnabled')) {
     return null;
   }
diff --git a/server/migrations/04-00800-user-disabled.js b/server/migrations/04-00800-user-disabled.js
new file mode 100644
index 00000000..a2f5e5db
--- /dev/null
+++ b/server/migrations/04-00800-user-disabled.js
@@ -0,0 +1,18 @@
+const Sequelize = require('sequelize');
+
+/**
+ * @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
+ */
+// eslint-disable-next-line no-unused-vars
+async function up(queryInterface, config, appLog, nedb) {
+  await queryInterface.addColumn('users', 'disabled', {
+    type: Sequelize.BOOLEAN,
+  });
+}
+
+module.exports = {
+  up,
+};
diff --git a/server/models/users.js b/server/models/users.js
index 990ccbb6..2e6f5d09 100644
--- a/server/models/users.js
+++ b/server/models/users.js
@@ -68,7 +68,7 @@ class Users {
         'name',
         'email',
         'role',
-        'data',
+        'disabled',
         'signupAt',
         'createdAt',
         'updatedAt',
@@ -77,8 +77,7 @@ class Users {
     });
 
     return users.map((user) => {
-      user.data = ensureJson(user.data);
-      return user;
+      return user.toJSON();
     });
   }
 
diff --git a/server/routes/users.js b/server/routes/users.js
index 67569350..29127981 100644
--- a/server/routes/users.js
+++ b/server/routes/users.js
@@ -5,6 +5,14 @@ const mustBeAdmin = require('../middleware/must-be-admin.js');
 const mustBeAuthenticated = require('../middleware/must-be-authenticated.js');
 const wrap = require('../lib/wrap');
 
+function cleanUser(user) {
+  if (!user) {
+    return user;
+  }
+  const { passhash, ...rest } = user;
+  return rest;
+}
+
 /**
  * @param {Req} req
  * @param {Res} res
@@ -12,7 +20,8 @@ const wrap = require('../lib/wrap');
 async function listUsers(req, res) {
   const { models } = req;
   const users = await models.users.findAll();
-  return res.utils.data(users);
+  const cleaned = users.map((u) => cleanUser(u));
+  return res.utils.data(cleaned);
 }
 
 /**
@@ -40,7 +49,17 @@ async function createUser(req, res) {
   if (req.config.smtpConfigured()) {
     email.sendInvite(req.body.email).catch((error) => appLog.error(error));
   }
-  return res.utils.data(user);
+  return res.utils.data(cleanUser(user));
+}
+
+/**
+ * @param {Req} req
+ * @param {Res} res
+ */
+async function getUser(req, res) {
+  const { params, models } = req;
+  const foundUser = await models.users.findOneById(params.id);
+  return res.utils.data(cleanUser(foundUser));
 }
 
 /**
@@ -75,9 +94,12 @@ async function updateUser(req, res) {
   if (body.data) {
     updateUser.data = body.data;
   }
+  if (body.hasOwnProperty('disabled')) {
+    updateUser.disabled = body.disabled;
+  }
 
   const updatedUser = await models.users.update(params.id, updateUser);
-  return res.utils.data(updatedUser);
+  return res.utils.data(cleanUser(updatedUser));
 }
 
 /**
@@ -95,6 +117,7 @@ async function deleteUser(req, res) {
 
 router.get('/api/users', mustBeAuthenticated, wrap(listUsers));
 router.post('/api/users', mustBeAdmin, wrap(createUser));
+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/users.js b/server/sequelize-db/users.js
index 8dc62b15..065cc3cd 100644
--- a/server/sequelize-db/users.js
+++ b/server/sequelize-db/users.js
@@ -27,6 +27,9 @@ module.exports = function (sequelize) {
       name: {
         type: DataTypes.STRING,
       },
+      disabled: {
+        type: DataTypes.BOOLEAN,
+      },
       passhash: {
         type: DataTypes.STRING,
       },


Test patch:
diff --git a/server/test/api/signin.js b/server/test/api/signin.js
index 7b9a88d4..2daa12c0 100644
--- a/server/test/api/signin.js
+++ b/server/test/api/signin.js
@@ -134,6 +134,68 @@ describe('api/signin', function () {
     });
   });
 
+  it('disabled user cannot log in', async function () {
+    const utils = new TestUtil({
+      authProxyEnabled: false,
+    });
+
+    await utils.init();
+
+    // Add admin user via signup
+    await request(utils.app)
+      .post('/api/signup')
+      .send({
+        password: 'admin',
+        passwordConfirmation: 'admin',
+        email: 'admin@test.com',
+      })
+      .expect(200);
+
+    // Add user via API using admin
+    await request(utils.app)
+      .post('/api/users')
+      .auth('admin@test.com', 'admin')
+      .send({
+        email: 'user@test.com',
+        role: 'editor',
+      })
+      .expect(200);
+
+    await request(utils.app)
+      .post('/api/signup')
+      .send({
+        password: 'password',
+        passwordConfirmation: 'password',
+        email: 'user@test.com',
+      })
+      .expect(200);
+
+    // User can sign in
+    await request(utils.app)
+      .post('/api/signin')
+      .send({
+        email: 'user@test.com',
+        password: 'password',
+      })
+      .expect(200);
+
+    // Disabled user can't sign in
+    const user = await utils.models.users.findOneByEmail('user@test.com');
+    await request(utils.app)
+      .put(`/api/users/${user.id}`)
+      .auth('admin@test.com', 'admin')
+      .send({ disabled: true })
+      .expect(200);
+
+    await request(utils.app)
+      .post('/api/signin')
+      .send({
+        email: 'user@test.com',
+        password: 'password',
+      })
+      .expect(401);
+  });
+
   describe('auth proxy', function () {
     it('logs in with session', async function () {
       const utils = new TestUtil({
diff --git a/server/test/api/users.js b/server/test/api/users.js
index f9f83372..20ab3684 100644
--- a/server/test/api/users.js
+++ b/server/test/api/users.js
@@ -31,14 +31,35 @@ describe('api/users', function () {
     assert.equal(user.name, 'user1');
     assert.equal(user.role, 'editor');
     assert.equal(user.data.create, true);
+    assert.equal(user.disabled, null);
     assert(user.updatedAt);
     assert(user.createdAt);
+    assert(!user.hasOwnProperty('passhash'));
   });
 
   it('Gets list of users', async function () {
     const body = await utils.get('admin', '/api/users');
     TestUtils.validateListSuccessBody(body);
     assert.equal(body.length, 4, '4 length');
+    const user = body.find((u) => u.email === 'admin@test.com');
+    assert.equal(typeof user.id, 'string');
+    assert.equal(user.role, 'admin');
+    assert(user.hasOwnProperty('name'));
+    assert(user.hasOwnProperty('disabled'));
+    assert.equal(typeof user.createdAt, 'string');
+    assert.equal(typeof user.updatedAt, 'string');
+    // passhash and data are sensitive and should not exist
+    assert(!user.hasOwnProperty('passhash'));
+    assert(!user.hasOwnProperty('data'));
+  });
+
+  it('Gets single user', async function () {
+    const u = await utils.get('admin', `/api/users/${user.id}`);
+    assert.equal(u.email, user.email);
+    // passhash should *not* be present
+    assert(!u.hasOwnProperty('passhash'));
+    // Requires admin access
+    await utils.get('editor', `/api/users/${user.id}`, 403);
   });
 
   it('Updates user', async function () {
@@ -57,6 +78,7 @@ describe('api/users', function () {
     assert.equal(body.passwordResetId, passwordResetId);
     assert.equal(body.data.test, true);
     assert(new Date(body.updatedAt) >= new Date(user.updatedAt));
+    assert(!body.hasOwnProperty('passhash'));
   });
 
   it('Requires authentication', function () {
diff --git a/server/test/middleware/passport-auth-proxy.js b/server/test/middleware/passport-auth-proxy.js
index a5f5c7ae..1d46577d 100644
--- a/server/test/middleware/passport-auth-proxy.js
+++ b/server/test/middleware/passport-auth-proxy.js
@@ -154,7 +154,7 @@ describe('passport-proxy-auth', function () {
     await utils.init();
 
     const { body } = await request(utils.app)
-      .get('/api/users')
+      .get('/api/users/test001')
       .set('X-WEBAUTH-ID', 'test001')
       .set('X-WEBAUTH-EMAIL', 'test@sqlpad.com')
       .set('X-WEBAUTH-NAME', 'Test user')
@@ -162,7 +162,7 @@ describe('passport-proxy-auth', function () {
       .set('X-WEBAUTH-CUSTOM-FIELD', 'custom data value')
       .expect(200);
 
-    const user = body[0];
+    const user = body;
     assert.equal(user.email, 'test@sqlpad.com');
     assert.equal(user.role, 'admin');
     assert.equal(user.name, 'Test user');
@@ -204,22 +204,26 @@ describe('passport-proxy-auth', function () {
     assert.equal(user.role, 'admin');
   });
 
-  it('Matches existing user via id', async function () {
+  it('Disabled user cannot log in (auth proxy)', async function () {
     const utils = new TestUtil({
       authProxyEnabled: true,
       authProxyHeaders: 'id:X-WEBAUTH-ID',
     });
     await utils.init(true);
 
-    const id = utils.users.admin.id;
+    const id = utils.users.editor.id;
 
-    const { body } = await request(utils.app)
-      .get('/api/users')
+    await request(utils.app)
+      .get('/api/app')
       .set('X-WEBAUTH-ID', id)
       .expect(200);
 
-    const user = body.find((user) => user.id === id);
-    assert.equal(user.role, 'admin');
+    await utils.models.users.update(id, { disabled: true });
+
+    await request(utils.app)
+      .get('/api/app')
+      .set('X-WEBAUTH-ID', id)
+      .expect(401);
   });
 
   it('Updates existing user if changes', async function () {