[MM-15401] Fix setting of extension when caching files (#2762)

* Get extension from Content-Disposition first

* User ImageCacheManager.cache over getCacheFile

* Add unit tests for ImageCacheManager.getCacheFile

* Add unit tests for ImageCacheManager.cache

* Use exports and require to be able to mock isDownloading

* Chain mockReturnValueOnce calls

* Fix getExtensionFromContentDisposition and its unit tests

app/components/file_attachment_list/file_attachment_list.js

@@ -5,7 +5,6 @@ import React, {Component} from 'react';
 import PropTypes from 'prop-types';
 import {
     Keyboard,
-    Platform,
     ScrollView,
     StyleSheet,
 } from 'react-native';
@@ -13,9 +12,10 @@ import {
 import {Client4} from 'mattermost-redux/client';
 
 import {isDocument, isGif, isVideo} from 'app/utils/file';
-import {getCacheFile} from 'app/utils/image_cache_manager';
+import ImageCacheManager from 'app/utils/image_cache_manager';
 import {previewImageAtIndex} from 'app/utils/images';
 import {preventDoubleTap} from 'app/utils/tap';
+import {emptyFunction} from 'app/utils/general';
 
 import FileAttachment from './file_attachment';
 
@@ -99,18 +99,12 @@ export default class FileAttachmentList extends Component {
                 }
 
                 let uri;
-                let cache;
                 if (file.localPath) {
                     uri = file.localPath;
                 } else if (isGif(file)) {
-                    cache = await getCacheFile(file.name, Client4.getFileUrl(file.id)); // eslint-disable-line no-await-in-loop
+                    uri = await ImageCacheManager.cache(file.name, Client4.getFileUrl(file.id), emptyFunction); // eslint-disable-line no-await-in-loop
                 } else {
-                    cache = await getCacheFile(file.name, Client4.getFilePreviewUrl(file.id)); // eslint-disable-line no-await-in-loop
-                }
-
-                if (cache) {
-                    const prefix = Platform.OS === 'android' ? 'file://' : '';
-                    uri = `${prefix}${cache.path}`;
+                    uri = await ImageCacheManager.cache(file.name, Client4.getFilePreviewUrl(file.id), emptyFunction); // eslint-disable-line no-await-in-loop
                 }
 
                 results.push({

app/utils/file.js

@@ -10,6 +10,7 @@ import {lookupMimeType} from 'mattermost-redux/utils/file_utils';
 import {DeviceTypes} from 'app/constants/';
 
 const EXTRACT_TYPE_REGEXP = /^\s*([^;\s]*)(?:;|\s|$)/;
+const CONTENT_DISPOSITION_REGEXP = /inline;filename=".*\.(?<ext>[a-z]+)";/i;
 const {DOCUMENTS_PATH, IMAGES_PATH, VIDEOS_PATH} = DeviceTypes;
 const DEFAULT_SERVER_MAX_FILE_SIZE = 50 * 1024 * 1024;// 50 Mb
 
@@ -234,3 +235,22 @@ export function getLocalFilePathFromFile(dir, file) {
 
     return null;
 }
+
+export function getExtensionFromContentDisposition(contentDisposition) {
+    const match = CONTENT_DISPOSITION_REGEXP.exec(contentDisposition);
+    let extension = match && match.groups.ext;
+    if (extension) {
+        if (!Object.keys(types).length) {
+            populateMaps();
+        }
+
+        extension = extension.toLowerCase();
+        if (types[extension]) {
+            return extension;
+        }
+
+        return null;
+    }
+
+    return null;
+}

app/utils/file.test.js

@@ -0,0 +1,47 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import {getExtensionFromContentDisposition} from 'app/utils/file';
+
+describe('getExtensionFromContentDisposition', () => {
+    it('should return the extracted the extension', () => {
+        const exts = [
+            'png',
+            'PNG',
+            'gif',
+            'GIF',
+            'bmp',
+            'BMP',
+            'jpg',
+            'JPG',
+            'jpeg',
+            'JPEG',
+        ];
+
+        exts.forEach((ext) => {
+            const contentDisposition = `inline;filename="test_image.${ext}"; filename*=UTF-8''test_image.${ext}"`;
+            const extension = getExtensionFromContentDisposition(contentDisposition);
+            expect(extension).toBe(ext.toLowerCase());
+        });
+    });
+
+    it('should return null for an invalid extension', () => {
+        const invalidExt = 'invalid';
+        const contentDisposition = `inline;filename="test_image.${invalidExt}"; filename*=UTF-8''test_image.${invalidExt}"`;
+        const extension = getExtensionFromContentDisposition(contentDisposition);
+        expect(extension).toBe(null);
+    });
+
+    it('should return null for no content disposition', () => {
+        const contentDispositions = [
+            undefined,
+            null,
+            '',
+        ];
+
+        contentDispositions.forEach((contentDisposition) => {
+            const extension = getExtensionFromContentDisposition(contentDisposition);
+            expect(extension).toBe(null);
+        });
+    });
+});

app/utils/image_cache_manager.js

@@ -9,7 +9,10 @@ import RNFetchBlob from 'rn-fetch-blob';
 import {Client4} from 'mattermost-redux/client';
 
 import {DeviceTypes} from 'app/constants';
-import {getExtensionFromMime} from 'app/utils/file';
+import {
+    getExtensionFromMime,
+    getExtensionFromContentDisposition,
+} from 'app/utils/file';
 import mattermostBucket from 'app/mattermost_bucket';
 
 const {IMAGES_PATH} = DeviceTypes;
@@ -26,10 +29,12 @@ export default class ImageCacheManager {
 
         const {path, exists} = await getCacheFile(filename, uri);
         const prefix = Platform.OS === 'android' ? 'file://' : '';
-        if (isDownloading(uri)) {
+        let pathWithPrefix = `${prefix}${path}`;
+
+        if (exports.isDownloading(uri)) {
             addListener(uri, listener);
         } else if (exists) {
-            listener(`${prefix}${path}`);
+            listener(pathWithPrefix);
         } else {
             addListener(uri, listener);
             if (uri.startsWith('http')) {
@@ -55,20 +60,27 @@ export default class ImageCacheManager {
                         throw new Error();
                     }
 
+                    const contentDisposition = this.downloadTask.respInfo.headers['Content-Disposition'];
                     const mimeType = this.downloadTask.respInfo.headers['Content-Type'];
-                    const ext = `.${getExtensionFromMime(mimeType) || getExtensionFromMime(DEFAULT_MIME_TYPE)}`;
-                    if (path.endsWith(ext)) {
-                        notifyAll(uri, `${prefix}${path}`);
-                    } else {
+                    const ext = `.${
+                        getExtensionFromContentDisposition(contentDisposition) ||
+                        getExtensionFromMime(mimeType) ||
+                        getExtensionFromMime(DEFAULT_MIME_TYPE)
+                    }`;
+
+                    if (!path.endsWith(ext)) {
                         const oldExt = path.substring(path.lastIndexOf('.'));
                         const newPath = path.replace(oldExt, ext);
                         await RNFetchBlob.fs.mv(path, newPath);
 
-                        notifyAll(uri, `${prefix}${newPath}`);
+                        pathWithPrefix = `${prefix}${newPath}`;
                     }
+
+                    notifyAll(uri, pathWithPrefix);
                 } catch (e) {
-                    RNFetchBlob.fs.unlink(`${prefix}${path}`);
+                    RNFetchBlob.fs.unlink(pathWithPrefix);
                     notifyAll(uri, uri);
+                    return null;
                 }
             } else {
                 // In case the uri we are trying to cache is already a local file just notify and return
@@ -77,6 +89,8 @@ export default class ImageCacheManager {
 
             unsubscribe(uri);
         }
+
+        return pathWithPrefix;
     };
 }
 
@@ -84,6 +98,7 @@ export const getCacheFile = async (name, uri) => {
     const filename = name || uri.substring(uri.lastIndexOf('/'), uri.indexOf('?') === -1 ? uri.length : uri.indexOf('?'));
     const defaultExt = `.${getExtensionFromMime(DEFAULT_MIME_TYPE)}`;
     const ext = filename.indexOf('.') === -1 ? defaultExt : filename.substring(filename.lastIndexOf('.'));
+
     let path = `${IMAGES_PATH}/${Math.abs(hashCode(uri))}${ext}`;
 
     try {
@@ -115,7 +130,7 @@ export const setSiteUrl = (url) => {
     siteUrl = url;
 };
 
-const isDownloading = (uri) => Boolean(ImageCacheManager.listeners[uri]);
+export const isDownloading = (uri) => Boolean(ImageCacheManager.listeners[uri]);
 
 const addListener = (uri, listener) => {
     if (!ImageCacheManager.listeners[uri]) {

app/utils/image_cache_manager.test.js

@@ -0,0 +1,295 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+import RNFetchBlob from 'rn-fetch-blob';
+
+import ImageCacheManager, {getCacheFile} from 'app/utils/image_cache_manager';
+import {emptyFunction} from 'app/utils/general';
+import * as fileUtils from 'app/utils/file';
+import mattermostBucket from 'app/mattermost_bucket';
+
+fileUtils.getExtensionFromMime = jest.fn();
+
+describe('getCacheFile', () => {
+    it('should return a path with correct extension for a non-cached file using file name', async () => {
+        RNFetchBlob.fs.exists.mockReturnValue(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null);
+
+        const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg'];
+        const fileUri = 'https://file-uri';
+        for (const ext of extensions) {
+            const fileName = `file${ext}`;
+            const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop
+
+            expect(exists).toEqual(false);
+            expect(path.endsWith(ext)).toEqual(true);
+        }
+    });
+
+    it('should return a path with correct extension for a non-cached file using file uri', async () => {
+        RNFetchBlob.fs.exists.mockReturnValue(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null);
+
+        const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg'];
+        const fileName = '';
+        for (const ext of extensions) {
+            const fileUri = `https://file-uri/file${ext}`;
+            const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop
+
+            expect(exists).toEqual(false);
+            expect(path.endsWith(ext)).toEqual(true);
+        }
+    });
+
+    it('should return a path with correct extension for a cached file using file name', async () => {
+        RNFetchBlob.fs.exists.mockReturnValue(true);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null);
+
+        const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg'];
+        const fileUri = 'https://file-uri';
+        for (const ext of extensions) {
+            const fileName = `file${ext}`;
+            const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop
+
+            expect(exists).toEqual(true);
+            expect(path.endsWith(ext)).toEqual(true);
+        }
+    });
+
+    it('should return a path with correct extension for a cached file using file uri', async () => {
+        RNFetchBlob.fs.exists.mockReturnValue(true);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null);
+
+        const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg'];
+        const fileName = '';
+        for (const ext of extensions) {
+            const fileUri = `https://file-uri/file${ext}`;
+            const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop
+
+            expect(exists).toEqual(true);
+            expect(path.endsWith(ext)).toEqual(true);
+        }
+    });
+
+    it('should return a path with default extension for a file and uri without an extension', async () => {
+        RNFetchBlob.fs.exists.mockReturnValue(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null);
+
+        const defaultExt = 'png';
+        fileUtils.getExtensionFromMime.mockReturnValue(defaultExt);
+
+        const fileNames = ['', 'file'];
+        const fileUris = ['', 'https://file-uri/file'];
+        for (const fileName of fileNames) {
+            for (const fileUri of fileUris) {
+                const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop
+
+                expect(exists).toEqual(false);
+                expect(path.endsWith(`.${defaultExt}`)).toEqual(true);
+            }
+        }
+    });
+
+    it('should return a path with a different extension for a file cached with the different extension', async () => {
+        const pathWithDiffExt = '/path/to/file.jpeg';
+        RNFetchBlob.fs.exists.mockReturnValue(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(pathWithDiffExt);
+
+        const fileNamesUris = [
+            {fileName: '', fileUri: 'https://file-uri/file.png'},
+            {fileName: 'file.png', fileUri: ''},
+        ];
+
+        for (const {fileName, fileUri} of fileNamesUris) {
+            const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop
+            expect(exists).toEqual(true);
+            expect(path).toEqual(pathWithDiffExt);
+        }
+    });
+});
+
+describe('ImageCacheManager.cache', () => {
+    const imageCacheManagerUtils = require('app/utils/image_cache_manager');
+    imageCacheManagerUtils.isDownloading = jest.fn();
+    RNFetchBlob.config.mockReturnValue(RNFetchBlob);
+    mattermostBucket.getPreference = jest.fn().mockReturnValue({});
+
+    beforeEach(() => {
+        ImageCacheManager.listeners = {};
+    });
+
+    it('should cache a file from an http uri', async () => {
+        const fileName = '';
+        const fileUris = ['http://file-uri', 'https://file-uri'];
+        for (const fileUri of fileUris) {
+            RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+            RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+            RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers: {}}});
+
+            const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+            expect(path).not.toEqual(null);
+        }
+    });
+
+    it('should get the extension from the content disposition', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        const ext = '.bmp';
+        const headers = {'Content-Disposition': `inline;filename="file${ext}"; filename*=UTF-8''file${ext}`};
+        RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers}});
+
+        const fileName = '';
+        const fileUri = 'https://file-uri';
+        const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+        expect(path.endsWith(ext)).toEqual(true);
+    });
+
+    it('should get the extension from the content type', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        const ext = 'jpg';
+        const headers = {'Content-Type': 'image/jpeg'};
+        RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers}});
+        fileUtils.getExtensionFromMime.
+            mockReturnValueOnce(ext). // first call in getCacheFile
+            mockReturnValueOnce(ext); // seconds call in cache using MIME type from header
+
+        const fileName = '';
+        const fileUri = 'https://file-uri';
+        const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+        expect(path.endsWith(`.${ext}`)).toEqual(true);
+    });
+
+    it('should default to .png', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        const ext = 'png';
+        RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers: {}}});
+        fileUtils.getExtensionFromMime.
+            mockReturnValueOnce(ext). // first call in getCacheFile
+            mockReturnValueOnce(null). // second call in cache using MIME type from header
+            mockReturnValueOnce(ext); // third call in cache using DEFAULT_MIME_TYPE
+
+        const fileName = '';
+        const fileUri = 'https://file-uri';
+        const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+        expect(path.endsWith(`.${ext}`)).toEqual(true);
+    });
+
+    it('should move file if path extension and extracted extension don\'t match', async () => {
+        const oldExt = 'png';
+        const fileNameUris = [
+            {fileName: '', fileUri: `https://file-uri/file.${oldExt}`},
+            {fileName: `file${oldExt}`, fileUri: `https://file-uri/file.${oldExt}`},
+        ];
+
+        for (const {fileName, fileUri} of fileNameUris) {
+            RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+            RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+            const ext = 'bmp';
+            const headers = {'Content-Type': 'image/bmp'};
+            RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers}});
+            fileUtils.getExtensionFromMime.
+                mockReturnValueOnce(ext). // first call in getCacheFile
+                mockReturnValueOnce(ext); // second call in cache using MIME type from header
+
+            const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+            expect(path.endsWith(`.${ext}`)).toEqual(true);
+
+            const oldPath = path.replace(ext, oldExt);
+            expect(RNFetchBlob.fs.mv).toHaveBeenCalledWith(oldPath, path);
+        }
+    });
+
+    it('should return cached file when it exists', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(true);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        const ext = '.png';
+        const fileName = `file${ext}`;
+        const fileUri = `http://file-uri/file${ext}`;
+        const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+        expect(path.endsWith(ext)).toEqual(true);
+    });
+
+    it('should not cache the file when respInfo.respType === "text"', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: 'text', headers: {}}});
+
+        const fileName = '';
+        const fileUri = 'https://file-uri';
+        const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+        expect(path).toEqual(null);
+        expect(RNFetchBlob.fs.unlink).toHaveBeenCalled();
+    });
+
+    it('should add the listener if it is downloading', async () => {
+        imageCacheManagerUtils.isDownloading.mockReturnValueOnce(true);
+
+        const fileName = 'file.png';
+        const fileUri = 'http://file-uri/file.png';
+        await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop
+
+        const expectedListeners = {[fileUri]: [emptyFunction]};
+        expect(ImageCacheManager.listeners).toMatchObject(expectedListeners);
+    });
+
+    it('should call listener with path if file exists', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(true);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        const fileName = 'file.png';
+        const fileUri = 'http://file-uri/file.png';
+        const listener = jest.fn();
+        const path = await ImageCacheManager.cache(fileName, fileUri, listener); // eslint-disable-line no-await-in-loop
+
+        expect(ImageCacheManager.listeners).toMatchObject({});
+        expect(listener).toHaveBeenCalledWith(path);
+    });
+
+    it('should call all listeners with path after downloading file then remove listeners', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+        RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers: {}}});
+
+        // Ensure isDownloading returns false so that we can proceed to
+        // download the file even if there are existing listeners.
+        imageCacheManagerUtils.isDownloading.mockReturnValueOnce(false);
+
+        const fileName = 'file.png';
+        const fileUri = 'http://file-uri/file.png';
+        const existingListener = jest.fn();
+        ImageCacheManager.listeners = {[fileUri]: [existingListener]};
+        const newListener = jest.fn();
+        const path = await ImageCacheManager.cache(fileName, fileUri, newListener); // eslint-disable-line no-await-in-loop
+
+        expect(ImageCacheManager.listeners).toMatchObject({});
+        expect(existingListener).toHaveBeenCalledWith(path);
+        expect(newListener).toHaveBeenCalledWith(path);
+    });
+
+    it('should call all listeners with the local file', async () => {
+        RNFetchBlob.fs.exists.mockReturnValueOnce(false);
+        RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null);
+
+        // Ensure isDownloading returns false so that we can proceed to
+        // download the file even if there are existing listeners.
+        imageCacheManagerUtils.isDownloading.mockReturnValueOnce(false);
+
+        const fileName = 'file.png';
+        const fileUri = 'file://file-uri/file.png';
+        const existingListener = jest.fn();
+        ImageCacheManager.listeners = {[fileUri]: [existingListener]};
+        const newListener = jest.fn();
+        await ImageCacheManager.cache(fileName, fileUri, newListener); // eslint-disable-line no-await-in-loop
+
+        expect(ImageCacheManager.listeners).toMatchObject({});
+        expect(existingListener).toHaveBeenCalledWith(fileUri);
+        expect(newListener).toHaveBeenCalledWith(fileUri);
+    });
+});

test/setup.js

@@ -82,7 +82,13 @@ jest.mock('rn-fetch-blob', () => ({
             DocumentDir: () => jest.fn(),
             CacheDir: () => jest.fn(),
         },
+        exists: jest.fn(),
+        existsWithDiffExt: jest.fn(),
+        unlink: jest.fn(),
+        mv: jest.fn(),
     },
+    fetch: jest.fn(),
+    config: jest.fn(),
 }));
 
 jest.mock('rn-fetch-blob/fs', () => ({
@@ -90,6 +96,10 @@ jest.mock('rn-fetch-blob/fs', () => ({
         DocumentDir: () => jest.fn(),
         CacheDir: () => jest.fn(),
     },
+    exists: jest.fn(),
+    existsWithDiffExt: jest.fn(),
+    unlink: jest.fn(),
+    mv: jest.fn(),
 }));
 
 global.requestAnimationFrame = (callback) => {