diff --git a/server/config/passport.js b/server/config/passport.js index b4995cd4bf..3ae3dfbe0b 100644 --- a/server/config/passport.js +++ b/server/config/passport.js @@ -22,7 +22,11 @@ function generateUniqueUsername(username) { } passport.serializeUser((user, done) => { - done(null, user.id); + if (user) { + done(null, user.id); + } else { + done(new Error('User is not available for serialization.')); + } }); passport.deserializeUser((id, done) => { diff --git a/server/controllers/aws.controller.js b/server/controllers/aws.controller.js index a201e01213..0d0e2d9027 100644 --- a/server/controllers/aws.controller.js +++ b/server/controllers/aws.controller.js @@ -39,37 +39,40 @@ export function getObjectKey(url) { } return objectKey; } - +// TODO: callback should be removed export async function deleteObjectsFromS3(keyList, callback) { const objectsToDelete = keyList?.map((key) => ({ Key: key })); - if (objectsToDelete.length > 0) { - const params = { - Bucket: process.env.S3_BUCKET, - Delete: { Objects: objectsToDelete } - }; + if (!objectsToDelete.length) { + callback?.(); + return; + } - try { - await s3Client.send(new DeleteObjectsCommand(params)); - if (callback) { - callback(); - } - } catch (error) { - console.error('Error deleting objects from S3: ', error); - if (callback) { - callback(error); - } - } - } else if (callback) { - callback(); + const params = { + Bucket: process.env.S3_BUCKET, + Delete: { Objects: objectsToDelete } + }; + + try { + const deleteResult = await s3Client.send(new DeleteObjectsCommand(params)); + callback?.(null, deleteResult); + } catch (error) { + callback?.(error); } } export function deleteObjectFromS3(req, res) { const { objectKey, userId } = req.params; const fullObjectKey = userId ? `${userId}/${objectKey}` : objectKey; - deleteObjectsFromS3([fullObjectKey], () => { - res.json({ success: true }); + + deleteObjectsFromS3([fullObjectKey], (error, result) => { + if (error) { + return res + .status(500) + .json({ error: 'Failed to delete object from s3.' }); + } + + return res.json({ success: true, message: 'Object deleted successfully.' }); }); } diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js index ec03debab9..c1c28f9a03 100644 --- a/server/controllers/project.controller/__test__/deleteProject.test.js +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -3,10 +3,7 @@ */ import { Request, Response } from 'jest-express'; -import Project, { - createMock, - createInstanceMock -} from '../../../models/project'; +import Project from '../../../models/project'; import User from '../../../models/user'; import deleteProject from '../../project.controller/deleteProject'; import { deleteObjectsFromS3 } from '../../aws.controller'; @@ -14,101 +11,71 @@ import { deleteObjectsFromS3 } from '../../aws.controller'; jest.mock('../../../models/project'); jest.mock('../../aws.controller'); -describe('project.controller', () => { - describe('deleteProject()', () => { - let ProjectMock; - let ProjectInstanceMock; - - beforeEach(() => { - ProjectMock = createMock(); - ProjectInstanceMock = createInstanceMock(); - }); - - afterEach(() => { - ProjectMock.restore(); - ProjectInstanceMock.restore(); - }); +// TODO: incomplete test, 500 response status needs to be added - it('returns 403 if project is not owned by authenticated user', (done) => { - const user = new User(); - const project = new Project(); - project.user = user; +describe('project.controller', () => { + let request; + let response; - const request = new Request(); - request.setParams({ project_id: project._id }); - request.user = { _id: 'abc123' }; + beforeEach(() => { + request = new Request(); + response = new Response(); + Project.findById = jest.fn(); + }); - const response = new Response(); + afterEach(() => { + request.resetMocked(); + response.resetMocked(); + }); - ProjectMock.expects('findById').resolves(project); + it('returns 403 if project is not owned by authenticated user', async () => { + const user = new User(); + const project = new Project(); + project.user = user; - const promise = deleteProject(request, response); + request.setParams({ project_id: project._id }); + request.user = { _id: 'abc123' }; - function expectations() { - expect(response.status).toHaveBeenCalledWith(403); - expect(response.json).toHaveBeenCalledWith({ - message: 'Authenticated user does not match owner of project' - }); + Project.findById.mockResolvedValue(project); - done(); - } + await deleteProject(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.status).toHaveBeenCalledWith(403); + expect(response.json).toHaveBeenCalledWith({ + message: 'Authenticated user does not match owner of project' }); + }); - it('returns 404 if project does not exist', (done) => { - const user = new User(); - const project = new Project(); - project.user = user; - - const request = new Request(); - request.setParams({ project_id: project._id }); - request.user = { _id: 'abc123' }; - - const response = new Response(); - - ProjectMock.expects('findById').resolves(null); - - const promise = deleteProject(request, response); + it('returns 404 if project does not exist', async () => { + request.setParams({ project_id: 'random_id' }); + request.user = { _id: 'abc123' }; - function expectations() { - expect(response.status).toHaveBeenCalledWith(404); - expect(response.json).toHaveBeenCalledWith({ - message: 'Project with that id does not exist' - }); + Project.findById.mockResolvedValue(null); - done(); - } + await deleteProject(request, response); - promise.then(expectations, expectations).catch(expectations); + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ + message: 'Project with that id does not exist' }); + }); - it('deletes project and dependent files from S3 ', (done) => { - const user = new User(); - const project = new Project(); - project.user = user; - - const request = new Request(); - request.setParams({ project_id: project._id }); - request.user = { _id: user._id }; - - const response = new Response(); - - ProjectMock.expects('findById').resolves(project); - - ProjectInstanceMock.expects('remove').yields(); + it('delete project and dependent files from S3', async () => { + const user = new User(); + const project = new Project(); + project.user = user; + project.remove = jest.fn().mockResolvedValue(); - const promise = deleteProject(request, response); + request.setParams({ project_id: project._id }); + request.user = { _id: user._id }; - function expectations() { - expect(response.status).toHaveBeenCalledWith(200); - expect(response.json).not.toHaveBeenCalled(); - expect(deleteObjectsFromS3).toHaveBeenCalled(); + Project.findById.mockResolvedValue(project); + deleteObjectsFromS3.mockResolvedValue(); - done(); - } + await deleteProject(request, response); - promise.then(expectations, expectations).catch(expectations); - }); + expect(response.status).toHaveBeenCalledWith(200); + expect(deleteObjectsFromS3).toHaveBeenCalled(); + expect(project.remove).toHaveBeenCalled(); }); }); diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js index 672c7bf0c3..19a8166eae 100644 --- a/server/controllers/project.controller/deleteProject.js +++ b/server/controllers/project.controller/deleteProject.js @@ -7,45 +7,42 @@ const ProjectDeletionError = createApplicationErrorClass( 'ProjectDeletionError' ); -function deleteFilesFromS3(files) { - deleteObjectsFromS3( - files - .filter((file) => { - if ( - file.url && +async function deleteFilesFromS3(files) { + const filteredFiles = files + .filter((file) => { + const isValidFile = + (file.url && (file.url.includes(process.env.S3_BUCKET_URL_BASE) || - file.url.includes(process.env.S3_BUCKET)) - ) { - if ( - !process.env.S3_DATE || - (process.env.S3_DATE && - isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt))) - ) { - return true; - } - } - return false; - }) - .map((file) => getObjectKey(file.url)) - ); + file.url.includes(process.env.S3_BUCKET)) && + !process.env.S3_DATE) || + (process.env.S3_DATE && + isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt))); + + return isValidFile; + }) + .map((file) => getObjectKey(file.url)); + + try { + await deleteObjectsFromS3(filteredFiles); + } catch (error) { + console.error('Failed to delete files from S3:', error); + } } -export default function deleteProject(req, res) { - function sendFailure(error) { +export default async function deleteProject(req, res) { + const sendFailure = (error) => { res.status(error.code).json({ message: error.message }); - } + }; - function sendProjectNotFound() { - sendFailure( - new ProjectDeletionError('Project with that id does not exist', { - code: 404 - }) - ); - } + try { + const project = await Project.findById(req.params.project_id); - function handleProjectDeletion(project) { - if (project == null) { - sendProjectNotFound(); + if (!project) { + sendFailure( + new ProjectDeletionError('Project with that id does not exist', { + code: 404 + }) + ); return; } @@ -59,19 +56,20 @@ export default function deleteProject(req, res) { return; } - deleteFilesFromS3(project.files); - - project.remove((removeProjectError) => { - if (removeProjectError) { - sendProjectNotFound(); - return; - } + try { + await deleteFilesFromS3(project.files); + } catch (error) { + sendFailure( + new ProjectDeletionError('Failed to delete associated project files.', { + code: 500 + }) + ); + return; + } - res.status(200).end(); - }); + await project.remove(); + res.status(200).end(); + } catch (error) { + sendFailure(error); } - - return Project.findById(req.params.project_id) - .then(handleProjectDeletion) - .catch(sendFailure); } diff --git a/server/models/user.js b/server/models/user.js index 4c67df2505..1c4704ec5e 100644 --- a/server/models/user.js +++ b/server/models/user.js @@ -163,7 +163,6 @@ userSchema.methods.comparePassword = async function comparePassword( candidatePassword ) { if (!this.password) { - console.error('No password is set for this user.'); return false; } diff --git a/server/routes/passport.routes.js b/server/routes/passport.routes.js index da9daaecc1..2bc639239c 100644 --- a/server/routes/passport.routes.js +++ b/server/routes/passport.routes.js @@ -3,50 +3,33 @@ import passport from 'passport'; const router = new Router(); -router.get('/auth/github', passport.authenticate('github')); -router.get('/auth/github/callback', (req, res, next) => { - passport.authenticate( - 'github', - { failureRedirect: '/login' }, - (err, user) => { - if (err) { - // use query string param to show error; - res.redirect('/account?error=github'); - return; - } +const authenticateOAuth = (service) => (req, res, next) => { + passport.authenticate(service, { failureRedirect: '/login' }, (err, user) => { + if (err) { + // use query string param to show error; + res.redirect(`/account?error=${service}`); + return; + } - req.logIn(user, (loginErr) => { - if (loginErr) { - next(loginErr); - return; - } - res.redirect('/'); - }); + if (!user) { + res.redirect(`/account?error=${service}NoUser`); + return; } - )(req, res, next); -}); -router.get('/auth/google', passport.authenticate('google')); -router.get('/auth/google/callback', (req, res, next) => { - passport.authenticate( - 'google', - { failureRedirect: '/login' }, - (err, user) => { - if (err) { - // use query string param to show error; - res.redirect('/account?error=google'); + req.logIn(user, (loginErr) => { + if (loginErr) { + next(loginErr); return; } + res.redirect('/'); + }); + })(req, res, next); +}; - req.logIn(user, (loginErr) => { - if (loginErr) { - next(loginErr); - return; - } - res.redirect('/'); - }); - } - )(req, res, next); -}); +router.get('/auth/github', passport.authenticate('github')); +router.get('/auth/github/callback', authenticateOAuth('github')); + +router.get('/auth/google', passport.authenticate('google')); +router.get('/auth/google/callback', authenticateOAuth('google')); export default router;