Lectures: Fix file names for downloads with chromium browsers (#9899)

src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java

@@ -1,6 +1,8 @@
 package de.tum.cit.aet.artemis.core.web;
 
 import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_CORE;
+import static org.apache.velocity.shaded.commons.io.FilenameUtils.getBaseName;
+import static org.apache.velocity.shaded.commons.io.FilenameUtils.getExtension;
 
 import java.io.IOException;
 import java.net.FileNameMap;
@@ -409,20 +411,18 @@ public ResponseEntity<byte[]> getExamUserImage(@PathVariable Long examUserId) {
     /**
      * GET /files/attachments/lecture/:lectureId/:filename : Get the lecture attachment
      *
-     * @param lectureId ID of the lecture, the attachment belongs to
-     * @param filename  the filename of the file
+     * @param lectureId      ID of the lecture, the attachment belongs to
+     * @param attachmentName the filename of the file
      * @return The requested file, 403 if the logged-in user is not allowed to access it, or 404 if the file doesn't exist
      */
-    @GetMapping("files/attachments/lecture/{lectureId}/{filename}")
+    @GetMapping("files/attachments/lecture/{lectureId}/{attachmentName}")
     @EnforceAtLeastStudent
-    public ResponseEntity<byte[]> getLectureAttachment(@PathVariable Long lectureId, @PathVariable String filename) {
-        log.debug("REST request to get lecture attachment : {}", filename);
-        String fileNameWithoutSpaces = filename.replaceAll(" ", "_");
-        sanitizeFilenameElseThrow(fileNameWithoutSpaces);
+    public ResponseEntity<byte[]> getLectureAttachment(@PathVariable Long lectureId, @PathVariable String attachmentName) {
+        log.debug("REST request to get lecture attachment : {}", attachmentName);
 
         List<Attachment> lectureAttachments = attachmentRepository.findAllByLectureId(lectureId);
-        Attachment attachment = lectureAttachments.stream().filter(lectureAttachment -> lectureAttachment.getLink().endsWith(fileNameWithoutSpaces)).findAny()
-                .orElseThrow(() -> new EntityNotFoundException("Attachment", filename));
+        Attachment attachment = lectureAttachments.stream().filter(lectureAttachment -> lectureAttachment.getName().equals(getBaseName(attachmentName))).findAny()
+                .orElseThrow(() -> new EntityNotFoundException("Attachment", attachmentName));
 
         // get the course for a lecture attachment
         Lecture lecture = attachment.getLecture();
@@ -431,7 +431,7 @@ public ResponseEntity<byte[]> getLectureAttachment(@PathVariable Long lectureId,
         // check if the user is authorized to access the requested attachment unit
         checkAttachmentAuthorizationOrThrow(course, attachment);
 
-        return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachment.getName()));
+        return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachmentName));
     }
 
     /**
@@ -487,7 +487,7 @@ public ResponseEntity<byte[]> getAttachmentUnitAttachment(@PathVariable Long att
 
         // check if the user is authorized to access the requested attachment unit
         checkAttachmentAuthorizationOrThrow(course, attachment);
-        return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachment.getName()));
+        return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachment.getName() + "." + getExtension(attachment.getLink())));
     }
 
     /**

src/main/webapp/app/lecture/lecture-attachments.component.html

@@ -58,7 +58,7 @@ <h4 jhiTranslate="artemisApp.lecture.attachments.attachments"></h4>
                                                 </td>
                                                 <td>
                                                     @if (!isDownloadingAttachmentLink) {
-                                                        <a class="text-primary" (click)="downloadAttachment(attachment.link || '')">
+                                                        <a class="text-primary" (click)="downloadAttachment(attachment.name || '', attachment.link || '')">
                                                             {{ attachment.name }}
                                                         </a>
                                                     } @else if (isDownloadingAttachmentLink === attachment.link) {

src/main/webapp/app/lecture/lecture-attachments.component.ts

@@ -254,10 +254,10 @@ export class LectureAttachmentsComponent implements OnDestroy {
         return item.id;
     }
 
-    downloadAttachment(downloadUrl: string): void {
+    downloadAttachment(downloadName: string, downloadUrl: string): void {
         if (!this.isDownloadingAttachmentLink) {
             this.isDownloadingAttachmentLink = downloadUrl;
-            this.fileService.downloadFile(this.fileService.replaceLectureAttachmentPrefixAndUnderscores(downloadUrl));
+            this.fileService.downloadFileByAttachmentName(downloadUrl, downloadName);
             this.isDownloadingAttachmentLink = undefined;
         }
     }

src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts

@@ -51,7 +51,7 @@ export class AttachmentUnitComponent extends LectureUnitDirective<AttachmentUnit
 
         if (this.lectureUnit().attachment?.link) {
             const link = this.lectureUnit().attachment!.link!;
-            this.fileService.downloadFile(this.fileService.replaceAttachmentPrefixAndUnderscores(link));
+            this.fileService.downloadFileByAttachmentName(link, this.lectureUnit().attachment!.name!);
             this.onCompletion.emit({ lectureUnit: this.lectureUnit(), completed: true });
         }
     }

src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html

@@ -112,7 +112,7 @@ <h3 jhiTranslate="artemisApp.courseOverview.lectureDetails.attachments"></h3>
                                     <li class="mb-3">
                                         <h5 class="mb-1">
                                             @if (!isDownloadingLink) {
-                                                <a class="text-primary" (click)="downloadAttachment(attachment.link)">
+                                                <a class="text-primary" (click)="downloadAttachment(attachment.link, attachment.name)">
                                                     {{ attachment.name }}
                                                 </a>
                                             }

src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts

@@ -128,10 +128,10 @@ export class CourseLectureDetailsComponent extends AbstractScienceComponent impl
         return attachment.link.split('.').pop()!;
     }
 
-    downloadAttachment(downloadUrl?: string): void {
-        if (!this.isDownloadingLink && downloadUrl) {
+    downloadAttachment(downloadUrl?: string, downloadName?: string): void {
+        if (!this.isDownloadingLink && downloadUrl && downloadName) {
             this.isDownloadingLink = downloadUrl;
-            this.fileService.downloadFile(this.fileService.replaceLectureAttachmentPrefixAndUnderscores(downloadUrl));
+            this.fileService.downloadFileByAttachmentName(downloadUrl, downloadName);
             this.isDownloadingLink = undefined;
         }
     }

src/main/webapp/app/shared/http/file.service.ts

@@ -82,6 +82,23 @@ export class FileService {
         return newWindow;
     }
 
+    /**
+     * Downloads the file from the provided downloadUrl and the attachment name
+     *
+     * @param downloadUrl url that is stored in the attachment model
+     * @param downloadName the name given to the attachment
+     */
+    downloadFileByAttachmentName(downloadUrl: string, downloadName: string) {
+        const downloadUrlComponents = downloadUrl.split('/');
+        // take the last element
+        const extension = downloadUrlComponents.pop()!.split('.').pop();
+        const restOfUrl = downloadUrlComponents.join('/');
+        const normalizedDownloadUrl = restOfUrl + '/' + encodeURIComponent(downloadName + '.' + extension);
+        const newWindow = window.open('about:blank');
+        newWindow!.location.href = normalizedDownloadUrl;
+        return newWindow;
+    }
+
     /**
      * Downloads the merged PDF file.
      *
@@ -124,12 +141,4 @@ export class FileService {
     replaceAttachmentPrefixAndUnderscores(link: string): string {
         return link.replace(/AttachmentUnit_\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}_/, '').replace(/_/g, ' ');
     }
-
-    /**
-     * Removes the prefix from the file name, and replaces underscore with spaces
-     * @param link
-     */
-    replaceLectureAttachmentPrefixAndUnderscores(link: string): string {
-        return link.replace(/LectureAttachment_\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}_/, '').replace(/_/g, ' ');
-    }
 }

