fix: unselect orgs under a row lock

Author: alexey-yarmosh

src/extensions/endpoints/sync-github-data/test/index.test.ts

@@ -16,6 +16,8 @@ describe('/sync-github-data endpoint', () => {
 	const itemsCreateOne = sinon.stub();
 	const itemsUpdateOne = sinon.stub();
 	const deleteMany = sinon.stub();
+	const userRow = sinon.stub();
+	const trx = sinon.stub().returns({ where: sinon.stub().returnsThis(), forUpdate: sinon.stub().returnsThis(), first: userRow });
 	const itemsServiceStub = sinon.stub().returns({
 		readOne,
 		readByQuery,
@@ -46,7 +48,7 @@ describe('/sync-github-data endpoint', () => {
 			NotificationsService: notificationsServiceStub,
 		},
 		database: {
-			transaction: async (callback: (trx: unknown) => unknown) => callback({}),
+			transaction: async (callback: (trx: unknown) => unknown) => callback(trx),
 		},
 		getSchema: sinon.stub().resolves({}),
 	} as unknown as EndpointExtensionContext;
@@ -85,6 +87,7 @@ describe('/sync-github-data endpoint', () => {
 		readByQuery.resolves([]);
 		itemsCreateOne.resolves('created-id');
 		deleteMany.resolves();
+		userRow.resolves({ selected_orgs: null });
 
 		readOne.resolves({
 			id: 'directus-id',
@@ -282,14 +285,7 @@ describe('/sync-github-data endpoint', () => {
 		});
 
 		it('should drop an org the user left from their selected list', async () => {
-			readOne.resolves({
-				id: 'directus-id',
-				external_identifier: '123456',
-				github_username: 'new-username',
-				github_organizations: [ 'jsdelivr' ],
-				github_oauth_token: 'user-github-token',
-				selected_orgs: [ 'org-1', 'org-2' ],
-			});
+			userRow.resolves({ selected_orgs: JSON.stringify([ 'org-1', 'org-2' ]) });
 
 			seedDirectus({
 				orgs: [],

src/extensions/lib/src/sync-orgs.ts

@@ -6,7 +6,6 @@ import type { GithubOrganization } from './github-api-client.js';
 type User = {
 	id: string;
 	external_identifier: string | null;
-	selected_orgs?: string[];
 };
 
 type Org = {
@@ -124,18 +123,21 @@ const removeMemberships = async (user: User, githubOrgs: GithubOrganization[], m
 	await Promise.all([
 		// The member's org tokens and approvals are removed by a database trigger.
 		membersService.deleteMany(removed.map(membership => membership.id)),
-		unselectOrgs(user, removed.map(membership => membership.org.id), context),
+		unselectOrgs(user.id, removed.map(membership => membership.org.id), context),
 	]);
 };
 
-export const unselectOrgs = async (user: Pick<User, 'id' | 'selected_orgs'>, removedOrgIds: string[], { services, getSchema }: ApiExtensionContext) => {
-	const selectedOrgs = user.selected_orgs ?? [];
-	const remaining = selectedOrgs.filter(orgId => !removedOrgIds.includes(orgId));
+export const unselectOrgs = async (userId: string, removedOrgIds: string[], { database, services, getSchema }: ApiExtensionContext) => {
+	await database.transaction(async (trx) => {
+		const user = await trx('directus_users').where({ id: userId }).forUpdate().first('selected_orgs');
+		const selectedOrgs: string[] = JSON.parse(user?.selected_orgs || '[]');
+		const remaining = selectedOrgs.filter(orgId => !removedOrgIds.includes(orgId));
 
-	if (remaining.length === selectedOrgs.length) {
-		return;
-	}
+		if (remaining.length === selectedOrgs.length) {
+			return;
+		}
 
-	const usersService = new services.UsersService({ schema: await getSchema() });
-	await usersService.updateOne(user.id, { selected_orgs: remaining }, { emitEvents: false });
+		const usersService = new services.UsersService({ schema: await getSchema(), knex: trx });
+		await usersService.updateOne(userId, { selected_orgs: remaining }, { emitEvents: false });
+	});
 };

src/extensions/operations/check-members-cron-handler/src/repositories/directus.ts

@@ -4,7 +4,7 @@ import type { Org } from '../types.js';
 
 type RemovedMembership = {
 	org: string;
-	user: { id: string; selected_orgs?: string[] };
+	user: string;
 };
 
 type Row = {
@@ -57,7 +57,7 @@ export const removeMemberships = async (membershipIds: string[], context: Operat
 	const { services, getSchema } = context;
 	const { ItemsService } = services;
 	const membersService = new ItemsService('gp_org_members', { schema: await getSchema() });
-	const removed = await membersService.readMany(membershipIds, { fields: [ 'org', 'user.id', 'user.selected_orgs' ] }) as RemovedMembership[];
+	const removed = await membersService.readMany(membershipIds, { fields: [ 'org', 'user' ] }) as RemovedMembership[];
 
 	await membersService.deleteMany(membershipIds);
 	// The members' org tokens and app approvals are removed by a database trigger.

src/extensions/operations/check-members-cron-handler/test/api.test.ts

@@ -32,6 +32,10 @@ describe('Check org members CRON handler', () => {
 	query.select = sinon.stub().callsFake((arg: unknown) => typeof arg === 'string' ? query : Promise.resolve(rows));
 
 	const database = sinon.stub().returns(query) as any;
+	const userRow = sinon.stub();
+	const userQuery = { where: sinon.stub().returnsThis(), forUpdate: sinon.stub().returnsThis(), first: userRow };
+	const trx = sinon.stub().returns(userQuery);
+	database.transaction = (callback: (trx: unknown) => unknown) => callback(trx);
 
 	const deleteMany = sinon.stub().resolves([]);
 	const readMany = sinon.stub().resolves([]);
@@ -92,6 +96,7 @@ describe('Check org members CRON handler', () => {
 		sinon.resetHistory();
 		deleteMany.resolves([]);
 		readMany.resolves([]);
+		userRow.resolves({ selected_orgs: null });
 		rows = [];
 	});
 
@@ -156,19 +161,24 @@ describe('Check org members CRON handler', () => {
 		rows = [ member(1), member(2) ];
 		nockSelfCheck('token-1', 200);
 		nockGraphql({ 1: [ org.orgGithubId ], 2: [ '555' ] });
-		readMany.resolves([{ org: org.orgId, user: { id: 'user-2', selected_orgs: [ org.orgId, 'other-org' ] } }]);
+		readMany.resolves([{ org: org.orgId, user: 'user-2' }]);
+		userRow.resolves({ selected_orgs: JSON.stringify([ org.orgId, 'other-org' ]) });
 
 		await operationApi.handler({}, context as any);
 
-		expect(readMany.args[0]).to.deep.equal([ [ 'membership-2' ], { fields: [ 'org', 'user.id', 'user.selected_orgs' ] }]);
+		expect(readMany.args[0]).to.deep.equal([ [ 'membership-2' ], { fields: [ 'org', 'user' ] }]);
+		expect(userQuery.where.args[0]).to.deep.equal([{ id: 'user-2' }]);
+		expect(userQuery.forUpdate.callCount).to.equal(1);
+		expect(services.UsersService.args[0]![0].knex).to.equal(trx);
 		expect(updateOne.args[0]).to.deep.equal([ 'user-2', { selected_orgs: [ 'other-org' ] }, { emitEvents: false }]);
 	});
 
 	it('should not touch the selected orgs of a member who did not have the org selected', async () => {
 		rows = [ member(1), member(2) ];
 		nockSelfCheck('token-1', 200);
 		nockGraphql({ 1: [ org.orgGithubId ], 2: [ '555' ] });
-		readMany.resolves([{ org: org.orgId, user: { id: 'user-2', selected_orgs: [ 'other-org' ] } }]);
+		readMany.resolves([{ org: org.orgId, user: 'user-2' }]);
+		userRow.resolves({ selected_orgs: JSON.stringify([ 'other-org' ]) });
 
 		await operationApi.handler({}, context as any);