Use v2 permission checker for files permissions #8689
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the file download routes to use the v2 DownloadFileController implementation, consolidating permission checking logic and removing duplicate code. The changes streamline file access endpoints while maintaining backward compatibility through deprecated routes.
- Replaced multiple individual route handlers with the DownloadFileController.customHandler pattern
- Removed redundant validation and storage logic from routes.ts by leveraging the v2 controller
- Cleaned up code formatting in DownloadFileController by removing unnecessary await and parentheses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/api/files/routes.ts | Refactored file download routes to use DownloadFileController, removed unused imports, consolidated deprecated redirect routes with prettier-ignore formatting |
| app/api/core/infrastructure/express/DownloadFileController.ts | Simplified stream creation by removing unnecessary await and parentheses when calling getFile().read() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/api/files/routes.ts
Outdated
| //prettier-ignore | ||
| app.use('/uploaded_documents/:fileName', (req, res) => { res.redirect(301, `/api/files/${req.params.fileName}`); }); | ||
| //prettier-ignore | ||
| app.get('/api/attachments/download', async (req, res) => { res.redirect(301, `/api/files/${req.query.file}?download=true`); }); |
There was a problem hiding this comment.
The redirect route for '/api/attachments/download' no longer validates the required 'file' query parameter. The original implementation had validation ensuring 'file' was a required string property. Now, if req.query.file is undefined, this will redirect to '/api/files/undefined?download=true', which could result in unexpected behavior. Consider adding validation or a check to ensure req.query.file exists before redirecting.
| app.get('/api/attachments/download', async (req, res) => { res.redirect(301, `/api/files/${req.query.file}?download=true`); }); | |
| app.get( | |
| '/api/attachments/download', | |
| validation.validateRequest({ | |
| type: 'object', | |
| properties: { | |
| query: { | |
| type: 'object', | |
| required: ['file'], | |
| properties: { | |
| file: { type: 'string' }, | |
| }, | |
| }, | |
| }, | |
| }), | |
| async (req, res) => { | |
| res.redirect(301, `/api/files/${req.query.file}?download=true`); | |
| } | |
| ); |
| app.use('/uploaded_documents/:fileName', (req, res) => { res.redirect(301, `/api/files/${req.params.fileName}`); }); | ||
| //prettier-ignore | ||
| app.get('/api/attachments/download', async (req, res) => { res.redirect(301, `/api/files/${req.query.file}?download=true`); }); | ||
| // Deprecated routes, keeping for Backwards compatibility |
There was a problem hiding this comment.
Duplicate comment "Deprecated routes, keeping for Backwards compatibility" appears on both line 282 and line 289. Remove one of these duplicate comments to improve code clarity.
| // Deprecated routes, keeping for Backwards compatibility |
No description provided.