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
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ describe('CredentialsService', () => {
jest.mocked(credentialsService.decrypt).mockReturnValue({ apiKey: 'regular-secret' });

await expect(
updateCredential('cred-id', memberUser, {
updateCredential(existingCredential, memberUser, {
data: { apiKey: '{{ $secrets.vault.myKey }}' },
}),
).rejects.toThrow('Lacking permissions to reference external secrets in credentials');
Expand All @@ -517,7 +517,7 @@ describe('CredentialsService', () => {
.mockReturnValue({ apiKey: '{{ $secrets.vault.oldKey }}' });

await expect(
updateCredential('cred-id', memberUser, {
updateCredential(existingCredential, memberUser, {
data: { apiKey: '{{ $secrets.vault.newKey }}' },
}),
).rejects.toThrow('Lacking permissions to reference external secrets in credentials');
Expand Down Expand Up @@ -546,7 +546,7 @@ describe('CredentialsService', () => {
mockExternalSecretsConfig.externalSecretsForProjects = true;

await expect(
updateCredential(existingCredential.id, ownerUser, {
updateCredential(existingCredential, ownerUser, {
data: { apiKey: secretExpression },
}),
).rejects.toThrow(
Expand All @@ -573,7 +573,7 @@ describe('CredentialsService', () => {

credentialsRepository.update = jest.fn().mockResolvedValue(undefined);

await updateCredential('cred-id', memberUser, {
await updateCredential(existingCredential, memberUser, {
name: 'Updated Name',
});
});
Expand All @@ -595,7 +595,7 @@ describe('CredentialsService', () => {

credentialsRepository.update = jest.fn().mockResolvedValue(undefined);

await updateCredential('cred-id', memberUser, {
await updateCredential(existingCredential, memberUser, {
data: { apiKey: 'another-regular-key' },
});
});
Expand All @@ -617,7 +617,7 @@ describe('CredentialsService', () => {
.mockResolvedValue(true);
credentialsRepository.update = jest.fn().mockResolvedValue(undefined);

await updateCredential('cred-id', ownerUser, {
await updateCredential(existingCredential, ownerUser, {
data: { apiKey: '{{ $secrets.vault.myKey }}' },
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,12 @@ export = {
): Promise<express.Response<Partial<CredentialsEntity>>> => {
const { id: credentialId } = req.params;

if (req.body.isGlobal !== undefined) {
const existingCredential = await getCredential(credentialId);
if (!existingCredential) {
return res.status(404).json({ message: 'Credential not found' });
}

if (req.body.isGlobal !== undefined && req.body.isGlobal !== existingCredential.isGlobal) {
if (!Container.get(LicenseState).isSharingLicensed()) {
return res.status(403).json({ message: 'You are not licensed for sharing credentials' });
}
Expand All @@ -135,11 +140,7 @@ export = {
}

try {
const updatedCredential = await updateCredential(credentialId, req.user, req.body);

if (!updatedCredential) {
return res.status(404).json({ message: 'Credential not found' });
}
const updatedCredential = await updateCredential(existingCredential, req.user, req.body);

return res.json(sanitizeCredentials(updatedCredential as CredentialsEntity));
} catch (error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export async function saveCredential(
}

export async function updateCredential(
credentialId: string,
existingCredential: ICredentialsDb,
user: User,
updateData: {
type?: string;
Expand All @@ -167,16 +167,13 @@ export async function updateCredential(
isResolvable?: boolean;
isPartialData?: boolean;
},
): Promise<ICredentialsDb | null> {
const existingCredential = await getCredential(credentialId);
if (!existingCredential) {
return null;
}

): Promise<ICredentialsDb> {
if (existingCredential.isManaged) {
throw new CredentialsIsNotUpdatableError('Managed credentials cannot be updated.');
}

const credentialId = existingCredential.id;

// Merge the update data with existing credential
const credentialData: Partial<CredentialsEntity> = {};

Expand Down Expand Up @@ -258,7 +255,8 @@ export async function updateCredential(

await Container.get(CredentialsRepository).update(credentialId, credentialData);

return await getCredential(credentialId);
// credential exists since we just updated it
return (await getCredential(credentialId))!;
}

export async function removeCredential(
Expand Down
31 changes: 31 additions & 0 deletions packages/cli/test/integration/public-api/credentials.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,37 @@ describe('PATCH /credentials/:id', () => {
isSharingLicensedSpy.mockRestore();
});

test('should allow sending isGlobal with same value when sharing is not licensed', async () => {
const savedCredential = await saveCredential(dbCredential(), { user: owner });

// Credential defaults to isGlobal=false, so sending isGlobal=false should succeed
const licenseState = Container.get(LicenseState);
const isSharingLicensedSpy = jest
.spyOn(licenseState, 'isSharingLicensed')
.mockReturnValue(false);

const updatePayload = {
name: 'Updated name',
isGlobal: false,
};

const response = await authOwnerAgent
.patch(`/credentials/${savedCredential.id}`)
.send(updatePayload);

expect(response.statusCode).toBe(200);
expect(response.body.name).toBe('Updated name');

// Verify credential was updated
const updatedCredential = await Container.get(CredentialsRepository).findOneByOrFail({
id: savedCredential.id,
});
expect(updatedCredential.name).toBe('Updated name');
expect(updatedCredential.isGlobal).toBeFalsy();

isSharingLicensedSpy.mockRestore();
});

test('should fail to update managed credentials', async () => {
const managedCredential = await saveCredential(
{ ...dbCredential(), isManaged: true },
Expand Down
Loading