Skip to content
Open
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
6 changes: 6 additions & 0 deletions .changeset/neat-planets-hope.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@rocket.chat/rest-typings': minor
'@rocket.chat/meteor': minor
---

Adds custom-sounds.delete API endpoint.
39 changes: 39 additions & 0 deletions apps/meteor/app/api/server/v1/custom-sounds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
isCustomSoundsGetOneProps,
isCustomSoundsListProps,
isCustomSoundsCreateProps,
isCustomSoundsDeleteProps,
isCustomSoundsUpdateProps,
ajv,
validateBadRequestErrorResponse,
Expand All @@ -16,6 +17,7 @@ import { escapeRegExp } from '@rocket.chat/string-helpers';

import { MAX_CUSTOM_SOUND_SIZE_BYTES, CUSTOM_SOUND_ALLOWED_MIME_TYPES } from '../../../../lib/constants';
import { SystemLogger } from '../../../../server/lib/logger/system';
import { deleteCustomSound } from '../../../custom-sounds/server/lib/deleteCustomSound';
import { insertOrUpdateSound } from '../../../custom-sounds/server/lib/insertOrUpdateSound';
import { uploadCustomSound } from '../../../custom-sounds/server/lib/uploadCustomSound';
import { getExtension, getMimeTypeFromFileName } from '../../../utils/lib/mimeTypes';
Expand Down Expand Up @@ -280,6 +282,43 @@ const customSoundsEndpoints = API.v1
return API.v1.failure(error instanceof Error ? error.message : 'Unknown error');
}
},
)
.post(
'custom-sounds.delete',
{
response: {
200: ajv.compile<{ success: boolean }>({
additionalProperties: false,
type: 'object',
properties: {
success: {
type: 'boolean',
description: 'Indicates if the request was successful.',
},
},
required: ['success'],
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
404: validateNotFoundErrorResponse,
},
authRequired: true,
body: isCustomSoundsDeleteProps,
permissionsRequired: ['manage-sounds'],
},
async function action() {
const { _id } = this.bodyParams;

try {
await deleteCustomSound(_id);

return API.v1.success({});
} catch (error: any) {
SystemLogger.error({ error });
return API.v1.failure(error instanceof Error ? error.message : 'Unknown error');
Comment on lines +313 to +319
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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve 5xx for unexpected delete failures.

This catch turns every exception from deleteCustomSound(_id) into a client-error response, so file-store or database failures during deletion look like request problems even when the input is valid. Please only translate expected domain errors here and rethrow/return a 5xx for unexpected failures.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/app/api/server/v1/custom-sounds.ts` around lines 313 - 319, The
catch currently maps all exceptions from deleteCustomSound(_id) to a client 4xx
failure; change it to only translate expected domain errors (e.g., not-found or
validation errors) into API.v1.failure with their message, and for any other
unexpected errors log them via SystemLogger.error({ error }) and return or
rethrow a 5xx response (e.g., API.v1.failure('Internal Server Error', 500) or
rethrow the error) so infrastructure/file-store/db failures produce a 5xx;
adjust the catch to inspect the error type/code (or instanceof specific domain
error classes) before deciding between API.v1.failure(error.message) and
API.v1.failure('Internal Server Error', 500), keeping references to
deleteCustomSound, SystemLogger.error, and API.v1.failure.

}
},
);

export type CustomSoundEndpoints = ExtractRoutesFromAPI<typeof customSoundsEndpoints>;
Expand Down
20 changes: 20 additions & 0 deletions apps/meteor/app/custom-sounds/server/lib/deleteCustomSound.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { api } from '@rocket.chat/core-services';
import { CustomSounds } from '@rocket.chat/models';
import { Meteor } from 'meteor/meteor';

import { RocketChatFileCustomSoundsInstance } from '../startup/custom-sounds';

export const deleteCustomSound = async (_id: string): Promise<void> => {
const sound = await CustomSounds.findOneById(_id);

if (!sound) {
throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', {
method: 'deleteCustomSound',
});
}

await RocketChatFileCustomSoundsInstance.deleteFile(`${sound._id}.${sound.extension}`);
await CustomSounds.removeById(_id);

void api.broadcast('notify.deleteCustomSound', { soundData: sound });
};
26 changes: 7 additions & 19 deletions apps/meteor/app/custom-sounds/server/methods/deleteCustomSound.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { api } from '@rocket.chat/core-services';
import type { ICustomSound } from '@rocket.chat/core-typings';
import type { ServerMethods } from '@rocket.chat/ddp-client';
import { CustomSounds } from '@rocket.chat/models';
import { check } from 'meteor/check';
import { Meteor } from 'meteor/meteor';

import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';
import { RocketChatFileCustomSoundsInstance } from '../startup/custom-sounds';
import { methodDeprecationLogger } from '../../../lib/server/lib/deprecationWarningLogger';
import { deleteCustomSound } from '../lib/deleteCustomSound';

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
Expand All @@ -16,24 +16,12 @@ declare module '@rocket.chat/ddp-client' {

Meteor.methods<ServerMethods>({
async deleteCustomSound(_id) {
let sound = null;

if (this.userId && (await hasPermissionAsync(this.userId, 'manage-sounds'))) {
sound = await CustomSounds.findOneById(_id);
} else {
methodDeprecationLogger.method('deleteCustomSound', '9.0.0', '/v1/custom-sounds.delete');
if (!this.userId || !(await hasPermissionAsync(this.userId, 'manage-sounds'))) {
throw new Meteor.Error('not_authorized');
}

if (sound == null) {
throw new Meteor.Error('Custom_Sound_Error_Invalid_Sound', 'Invalid sound', {
method: 'deleteCustomSound',
});
}

await RocketChatFileCustomSoundsInstance.deleteFile(`${sound._id}.${sound.extension}`);
await CustomSounds.removeById(_id);
void api.broadcast('notify.deleteCustomSound', { soundData: sound });

check(_id, String);
await deleteCustomSound(_id);
return true;
},
});
8 changes: 4 additions & 4 deletions apps/meteor/client/views/admin/customSounds/EditSound.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Box, Button, ButtonGroup, Margins, TextInput, Field, FieldLabel, FieldRow, IconButton } from '@rocket.chat/fuselage';
import { GenericModal, ContextualbarScrollableContent, ContextualbarFooter } from '@rocket.chat/ui-client';
import { useSetModal, useToastMessageDispatch, useMethod } from '@rocket.chat/ui-contexts';
import { useSetModal, useToastMessageDispatch, useEndpoint } from '@rocket.chat/ui-contexts';
import fileSize from 'filesize';
import type { ReactElement, SyntheticEvent } from 'react';
import { useCallback, useState, useMemo, useEffect } from 'react';
Expand Down Expand Up @@ -36,7 +36,7 @@ function EditSound({ close, onChange, data, ...props }: EditSoundProps): ReactEl
setFile(undefined);
}, [_id, previousName]);

const deleteCustomSound = useMethod('deleteCustomSound');
const deleteCustomSoundEndpoint = useEndpoint('POST', '/v1/custom-sounds.delete');

const { mutate: saveAction } = useEndpointUploadMutation('/v1/custom-sounds.update', {
onSuccess: () => {
Expand Down Expand Up @@ -76,7 +76,7 @@ function EditSound({ close, onChange, data, ...props }: EditSoundProps): ReactEl
const handleDeleteButtonClick = useCallback(() => {
const handleDelete = async (): Promise<void> => {
try {
await deleteCustomSound(_id);
await deleteCustomSoundEndpoint({ _id });
dispatchToastMessage({ type: 'success', message: t('Custom_Sound_Has_Been_Deleted') });
} catch (error) {
dispatchToastMessage({ type: 'error', message: error });
Expand All @@ -94,7 +94,7 @@ function EditSound({ close, onChange, data, ...props }: EditSoundProps): ReactEl
{t('Custom_Sound_Delete_Warning')}
</GenericModal>,
);
}, [_id, close, deleteCustomSound, dispatchToastMessage, onChange, setModal, t]);
}, [_id, close, deleteCustomSoundEndpoint, dispatchToastMessage, onChange, setModal, t]);

const [clickUpload] = useSingleFileInput(
handleChangeFile,
Expand Down
85 changes: 73 additions & 12 deletions apps/meteor/tests/end-to-end/api/custom-sounds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,7 @@ async function createCustomSound(fileName: string, filePath: string): Promise<st
}

async function deleteCustomSound(_id: string) {
await request
.post(api('method.call/deleteCustomSound'))
.set(credentials)
.send({
message: JSON.stringify({
msg: 'method',
id: '1',
method: 'deleteCustomSound',
params: [_id],
}),
})
.expect(200);
await request.post(api('custom-sounds.delete')).set(credentials).send({ _id }).expect(200);
}

describe('[CustomSounds]', () => {
Expand Down Expand Up @@ -415,6 +404,78 @@ describe('[CustomSounds]', () => {
});
});

describe('[/custom-sounds.delete]', () => {
let soundToDeleteId: string;
let soundDeleted: boolean = false;

before(async () => {
soundToDeleteId = await createCustomSound(`sound-to-delete-${randomUUID()}`, mockWavAudioPath);
});

after(async () => {
if (soundToDeleteId && !soundDeleted) {
await deleteCustomSound(soundToDeleteId);
}
});

it('should return unauthorized if the user is not authenticated', async () => {
await request.post(api('custom-sounds.delete')).send({ _id: soundToDeleteId }).expect(401);
});

it('should return a 400 if attempting to delete a sound that does not exist', async () => {
await request
.post(api('custom-sounds.delete'))
.set(credentials)
.send({ _id: 'invalid-non-existent-id' })
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.equal('Invalid sound [Custom_Sound_Error_Invalid_Sound]');
});
});

it('should reject requests with invalid parameter types', async () => {
await request
.post(api('custom-sounds.delete'))
.set(credentials)
.send({ _id: { $ne: null } })
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
});
});

describe('without manage-sounds permission', async () => {
let unauthorizedUser: IUser;
let unauthorizedUserCredentials: Credentials;

before(async () => {
unauthorizedUser = await createUser();
unauthorizedUserCredentials = await login(unauthorizedUser.username, password);
});

after(async () => {
await deleteUser(unauthorizedUser);
});

it('should return forbidden if user does not have the manage-sounds permission', async () => {
await request.post(api('custom-sounds.delete')).set(unauthorizedUserCredentials).send({ _id: soundToDeleteId }).expect(403);
});
});

it('should successfully delete a custom sound when providing a valid _id', async () => {
await request
.post(api('custom-sounds.delete'))
.set(credentials)
.send({ _id: soundToDeleteId })
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
});
soundDeleted = true;
});
});

describe('Accessing custom sounds', () => {
it('should return forbidden if the there is no fileId on the url', (done) => {
void request
Expand Down
16 changes: 16 additions & 0 deletions packages/rest-typings/src/v1/customSounds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,19 @@ const CustomSoundsUpdateSchema = {
};

export const isCustomSoundsUpdateProps = ajv.compile<CustomSoundsUpdate>(CustomSoundsUpdateSchema);

type CustomSoundsDelete = { _id: ICustomSound['_id'] };

const CustomSoundsDeleteSchema = {
type: 'object',
properties: {
_id: {
type: 'string',
minLength: 1,
},
},
required: ['_id'],
additionalProperties: false,
};

export const isCustomSoundsDeleteProps = ajv.compile<CustomSoundsDelete>(CustomSoundsDeleteSchema);
Loading