Merge pull request #51152 from nextcloud/fix/files-sharing-download

fix(files_sharing): ensure downloaded file has the correct filename

apps/files/src/actions/downloadAction.spec.ts

@@ -105,7 +105,7 @@ describe('Download action execute tests', () => {
 
 		// Silent action
 		expect(exec).toBe(null)
-		expect(link.download).toEqual('')
+		expect(link.download).toBe('foobar.txt')
 		expect(link.href).toEqual('https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt')
 		expect(link.click).toHaveBeenCalledTimes(1)
 	})
@@ -123,7 +123,26 @@ describe('Download action execute tests', () => {
 
 		// Silent action
 		expect(exec).toStrictEqual([null])
-		expect(link.download).toEqual('')
+		expect(link.download).toEqual('foobar.txt')
+		expect(link.href).toEqual('https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt')
+		expect(link.click).toHaveBeenCalledTimes(1)
+	})
+
+	test('Download single file with displayname set', async () => {
+		const file = new File({
+			id: 1,
+			source: 'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',
+			owner: 'admin',
+			mime: 'text/plain',
+			displayname: 'baz.txt',
+			permissions: Permission.READ,
+		})
+
+		const exec = await action.execBatch!([file], view, '/')
+
+		// Silent action
+		expect(exec).toStrictEqual([null])
+		expect(link.download).toEqual('baz.txt')
 		expect(link.href).toEqual('https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt')
 		expect(link.click).toHaveBeenCalledTimes(1)
 	})

apps/files/src/actions/downloadAction.ts

@@ -9,9 +9,15 @@ import { isDownloadable } from '../utils/permissions'
 
 import ArrowDownSvg from '@mdi/svg/svg/arrow-down.svg?raw'
 
