From fa8b298819e19ba9be4565ae73f227bb0f380f89 Mon Sep 17 00:00:00 2001 From: Mingze Xiao Date: Thu, 24 Oct 2019 14:22:37 -0700 Subject: [PATCH] fix(mediaviewer): address comments --- src/lib/Preview.js | 33 +++++++++++--- src/lib/__tests__/Preview-test.js | 31 +++++++++++++ src/lib/viewers/media/DashViewer.js | 13 ++++-- src/lib/viewers/media/MediaBaseViewer.js | 43 +++++++------------ .../media/__tests__/MediaBaseViewer-test.js | 24 ----------- 5 files changed, 82 insertions(+), 62 deletions(-) diff --git a/src/lib/Preview.js b/src/lib/Preview.js index 8a4dbc952b..a01531a716 100644 --- a/src/lib/Preview.js +++ b/src/lib/Preview.js @@ -70,6 +70,7 @@ const DEFAULT_DISABLED_VIEWERS = ['Office']; // viewers disabled by default const PREFETCH_COUNT = 4; // number of files to prefetch const MOUSEMOVE_THROTTLE_MS = 1500; // for showing or hiding the navigation icons const RETRY_COUNT = 3; // number of times to retry network request for a file +const RETRY_TOKEN_COUNT = 3; // number of times to retry refreshing access token const KEYDOWN_EXCEPTIONS = ['INPUT', 'SELECT', 'TEXTAREA']; // Ignore keydown events on these elements const LOG_RETRY_TIMEOUT_MS = 500; // retry interval for logging preview event const LOG_RETRY_COUNT = 3; // number of times to retry logging preview event @@ -109,9 +110,6 @@ class Preview extends EventEmitter { /** @property {Object} - Map of disabled viewer names */ disabledViewers = {}; - /** @property {string|Function} - Access token */ - token = ''; - /** @property {Object} - Current viewer instance */ viewer; @@ -127,6 +125,9 @@ class Preview extends EventEmitter { /** @property {number} - Number of times a particular preview has been retried */ retryCount = 0; + /** @property {number} - Number of times refreshing token has been retried */ + retryTokenCount = 0; + /** @property {number} - Number of times a particular logging call cas been retried */ logRetryCount = 0; @@ -974,9 +975,6 @@ class Preview extends EventEmitter { // Add the response interceptor to the preview instance this.options.responseInterceptor = options.responseInterceptor; - // Add the token generator to refresh the token if necessary - this.options.tokenGenerator = options.token; - // Disable or enable viewers based on viewer options Object.keys(this.options.viewers).forEach(viewerName => { const isDisabled = this.options.viewers[viewerName].disabled; @@ -1005,6 +1003,7 @@ class Preview extends EventEmitter { location: this.location, cache: this.cache, ui: this.ui, + refreshToken: this.refreshToken, }); } @@ -1885,6 +1884,28 @@ class Preview extends EventEmitter { const fileId = typeof fileIdOrFile === 'string' ? fileIdOrFile : fileIdOrFile.id; return getProp(this.previewOptions, `fileOptions.${fileId}.${optionName}`); } + + /** + * Refresh the access token + * + * @private + * @return {Promise} + */ + refreshToken = () => { + if (typeof this.previewOptions.token !== 'function') { + return Promise.reject(new Error('Token expired and cannot refresh token.')); + } + if (this.retryTokenCount >= RETRY_TOKEN_COUNT) { + return Promise.reject(new Error('Reach refreshing token limit.')); + } + this.retryTokenCount += 1; + return getTokens(this.file.id, this.previewOptions.token).then(tokenOrTokenMap => { + if (!tokenOrTokenMap || typeof tokenOrTokenMap === 'string') { + return tokenOrTokenMap; + } + return tokenOrTokenMap[this.file.id]; + }); + }; } global.Box = global.Box || {}; diff --git a/src/lib/__tests__/Preview-test.js b/src/lib/__tests__/Preview-test.js index b0db3fe09c..7b5540f7fa 100644 --- a/src/lib/__tests__/Preview-test.js +++ b/src/lib/__tests__/Preview-test.js @@ -17,6 +17,7 @@ const tokens = require('../tokens'); const PREFETCH_COUNT = 4; // number of files to prefetch const MOUSEMOVE_THROTTLE = 1500; // for showing or hiding the navigation icons const KEYDOWN_EXCEPTIONS = ['INPUT', 'SELECT', 'TEXTAREA']; // Ignore keydown events on these elements +const RETRY_TOKEN_COUNT = 3; // number of times to retry refreshing access token const sandbox = sinon.sandbox.create(); @@ -2837,5 +2838,35 @@ describe('lib/Preview', () => { expect(preview.getFileOption('123', 'fileVersionId')).to.equal(undefined); }); }); + + describe('refreshToken()', () => { + it('should return a new token if the previewOptions.token is a function', done => { + preview.file = { + id: 'file_123', + }; + preview.previewOptions.token = id => Promise.resolve({ [id]: 'new_token' }); + preview.refreshToken().then(token => { + expect(token).to.equal('new_token'); + done(); + }); + }); + + it('should reject if tried to refresh token more than 3 times', done => { + preview.previewOptions.token = id => Promise.resolve({ [id]: 'new_token' }); + preview.retryTokenCount = RETRY_TOKEN_COUNT + 1; + preview.refreshToken().catch(error => { + expect(error.message).to.equal('Reach refreshing token limit.'); + done(); + }); + }); + + it('should reject if previewOptions.token is not a function', done => { + preview.previewOptions.token = 'token'; + preview.refreshToken().catch(error => { + expect(error.message).to.equal('Token expired and cannot refresh token.'); + done(); + }); + }); + }); }); /* eslint-enable no-unused-expressions */ diff --git a/src/lib/viewers/media/DashViewer.js b/src/lib/viewers/media/DashViewer.js index 0e7768f469..6c0906572a 100644 --- a/src/lib/viewers/media/DashViewer.js +++ b/src/lib/viewers/media/DashViewer.js @@ -510,10 +510,15 @@ class DashViewer extends VideoBaseViewer { normalizedShakaError.code === shaka.util.Error.Code.BAD_HTTP_STATUS && normalizedShakaError.data[1] === 401 // token expired ) { - this.refreshToken().then(newToken => { - this.options.token = newToken; - this.player.retryStreaming(); - }); + this.options + .refreshToken() + .then(newToken => { + this.options.token = newToken; + this.player.retryStreaming(); + }) + .catch(tokenError => { + this.triggerError(tokenError); + }); return; } diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js index 4ef437dfa0..bfbb8e4654 100644 --- a/src/lib/viewers/media/MediaBaseViewer.js +++ b/src/lib/viewers/media/MediaBaseViewer.js @@ -4,7 +4,6 @@ import Browser from '../../Browser'; import MediaControls from './MediaControls'; import PreviewError from '../../PreviewError'; import Timer from '../../Timer'; -import getTokens from '../../tokens'; import { CLASS_ELEM_KEYBOARD_FOCUS, CLASS_HIDDEN, CLASS_IS_BUFFERING, CLASS_IS_VISIBLE } from '../../constants'; import { ERROR_CODE, MEDIA_METRIC, MEDIA_METRIC_EVENTS, VIEWER_EVENT } from '../../events'; import { getProp } from '../../util'; @@ -269,21 +268,6 @@ class MediaBaseViewer extends BaseViewer { this.wrapperEl.classList.add(CLASS_IS_VISIBLE); } - /** - * Refresh the access token - * - * @private - * @return {Promise} - */ - refreshToken = () => { - return getTokens(this.options.file.id, this.options.tokenGenerator).then(tokenOrTokenMap => { - if (!tokenOrTokenMap || typeof tokenOrTokenMap === 'string') { - return tokenOrTokenMap; - } - return tokenOrTokenMap[this.options.file.id]; - }); - }; - /** * Handles media element loading errors. * @@ -301,18 +285,21 @@ class MediaBaseViewer extends BaseViewer { const errorDetails = errorCode ? { error_code: errorCode, error_message: errorMessage } : {}; // refresh the token if token expired - if ( - errorCode === MediaError.MEDIA_ERR_NETWORK && - errorMessage.includes(MEDIA_TOKEN_EXPIRE_ERROR) && - typeof this.options.tokenGenerator === 'function' - ) { - this.refreshToken().then(newToken => { - const { currentTime } = this.mediaEl; - this.currentTime = currentTime; - this.options.token = newToken; - this.mediaUrl = this.createContentUrlWithAuthParams(this.options.representation.content.url_template); - this.mediaEl.src = this.mediaUrl; - }); + if (errorCode === MediaError.MEDIA_ERR_NETWORK && errorMessage.includes(MEDIA_TOKEN_EXPIRE_ERROR)) { + this.options + .refreshToken() + .then(newToken => { + const { currentTime } = this.mediaEl; + this.currentTime = currentTime; + this.options.token = newToken; + this.mediaUrl = this.createContentUrlWithAuthParams( + this.options.representation.content.url_template, + ); + this.mediaEl.src = this.mediaUrl; + }) + .catch(error => { + this.triggerError(error); + }); return; } diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js index e2a0a7b842..8d1b674ab9 100644 --- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js +++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js @@ -185,30 +185,6 @@ describe('lib/viewers/media/MediaBaseViewer', () => { }); }); - describe('refreshToken()', () => { - it('should return the same token if the tokenGenerator is a string', done => { - media.options.file = { - id: 'file_123', - }; - media.options.tokenGenerator = 'new_token'; - media.refreshToken().then(token => { - expect(token).to.equal('new_token'); - done(); - }); - }); - - it('should return a new token if the tokenGenerator is a function', done => { - media.options.file = { - id: 'file_123', - }; - media.options.tokenGenerator = id => Promise.resolve({ [id]: 'new_token' }); - media.refreshToken().then(token => { - expect(token).to.equal('new_token'); - done(); - }); - }); - }); - describe('errorHandler()', () => { before(() => { window.MediaError = {