Skip to content

Commit

Permalink
Fix: Fail intentionally when preview image files with deleted reps (#964
Browse files Browse the repository at this point in the history
)
  • Loading branch information
Jeremy Press authored Mar 27, 2019
1 parent ca4620a commit f5c4de4
Show file tree
Hide file tree
Showing 11 changed files with 156 additions and 34 deletions.
24 changes: 24 additions & 0 deletions src/lib/__tests__/api-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,30 @@ describe('API helper', () => {
sandbox.verifyAndRestore();
});

describe('parseResponse()', () => {
it('should return the full response when the status is 202 or 204', () => {
const response = {
status: 202,
data: 'foo'
};

expect(api.parseResponse(response)).to.equal(response);

response.status = 204;
expect(api.parseResponse(response)).to.equal(response);
});

it('should only return the data', () => {
const data = 'foo';
const response = {
status: 200,
data
};

expect(api.parseResponse(response)).to.equal(data);
});
});

describe('get()', () => {
const url = '/foo/bar';

Expand Down
22 changes: 22 additions & 0 deletions src/lib/__tests__/util-test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable no-unused-expressions */
import Location from '../Location';
import * as util from '../util';
import { ERROR_CODE } from '../events';
import DownloadReachability from '../DownloadReachability';

const sandbox = sinon.sandbox.create();
Expand Down Expand Up @@ -706,4 +707,25 @@ describe('lib/util', () => {
}
);
});

