Merge pull request ExpressGateway#862 from bigpictureio/fix_creds_leak

clean up logic for removing creds on delete user
yilu-tech · Jan 6, 2019 · 6a31391 · 6a31391
2 parents 0ce2b9a + e1a2968
commit 6a31391
Show file tree

Hide file tree

Showing 3 changed files with 145 additions and 160 deletions.
diff --git a/lib/services/consumers/user.service.js b/lib/services/consumers/user.service.js
@@ -117,19 +117,18 @@ s.activate = function (id) {
 
 s.remove = function (userId) {
   return this.get(userId) // validate user exists
-    .then(function (user) {
-      return !user ? false // user does not exist
-        : userDao.remove(userId)
-          .then(function (userDeleted) {
-            if (!userDeleted) {
-              return Promise.reject(new Error('user delete failed')); // TODO: replace with server error
-            } else {
-              return Promise.all([applicationService.removeAll(userId), // Cascade delete all apps associated with user
-                credentialService.removeAllCredentials(user.id)]) // Cascade delete all user credentials
-                .catch(() => Promise.reject(new Error('failed to delete user\'s applications or credentials'))) // TODO: replace with server error
-                .then(() => true);
-            }
-          });
+    .then(user => Promise.all([user, !user ? false : userDao.remove(userId)]))
+    .then(([user, userDeleted]) => {
+      if (!user) {
+        return false;
+      } else if (user && !userDeleted) {
+        throw new Error('user delete failed');
+      } else {
+        return Promise.all([applicationService.removeAll(userId), // Cascade delete all apps associated with user
+          credentialService.removeAllCredentials(user.id)]) // Cascade delete all user credentials
+          .catch(() => Promise.reject(new Error('failed to delete user\'s applications or credentials'))) // TODO: replace with server error
+          .then(() => true);
+      }
     });
 };
 

diff --git a/lib/services/credentials/credential.dao.js b/lib/services/credentials/credential.dao.js
@@ -145,20 +145,46 @@ dao.removeCredential = function (id, type) {
   return db.del(buildIdKey(type, id));
 };
 
+/*
+ * Remove all credentials
+ *
+ * @id {String}
+ */
 dao.removeAllCredentials = function (id) {
   const dbTransaction = db.multi();
   const credentialTypes = Object.keys(config.models.credentials.properties);
-  const awaitAllPromises = [];
-  credentialTypes.forEach((type) => {
-    if (type === 'key-auth') {
+  const awaitAllPromises = credentialTypes.map(type => {
+    const authKey = buildIdKey(type, id);
+
+    if (type === 'key-auth' || type === 'jwt') {
+      const promises = [];
+
       // id in this call is actually consumerId, so we need to get all referenced keyIds
-      awaitAllPromises.push(db.smembers(buildIdKey(type, id)).then(ids => {
-        ids.map(keyId => {
-          dbTransaction.del(buildIdKey(type, keyId));
+      // Get a list of all keys the user owns and all the scopes so we can remove keys from them
+      Promise.all([db.smembers(authKey), dao.getAllScopes()])
+        .then(([ids, scopes]) => {
+          // Delete each key and remove key from scopes if they exist
+          ids.forEach(keyId => {
+            const idKey = buildIdKey(type, keyId);
+
+            // Delete key
+            promises.push(dbTransaction.del(idKey));
+
+            // Delete key from all scopes
+            if (scopes) {
+              scopes.forEach(scope => {
+                promises.push(dbTransaction.hdel(buildScopeKey(scope), idKey));
+              });
+            }
+          });
         });
-      }));
+
+      // Now delete the main key for the user that lists all creds
+      promises.push(dbTransaction.del(authKey));
+
+      return Promise.all(promises);
     } else {
-      dbTransaction.del(buildIdKey(type, id));
+      return dbTransaction.del(authKey);
     }
   });