fix(editor): Fixes issue that prevented some users from revoking their end-user credentials (backport to release-candidate/2.39.x) (#38455)

Co-authored-by: James Martin <393605+ymhr@users.noreply.github.com>
This commit is contained in:
n8n-assistant[bot] 2026-09-11 12:13:22 +00:00 committed by GitHub
parent b10537fee3
commit 3d8f578a39
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 330 additions and 49 deletions

View File

@ -89,6 +89,8 @@ describe('POST /credentials/:id/authorize - CORS Integration', () => {
id: randomId(),
name: 'Test OAuth2 Credential',
type: 'oAuth2Api',
// These routes only serve end-user credentials.
isResolvable: true,
data: cipher.encrypt({ clientId: 'test-client-id' }),
}),
);
@ -207,6 +209,8 @@ describe('DELETE /credentials/:id/revoke - CORS Integration', () => {
id: randomId(),
name: 'Test OAuth2 Credential',
type: 'oAuth2Api',
// These routes only serve end-user credentials.
isResolvable: true,
data: cipher.encrypt({ clientId: 'test-client-id' }),
}),
);

View File

@ -107,6 +107,8 @@ async function setupTestData() {
id: randomId(),
name: 'Test OAuth2 Credential',
type: 'oAuth2Api',
// These routes only serve end-user credentials.
isResolvable: true,
data: cipher.encrypt({ clientId: 'test-client-id' }),
}),
);

View File

