fix: include members attribute for scim groups unless opting out explicitly

Author: n1ru4l

integration-tests/testkit/scim.ts

@@ -115,6 +115,7 @@ export type SCIMListQuery = {
   count?: string | number;
   startIndex?: string | number;
   filter?: string;
+  excludedAttributes?: 'members';
 };
 
 export function createScimTestkit({ baseUrl, headers }: { baseUrl: string; headers: HeadersInit }) {

integration-tests/tests/api/auth/scim.spec.ts

@@ -2922,6 +2922,67 @@ describe.concurrent('/Groups', () => {
     });
   });
   describe.concurrent('GET', () => {
+    test.concurrent(
+      'lists members by default and excludes them when requested',
+      async ({ expect }) => {
+        const seed = initSeed();
+        const owner = await seed.createOwner();
+        const org = await owner.createOrg();
+        await org.setFeatureFlag('scim', true);
+        const oidc = await org.createOIDCIntegration();
+        const domain = await oidc.registerFakeDomain();
+        const accessToken = await org.createOrganizationAccessToken({
+          permissions: ['scim:provision'],
+          resources: { mode: ResourceAssignmentModeType.Granular },
+        });
+        const scim = createScimTestkit({
+          baseUrl,
+          headers: {
+            'Content-Type': 'application/scim+json',
+            Authorization: 'Bearer ' + accessToken.privateAccessKey,
+          },
+        });
+        const user = await scim
+          .createUser({
+            ...newUserValues(),
+            emails: [{ primary: true, type: 'work', value: 'listed-member@' + domain }],
+          })
+          .then(response => response.body);
+        const group = await scim
+          .createGroup({
+            ...newGroupValues(),
+            displayName: 'Group with listed member',
+            members: [{ value: user.id }],
+          })
+          .then(response => response.body);
+        const expectedMembers = [
+          {
+            value: user.id,
+            $ref: baseUrl + '/scim/v2/Users/' + user.id,
+          },
+        ];
+
+        const listResponse = await scim.listGroups();
+        expect(listResponse.body.Resources).toContainEqual(
+          expect.objectContaining({ id: group.id, members: expectedMembers }),
+        );
+
+        const filteredResponse = await scim.listGroups({
+          filter: `id eq "${group.id}"`,
+        });
+        expect(filteredResponse.body.Resources[0]?.members).toEqual(expectedMembers);
+
+        const excludedListResponse = await scim.listGroups({ excludedAttributes: 'members' });
+        expect(excludedListResponse.body.Resources[0]).not.toHaveProperty('members');
+
+        const excludedFilteredResponse = await scim.listGroups({
+          filter: `id eq "${group.id}"`,
+          excludedAttributes: 'members',
+        });
+        expect(excludedFilteredResponse.body.Resources[0]).not.toHaveProperty('members');
+      },
+    );
+
     test.concurrent('excludes members when requested', async ({ expect }) => {
       const seed = initSeed();
       const owner = await seed.createOwner();

packages/services/api/src/modules/organization/providers/group-member-store.ts

@@ -102,6 +102,28 @@ export class GroupMemberStore {
     return z.array(GroupMemberModel).parse(result);
   }
 
+  async getGroupMembersForOrganizationIdAndGroupIds(
+    organizationId: string,
+    groupIds: Array<string>,
+  ) {
+    const result = await this.pool.any(psql`
+      SELECT ${groupMemberFields}
+      FROM "group_members"
+      WHERE
+        "organization_id" = ${organizationId}
+        AND "group_id" = ANY(${psql.array(groupIds, 'uuid')})
+    `);
+    const records =  z.array(GroupMemberModel).parse(result);
+    const groupMembersByGroupId = new Map<string, Array<GroupMember>>();
+    for (const groupMember of records) {
+      const members = groupMembersByGroupId.get(groupMember.groupId) ?? [];
+      members.push(groupMember);
+      groupMembersByGroupId.set(groupMember.groupId, members);
+    }
+
+    return groupMembersByGroupId
+  }
+
   async addGroupMembersToGroupByOrganizationIdAndGroupId(
     organizationId: string,
     groupId: string,

packages/services/server/src/scim.ts

@@ -91,6 +91,8 @@ const GetGroupQueryModel = z.object({
   excludedAttributes: z.literal('members').optional(),
 });
 
+const GetGroupsQueryModel = QuerySchemaModel.merge(GetGroupQueryModel);
+
 const SharedUserRouteParams = z.object({
   userId: z.string().uuid(),
 });
@@ -1194,7 +1196,7 @@ export const createSCIMPlugin =
         return reply.status(result.error.status).send(result.error);
       }
 
-      const queryParse = QuerySchemaModel.safeParse(req.query);
+      const queryParse = GetGroupsQueryModel.safeParse(req.query);
       if (queryParse.error) {
         return reply.status(403).send(
           createSCIMError({
@@ -1205,6 +1207,7 @@ export const createSCIMPlugin =
       }
 
       const groupStore = new GroupStore(result.logger, pool);
+      const groupMemberStore = new GroupMemberStore(result.logger, pool);
 
       const startIndex = queryParse.data.startIndex ?? 1;
       const count = queryParse.data.count ?? 100;
@@ -1252,12 +1255,20 @@ export const createSCIMPlugin =
           }
         }
 
+        const groupMembers =
+          group && queryParse.data.excludedAttributes !== 'members'
+            ? await groupMemberStore.getGroupMembersForOrganizationIdAndGroupId(
+                result.organizationId,
+                group.id,
+              )
+            : undefined;
+
         return reply.status(200).send({
           schemas: ['urn:ietf:params:scim:api:messages:2.0:ListResponse'],
           totalResults: group ? 1 : 0,
           startIndex,
           itemsPerPage: group ? 1 : 0,
-          Resources: group ? [createSCIMGroupObjectFromGroup(baseUri, group)] : [],
+          Resources: group ? [createSCIMGroupObjectFromGroup(baseUri, group, groupMembers)] : [],
         } satisfies SCIMListResponseObject);
       }
 
@@ -1269,8 +1280,24 @@ export const createSCIMPlugin =
         },
       );
 
+      const groupMembersByGroupId =
+        queryParse.data.excludedAttributes !== 'members' && pagedGroups.length > 0
+          ? await groupMemberStore.getGroupMembersForOrganizationIdAndGroupIds(
+              result.organizationId,
+              pagedGroups.map(group => group.id),
+            )
+          : null;
+
       for (const group of pagedGroups) {
-        groups.push(createSCIMGroupObjectFromGroup(baseUri, group));
+        groups.push(
+          createSCIMGroupObjectFromGroup(
+            baseUri,
+            group,
+            groupMembersByGroupId === null
+              ? undefined
+              : (groupMembersByGroupId.get(group.id) ?? []),
+          ),
+        );
       }
 
       return reply.status(200).send({
@@ -1957,11 +1984,7 @@ function createSCIMGroupObjectFromGroup(
   baseUri: string,
   group: Group,
   /**
-   * The members are optional as they do not need to be included within actions such as
-   * "list all groups".
-   *
-   * Only when a specific group object is requested or updated we include the list of members
-   * so the SCIM provider can see if a user is or is not a member of an organization.
+   * Members are omitted when the client requests `excludedAttributes=members`.
    */
   members?: Array<GroupMember>,
 ): SCIMGroupObject {