fix: addressed more comments

backend/src/ee/routes/v1/group-router.ts

@@ -458,7 +458,9 @@ export const registerGroupRouter = async (server: FastifyZodProvider) => {
         machineIdentityId: z.string().trim().describe(GROUPS.ADD_MACHINE_IDENTITY.machineIdentityId)
       }),
       response: {
-        200: GroupIdentityResponseSchema
+        200: z.object({
+          id: z.string()
+        })
       }
     },
     handler: async (req) => {
@@ -528,7 +530,9 @@ export const registerGroupRouter = async (server: FastifyZodProvider) => {
         machineIdentityId: z.string().trim().describe(GROUPS.DELETE_MACHINE_IDENTITY.machineIdentityId)
       }),
       response: {
-        200: GroupIdentityResponseSchema
+        200: z.object({
+          id: z.string()
+        })
       }
     },
     handler: async (req) => {

backend/src/ee/services/group/group-fns.ts

@@ -291,21 +291,34 @@ export const addUsersToGroupByUserIds = async ({
  * Add identities with identity ids [identityIds] to group [group].
  * @param {group} group - group to add identity(s) to
  * @param {string[]} identityIds - id(s) of organization scoped identity(s) to add to group
+ * @returns {Promise<{ id: string }[]>} - id(s) of added identity(s)
  */
 export const addIdentitiesToGroup = async ({
   group,
   identityIds,
   identityDAL,
-  identityOrgMembershipDAL,
-  identityGroupMembershipDAL
+  identityGroupMembershipDAL,
+  membershipDAL
 }: TAddIdentitiesToGroup) => {
   return identityDAL.transaction(async (tx) => {
     const identityIdsSet = new Set(identityIds);
     const identityIdsArray = Array.from(identityIdsSet);
 
-    const foundIdentities = await identityOrgMembershipDAL.findByIds(identityIdsArray, group.orgId, tx);
+    // ensure all identities exist and belong to the org via org scoped membership
+    const foundIdentitiesMemberships = await membershipDAL.find(
+      {
+        scope: AccessScope.Organization,
+        scopeOrgId: group.orgId,
+        $in: {
+          actorIdentityId: identityIdsArray
+        }
+      },
+      { tx }
+    );
 
-    const existingIdentityOrgMembershipsIdentityIdsSet = new Set(foundIdentities.map((u) => u.id));
+    const existingIdentityOrgMembershipsIdentityIdsSet = new Set(
+      foundIdentitiesMemberships.map((u) => u.actorIdentityId as string)
+    );
 
     identityIdsArray.forEach((identityId) => {
       if (!existingIdentityOrgMembershipsIdentityIdsSet.has(identityId)) {
@@ -333,14 +346,14 @@ export const addIdentitiesToGroup = async ({
     }
 
     await identityGroupMembershipDAL.insertMany(
-      foundIdentities.map((identity) => ({
-        identityId: identity.id,
+      foundIdentitiesMemberships.map((membership) => ({
+        identityId: membership.actorIdentityId as string,
         groupId: group.id
       })),
       tx
     );
 
-    return foundIdentities;
+    return identityIdsArray.map((identityId) => ({ id: identityId }));
   });
 };
 
@@ -484,26 +497,42 @@ export const removeUsersFromGroupByUserIds = async ({
  * Remove identities with identity ids [identityIds] from group [group].
  * @param {group} group - group to remove identity(s) from
  * @param {string[]} identityIds - id(s) of identity(s) to remove from group
+ * @returns {Promise<{ id: string }[]>} - id(s) of removed identity(s)
  */
 export const removeIdentitiesFromGroup = async ({
   group,
   identityIds,
   identityDAL,
-  identityOrgMembershipDAL,
+  membershipDAL,
   identityGroupMembershipDAL
 }: TRemoveIdentitiesFromGroup) => {
   return identityDAL.transaction(async (tx) => {
     const identityIdsSet = new Set(identityIds);
     const identityIdsArray = Array.from(identityIdsSet);
 
-    const foundIdentities = await identityOrgMembershipDAL.findByIds(identityIdsArray, group.orgId, tx);
+    // ensure all identities exist and belong to the org via org scoped membership
+    const foundIdentitiesMemberships = await membershipDAL.find(
+      {
+        scope: AccessScope.Organization,
+        scopeOrgId: group.orgId,
+        $in: {
+          actorIdentityId: identityIdsArray
+        }
+      },
+      { tx }
+    );
 
-    if (foundIdentities.length !== identityIdsArray.length) {
+    const foundIdentitiesMembershipsIdentityIdsSet = new Set(
+      foundIdentitiesMemberships.map((u) => u.actorIdentityId as string)
+    );
+
+    if (foundIdentitiesMembershipsIdentityIdsSet.size !== identityIdsArray.length) {
       throw new NotFoundError({
         message: `Machine identities not found`
       });
     }
 
+    // check if identity group membership already exists
     const existingIdentityGroupMemberships = await identityGroupMembershipDAL.find(
       {
         groupId: group.id,
@@ -536,7 +565,7 @@ export const removeIdentitiesFromGroup = async ({
       tx
     );
 
-    return foundIdentities;
+    return identityIdsArray.map((identityId) => ({ id: identityId }));
   });
 };
 

backend/src/ee/services/group/group-service.ts

@@ -6,7 +6,7 @@ import { TOidcConfigDALFactory } from "@app/ee/services/oidc/oidc-config-dal";
 import { BadRequestError, NotFoundError, PermissionBoundaryError, UnauthorizedError } from "@app/lib/errors";
 import { alphaNumericNanoId } from "@app/lib/nanoid";
 import { TIdentityDALFactory } from "@app/services/identity/identity-dal";
-import { TIdentityOrgDALFactory } from "@app/services/identity/identity-org-dal";
+import { TMembershipDALFactory } from "@app/services/membership/membership-dal";
 import { TMembershipRoleDALFactory } from "@app/services/membership/membership-role-dal";
 import { TMembershipGroupDALFactory } from "@app/services/membership-group/membership-group-dal";
 import { TOrgDALFactory } from "@app/services/org/org-dal";
@@ -46,7 +46,6 @@ import { TUserGroupMembershipDALFactory } from "./user-group-membership-dal";
 type TGroupServiceFactoryDep = {
   userDAL: Pick<TUserDALFactory, "find" | "findUserEncKeyByUserIdsBatch" | "transaction" | "findUserByUsername">;
   identityDAL: Pick<TIdentityDALFactory, "findOne" | "find" | "transaction">;
-  identityOrgMembershipDAL: Pick<TIdentityOrgDALFactory, "findByIds">;
   identityGroupMembershipDAL: Pick<TIdentityGroupMembershipDALFactory, "find" | "delete" | "insertMany">;
   groupDAL: Pick<
     TGroupDALFactory,
@@ -62,6 +61,7 @@ type TGroupServiceFactoryDep = {
     | "findAllGroupProjects"
   >;
   membershipGroupDAL: Pick<TMembershipGroupDALFactory, "find" | "findOne" | "create">;
+  membershipDAL: Pick<TMembershipDALFactory, "find" | "findOne">;
   membershipRoleDAL: Pick<TMembershipRoleDALFactory, "create" | "delete">;
   orgDAL: Pick<TOrgDALFactory, "findMembership" | "countAllOrgMembers" | "findById">;
   userGroupMembershipDAL: Pick<
@@ -83,7 +83,7 @@ export type TGroupServiceFactory = ReturnType<typeof groupServiceFactory>;
 
 export const groupServiceFactory = ({
   identityDAL,
-  identityOrgMembershipDAL,
+  membershipDAL,
   identityGroupMembershipDAL,
   userDAL,
   groupDAL,
@@ -672,17 +672,21 @@ export const groupServiceFactory = ({
         details: { missingPermissions: permissionBoundary.missingPermissions }
       });
 
-    const [identity] = await identityOrgMembershipDAL.findByIds([identityId], group.orgId);
+    const identityMembership = await membershipDAL.findOne({
+      scope: AccessScope.Organization,
+      scopeOrgId: group.orgId,
+      actorIdentityId: identityId
+    });
 
-    if (!identity) {
+    if (!identityMembership) {
       throw new NotFoundError({ message: `Identity with id ${identityId} is not part of the organization` });
     }
 
     const identities = await addIdentitiesToGroup({
       group,
-      identityIds: [identity.id],
+      identityIds: [identityId],
       identityDAL,
-      identityOrgMembershipDAL,
+      membershipDAL,
       identityGroupMembershipDAL
     });
 
@@ -824,16 +828,21 @@ export const groupServiceFactory = ({
         details: { missingPermissions: permissionBoundary.missingPermissions }
       });
 
-    const [identity] = await identityOrgMembershipDAL.findByIds([identityId], group.orgId);
+    const identityMembership = await membershipDAL.findOne({
+      scope: AccessScope.Organization,
+      scopeOrgId: group.orgId,
+      actorIdentityId: identityId
+    });
 
-    if (!identity)
+    if (!identityMembership) {
       throw new NotFoundError({ message: `Identity with id ${identityId} is not part of the organization` });
+    }
 
     const identities = await removeIdentitiesFromGroup({
       group,
-      identityIds: [identity.id],
+      identityIds: [identityId],
       identityDAL,
-      identityOrgMembershipDAL,
+      membershipDAL,
       identityGroupMembershipDAL
     });
 

backend/src/ee/services/group/group-types.ts

@@ -4,7 +4,7 @@ import { TGroups } from "@app/db/schemas";
 import { TUserGroupMembershipDALFactory } from "@app/ee/services/group/user-group-membership-dal";
 import { OrderByDirection, TGenericPermission } from "@app/lib/types";
 import { TIdentityDALFactory } from "@app/services/identity/identity-dal";
-import { TIdentityOrgDALFactory } from "@app/services/identity/identity-org-dal";
+import { TMembershipDALFactory } from "@app/services/membership/membership-dal";
 import { TMembershipGroupDALFactory } from "@app/services/membership-group/membership-group-dal";
 import { TOrgDALFactory } from "@app/services/org/org-dal";
 import { TProjectDALFactory } from "@app/services/project/project-dal";
@@ -129,8 +129,8 @@ export type TAddIdentitiesToGroup = {
   group: TGroups;
   identityIds: string[];
   identityDAL: Pick<TIdentityDALFactory, "transaction">;
-  identityOrgMembershipDAL: Pick<TIdentityOrgDALFactory, "findByIds">;
   identityGroupMembershipDAL: Pick<TIdentityGroupMembershipDALFactory, "find" | "insertMany">;
+  membershipDAL: Pick<TMembershipDALFactory, "find">;
 };
 
 export type TRemoveUsersFromGroupByUserIds = {
@@ -147,7 +147,7 @@ export type TRemoveIdentitiesFromGroup = {
   group: TGroups;
   identityIds: string[];
   identityDAL: Pick<TIdentityDALFactory, "find" | "transaction">;
-  identityOrgMembershipDAL: Pick<TIdentityOrgDALFactory, "findByIds">;
+  membershipDAL: Pick<TMembershipDALFactory, "find">;
   identityGroupMembershipDAL: Pick<TIdentityGroupMembershipDALFactory, "find" | "delete">;
 };
 

backend/src/server/routes/index.ts

@@ -757,7 +757,7 @@ export const registerRoutes = async (
   });
   const groupService = groupServiceFactory({
     identityDAL,
-    identityOrgMembershipDAL,
+    membershipDAL,
     identityGroupMembershipDAL,
     userDAL,
     groupDAL,

backend/src/services/identity/identity-org-dal.ts

@@ -674,24 +674,5 @@ export const identityOrgDALFactory = (db: TDbClient) => {
     }
   };
 
-  const findByIds = async (identityIds: string[], orgId: string, tx?: Knex) => {
-    try {
-      const identities = await (tx || db.replicaNode())(TableName.Identity)
-        .join(TableName.Membership, `${TableName.Membership}.actorIdentityId`, `${TableName.Identity}.id`)
-        .where(`${TableName.Membership}.scope`, AccessScope.Organization)
-        .where(`${TableName.Membership}.scopeOrgId`, orgId)
-        .whereNotNull(`${TableName.Membership}.actorIdentityId`)
-        .whereNull(`${TableName.Identity}.projectId`)
-        .whereIn(`${TableName.Identity}.id`, identityIds)
-        .distinctOn(`${TableName.Identity}.id`)
-        .orderBy(`${TableName.Identity}.id`)
-        .select(selectAllTableCols(TableName.Identity));
-
-      return identities;
-    } catch (error) {
-      throw new DatabaseError({ error, name: "findByIds" });
-    }
-  };
-
-  return { find, findOne, countAllOrgIdentities, searchIdentities, findByIds };
+  return { find, findOne, countAllOrgIdentities, searchIdentities };
 };