describe('handleRepresentationBlobFetch()', () => {
it('should reject if the response is a 202', () => {
const response = {
status: 202
};

util.handleRepresentationBlobFetch(response).catch((e) => expect(e.code).to.equal(ERROR_CODE.DELETED_REPS));
});

it('should pass the response through', () => {
const response = {
status: 200,
body: 'body'
};

util
.handleRepresentationBlobFetch(response)
.then((passedResponse) => expect(passedResponse).to.equal(response));
});
});
});
4 changes: 2 additions & 2 deletions src/lib/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ const api = {
*
* @private
* @param {Response} response - Response to parse
* @return {Promise|Response} Response if 204, otherwise promise that resolves with JSON
* @return {Promise|Response} Response if 204 or 202, otherwise promise that resolves with JSON
*/
parseResponse: (response) => {
if (response.status === 204) {
if (response.status === 204 || response.status === 202) {
return response;
}

Expand Down
31 changes: 31 additions & 0 deletions src/lib/util.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import Uri from 'jsuri';
import { decode } from 'box-react-ui/lib/utils/keys';
import api from './api';
import DownloadReachability from './DownloadReachability';
import Location from './Location';
import PreviewError from './PreviewError';
import { ERROR_CODE } from './events';

const HEADER_CLIENT_NAME = 'X-Box-Client-Name';
const HEADER_CLIENT_VERSION = 'X-Box-Client-Version';
Expand Down Expand Up @@ -716,3 +719,31 @@ export function isLocalStorageAvailable() {
return false;
}
}

/**
* Handles a blob response from axios.
* Generates a deleted reps error on a 202 response, which has no body and is unusable.
*
* @param {Object} response - A response from axios
* @return {Promise} - Resolves with the representation data, rejects with a deleted rep error
*/
export function handleRepresentationBlobFetch(response) {
const status = getProp(response, 'status');
if (status === 202) {
const error = new PreviewError(ERROR_CODE.DELETED_REPS, __('error_refresh'), { isRepDeleted: true });
return Promise.reject(error);
}

return Promise.resolve(response);
}

/**
* Method to fetch a given representation as a blob via an authenticated content URL.
* Useful for determining the true state of a "successful" representation
*
* @param {string} contentUrl - a representation's authenticated content URL
* @return {Promise} - Response from axios
*/
export function fetchRepresentationAsBlob(contentUrl) {
return api.get(contentUrl, { type: 'blob' }).then(handleRepresentationBlobFetch);
}
8 changes: 5 additions & 3 deletions src/lib/viewers/image/ImageBaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ const CSS_CLASS_ZOOMABLE = 'zoomable';
const CSS_CLASS_PANNABLE = 'pannable';

class ImageBaseViewer extends BaseViewer {
/** @property {string} - Url used to download an image representation */
downloadUrl;

/** @inheritdoc */
constructor(options) {
super(options);
Expand Down Expand Up @@ -320,9 +323,8 @@ class ImageBaseViewer extends BaseViewer {
// eslint-disable-next-line
console.error(err);

// Display a generic error message but log the real one
const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh'), {}, err.message);
super.handleDownloadError(error, imgUrl);
const genericDownloadError = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh'), err.message);
super.handleDownloadError(err instanceof PreviewError ? err : genericDownloadError, imgUrl);
}

/**
Expand Down
18 changes: 12 additions & 6 deletions src/lib/viewers/image/ImageViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ class ImageViewer extends ImageBaseViewer {

const { representation, viewer } = this.options;
const template = representation.content.url_template;
const downloadUrl = this.createContentUrlWithAuthParams(template, viewer.ASSET);
this.downloadUrl = this.createContentUrlWithAuthParams(template, viewer.ASSET);

this.bindDOMListeners();
return this.getRepStatus()
.getPromise()
.then(() => this.handleAssetAndRepLoad(downloadUrl))
.then(this.handleAssetAndRepLoad)
.catch(this.handleAssetError);
}

Expand All @@ -72,11 +72,17 @@ class ImageViewer extends ImageBaseViewer {
* @override
* @return {void}
*/
handleAssetAndRepLoad(downloadUrl) {
handleAssetAndRepLoad() {
this.startLoadTimer();
this.imageEl.src = downloadUrl;

super.handleAssetAndRepLoad();
return util
.fetchRepresentationAsBlob(this.downloadUrl)
.then((blob) => {
const srcUrl = URL.createObjectURL(blob);
this.imageEl.src = srcUrl;
super.handleAssetAndRepLoad();
})
.catch(this.handleImageDownloadError);
}

/**
Expand Down Expand Up @@ -374,7 +380,7 @@ class ImageViewer extends ImageBaseViewer {
* @return {void}
*/
handleImageDownloadError(err) {
this.handleDownloadError(err, this.imageEl.src);
this.handleDownloadError(err, this.downloadUrl);
}

/**
Expand Down
19 changes: 14 additions & 5 deletions src/lib/viewers/image/MultiImageViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import PageControls from '../../PageControls';
import './MultiImage.scss';
import { ICON_FULLSCREEN_IN, ICON_FULLSCREEN_OUT } from '../../icons/icons';
import { CLASS_INVISIBLE, CLASS_MULTI_IMAGE_PAGE } from '../../constants';
import { pageNumberFromScroll } from '../../util';
import { pageNumberFromScroll, fetchRepresentationAsBlob } from '../../util';

const PADDING_BUFFER = 100;
const CSS_CLASS_IMAGE = 'bp-images';
Expand Down Expand Up @@ -148,7 +148,14 @@ class MultiImageViewer extends ImageBaseViewer {
this.singleImageEls[index].setAttribute('data-page-number', index + 1);
this.singleImageEls[index].classList.add(CLASS_MULTI_IMAGE_PAGE);

this.singleImageEls[index].src = imageUrl;
this.downloadUrl = imageUrl;

fetchRepresentationAsBlob(imageUrl)
.then((blob) => {
const srcUrl = URL.createObjectURL(blob);
this.singleImageEls[index].src = srcUrl;
})
.catch(this.handleMultiImageDownloadError);
}

/** @inheritdoc */
Expand Down Expand Up @@ -267,16 +274,18 @@ class MultiImageViewer extends ImageBaseViewer {
* @return {void}
*/
handleMultiImageDownloadError(err) {
if (this.isDestroyed()) {
return;
}

this.singleImageEls.forEach((el, index) => {
this.unbindImageListeners(index);
});

// Since we're using the src to get the hostname, we can always use the src of the first page
const { src } = this.singleImageEls[0];
// Clear any images we may have started to load.
this.singleImageEls = [];

this.handleDownloadError(err, src);
this.handleDownloadError(err, this.downloadUrl);
}

/**
Expand Down
27 changes: 14 additions & 13 deletions src/lib/viewers/image/__tests__/ImageViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ describe('lib/viewers/image/ImageViewer', () => {
image.wrapperEl.style.height = '50px';

sandbox.stub(image, 'getRepStatus').returns({ getPromise: () => Promise.resolve() });
sandbox.stub(util, 'fetchRepresentationAsBlob').returns(Promise.resolve(imageUrl));
sandbox.stub(URL, 'createObjectURL').returns(imageUrl);
image.setup();
image.load(imageUrl).catch(() => {});
});
Expand Down Expand Up @@ -273,21 +275,18 @@ describe('lib/viewers/image/ImageViewer', () => {
});
});

it('should zoom the width & height when the image rotated', () => {
it('should increase scale when the image is rotated', () => {
sandbox.stub(image, 'isRotated').returns(true);
sandbox.stub(image, 'getTransformWidthAndHeight').returns({
width: 100,
height: 100
});
stubs.setScale = sandbox.stub(image, 'setScale');

image.imageEl.style.transform = 'rotate(90deg)';
image.imageEl.style.width = '200px';
image.imageEl.setAttribute('originalWidth', '150');
image.imageEl.setAttribute('originalHeight', '100');
image.imageEl.src = imageUrl;
const origImageSize = image.imageEl.getBoundingClientRect();
image.zoomIn();
const newImageSize = image.imageEl.getBoundingClientRect();
expect(newImageSize.width).gt(origImageSize.width);
expect(newImageSize.height).gt(origImageSize.height);

expect(stubs.setScale).to.be.calledWith(undefined, 120);
expect(stubs.adjustZoom).to.be.called;
image.imageEl.style.transform = '';
});

it('should reset dimensions and adjust padding when called with reset', () => {
Expand Down Expand Up @@ -599,11 +598,13 @@ describe('lib/viewers/image/ImageViewer', () => {
done();
})
);
sandbox.stub(URL, 'URL.createObjectURL(blob)').returns(url);

image.handleAssetAndRepLoad(url);
image.handleAssetAndRepLoad(url).then(() => {
expect(imageEl.src).to.be.equal(url);
});

expect(startLoadTimer).to.be.called;
expect(imageEl.url).to.be.equal(url);
expect(loadBoxAnnotations).to.be.called;
expect(createAnnotator).to.be.called;
});
Expand Down
25 changes: 20 additions & 5 deletions src/lib/viewers/image/__tests__/MultiImageViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,17 @@ describe('lib/viewers/image/MultiImageViewer', () => {
expect(stubs.bindImageListeners).to.be.called;
});

it('should set the image source', () => {
it('should set the download URL and fetch the page as a blob', () => {
stubs.fetchRepresentationAsBlob = sandbox
.stub(util, 'fetchRepresentationAsBlob')
.returns(Promise.resolve('foo'));
multiImage.singleImageEls = [stubs.singleImageEl];
const repUrl = 'file/100/content/{page}.png';

multiImage.setupImageEls('file/100/content/{page}.png', 0);
expect(multiImage.singleImageEls[0].src).to.be.equal('file/100/content/{page}.png');
multiImage.setupImageEls(repUrl, 0);

expect(multiImage.downloadUrl).to.be.equal(repUrl);
expect(stubs.fetchRepresentationAsBlob).to.be.called;
});

it('should set the page number for each image el', () => {
Expand Down Expand Up @@ -404,13 +410,22 @@ describe('lib/viewers/image/MultiImageViewer', () => {
sandbox.stub(multiImage, 'unbindImageListeners');
});

it('should do nothing if the viewer is already destroyed', () => {
sandbox.stub(multiImage, 'isDestroyed').returns(true);
multiImage.downloadUrl = multiImage.singleImageEls[0];
multiImage.handleMultiImageDownloadError('err');

expect(multiImage.handleDownloadError).to.not.be.called;
expect(multiImage.unbindImageListeners).to.not.be.called;
});

it('unbind the image listeners, clear the image Els array, and handle the download error', () => {
const { src } = multiImage.singleImageEls[0];
multiImage.downloadUrl = multiImage.singleImageEls[0];

multiImage.handleMultiImageDownloadError('err');

expect(multiImage.singleImageEls).to.deep.equal([]);
expect(multiImage.handleDownloadError).to.be.calledWith('err', src);
expect(multiImage.handleDownloadError).to.be.calledWith('err', multiImage.downloadUrl);
expect(multiImage.unbindImageListeners).to.be.calledTwice;
});
});
Expand Down
10 changes: 10 additions & 0 deletions test/integration/sanity/DeletedReps.e2e.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ describe('Previewing a file with deleted representations', () => {
const token = Cypress.env('ACCESS_TOKEN');

const fileIdDoc = Cypress.env('FILE_ID_DOC');
const fileIdImage = Cypress.env('FILE_ID_IMAGE');
const fileIdPresentation = Cypress.env('FILE_ID_PRESENTATION');
const fileIdTiff = Cypress.env('FILE_ID_TIFF');

const REPS_ERROR = 'error_deleted_reps';

Expand Down Expand Up @@ -53,6 +55,14 @@ describe('Previewing a file with deleted representations', () => {
{
viewer: 'Presentation',
fileId: fileIdPresentation
},
{
viewer: 'Image',
fileId: fileIdImage
},
{
viewer: 'MultiImage',
fileId: fileIdTiff
}
].forEach((test) => {
it(test.viewer, () => {
Expand Down
2 changes: 2 additions & 0 deletions test/support/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ Cypress.env({
FILE_ID_DOC: '415542803939',
FILE_ID_DOC_LARGE: '420985736453',
FILE_ID_HTML: '420872247534',
FILE_ID_IMAGE: '428416513276',
FILE_ID_JS: '416043839991',
FILE_ID_JSON: '420868959262',
FILE_ID_MP3: '415542687401',
FILE_ID_PRESENTATION_WITH_LINKS: '416052594867',
FILE_ID_PRESENTATION: '415537552367',
FILE_ID_TIFF: '428410193698',
FILE_ID_TXT: '420870357452',
FILE_ID_VIDEO_SMALL: '415542846356',
FILE_ID_VIDEO_SUBTITLES_TRACKS: '415542245854',
Expand Down

0 comments on commit f5c4de4

Please sign in to comment.