src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java

@@ -1,5 +1,6 @@
 package de.tum.cit.aet.artemis.lecture;
 
+import static org.apache.velocity.shaded.commons.io.FilenameUtils.getExtension;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.verify;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
@@ -69,8 +70,11 @@ void initTestCase() {
     void createAttachment() throws Exception {
         Attachment actualAttachment = request.postWithMultipartFile("/api/attachments", attachment, "attachment",
                 new MockMultipartFile("file", "test.txt", MediaType.TEXT_PLAIN_VALUE, "testContent".getBytes()), Attachment.class, HttpStatus.CREATED);
-        assertThat(actualAttachment.getLink()).isNotNull();
-        MvcResult file = request.performMvcRequest(get(actualAttachment.getLink())).andExpect(status().isOk()).andExpect(content().contentType(MediaType.TEXT_PLAIN_VALUE))
+        String actualLink = actualAttachment.getLink();
+        assertThat(actualLink).isNotNull();
+        // getLectureAttachment uses the provided file name to fetch the attachment which has that attachment name (not filename)
+        String linkWithCorrectFileName = actualLink.substring(0, actualLink.lastIndexOf('/') + 1) + attachment.getName() + "." + getExtension(actualAttachment.getLink());
+        MvcResult file = request.performMvcRequest(get(linkWithCorrectFileName)).andExpect(status().isOk()).andExpect(content().contentType(MediaType.TEXT_PLAIN_VALUE))
                 .andReturn();
         assertThat(file.getResponse().getContentAsByteArray()).isNotEmpty();
         var expectedAttachment = attachmentRepository.findById(actualAttachment.getId()).orElseThrow();

src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts

@@ -82,7 +82,7 @@ describe('AttachmentUnitComponent', () => {
     });
 
     it('should handle download', () => {
-        const downloadFileSpy = jest.spyOn(fileService, 'downloadFile');
+        const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName');
         const onCompletionEmitSpy = jest.spyOn(component.onCompletion, 'emit');
 
         fixture.detectChanges();
@@ -113,7 +113,7 @@ describe('AttachmentUnitComponent', () => {
     });
 
     it('should download attachment when clicked', () => {
-        const downloadFileSpy = jest.spyOn(fileService, 'downloadFile');
+        const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName');
 
         fixture.detectChanges();
 

src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts

@@ -377,7 +377,7 @@ describe('LectureAttachmentsComponent', () => {
         fixture.detectChanges();
         comp.isDownloadingAttachmentLink = undefined;
         expect(comp.isDownloadingAttachmentLink).toBeUndefined();
-        comp.downloadAttachment('https://my/own/download/url');
+        comp.downloadAttachment('https://my/own/download/url', 'test');
         expect(comp.isDownloadingAttachmentLink).toBeUndefined();
     }));
 

src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts

@@ -279,13 +279,13 @@ describe('CourseLectureDetailsComponent', () => {
 
     it('should download file for attachment', fakeAsync(() => {
         const fileService = TestBed.inject(FileService);
-        const downloadFileSpy = jest.spyOn(fileService, 'downloadFile');
+        const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName');
         const attachment = getAttachmentUnit(lecture, 1, dayjs()).attachment!;
 
-        courseLecturesDetailsComponent.downloadAttachment(attachment.link);
+        courseLecturesDetailsComponent.downloadAttachment(attachment.link, attachment.name);
 
         expect(downloadFileSpy).toHaveBeenCalledOnce();
-        expect(downloadFileSpy).toHaveBeenCalledWith(attachment.link);
+        expect(downloadFileSpy).toHaveBeenCalledWith(attachment.link, attachment.name);
         expect(courseLecturesDetailsComponent.isDownloadingLink).toBeUndefined();
     }));
 

src/test/javascript/spec/component/shared/http/file.service.spec.ts

@@ -3,6 +3,7 @@ import { HttpTestingController, provideHttpClientTesting } from '@angular/common
 import { TestBed } from '@angular/core/testing';
 import { v4 as uuid } from 'uuid';
 import { provideHttpClient } from '@angular/common/http';
+import { ProgrammingLanguage, ProjectType } from 'app/entities/programming/programming-exercise.model';
 
 jest.mock('uuid', () => ({
     v4: jest.fn(),
@@ -77,4 +78,157 @@ describe('FileService', () => {
             expect(v4Mock).toHaveBeenCalledTimes(3);
         });
     });
+
+    describe('getTemplateFile', () => {
+        it('should fetch the template file without project type', () => {
+            const language = ProgrammingLanguage.JAVA;
+            const expectedUrl = `api/files/templates/JAVA`;
+            const response = 'template content';
+
+            fileService.getTemplateFile(language).subscribe((data) => {
+                expect(data).toEqual(response);
+            });
+
+            const req = httpMock.expectOne({
+                url: expectedUrl,
+                method: 'GET',
+            });
+            expect(req.request.responseType).toBe('text');
+            req.flush(response);
+        });
+
+        it('should fetch the template file with project type', () => {
+            const language = ProgrammingLanguage.JAVA;
+            const projectType = ProjectType.PLAIN_MAVEN;
+            const expectedUrl = `api/files/templates/JAVA/PLAIN_MAVEN`;
+            const response = 'template content';
+
+            fileService.getTemplateFile(language, projectType).subscribe((data) => {
+                expect(data).toEqual(response);
+            });
+
+            const req = httpMock.expectOne({
+                url: expectedUrl,
+                method: 'GET',
+            });
+            expect(req.request.responseType).toBe('text');
+            req.flush(response);
+        });
+    });
+
+    describe('downloadMergedFile', () => {
+        it('should download the merged PDF file', () => {
+            const lectureId = 123;
+            const expectedUrl = `api/files/attachments/lecture/${lectureId}/merge-pdf`;
+            const blobResponse = new Blob(['PDF content'], { type: 'application/pdf' });
+
+            fileService.downloadMergedFile(lectureId).subscribe((response) => {
+                expect(response.body).toEqual(blobResponse);
+                expect(response.status).toBe(200);
+            });
+
+            const req = httpMock.expectOne({
+                url: expectedUrl,
+                method: 'GET',
+            });
+            expect(req.request.responseType).toBe('blob');
+            req.flush(blobResponse, { status: 200, statusText: 'OK' });
+        });
+    });
+
+    describe('getAeolusTemplateFile', () => {
+        it('should fetch the aeolus template file with all parameters', () => {
+            const language = ProgrammingLanguage.PYTHON;
+            const projectType = ProjectType.PLAIN;
+            const staticAnalysis = true;
+            const sequentialRuns = false;
+            const coverage = true;
+            const expectedUrl = `api/files/aeolus/templates/PYTHON/PLAIN?staticAnalysis=true&sequentialRuns=false&testCoverage=true`;
+            const response = 'aeolus template content';
+
+            fileService.getAeolusTemplateFile(language, projectType, staticAnalysis, sequentialRuns, coverage).subscribe((data) => {
+                expect(data).toEqual(response);
+            });
+
+            const req = httpMock.expectOne({
+                url: expectedUrl,
+                method: 'GET',
+            });
+            expect(req.request.responseType).toBe('text');
+            req.flush(response);
+        });
+
+        it('should fetch the aeolus template file with missing optional parameters', () => {
+            const expectedUrl = `api/files/aeolus/templates/PYTHON?staticAnalysis=false&sequentialRuns=false&testCoverage=false`;
+            const response = 'aeolus template content';
+
+            fileService.getAeolusTemplateFile(ProgrammingLanguage.PYTHON).subscribe((data) => {
+                expect(data).toEqual(response);
+            });
+
+            const req = httpMock.expectOne({
+                url: expectedUrl,
+                method: 'GET',
+            });
+            expect(req.request.responseType).toBe('text');
+            req.flush(response);
+        });
+    });
+
+    describe('getTemplateCodeOfConduct', () => {
+        it('should fetch the template code of conduct', () => {
+            const expectedUrl = `api/files/templates/code-of-conduct`;
+            const response = 'code of conduct content';
+
+            fileService.getTemplateCodeOfCondcut().subscribe((data) => {
+                expect(data.body).toEqual(response);
+            });
+
+            const req = httpMock.expectOne({
+                url: expectedUrl,
+                method: 'GET',
+            });
+            expect(req.request.responseType).toBe('text');
+            req.flush(response);
+        });
+    });
+
+    describe('downloadFile', () => {
+        it('should open a new window with the normalized URL', () => {
+            const downloadUrl = 'http://example.com/files/some file name.txt';
+            const encodedUrl = 'http://example.com/files/some%20file%20name.txt';
+            const newWindowMock = { location: { href: '' } } as Window;
+
+            jest.spyOn(window, 'open').mockReturnValue(newWindowMock);
+
+            const newWindow = fileService.downloadFile(downloadUrl);
+            expect(newWindow).not.toBeNull();
+            expect(newWindow!.location.href).toBe(encodedUrl);
+        });
+    });
+
+    describe('downloadFileByAttachmentName', () => {
+        it('should open a new window with the normalized URL and attachment name', () => {
+            const downloadUrl = 'http://example.com/files/attachment.txt';
+            const downloadName = 'newAttachment';
+            const encodedUrl = 'http://example.com/files/newAttachment.txt';
+            const newWindowMock = { location: { href: '' } } as Window;
+
+            jest.spyOn(window, 'open').mockReturnValue(newWindowMock);
+
+            const newWindow = fileService.downloadFileByAttachmentName(downloadUrl, downloadName);
+            expect(newWindow).not.toBeNull();
+            expect(newWindow!.location.href).toBe(encodedUrl);
+        });
+    });
+
+    describe('replaceAttachmentPrefixAndUnderscores', () => {
+        it('should replace the prefix and underscores in a file name', () => {
+            const fileName = 'AttachmentUnit_2023-01-01T00-00-00-000_some_file_name';
+            const expected = 'some file name';
+
+            const result = fileService.replaceAttachmentPrefixAndUnderscores(fileName);
+            expect(result).toBe(expected);
+        });
+    });
 });

src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts

@@ -8,6 +8,9 @@ export class MockFileService {
     downloadFile = () => {
         return { subscribe: (fn: (value: any) => void) => fn({ body: new Window() }) };
     };
+    downloadFileByAttachmentName = () => {
+        return { subscribe: (fn: (value: any) => void) => fn({ body: new Window() }) };
+    };
 
     getTemplateFile = () => {
         return of();