From 777d045246635db68d7f83e220c367a5a2b2dd6f Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 28 Nov 2021 01:47:44 -0500
Subject: [PATCH] GroupManager: Clean up any mappings when deleting a group

---
 CHANGELOG.md                                  |  2 ++
 src/node/db/GroupManager.js                   | 28 ++++++++-----------
 .../backend/specs/api/sessionsAndGroups.js    | 25 +++++++++++++++--
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a16f29c85..2643885d8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,8 @@
 ### Notable enhancements and fixes
 
 * Fixed a potential attribute pool corruption bug with `copyPadWithoutHistory`.
+* Mappings created by the `createGroupIfNotExistsFor` HTTP API are now removed
+  from the database when the group is deleted.
 
 #### For plugin authors
 
diff --git a/src/node/db/GroupManager.js b/src/node/db/GroupManager.js
index de5efc2e4..29ab1b598 100644
--- a/src/node/db/GroupManager.js
+++ b/src/node/db/GroupManager.js
@@ -61,6 +61,7 @@ exports.deleteGroup = async (groupID) => {
     // writes the result. Setting a property to `undefined` deletes that property (JSON.stringify()
     // ignores such properties).
     db.setSub('groups', [groupID], undefined),
+    ...Object.keys(group.mappings || {}).map(async (m) => await db.remove(`mapper2group:${m}`)),
   ]);
 
   // Remove the group record after updating the `groups` record so that the state is consistent.
@@ -76,7 +77,7 @@ exports.doesGroupExist = async (groupID) => {
 
 exports.createGroup = async () => {
   const groupID = `g.${randomString(16)}`;
-  await db.set(`group:${groupID}`, {pads: {}});
+  await db.set(`group:${groupID}`, {pads: {}, mappings: {}});
   // Add the group to the `groups` record after the group's individual record is created so that
   // the state is consistent. Note: UeberDB's setSub() method atomically reads the record, updates
   // the appropriate property, and writes the result.
@@ -85,27 +86,20 @@ exports.createGroup = async () => {
 };
 
 exports.createGroupIfNotExistsFor = async (groupMapper) => {
-  // ensure mapper is optional
   if (typeof groupMapper !== 'string') {
     throw new CustomError('groupMapper is not a string', 'apierror');
   }
-
-  // try to get a group for this mapper
   const groupID = await db.get(`mapper2group:${groupMapper}`);
-
-  if (groupID) {
-    // there is a group for this mapper
-    const exists = await exports.doesGroupExist(groupID);
-
-    if (exists) return {groupID};
-  }
-
-  // hah, the returned group doesn't exist, let's create one
+  if (groupID && await exports.doesGroupExist(groupID)) return {groupID};
   const result = await exports.createGroup();
-
-  // create the mapper entry for this group
-  await db.set(`mapper2group:${groupMapper}`, result.groupID);
-
+  await Promise.all([
+    db.set(`mapper2group:${groupMapper}`, result.groupID),
+    // Remember the mapping in the group record so that it can be cleaned up when the group is
+    // deleted. Although the core Etherpad API does not support multiple mappings for the same
+    // group, the database record does support multiple mappings in case a plugin decides to extend
+    // the core Etherpad functionality. (It's also easy to implement it this way.)
+    db.setSub(`group:${result.groupID}`, ['mappings', groupMapper], 1),
+  ]);
   return result;
 };
 
diff --git a/src/tests/backend/specs/api/sessionsAndGroups.js b/src/tests/backend/specs/api/sessionsAndGroups.js
index 83fdac698..eb181f01f 100644
--- a/src/tests/backend/specs/api/sessionsAndGroups.js
+++ b/src/tests/backend/specs/api/sessionsAndGroups.js
@@ -2,6 +2,7 @@
 
 const assert = require('assert').strict;
 const common = require('../../common');
+const db = require('../../../../node/db/DB');
 
 let agent;
 const apiKey = common.apiKey;
@@ -89,13 +90,33 @@ describe(__filename, function () {
     });
 
     it('createGroupIfNotExistsFor', async function () {
-      await agent.get(`${endPoint('createGroupIfNotExistsFor')}&groupMapper=management`)
+      const mapper = makeid();
+      let groupId;
+      await agent.get(`${endPoint('createGroupIfNotExistsFor')}&groupMapper=${mapper}`)
           .expect(200)
           .expect('Content-Type', /json/)
           .expect((res) => {
             assert.equal(res.body.code, 0);
-            assert(res.body.data.groupID);
+            groupId = res.body.data.groupID;
+            assert(groupId);
           });
+      // Passing the same mapper should return the same group ID.
+      await agent.get(`${endPoint('createGroupIfNotExistsFor')}&groupMapper=${mapper}`)
+          .expect(200)
+          .expect('Content-Type', /json/)
+          .expect((res) => {
+            assert.equal(res.body.code, 0);
+            assert.equal(res.body.data.groupID, groupId);
+          });
+      // Deleting the group should clean up the mapping.
+      assert.equal(await db.get(`mapper2group:${mapper}`), groupId);
+      await agent.get(`${endPoint('deleteGroup')}&groupID=${groupId}`)
+          .expect(200)
+          .expect('Content-Type', /json/)
+          .expect((res) => {
+            assert.equal(res.body.code, 0);
+          });
+      assert(await db.get(`mapper2group:${mapper}`) == null);
     });
 
     // Test coverage for https://github.com/ether/etherpad-lite/issues/4227