diff --git a/CHANGELOG.md b/CHANGELOG.md index 6afc637b..4eba83c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - none yet ### Fixed -- none yet +- Safer communication protocol in intents. ### Added - `cancel()` method on service object returned by `cozy.client.intents.createService()`` diff --git a/src/intents.js b/src/intents.js index dac6a55f..defb3637 100644 --- a/src/intents.js +++ b/src/intents.js @@ -3,7 +3,7 @@ import {cozyFetchJSON} from './fetch' const intentClass = 'coz-intent' // inject iframe for service in given element -function injectService (url, element, data) { +function injectService (url, element, intent, data) { const document = element.ownerDocument if (!document) throw new Error('Cannot retrieve document object from given element') @@ -23,7 +23,7 @@ function injectService (url, element, data) { const messageHandler = (event) => { if (event.origin !== serviceOrigin) return - if (event.data === 'intent:ready') { + if (event.data.type === `intent-${intent._id}:ready`) { handshaken = true return event.source.postMessage(data, event.origin) } @@ -31,13 +31,28 @@ function injectService (url, element, data) { window.removeEventListener('message', messageHandler) iframe.parentNode.removeChild(iframe) - if (event.data === 'intent:error') { + if (event.data.type === `intent-${intent._id}:error`) { return reject(new Error('Intent error')) } - return handshaken - ? resolve(event.data) - : reject(new Error('Unexpected handshake message from intent service')) + if (handshaken && event.data.type === `intent-${intent._id}:cancel`) { + return resolve(null) + } + + if (handshaken && event.data.type === `intent-${intent._id}:done`) { + return resolve(event.data.document) + } + + if (!handshaken) { + return reject(new Error('Unexpected handshake message from intent service')) + } + + // We may be in a state where the messageHandler is still attached to then + // window, but will not be needed anymore. For example, the service failed + // before adding the `unload` listener, so no `intent:cancel` message has + // never been sent. + // So we simply ignore other messages, and this listener will stay here, + // waiting for a message which will never come, forever (almost). } window.addEventListener('message', messageHandler) @@ -68,7 +83,7 @@ export function create (cozy, action, type, data = {}, permissions = []) { return Promise.reject(new Error('Unable to find a service')) } - return injectService(service.href, element, data) + return injectService(service.href, element, intent, data) }) } @@ -85,7 +100,9 @@ function listenClientData (intent, window) { } window.addEventListener('message', messageEventListener) - window.parent.postMessage('intent:ready', intent.attributes.client) + window.parent.postMessage({ + type: `intent-${intent._id}:ready` + }, intent.attributes.client) }) } @@ -99,21 +116,34 @@ export function createService (cozy, intentId, serviceWindow) { return cozyFetchJSON(cozy, 'GET', `/intents/${intentId}`) .then(intent => { - return listenClientData(intent, serviceWindow) - .then(data => { - let terminated = false + let terminated = false - const terminate = (doc) => { - if (terminated) throw new Error('Intent service has already been terminated') - terminated = true - serviceWindow.parent.postMessage(doc, intent.attributes.client) - } + const terminate = (message) => { + if (terminated) throw new Error('Intent service has already been terminated') + terminated = true + serviceWindow.parent.postMessage(message, intent.attributes.client) + } + + const cancel = () => { + terminate({type: `intent-${intent._id}:cancel`}) + } + // Prevent unfulfilled client promises when this window unloads for a + // reason or another. + serviceWindow.addEventListener('unload', () => { + if (!terminated) cancel() + }) + + return listenClientData(intent, serviceWindow) + .then(data => { return { getData: () => data, getIntent: () => intent, - terminate: terminate, - cancel: () => terminate(null) + terminate: (doc) => terminate({ + type: `intent-${intent._id}:done`, + document: doc + }), + cancel: cancel } }) }) diff --git a/test/unit/intents.js b/test/unit/intents.js index 03885bd1..03e73a00 100644 --- a/test/unit/intents.js +++ b/test/unit/intents.js @@ -142,7 +142,9 @@ describe('Intents', function () { const handshakeEventMessageMock = { origin: serviceUrl, - data: 'intent:ready', + data: { + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:ready' + }, source: iframeWindowMock } @@ -195,7 +197,9 @@ describe('Intents', function () { const handshakeEventMessageMock = { origin: serviceUrl, - data: 'intent:error', + data: { + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:error' + }, source: iframeWindowMock } @@ -222,7 +226,9 @@ describe('Intents', function () { const handshakeEventMessageMock = { origin: serviceUrl, - data: 'intent:ready', + data: { + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:ready' + }, source: iframeWindowMock } @@ -232,7 +238,10 @@ describe('Intents', function () { const resolveEventMessageMock = { origin: serviceUrl, - data: result, + data: { + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:done', + document: result + }, source: iframeWindowMock } @@ -273,6 +282,33 @@ describe('Intents', function () { beforeEach(mock.mockAPI('GetIntent')) + it('starts handshake', async function () { + const windowMock = mockWindow() + + const clientHandshakeEventMessageMock = { + origin: expectedIntent.attributes.client, + data: { foo: 'bar' } + } + + windowMock.parent.postMessage.callsFake(() => { + const messageEventListener = windowMock.addEventListener.secondCall.args[1] + windowMock.addEventListener.withArgs('message', messageEventListener).calledOnce.should.be.true() + + messageEventListener(clientHandshakeEventMessageMock) + windowMock.removeEventListener.withArgs('message', messageEventListener).calledOnce.should.be.true() + }) + + return cozy.client.intents.createService(expectedIntent._id, windowMock) + .then(service => { + const messageMatch = sinon.match({ + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:ready' + }) + + windowMock.parent.postMessage + .withArgs(messageMatch, expectedIntent.attributes.client).calledOnce.should.be.true() + }) + }) + it('should manage handshake', async function () { const windowMock = mockWindow() @@ -282,7 +318,7 @@ describe('Intents', function () { } windowMock.parent.postMessage.callsFake(() => { - const messageEventListener = windowMock.addEventListener.firstCall.args[1] + const messageEventListener = windowMock.addEventListener.secondCall.args[1] windowMock.addEventListener.withArgs('message', messageEventListener).calledOnce.should.be.true() messageEventListener(clientHandshakeEventMessageMock) @@ -334,7 +370,7 @@ describe('Intents', function () { } windowMock.parent.postMessage.callsFake(() => { - const messageEventListener = windowMock.addEventListener.firstCall.args[1] + const messageEventListener = windowMock.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) }) @@ -342,8 +378,13 @@ describe('Intents', function () { service.terminate(result) + const messageMatch = sinon.match({ + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:done', + document: result + }) + windowMock.parent.postMessage - .withArgs(result, expectedIntent.attributes.client).calledOnce.should.be.true() + .withArgs(messageMatch, expectedIntent.attributes.client).calledOnce.should.be.true() }) it('should send result document to Client also with no params', async function () { @@ -359,7 +400,7 @@ describe('Intents', function () { } window.parent.postMessage.callsFake(() => { - const messageEventListener = window.addEventListener.firstCall.args[1] + const messageEventListener = window.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) }) @@ -367,8 +408,13 @@ describe('Intents', function () { service.terminate(result) + const messageMatch = sinon.match({ + type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:done', + document: result + }) + window.parent.postMessage - .withArgs(result, expectedIntent.attributes.client).calledOnce.should.be.true() + .withArgs(messageMatch, expectedIntent.attributes.client).calledOnce.should.be.true() delete global.window }) @@ -386,7 +432,7 @@ describe('Intents', function () { } windowMock.parent.postMessage.callsFake(() => { - const messageEventListener = windowMock.addEventListener.firstCall.args[1] + const messageEventListener = windowMock.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) }) @@ -410,7 +456,7 @@ describe('Intents', function () { } windowMock.parent.postMessage.callsFake(() => { - const messageEventListener = windowMock.addEventListener.firstCall.args[1] + const messageEventListener = windowMock.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) }) @@ -418,8 +464,10 @@ describe('Intents', function () { service.cancel() + const messageMatch = sinon.match({type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:cancel'}) + windowMock.parent.postMessage - .withArgs(null, expectedIntent.attributes.client).calledOnce.should.be.true() + .withArgs(messageMatch, expectedIntent.attributes.client).calledOnce.should.be.true() }) it('should send null to Client also with no parameters', async function () { @@ -431,7 +479,7 @@ describe('Intents', function () { } window.parent.postMessage.callsFake(() => { - const messageEventListener = window.addEventListener.firstCall.args[1] + const messageEventListener = window.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) }) @@ -439,8 +487,10 @@ describe('Intents', function () { service.cancel() + const messageMatch = sinon.match({type: 'intent-77bcc42c-0fd8-11e7-ac95-8f605f6e8338:cancel'}) + window.parent.postMessage - .withArgs(null, expectedIntent.attributes.client).calledOnce.should.be.true() + .withArgs(messageMatch, expectedIntent.attributes.client).calledOnce.should.be.true() delete global.window }) @@ -454,7 +504,7 @@ describe('Intents', function () { } windowMock.parent.postMessage.callsFake(() => { - const messageEventListener = windowMock.addEventListener.firstCall.args[1] + const messageEventListener = windowMock.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) }) @@ -480,7 +530,7 @@ describe('Intents', function () { } windowMock.parent.postMessage.callsFake(() => { - const messageEventListener = windowMock.addEventListener.firstCall.args[1] + const messageEventListener = windowMock.addEventListener.secondCall.args[1] messageEventListener(clientHandshakeEventMessageMock) })