Refactored groupPermissions

libs/model/src/community/CreateGroup.command.ts

@@ -1,7 +1,7 @@
 import { InvalidState, type Command } from '@hicommonwealth/core';
 import * as schemas from '@hicommonwealth/schemas';
 import { Op } from 'sequelize';
-import { models, sequelize } from '../database';
+import { models } from '../database';
 import { isCommunityAdminOrModerator } from '../middleware';
 import { mustNotExist } from '../middleware/guards';
 import { GroupAttributes } from '../models';
@@ -54,26 +54,6 @@ export function CreateGroup(): Command<typeof schemas.CreateGroup> {
             } as GroupAttributes,
             { transaction },
           );
-          if (topicsToAssociate.length > 0) {
-            // add group to all specified topics
-            await models.Topic.update(
-              {
-                group_ids: sequelize.fn(
-                  'array_append',
-                  sequelize.col('group_ids'),
-                  group.id,
-                ),
-              },
-              {
-                where: {
-                  id: {
-                    [Op.in]: topicsToAssociate.map(({ id }) => id!),
-                  },
-                },
-                transaction,
-              },
-            );
-          }
           return group.toJSON();
         },
       );

libs/model/src/models/topic.ts

@@ -16,7 +16,6 @@ export type TopicAttributes = {
   updated_at?: Date;
   deleted_at?: Date;
   default_offchain_template?: string;
-  group_ids?: number[];
   telegram?: string;
 
   // associations
@@ -57,11 +56,6 @@ export default (
         allowNull: true,
       },
       channel_id: { type: Sequelize.STRING, allowNull: true },
-      group_ids: {
-        type: Sequelize.ARRAY(Sequelize.INTEGER),
-        allowNull: false,
-        defaultValue: [],
-      },
       telegram: { type: Sequelize.STRING, allowNull: true },
     },
     {

packages/commonwealth/server/controllers/server_groups_methods/create_group.ts

@@ -3,9 +3,8 @@ import {
   CommunityAttributes,
   GroupAttributes,
   UserInstance,
-  sequelize,
 } from '@hicommonwealth/model';
-import { GroupMetadata } from '@hicommonwealth/schemas';
+import { GroupMetadata, PermissionEnum } from '@hicommonwealth/schemas';
 import { Requirement } from '@hicommonwealth/shared';
 import { Op, Transaction } from 'sequelize';
 import z from 'zod';
@@ -117,25 +116,22 @@ export async function __createGroup(
       { transaction: t },
     );
     if (topicsToAssociate.length > 0) {
-      // add group to all specified topics
-      await this.models.Topic.update(
-        {
-          group_ids: sequelize.fn(
-            'array_append',
-            sequelize.col('group_ids'),
-            group.id,
-          ),
-        },
-        {
-          // @ts-expect-error StrictNullChecks
-          where: {
-            id: {
-              [Op.in]: topicsToAssociate.map(({ id }) => id),
-            },
+      // Create an array of promises
+      const createPermissionsPromises = topicsToAssociate.map((topic) => {
+        return this.models.GroupPermission.create(
+          {
+            group_id: group.id,
+            topic_id: topic.id,
+            allowed_actions: Object.values(PermissionEnum),
           },
-          transaction,
-        },
-      );
+          {
+            transaction,
+          },
+        );
+      });
+
+      // Await all promises to complete
+      await Promise.all(createPermissionsPromises);
     }
     return group.toJSON();
   };

packages/commonwealth/server/controllers/server_groups_methods/refresh_membership.ts

@@ -1,10 +1,10 @@
-import { AppError } from '@hicommonwealth/core';
+import { AppError } from '@hicommonwealth/core/src/index';
 import {
   AddressInstance,
   MembershipRejectReason,
   UserInstance,
 } from '@hicommonwealth/model';
-import { Op } from 'sequelize';
+import { QueryTypes } from 'sequelize';
 import { refreshMembershipsForAddress } from '../../util/requirementsModule/refreshMembershipsForAddress';
 import { ServerGroupsController } from '../server_groups_controller';
 
@@ -27,21 +27,25 @@ export async function __refreshMembership(
   this: ServerGroupsController,
   { address, topicId }: RefreshMembershipOptions,
 ): Promise<RefreshMembershipResult> {
-  // get all groups in the community
-  let groups = await this.models.Group.findAll({
-    where: {
-      community_id: address.community_id,
+  // get all groups in the community with the topic_ids if the topicId is passed in
+  const groups = await this.models.sequelize.query(
+    `
+    SELECT G.* FROM "Groups" G
+    LEFT JOIN "GroupPermissions" GP ON :topicId IS NOT NULL AND G.id = GP.group_id 
+    WHERE community_id = :communityId AND (:topicId IS NULL OR GP.topic_id = :topicId)
+    `,
+    {
+      type: QueryTypes.SELECT,
+      raw: true,
+      replacements: {
+        communityId: address.community_id,
+        topicId: topicId ?? null,
+      },
     },
-  });
+  );
 
-  // optionally filter to only groups associated with topic
-  if (topicId) {
-    const topic = await this.models.Topic.findByPk(topicId);
-    if (!topic) {
-      throw new AppError(Errors.TopicNotFound);
-    }
-    // @ts-expect-error StrictNullChecks
-    groups = groups.filter((g) => topic.group_ids.includes(g.id));
+  if (groups.length === 0 && topicId) {
+    throw new AppError(Errors.TopicNotFound);
   }
 
   const memberships = await refreshMembershipsForAddress(
@@ -51,21 +55,13 @@ export async function __refreshMembership(
     true, // use fresh balances
   );
 
-  const topics = await this.models.Topic.findAll({
-    where: {
-      group_ids: {
-        [Op.overlap]: groups.map((g) => g.id),
-      },
-    },
-    attributes: ['id', 'group_ids'],
-  });
-
   // transform memberships to result shape
   const results = memberships.map((membership) => ({
     groupId: membership.group_id,
-    topicIds: topics
+    topicIds: groups
       // @ts-expect-error StrictNullChecks
-      .filter((t) => t.group_ids.includes(membership.group_id))
+      .filter((g) => g.group_id === membership.group_id)
+      .map((g) => g.topic_id)
       .map((t) => t.id),
     allowed: !membership.reject_reason,
     rejectReason: membership.reject_reason,

packages/commonwealth/server/controllers/server_topics_methods/update_topic.ts

@@ -53,7 +53,6 @@ export async function __updateTopic(
     name,
     description,
     telegram,
-    group_ids,
     featured_in_sidebar,
     featured_in_new_post,
   } = body;
@@ -78,9 +77,6 @@ export async function __updateTopic(
   if (typeof telegram !== 'undefined') {
     topic.telegram = telegram || '';
   }
-  if (Array.isArray(group_ids)) {
-    topic.group_ids = group_ids;
-  }
   if (typeof featured_in_sidebar !== 'undefined') {
     topic.featured_in_sidebar = featured_in_sidebar || false;
   }

packages/commonwealth/server/migrations/20240718135145-group-permission-refactor.js

@@ -0,0 +1,61 @@
+'use strict';
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+  async up(queryInterface) {
+    try {
+      await queryInterface.sequelize.transaction(async (transaction) => {
+        // Turn (topic_id, group_id) into the composite primary key
+        await queryInterface.sequelize.query(
+          `
+          ALTER TABLE "GroupPermissions" ADD COLUMN topic_id INTEGER;
+          ALTER TABLE "GroupPermissions" ADD CONSTRAINT GroupPermissions_topic_id_fkey
+          FOREIGN KEY (topic_id) REFERENCES "Topics"(id);
+          ALTER TABLE "GroupPermissions" DROP CONSTRAINT "GroupPermissions_pkey";
+          ALTER TABLE "GroupPermissions" ADD PRIMARY KEY (topic_id, group_id);
+          `,
+          { transaction },
+        );
+
+        const topics = await queryInterface.sequelize.query(
+          `SELECT id, group_ids FROM "Topics" WHERE array_length(group_ids, 1) > 0;`,
+          {
+            type: queryInterface.sequelize.QueryTypes.SELECT,
+            raw: true,
+            transaction,
+          },
+        );
+
+        for (const t of topics) {
+          for (const g of t.group_ids) {
+            // we have consistency issues with the old model.
+            // Some group_ids from topics are referencing non-existing groups, so skip them if we encounter them
+            if (g < 52) {
+              continue;
+            }
+            await queryInterface.sequelize.query(
+              `
+                INSERT INTO "GroupPermissions"(group_id, topic_id, created_at, updated_at, allowed_actions) VALUES 
+                (
+                 ${g}, ${t.id}, NOW(), NOW(),
+                 '{ CREATE_THREAD, CREATE_COMMENT, CREATE_THREAD_REACTION, CREATE_THREAD_REACTION,
+                 CREATE_COMMENT_REACTION, UPDATE_POLL }'
+                );
+              `,
+              { transaction },
+            );
+          }
+        }
+      });
+    } catch (e) {
+      console.log('Migration failed');
+      throw e;
+    }
+
+    // For whatever reason this cannot be in the transaction, otherwise it will fail.
+    // This should still be safe since we will abort if the above transaction rolls back before we get to this line.
+    await queryInterface.removeColumn('Topics', 'group_ids');
+  },
+
+  async down() {},
+};

packages/commonwealth/server/routes/topics/update_topic_handler.ts

@@ -36,7 +36,6 @@ export const updateTopicHandler = async (
     featured_in_new_post: z.coerce.boolean().optional(),
     default_offchain_template: z.string().nullable().optional(),
     telegram: z.string().nullable().optional(),
-    group_ids: z.array(z.number()).optional(),
   });
 
   const validationResult = validationSchema.safeParse({

packages/commonwealth/server/util/requirementsModule/refreshMembershipsForAddress.ts

@@ -18,7 +18,6 @@ import validateGroupMembership from './validateGroupMembership';
  * refreshMembershipsForAddress refreshes the memberships for the given address
  * @param address Address associated with memberships
  * @param groups Groups to check requirements from
- * @param topics Topics associated with groups
  * @param cacheRefresh if true, forces TBC cache to refresh and force updates membership
  * @returns MembershipInstance[]
  */

packages/commonwealth/server/util/requirementsModule/validateTopicGroupsMembership.ts

@@ -25,45 +25,32 @@ export async function validateTopicGroupsMembership(
   address: AddressAttributes,
   action: GroupPermissionAction,
 ): Promise<{ isValid: boolean; message?: string }> {
-  // check via new TBC with groups
-
-  // get all groups of topic
-  const topic = await models.Topic.findOne({
-    where: {
-      community_id: communityId,
-      id: topicId,
-    },
-  });
-  if (!topic) {
-    return { isValid: false, message: 'Topic not found' };
-  }
-
-  if (topic?.group_ids?.length === 0) {
-    return { isValid: true };
-  }
-
   const groups: (GroupInstance & {
     allowed_actions?: GroupPermissionAction[];
   })[] = await models.sequelize.query(
     `
         SELECT g.*, gp.allowed_actions FROM "Groups" as g LEFT JOIN "GroupPermissions" gp ON g.id = gp.group_id
-        WHERE g.community_id = :communityId AND g.id IN(:groupIds);
+        WHERE g.community_id = :communityId AND g.topic_id = :topicId;
       `,
     {
       type: QueryTypes.SELECT,
       raw: true,
-      replacements: { communityId, groupIds: topic.group_ids },
+      replacements: { communityId, topicId: topicId },
     },
   );
 
-  // There are 2 cases here. We either have the old group permission system where the group doesn't have
-  // any allowed_actions, or we have the new fine-grained permission system where the action must be in
-  // the allowed_actions list.
-  const permissionedGroups = groups.filter(
+  // if there are no groups for this topic, then anyone can perform any action on it (default behaviour).
+  if (groups.length === 0) {
+    return { isValid: true };
+  }
+
+  const groupsMatchingAction = groups.filter(
     (g) => !g.allowed_actions || g.allowed_actions.includes(action),
   );
 
-  if (permissionedGroups.length === 0) {
+  // if no group allows the specified action for the given topic, then reject because regardless of membership the user
+  // will not be allowed.
+  if (groupsMatchingAction.length === 0) {
     return {
       isValid: false,
       message: `User does not have permission to perform action ${action}`,
@@ -77,7 +64,7 @@ export async function validateTopicGroupsMembership(
   const memberships = await refreshMembershipsForAddress(
     models,
     address,
-    permissionedGroups,
+    groupsMatchingAction,
     false, // use cached balances
   );