@ -67,31 +67,128 @@ describe('DynamicCredentialsController', () => {
});
urlService.getInstanceBaseUrl.mockReturnValue('http://localhost:5678');
// Default: caller can access the credential
credentialsFinderService.findCredentialForUser.mockResolvedValue(mock<CredentialsEntity>());
// Default: the credential exists and is an end-user (resolvable) credential.
enterpriseCredentialsService.getOne.mockResolvedValue(
mock<CredentialsEntity>({ id: '1', type: 'googleOAuth2Api', isResolvable: true }),
);
});
describe('in-app access control', () => {
it('returns 404 when an authenticated user cannot access the credential', async () => {
credentialsFinderService.findCredentialForUser.mockResolvedValue(null);
const user = mock<AuthenticatedRequest['user']>({ id: 'user-123' });
const req = mock<AuthenticatedRequest>({
user,
params: { id: 'foreign-credential' },
const resolverEntity: DynamicCredentialResolver = {
id: 'resolver-123',
name: 'Test Resolver',
type: 'oauth2-introspection-identifier',
config: 'encrypted-config',
createdAt: new Date(),
updatedAt: new Date(),
generateId: vi.fn(),
setUpdateDate: vi.fn(),
};
const resolver = {
metadata: {
name: 'oauth2-introspection-identifier',
description: 'OAuth2 Introspection Identifier',
},
getSecret: vi.fn(),
setSecret: vi.fn(),
validateOptions: vi.fn(),
deleteSecret: vi.fn().mockResolvedValue(undefined),
};
const sessionRequest = (credentialId: string) =>
mock<AuthenticatedRequest>({
user: mock<AuthenticatedRequest['user']>({ id: 'user-123' }),
params: { id: credentialId },
query: { resolverId: 'resolver-123' },
headers: { authorization: 'Bearer token123' },
});
const credential = (id: string, isResolvable: boolean) =>
mock<CredentialsEntity>({ id, type: 'googleOAuth2Api', isResolvable });
// Connecting an end-user credential carries no scope check, and the resolver
// keys every read and write on the caller's own identity — so disconnecting
// and reconnecting must not need one either. A user with no project role can
// only ever reach their own stored token.
it('lets a session user with no scopes authorize a resolvable credential', async () => {
const req = sessionRequest('someone-elses-credential');
const res = mock<Response>();
enterpriseCredentialsService.getOne.mockResolvedValueOnce(
credential('someone-elses-credential', true),
);
resolverRepository.findOneBy.mockResolvedValueOnce(resolverEntity);
resolverRegistry.getResolverByTypename.mockReturnValueOnce(resolver);
oauthService.generateAOauth2AuthUri.mockResolvedValueOnce(
'https://example.domain/oauth2/auth',
);
await expect(controller.authorizeCredential(req, res)).resolves.toContain(
'https://example.domain/oauth2/auth',
);
expect(enterpriseCredentialsService.getOne).toHaveBeenCalledWith('someone-elses-credential');
expect(credentialsFinderService.findCredentialForUser).not.toHaveBeenCalled();
});
it('lets a session user with no scopes revoke a resolvable credential', async () => {
const req = sessionRequest('someone-elses-credential');
const res = mock<Response>();
res.status.mockReturnThis();
enterpriseCredentialsService.getOne.mockResolvedValueOnce(
credential('someone-elses-credential', true),
);
resolverRepository.findOneBy.mockResolvedValueOnce(resolverEntity);
resolverRegistry.getResolverByTypename.mockReturnValueOnce(resolver);
cipher.decryptV2.mockResolvedValueOnce('{}');
await controller.revokeCredential(req, res);
expect(resolver.deleteSecret).toHaveBeenCalledTimes(1);
expect(res.status).toHaveBeenCalledWith(204);
expect(credentialsFinderService.findCredentialForUser).not.toHaveBeenCalled();
});
it('rejects an authorize for a credential that is not resolvable', async () => {
const req = sessionRequest('fixed-credential');
const res = mock<Response>();
enterpriseCredentialsService.getOne.mockResolvedValueOnce(
credential('fixed-credential', false),
);
await expect(controller.authorizeCredential(req, res)).rejects.toThrow(
'Credential not found',
);
expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith(
'foreign-credential',
user,
['credential:update'],
expect(oauthService.generateAOauth2AuthUri).not.toHaveBeenCalled();
});
it('rejects a revoke for a credential that is not resolvable', async () => {
const req = sessionRequest('fixed-credential');
const res = mock<Response>();
enterpriseCredentialsService.getOne.mockResolvedValueOnce(
credential('fixed-credential', false),
);
expect(enterpriseCredentialsService.getOne).not.toHaveBeenCalled();
await expect(controller.revokeCredential(req, res)).rejects.toThrow('Credential not found');
expect(resolver.deleteSecret).not.toHaveBeenCalled();
});
it('reports an unknown id the same way as a credential that is not resolvable', async () => {
const req = sessionRequest('no-such-credential');
const res = mock<Response>();
enterpriseCredentialsService.getOne.mockResolvedValueOnce(null);
await expect(controller.authorizeCredential(req, res)).rejects.toThrow(
'Credential not found',
);
enterpriseCredentialsService.getOne.mockResolvedValueOnce(null);
await expect(controller.revokeCredential(req, res)).rejects.toThrow('Credential not found');
});
});
@ -137,6 +234,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -154,6 +252,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'httpBasicAuth',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -174,6 +273,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -195,6 +295,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -219,6 +320,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -260,6 +362,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'twitterOAuth1Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -301,6 +404,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const mockResolverWithValidation = {
...mockResolver,
@ -345,6 +449,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -366,7 +471,11 @@ describe('DynamicCredentialsController', () => {
});
it('sets the state userId when the resolver binds the link to a user', async () => {
const mockCredential = mock<CredentialsEntity>({ id: '1', type: 'googleOAuth2Api' });
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
query: { resolverId: 'resolver-123' },
@ -396,7 +505,11 @@ describe('DynamicCredentialsController', () => {
});
it('leaves the state userId unset when the link is unbound', async () => {
const mockCredential = mock<CredentialsEntity>({ id: '1', type: 'googleOAuth2Api' });
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
query: { resolverId: 'resolver-123' },
@ -469,7 +582,11 @@ describe('DynamicCredentialsController', () => {
it('materializes the OAuth2 flow and redirects to the provider for a valid intent', async () => {
const req = mock<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
const res = mock<Response>();
const mockCredential = mock<CredentialsEntity>({ id: 'cred-1', type: 'googleOAuth2Api' });
const mockCredential = mock<CredentialsEntity>({
id: 'cred-1',
type: 'googleOAuth2Api',
isResolvable: true,
});
authorizeIntentService.get.mockResolvedValue({
credentialId: 'cred-1',
@ -502,7 +619,11 @@ describe('DynamicCredentialsController', () => {
it('materializes the OAuth1 flow for an OAuth1 credential', async () => {
const req = mock<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
const res = mock<Response>();
const mockCredential = mock<CredentialsEntity>({ id: 'cred-1', type: 'twitterOAuth1Api' });
const mockCredential = mock<CredentialsEntity>({
id: 'cred-1',
type: 'twitterOAuth1Api',
isResolvable: true,
});
authorizeIntentService.get.mockResolvedValue({
credentialId: 'cred-1',
@ -525,7 +646,11 @@ describe('DynamicCredentialsController', () => {
it('renders an error when materializing the provider URL fails', async () => {
const req = mock<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
const res = mock<Response>();
const mockCredential = mock<CredentialsEntity>({ id: 'cred-1', type: 'googleOAuth2Api' });
const mockCredential = mock<CredentialsEntity>({
id: 'cred-1',
type: 'googleOAuth2Api',
isResolvable: true,
});
authorizeIntentService.get.mockResolvedValue({
credentialId: 'cred-1',
@ -549,7 +674,11 @@ describe('DynamicCredentialsController', () => {
user: mock<AuthenticatedRequest['user']>({ id: 'user-1' }),
});
const res = mock<Response>();
const mockCredential = mock<CredentialsEntity>({ id: 'cred-1', type: 'googleOAuth2Api' });
const mockCredential = mock<CredentialsEntity>({
id: 'cred-1',
type: 'googleOAuth2Api',
isResolvable: true,
});
authorizeIntentService.get.mockResolvedValue({
credentialId: 'cred-1',
@ -580,7 +709,11 @@ describe('DynamicCredentialsController', () => {
user: mock<AuthenticatedRequest['user']>({ id: 'user-1' }),
});
const res = mock<Response>();
const mockCredential = mock<CredentialsEntity>({ id: 'cred-1', type: 'twitterOAuth1Api' });
const mockCredential = mock<CredentialsEntity>({
id: 'cred-1',
type: 'twitterOAuth1Api',
isResolvable: true,
});
authorizeIntentService.get.mockResolvedValue({
credentialId: 'cred-1',
@ -700,7 +833,11 @@ describe('DynamicCredentialsController', () => {
user: mock<AuthenticatedRequest['user']>({ id: 'some-other-user' }),
});
const res = mock<Response>();
const mockCredential = mock<CredentialsEntity>({ id: 'cred-1', type: 'googleOAuth2Api' });
const mockCredential = mock<CredentialsEntity>({
id: 'cred-1',
type: 'googleOAuth2Api',
isResolvable: true,
});
// No userId on the intent → link is unbound.
authorizeIntentService.get.mockResolvedValue({
@ -749,6 +886,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const req = mock<Request>({
params: { id: '1' },
@ -770,6 +908,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const mockResolver = {
metadata: {
@ -815,6 +954,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'googleOAuth2Api',
isResolvable: true,
});
const mockResolver = {
metadata: {
@ -850,6 +990,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: 'cred-456',
type: 'googleOAuth2Api',
isResolvable: true,
});
const mockResolver = {
metadata: {
@ -901,6 +1042,7 @@ describe('DynamicCredentialsController', () => {
const mockCredential = mock<CredentialsEntity>({
id: '1',
type: 'twitterOAuth1Api',
isResolvable: true,
});
const mockResolver = {
metadata: {

View File

@ -1,8 +1,7 @@
import { Time } from '@n8n/constants';
import { CredentialsEntity, AuthenticatedRequest, isAuthenticatedRequest, User } from '@n8n/db';
import { CredentialsEntity, AuthenticatedRequest, isAuthenticatedRequest } from '@n8n/db';
import { Delete, Get, Options, Param, Post, RestController } from '@n8n/decorators';
import { Container } from '@n8n/di';
import type { Scope } from '@n8n/permissions';
import { ensureError } from '@n8n/utils/errors/ensure-error';
import { Request, Response } from 'express';
import { Cipher } from 'n8n-core';
@ -50,20 +49,17 @@ export class DynamicCredentialsController {
private readonly urlService: UrlService,
) {}
private async findCredentialToUse(
credentialId: string,
user?: User,
scope?: Scope,
): Promise<CredentialsEntity> {
// External (static-token) callers have no n8n user; their identity is
// validated by the resolver, so we resolve the credential by id. When the
// request carries an n8n session user, enforce that user's access instead.
const credential =
user && scope
? await this.credentialsFinderService.findCredentialForUser(credentialId, user, [scope])
: await this.enterpriseCredentialsService.getOne(credentialId);
private async findCredentialToUse(credentialId: string): Promise<CredentialsEntity> {
// No project scope is checked: the connect half of this flow never had one, and
// the resolver keys every read and write on the caller's own identity, so a
// caller can only reach their own stored token.
const credential = await this.enterpriseCredentialsService.getOne(credentialId);
if (!credential) {
// These routes serve only end-user credentials, whose token is per-caller. A
// fixed credential's token lives on the shared row, so changing it is an edit
// and stays on the `credential:update` paths. Folded into the not-found branch
// so the response cannot tell "fixed" from "no such credential".
if (!credential?.isResolvable) {
throw new NotFoundError('Credential not found');
}
@ -119,8 +115,7 @@ export class DynamicCredentialsController {
async revokeCredential(req: Request, res: Response): Promise<void> {
this.dynamicCredentialCorsService.applyCorsHeadersIfEnabled(req, res, ['delete', 'options']);
const credentialContext = this.dynamicCredentialWebService.getCredentialContextFromRequest(req);
const user = isAuthenticatedRequest(req) ? req.user : undefined;
const credential = await this.findCredentialToUse(req.params.id, user, 'credential:update');
const credential = await this.findCredentialToUse(req.params.id);
const resolverId = req.query.resolverId as string | undefined;
const { resolver, resolverEntity } = await this.getResolverInstance(resolverId);
@ -161,8 +156,7 @@ export class DynamicCredentialsController {
async authorizeCredential(req: Request, res: Response): Promise<string> {
this.dynamicCredentialCorsService.applyCorsHeadersIfEnabled(req, res, ['post', 'options']);
const credentialContext = this.dynamicCredentialWebService.getCredentialContextFromRequest(req);
const user = isAuthenticatedRequest(req) ? req.user : undefined;
const credential = await this.findCredentialToUse(req.params.id, user, 'credential:update');
const credential = await this.findCredentialToUse(req.params.id);
const resolverId = req.query.resolverId as string | undefined;
const { resolver, resolverEntity } = await this.getResolverInstance(resolverId);

View File

@ -3,11 +3,20 @@ import { mockInstance, getPersonalProject, testDb } from '@n8n/backend-test-util
import type { CredentialsEntity, User } from '@n8n/db';
import { GLOBAL_OWNER_ROLE } from '@n8n/db';
import { Container } from '@n8n/di';
import { Cipher } from 'n8n-core';
import nock from 'nock';
import { mock } from 'vitest-mock-extended';
import { CredentialsHelper } from '@/credentials-helper';
import {
SYSTEM_RESOLVER_ID,
SYSTEM_RESOLVER_NAME,
SYSTEM_RESOLVER_TYPE,
} from '@/modules/dynamic-credentials.ee/constants';
import { DynamicCredentialUserEntryStorage } from '@/modules/dynamic-credentials.ee/credential-resolvers/storage/dynamic-credential-user-entry-storage';
import type { DynamicCredentialResolver } from '@/modules/dynamic-credentials.ee/database/entities/credential-resolver';
import { DynamicCredentialResolverRepository } from '@/modules/dynamic-credentials.ee/database/repositories/credential-resolver.repository';
import { DynamicCredentialUserEntryRepository } from '@/modules/dynamic-credentials.ee/database/repositories/dynamic-credential-user-entry.repository';
import { DynamicCredentialsConfig } from '@/modules/dynamic-credentials.ee/dynamic-credentials.config';
import { DynamicCredentialResolverService } from '@/modules/dynamic-credentials.ee/services/credential-resolver.service';
import { Telemetry } from '@/telemetry';
@ -271,27 +280,157 @@ describe('Dynamic Credentials API', () => {
});
});
describe("when an unrelated authenticated member targets another user's credential", () => {
it('should not return an authorization URL for a credential the member cannot access', async () => {
// These two used to expect 403/404: the endpoints were gated on
// `credential:update`, so a user could connect their own account and then not
// disconnect or reconnect it. Connect and disconnect are two halves of one
// self-service action, and the connect half — the intent link, the OAuth
// callback, `/my-connection` — never carried a scope check. The delete is
// self-scoped as well: the resolver derives the storage key from the caller's
// own identity and the route has no `:userId`, so the worst a caller can do is
// clear their own row.
describe('when an authenticated member holds no project role on the credential', () => {
it('should return an authorization URL for an end-user credential', async () => {
const response = await testServer
.authAgentFor(unrelatedMember)
.post(`/credentials/${savedCredential.id}/authorize`)
.query({ resolverId: resolver.id })
.set('Authorization', 'Bearer test-token');
.set('Authorization', 'Bearer test-token')
.expect(200);
expect([403, 404]).toContain(response.status);
expect(response.body?.data).toBeUndefined();
expect(response.body.data).toContain('https://test.domain/oauth2/auth');
});
it('should not revoke a credential the member cannot access', async () => {
const response = await testServer
it('should revoke their own connection to an end-user credential', async () => {
await testServer
.authAgentFor(unrelatedMember)
.delete(`/credentials/${savedCredential.id}/revoke`)
.query({ resolverId: resolver.id })
.set('Authorization', 'Bearer test-token');
.set('Authorization', 'Bearer test-token')
.expect(204);
});
});
expect([403, 404]).toContain(response.status);
describe('when the credential is not an end-user credential', () => {
let fixedCredential: CredentialsEntity;
beforeAll(async () => {
// Same shape as `savedCredential`, but a fixed credential: its OAuth token
// lives on the shared row, so these routes must not reach it.
fixedCredential = await saveCredential(
{
name: 'Test Fixed Credential',
type: 'oAuth2Api',
data: {
clientId: 'test-client-id',
clientSecret: 'test-client-secret',
authUrl: 'https://test.domain/oauth2/auth',
accessTokenUrl: 'https://test.domain/oauth2/token',
grantType: 'authorizationCode',
},
},
{ user: owner, role: 'credential:owner' },
);
});
it('should refuse to authorize it, indistinguishably from an unknown id', async () => {
const fixed = await testServer
.authAgentFor(owner)
.post(`/credentials/${fixedCredential.id}/authorize`)
.query({ resolverId: resolver.id })
.set('Authorization', 'Bearer test-token')
.expect(404);
const unknown = await testServer
.authAgentFor(owner)
.post('/credentials/no-such-credential/authorize')
.query({ resolverId: resolver.id })
.set('Authorization', 'Bearer test-token')
.expect(404);
expect(fixed.body?.data).toBeUndefined();
expect(fixed.body.message).toBe(unknown.body.message);
});
it('should refuse to revoke it, indistinguishably from an unknown id', async () => {
const fixed = await testServer
.authAgentFor(owner)
.delete(`/credentials/${fixedCredential.id}/revoke`)
.query({ resolverId: resolver.id })
.set('Authorization', 'Bearer test-token')
.expect(404);
const unknown = await testServer
.authAgentFor(owner)
.delete('/credentials/no-such-credential/revoke')
.query({ resolverId: resolver.id })
.set('Authorization', 'Bearer test-token')
.expect(404);
expect(fixed.body.message).toBe(unknown.body.message);
});
});
});
describe('DELETE /credentials/:id/revoke with the n8n resolver', () => {
let userEntryRepository: DynamicCredentialUserEntryRepository;
let storage: DynamicCredentialUserEntryStorage;
let outsider: User;
let bystander: User;
beforeAll(async () => {
// The outer truncate drops the seeded system resolver, so put it back.
const resolverRepository = Container.get(DynamicCredentialResolverRepository);
await resolverRepository.save(
resolverRepository.create({
id: SYSTEM_RESOLVER_ID,
name: SYSTEM_RESOLVER_NAME,
type: SYSTEM_RESOLVER_TYPE,
config: await Container.get(Cipher).encryptV2({}),
}),
);
userEntryRepository = Container.get(DynamicCredentialUserEntryRepository);
storage = Container.get(DynamicCredentialUserEntryStorage);
// Neither user has any project relation to the owner's credential.
outsider = await createUser();
bystander = await createUser();
});
beforeEach(async () => {
await storage.setCredentialData(
savedCredential.id,
outsider.id,
SYSTEM_RESOLVER_ID,
'outsider-token',
{},
);
await storage.setCredentialData(
savedCredential.id,
bystander.id,
SYSTEM_RESOLVER_ID,
'bystander-token',
{},
);
});
it("should clear only the calling user's connection", async () => {
await testServer
.authAgentFor(outsider)
.delete(`/credentials/${savedCredential.id}/revoke`)
.query({ resolverId: SYSTEM_RESOLVER_ID, authSource: 'cookie' })
.expect(204);
await expect(
userEntryRepository.find({
where: { credentialId: savedCredential.id, userId: outsider.id },
}),
).resolves.toHaveLength(0);
await expect(
userEntryRepository.find({
where: { credentialId: savedCredential.id, userId: bystander.id },
}),
).resolves.toHaveLength(1);
});
});
});