-const triggerDownload = function(url: string) {
+/**
+ * Trigger downloading a file.
+ *
+ * @param url The url of the asset to download
+ * @param name Optionally the recommended name of the download (browsers might ignore it)
+ */
+function triggerDownload(url: string, name?: string) {
 	const hiddenElement = document.createElement('a')
-	hiddenElement.download = ''
+	hiddenElement.download = name ?? ''
 	hiddenElement.href = url
 	hiddenElement.click()
 }
@@ -43,7 +49,7 @@ const downloadNodes = function(nodes: Node[]) {
 
 	if (nodes.length === 1) {
 		if (nodes[0].type === FileType.File) {
-			return triggerDownload(nodes[0].encodedSource)
+			return triggerDownload(nodes[0].encodedSource, nodes[0].displayname)
 		} else {
 			url = new URL(nodes[0].encodedSource)
 			url.searchParams.append('accept', 'zip')

cypress/e2e/files_sharing/public-share/download-files.cy.ts

@@ -4,136 +4,170 @@
  */
 // @ts-expect-error The package is currently broken - but works...
 import { deleteDownloadsFolderBeforeEach } from 'cypress-delete-downloads-folder'
-
+import { createShare, getShareUrl, setupPublicShare, type ShareContext } from './setup-public-share.ts'
+import { getRowForFile, getRowForFileId, triggerActionForFile, triggerActionForFileId } from '../../files/FilesUtils.ts'
 import { zipFileContains } from '../../../support/utils/assertions.ts'
-import { getRowForFile, triggerActionForFile } from '../../files/FilesUtils.ts'
-import { getShareUrl, setupPublicShare } from './setup-public-share.ts'
 
 describe('files_sharing: Public share - downloading files', { testIsolation: true }, () => {
 
-	before(() => setupPublicShare())
+	// in general there is no difference except downloading
+	// as file shares have the source of the share token but a different displayname
+	describe('file share', () => {
+		let fileId: number
 
-	deleteDownloadsFolderBeforeEach()
+		before(() => {
+			cy.createRandomUser().then((user) => {
+				const context: ShareContext = { user }
+				cy.uploadContent(user, new Blob(['<content>foo</content>']), 'text/plain', '/file.txt')
+					.then(({ headers }) => { fileId = Number.parseInt(headers['oc-fileid']) })
+				cy.login(user)
+				createShare(context, 'file.txt')
+					.then(() => cy.logout())
+					.then(() => cy.visit(context.url!))
+			})
+		})
 
-	beforeEach(() => {
-		cy.logout()
-		cy.visit(getShareUrl())
-	})
-
-	it('Can download all files', () => {
-		getRowForFile('foo.txt').should('be.visible')
-
-		cy.get('[data-cy-files-list]').within(() => {
-			cy.findByRole('checkbox', { name: /Toggle selection for all files/i })
-				.should('exist')
-				.check({ force: true })
-
-			// see that two files are selected
-			cy.contains('2 selected').should('be.visible')
-
-			// click download
-			cy.findByRole('button', { name: 'Download (selected)' })
+		it('can download the file', () => {
+			getRowForFileId(fileId)
 				.should('be.visible')
-				.click()
-
-			// check a file is downloaded
+			getRowForFileId(fileId)
+				.find('[data-cy-files-list-row-name]')
+				.should((el) => expect(el.text()).to.match(/file\s*\.txt/)) // extension is sparated so there might be a space between
+			triggerActionForFileId(fileId, 'download')
+			// check a file is downloaded with the correct name
 			const downloadsFolder = Cypress.config('downloadsFolder')
-			cy.readFile(`${downloadsFolder}/download.zip`, null, { timeout: 15000 })
+			cy.readFile(`${downloadsFolder}/file.txt`, 'utf-8', { timeout: 15000 })
 				.should('exist')
-				.and('have.length.gt', 30)
-				// Check all files are included
-				.and(zipFileContains([
-					'foo.txt',
-					'subfolder/',
-					'subfolder/bar.txt',
-				]))
+				.and('have.length.gt', 5)
+				.and('contain', '<content>foo</content>')
 		})
 	})
 
-	it('Can download selected files', () => {
-		getRowForFile('subfolder')
-			.should('be.visible')
+	describe('folder share', () => {
+		before(() => setupPublicShare())
 
-		cy.get('[data-cy-files-list]').within(() => {
+		deleteDownloadsFolderBeforeEach()
+
+		beforeEach(() => {
+			cy.logout()
+			cy.visit(getShareUrl())
+		})
+
+		it('Can download all files', () => {
+			getRowForFile('foo.txt').should('be.visible')
+
+			cy.get('[data-cy-files-list]').within(() => {
+				cy.findByRole('checkbox', { name: /Toggle selection for all files/i })
+					.should('exist')
+					.check({ force: true })
+
+				// see that two files are selected
+				cy.contains('2 selected').should('be.visible')
+
+				// click download
+				cy.findByRole('button', { name: 'Download (selected)' })
+					.should('be.visible')
+					.click()
+
+				// check a file is downloaded
+				const downloadsFolder = Cypress.config('downloadsFolder')
+				cy.readFile(`${downloadsFolder}/download.zip`, null, { timeout: 15000 })
+					.should('exist')
+					.and('have.length.gt', 30)
+					// Check all files are included
+					.and(zipFileContains([
+						'foo.txt',
+						'subfolder/',
+						'subfolder/bar.txt',
+					]))
+			})
+		})
+
+		it('Can download selected files', () => {
 			getRowForFile('subfolder')
-				.findByRole('checkbox')
-				.check({ force: true })
-
-			// see that two files are selected
-			cy.contains('1 selected').should('be.visible')
-
-			// click download
-			cy.findByRole('button', { name: 'Download (selected)' })
 				.should('be.visible')
-				.click()
 
-			// check a file is downloaded
-			const downloadsFolder = Cypress.config('downloadsFolder')
-			cy.readFile(`${downloadsFolder}/subfolder.zip`, null, { timeout: 15000 })
-				.should('exist')
-				.and('have.length.gt', 30)
-				// Check all files are included
-				.and(zipFileContains([
-					'subfolder/',
-					'subfolder/bar.txt',
-				]))
+			cy.get('[data-cy-files-list]').within(() => {
+				getRowForFile('subfolder')
+					.findByRole('checkbox')
+					.check({ force: true })
+
+				// see that two files are selected
+				cy.contains('1 selected').should('be.visible')
+
+				// click download
+				cy.findByRole('button', { name: 'Download (selected)' })
+					.should('be.visible')
+					.click()
+
+				// check a file is downloaded
+				const downloadsFolder = Cypress.config('downloadsFolder')
+				cy.readFile(`${downloadsFolder}/subfolder.zip`, null, { timeout: 15000 })
+					.should('exist')
+					.and('have.length.gt', 30)
+					// Check all files are included
+					.and(zipFileContains([
+						'subfolder/',
+						'subfolder/bar.txt',
+					]))
+			})
 		})
-	})
 
-	it('Can download folder by action', () => {
-		getRowForFile('subfolder')
-			.should('be.visible')
+		it('Can download folder by action', () => {
+			getRowForFile('subfolder')
+				.should('be.visible')
 
-		cy.get('[data-cy-files-list]').within(() => {
-			triggerActionForFile('subfolder', 'download')
+			cy.get('[data-cy-files-list]').within(() => {
+				triggerActionForFile('subfolder', 'download')
 
-			// check a file is downloaded
-			const downloadsFolder = Cypress.config('downloadsFolder')
-			cy.readFile(`${downloadsFolder}/subfolder.zip`, null, { timeout: 15000 })
-				.should('exist')
-				.and('have.length.gt', 30)
-				// Check all files are included
-				.and(zipFileContains([
-					'subfolder/',
-					'subfolder/bar.txt',
-				]))
+				// check a file is downloaded
+				const downloadsFolder = Cypress.config('downloadsFolder')
+				cy.readFile(`${downloadsFolder}/subfolder.zip`, null, { timeout: 15000 })
+					.should('exist')
+					.and('have.length.gt', 30)
+					// Check all files are included
+					.and(zipFileContains([
+						'subfolder/',
+						'subfolder/bar.txt',
+					]))
+			})
 		})
-	})
 
-	it('Can download file by action', () => {
-		getRowForFile('foo.txt')
-			.should('be.visible')
-
-		cy.get('[data-cy-files-list]').within(() => {
-			triggerActionForFile('foo.txt', 'download')
-
-			// check a file is downloaded
-			const downloadsFolder = Cypress.config('downloadsFolder')
-			cy.readFile(`${downloadsFolder}/foo.txt`, 'utf-8', { timeout: 15000 })
-				.should('exist')
-				.and('have.length.gt', 5)
-				.and('contain', '<content>foo</content>')
-		})
-	})
-
-	it('Can download file by selection', () => {
-		getRowForFile('foo.txt')
-			.should('be.visible')
-
-		cy.get('[data-cy-files-list]').within(() => {
+		it('Can download file by action', () => {
 			getRowForFile('foo.txt')
-				.findByRole('checkbox')
-				.check({ force: true })
+				.should('be.visible')
 
-			cy.findByRole('button', { name: 'Download (selected)' })
-				.click()
+			cy.get('[data-cy-files-list]').within(() => {
+				triggerActionForFile('foo.txt', 'download')
 
-			// check a file is downloaded
-			const downloadsFolder = Cypress.config('downloadsFolder')
-			cy.readFile(`${downloadsFolder}/foo.txt`, 'utf-8', { timeout: 15000 })
-				.should('exist')
-				.and('have.length.gt', 5)
-				.and('contain', '<content>foo</content>')
+				// check a file is downloaded
+				const downloadsFolder = Cypress.config('downloadsFolder')
+				cy.readFile(`${downloadsFolder}/foo.txt`, 'utf-8', { timeout: 15000 })
+					.should('exist')
+					.and('have.length.gt', 5)
+					.and('contain', '<content>foo</content>')
+			})
+		})
+
+		it('Can download file by selection', () => {
+			getRowForFile('foo.txt')
+				.should('be.visible')
+
+			cy.get('[data-cy-files-list]').within(() => {
+				getRowForFile('foo.txt')
+					.findByRole('checkbox')
+					.check({ force: true })
+
+				cy.findByRole('button', { name: 'Download (selected)' })
+					.click()
+
+				// check a file is downloaded
+				const downloadsFolder = Cypress.config('downloadsFolder')
+				cy.readFile(`${downloadsFolder}/foo.txt`, 'utf-8', { timeout: 15000 })
+					.should('exist')
+					.and('have.length.gt', 5)
+					.and('contain', '<content>foo</content>')
+			})
 		})
 	})
 })

cypress/e2e/files_sharing/public-share/setup-public-share.ts

@@ -79,9 +79,13 @@ function checkExpirationDateState(enforced: boolean, hasDefault: boolean) {
 	cy.get('input[data-cy-files-sharing-expiration-date-input]')
 		.invoke('val')
 		.then((val) => {
+			// eslint-disable-next-line no-unused-expressions
+			expect(val).to.not.be.undefined
+
+			const inputDate = new Date(typeof val === 'number' ? val : String(val))
 			const expectedDate = new Date()
 			expectedDate.setDate(expectedDate.getDate() + 2)
-			expect(new Date(val).toDateString()).to.eq(expectedDate.toDateString())
+			expect(inputDate.toDateString()).to.eq(expectedDate.toDateString())
 		})
 
 }