Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions packages/cli/src/modules/mcp/__tests__/mcp-oauth-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,52 @@ describe('McpOAuthService', () => {
});
});

describe('deleteClient', () => {
it('should delete client when user has consent', async () => {
const client = {
id: 'client-123',
name: 'Test Client',
} as OAuthClient;

oauthClientRepository.findOne.mockResolvedValue(client);
userConsentRepository.findOneBy.mockResolvedValue({
userId: 'user-456',
clientId: 'client-123',
} as any);
oauthClientRepository.delete.mockResolvedValue({} as any);

await service.deleteClient('client-123', 'user-456');

expect(oauthClientRepository.delete).toHaveBeenCalledWith({ id: 'client-123' });
});

it('should throw when client does not exist', async () => {
oauthClientRepository.findOne.mockResolvedValue(null);

await expect(service.deleteClient('nonexistent', 'user-456')).rejects.toThrow(
'OAuth client with ID nonexistent not found',
);

expect(oauthClientRepository.delete).not.toHaveBeenCalled();
});

it('should throw when user has no consent for the client', async () => {
const client = {
id: 'client-123',
name: 'Test Client',
} as OAuthClient;

oauthClientRepository.findOne.mockResolvedValue(client);
userConsentRepository.findOneBy.mockResolvedValue(null);

await expect(service.deleteClient('client-123', 'other-user')).rejects.toThrow(
'OAuth client with ID client-123 not found',
);

expect(oauthClientRepository.delete).not.toHaveBeenCalled();
});
});

describe('revokeToken', () => {
it('should revoke access token when type hint is access_token', async () => {
const client = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { testDb } from '@n8n/backend-test-utils';
import type { User } from '@n8n/db';
import { Container } from '@n8n/di';

import { createOwner, createMember } from '@test-integration/db/users';
import { setupTestServer } from '@test-integration/utils';

import { OAuthClientRepository } from '../database/repositories/oauth-client.repository';
import { UserConsentRepository } from '../database/repositories/oauth-user-consent.repository';

const testServer = setupTestServer({ endpointGroups: ['mcp'], modules: ['mcp'] });

let owner: User;
let member: User;
let oauthClientRepository: OAuthClientRepository;
let userConsentRepository: UserConsentRepository;

beforeAll(async () => {
owner = await createOwner();
member = await createMember();
oauthClientRepository = Container.get(OAuthClientRepository);
userConsentRepository = Container.get(UserConsentRepository);
});

afterEach(async () => {
await testDb.truncate(['OAuthClient', 'UserConsent']);
});

describe('DELETE /rest/mcp/oauth-clients/:clientId', () => {
test('should allow a user to delete their own OAuth client', async () => {
const client = await oauthClientRepository.save({
id: 'owner-client-id',
name: 'Owner Client',
redirectUris: ['https://example.com/callback'],
grantTypes: ['authorization_code'],
tokenEndpointAuthMethod: 'none',
});

await userConsentRepository.save({
userId: owner.id,
clientId: client.id,
grantedAt: Date.now(),
});

const response = await testServer.authAgentFor(owner).delete(`/mcp/oauth-clients/${client.id}`);

expect(response.statusCode).toBe(200);
expect(response.body.data).toMatchObject({
success: true,
});

const deletedClient = await oauthClientRepository.findOneBy({ id: client.id });
expect(deletedClient).toBeNull();
});

test("should return 404 when a user tries to delete another user's OAuth client", async () => {
const client = await oauthClientRepository.save({
id: 'owner-only-client',
name: 'Owner Only Client',
redirectUris: ['https://example.com/callback'],
grantTypes: ['authorization_code'],
tokenEndpointAuthMethod: 'none',
});

await userConsentRepository.save({
userId: owner.id,
clientId: client.id,
grantedAt: Date.now(),
});

const response = await testServer
.authAgentFor(member)
.delete(`/mcp/oauth-clients/${client.id}`);

expect(response.statusCode).toBe(404);

// Verify the client was NOT deleted
const existingClient = await oauthClientRepository.findOneBy({ id: client.id });
expect(existingClient).not.toBeNull();
});

test('should return 404 when deleting a non-existent OAuth client', async () => {
const response = await testServer
.authAgentFor(owner)
.delete('/mcp/oauth-clients/non-existent-id');

expect(response.statusCode).toBe(404);
});
});
11 changes: 9 additions & 2 deletions packages/cli/src/modules/mcp/mcp-oauth-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,10 @@ export class McpOAuthService implements OAuthServerProvider {
}

/**
* Delete an OAuth client and all related data
* Delete an OAuth client and all related data.
* Verifies that the requesting user has a consent relationship with the client.
*/
async deleteClient(clientId: string): Promise<void> {
async deleteClient(clientId: string, userId: string): Promise<void> {
// First check if the client exists
const client = await this.oauthClientRepository.findOne({
where: { id: clientId },
Expand All @@ -222,6 +223,12 @@ export class McpOAuthService implements OAuthServerProvider {
throw new Error(`OAuth client with ID ${clientId} not found`);
}

// Verify the requesting user has a consent relationship with this client
const consent = await this.userConsentRepository.findOneBy({ clientId, userId });

@cubic-dev-ai cubic-dev-ai Bot Mar 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The new delete authorization checks consent instead of client ownership, so a non-owner user with consent to the same clientId can still delete the shared OAuth client.

According to linked Linear issue ADO-5000, this does not fully satisfy the required ownership check for cross-user deletion prevention.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/modules/mcp/mcp-oauth-service.ts, line 227:

<comment>The new delete authorization checks consent instead of client ownership, so a non-owner user with consent to the same `clientId` can still delete the shared OAuth client.

According to linked Linear issue ADO-5000, this does not fully satisfy the required ownership check for cross-user deletion prevention.</comment>

<file context>
@@ -222,6 +223,12 @@ export class McpOAuthService implements OAuthServerProvider {
 		}
 
+		// Verify the requesting user has a consent relationship with this client
+		const consent = await this.userConsentRepository.findOneBy({ clientId, userId });
+		if (!consent) {
+			throw new Error(`OAuth client with ID ${clientId} not found`);
</file context>
Fix with Cubic

if (!consent) {
throw new Error(`OAuth client with ID ${clientId} not found`);
}

this.logger.info('Deleting OAuth client and related data', { clientId });

await this.oauthClientRepository.delete({ id: clientId });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class McpOAuthClientsController {
});

try {
await this.mcpOAuthService.deleteClient(clientId);
await this.mcpOAuthService.deleteClient(clientId, req.user.id);

this.logger.info('OAuth client deleted successfully', {
clientId,
Expand Down
Loading