Skip to content

Commit

Permalink
Make Templates an ampdoc-level service to avoid FIE and shadowdoc con…
Browse files Browse the repository at this point in the history 
…flicts (#32853)

* Make Templates an ampdoc-level service to avoid FIE and shadowdoc conflicts

* cleanup

* fix presubmits

* fixes

* fix ads tests

* fix amp-list tests

* fix amp-date-display tests
  • Loading branch information
Dima Voytenko committed Feb 24, 2021
1 parent f9fd4e5 commit a0578f7
Show file tree
Hide file tree
Showing 42 changed files with 345 additions and 160 deletions.
5 changes: 4 additions & 1 deletion build-system/tasks/presubmit-checks.js
View file
Expand Up @@ -228,10 +228,11 @@ const forbiddenTerms = {
'src/service/storage-impl.js',
],
},
'installTemplatesService': {
'installTemplatesServiceForDoc': {
message: privateServiceFactory,
allowlist: [
'src/runtime.js',
'src/inabox/inabox-services.js',
'src/service/core-services.js',
'src/service/template-impl.js',
],
Expand Down Expand Up @@ -301,6 +302,7 @@ const forbiddenTerms = {
allowlist: [
// Do not allowlist additional "extensions/*" paths.
// TODO(#22414): Remove paths as they are migrated off of sync API.
'extensions/amp-a4a/0.1/amp-ad-template-helper.js',
'extensions/amp-analytics/0.1/instrumentation.js',
'extensions/amp-analytics/0.1/variables.js',
'extensions/amp-fx-collection/0.1/providers/fx-provider.js',
Expand All @@ -310,6 +312,7 @@ const forbiddenTerms = {
'src/service.js',
'src/service/cid-impl.js',
'src/service/origin-experiments-impl.js',
'src/service/template-impl.js',
'src/services.js',
'src/utils/display-observer.js',
'testing/test-helper.js',
Expand Down
7 changes: 6 additions & 1 deletion extensions/amp-a4a/0.1/amp-ad-network-base.js
View file
Expand Up @@ -172,7 +172,12 @@ export class AmpAdNetworkBase extends AMP.BaseElement {
response.headers.get('AMP-Ad-Response-Type') || 'default';
devAssert(this.validators_[validatorType], 'Validator never registered!');
return this.validators_[validatorType]
.validate(this.context_, unvalidatedBytes, response.headers)
.validate(
this.context_,
this.element,
unvalidatedBytes,
response.headers
)
.catch((err) =>
Promise.reject({type: FailureType.VALIDATOR_ERROR, msg: err})
);
Expand Down
35 changes: 27 additions & 8 deletions extensions/amp-a4a/0.1/amp-ad-template-helper.js
View file
Expand Up @@ -20,6 +20,10 @@ import {createElementWithAttributes} from '../../../src/dom';
import {devAssert} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {getMode} from '../../../src/mode';
import {
getServiceForDoc,
registerServiceBuilderForDoc,
} from '../../../src/service';
import {isArray} from '../../../src/types';
import {parseUrlDeprecated} from '../../../src/url';
import {urls} from '../../../src/config';
Expand All @@ -34,13 +38,15 @@ const TEMPLATE_CORS_CONFIG = {
credentials: 'omit',
};

const SERVICE_ID = 'AmpAdTemplateHelper';

export class AmpAdTemplateHelper {
/**
* @param {!Window} win
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
*/
constructor(win) {
/** @private {!Window} */
this.win_ = win;
constructor(ampdoc) {
/** @private @const */
this.ampdoc_ = ampdoc;

/** @private {LruCache} */
this.cache_ = new LruCache(5);
Expand All @@ -53,14 +59,15 @@ export class AmpAdTemplateHelper {
* @return {!Promise<string>}
*/
fetch(templateUrl) {
const {win} = this.ampdoc_;
const proxyUrl =
getMode(this.win_).localDev && !isNaN(templateUrl)
? `http://ads.localhost:${this.win_.location.port}` +
getMode(win).localDev && !isNaN(templateUrl)
? `http://ads.localhost:${win.location.port}` +
`/a4a_template/adzerk/${templateUrl}`
: this.getTemplateProxyUrl_(templateUrl);
let templatePromise = this.cache_.get(proxyUrl);
if (!templatePromise) {
templatePromise = Services.xhrFor(this.win_)
templatePromise = Services.xhrFor(win)
.fetchText(proxyUrl, TEMPLATE_CORS_CONFIG)
.then((response) => response.text());
this.cache_.put(proxyUrl, templatePromise);
Expand All @@ -75,7 +82,7 @@ export class AmpAdTemplateHelper {
* @return {!Promise<!Element>} Promise which resolves after rendering completes.
*/
render(templateValues, element) {
return Services.templatesFor(this.win_).findAndRenderTemplate(
return Services.templatesForDoc(this.ampdoc_).findAndRenderTemplate(
element,
templateValues
);
Expand Down Expand Up @@ -132,3 +139,15 @@ export class AmpAdTemplateHelper {
loc.pathname;
}
}

/**
* @param {!Element|!../../../src/service/ampdoc-impl.AmpDoc} target
* @return {!AmpAdTemplateHelper}
*/
export function getAmpAdTemplateHelper(target) {
registerServiceBuilderForDoc(target, SERVICE_ID, AmpAdTemplateHelper);
return /** @type {!AmpAdTemplateHelper} */ (getServiceForDoc(
target,
SERVICE_ID
));
}
8 changes: 7 additions & 1 deletion extensions/amp-a4a/0.1/amp-ad-type-defs.js
View file
Expand Up @@ -84,12 +84,18 @@ export let CrossDomainDataDef;
export class Validator {
/**
* @param {!Object} unusedContext
* @param {!Element} unusedContainerElement
* @param {!ArrayBuffer} unusedUnvalidatedBytes
* @param {!Headers} unusedHeaders
* @return {!Promise<!ValidatorOutput>}
* @abstract
*/
validate(unusedContext, unusedUnvalidatedBytes, unusedHeaders) {}
validate(
unusedContext,
unusedContainerElement,
unusedUnvalidatedBytes,
unusedHeaders
) {}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-a4a/0.1/cryptographic-validator.js
View file
Expand Up @@ -62,7 +62,7 @@ export class CryptographicValidator extends Validator {
}

/** @override */
validate(context, unvalidatedBytes, headers) {
validate(context, containerElement, unvalidatedBytes, headers) {
return this.getSignatureVerifier_(context.win)
.verify(
unvalidatedBytes,
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-a4a/0.1/template-renderer.js
View file
Expand Up @@ -16,7 +16,7 @@

import {Renderer} from './amp-ad-type-defs';
import {devAssert} from '../../../src/log';
import {getAmpAdTemplateHelper} from './template-validator';
import {getAmpAdTemplateHelper} from './amp-ad-template-helper';
import {renderCreativeIntoFriendlyFrame} from './friendly-frame-util';

/**
Expand Down Expand Up @@ -72,7 +72,7 @@ export class TemplateRenderer extends Renderer {
if (!data) {
return Promise.resolve();
}
const templateHelper = getAmpAdTemplateHelper(context.win);
const templateHelper = getAmpAdTemplateHelper(element);
return templateHelper
.render(data, this.getDocument(iframe).body)
.then((renderedElement) => {
Expand Down
20 changes: 3 additions & 17 deletions extensions/amp-a4a/0.1/template-validator.js
View file
Expand Up @@ -15,9 +15,9 @@
*/

import {AdResponseType, Validator, ValidatorResult} from './amp-ad-type-defs';
import {AmpAdTemplateHelper} from '../../amp-a4a/0.1/amp-ad-template-helper';
import {Services} from '../../../src/services';
import {getAmpAdMetadata} from './amp-ad-utils';
import {getAmpAdTemplateHelper} from './amp-ad-template-helper';
import {pushIfNotExist} from '../../../src/utils/array';
import {tryParseJson} from '../../../src/json';
import {utf8Decode} from '../../../src/utils/bytes';
Expand All @@ -27,26 +27,12 @@ export const AMP_TEMPLATED_CREATIVE_HEADER_NAME = 'AMP-Ad-Template-Extension';
export const DEPRECATED_AMP_TEMPLATED_CREATIVE_HEADER_NAME =
'AMP-template-amp-creative';

/** @type {?AmpAdTemplateHelper} */
let ampAdTemplateHelper;

/**
* Returns the global template helper.
* @param {!Window} win
* @return {!AmpAdTemplateHelper}
*/
export function getAmpAdTemplateHelper(win) {
return (
ampAdTemplateHelper || (ampAdTemplateHelper = new AmpAdTemplateHelper(win))
);
}

/**
* Validator for Template ads.
*/
export class TemplateValidator extends Validator {
/** @override */
validate(context, unvalidatedBytes, headers) {
validate(context, containerElement, unvalidatedBytes, headers) {
const body = utf8Decode(/** @type {!ArrayBuffer} */ (unvalidatedBytes));
const parsedResponseBody = /** @type {./amp-ad-type-defs.AmpTemplateCreativeDef} */ (tryParseJson(
body
Expand Down Expand Up @@ -75,7 +61,7 @@ export class TemplateValidator extends Validator {
);
}

return getAmpAdTemplateHelper(context.win)
return getAmpAdTemplateHelper(containerElement)
.fetch(parsedResponseBody.templateUrl)
.then((template) => {
const creativeMetadata = getAmpAdMetadata(template);
Expand Down
12 changes: 5 additions & 7 deletions extensions/amp-a4a/0.1/test/test-amp-ad-template-helper.js
View file
Expand Up @@ -17,23 +17,25 @@
import {AmpAdTemplateHelper} from '../amp-ad-template-helper';
import {AmpMustache} from '../../../amp-mustache/0.1/amp-mustache';
import {Xhr} from '../../../../src/service/xhr-impl';
import {registerExtendedTemplateForDoc} from '../../../../src/service/template-impl';

describes.fakeWin('AmpAdTemplateHelper', {amp: true}, (env) => {
const cdnUrl =
'https://adserver-com.cdn.ampproject.org/ad/s/' +
'adserver.com/amp_template_1';
const canonicalUrl = 'https://adserver.com/amp_template_1';

let win, doc;
let win, doc, ampdoc;
let fetchTextMock;
let ampAdTemplateHelper;

beforeEach(() => {
win = env.win;
win.__AMP_MODE = {localDev: false};
doc = win.document;
ampdoc = env.ampdoc;
fetchTextMock = env.sandbox.stub(Xhr.prototype, 'fetchText');
ampAdTemplateHelper = new AmpAdTemplateHelper(win);
ampAdTemplateHelper = new AmpAdTemplateHelper(ampdoc);
});

it('should return a promise resolving to a string template', () => {
Expand Down Expand Up @@ -67,11 +69,7 @@ describes.fakeWin('AmpAdTemplateHelper', {amp: true}, (env) => {
});

it('should render a template with correct values', () => {
win.AMP.registerTemplate('amp-mustache', AmpMustache);
});

it('should render a template with correct values', () => {
win.AMP.registerTemplate('amp-mustache', AmpMustache);
registerExtendedTemplateForDoc(ampdoc, 'amp-mustache', AmpMustache);
const parentDiv = doc.createElement('div');
parentDiv./*OK*/ innerHTML =
'<template type="amp-mustache"><p>{{foo}}</p></template>';
Expand Down
19 changes: 17 additions & 2 deletions extensions/amp-a4a/0.1/test/test-cryptographic-validator.js
View file
Expand Up @@ -34,10 +34,14 @@ describes.realWin('CryptographicValidator', realWinConfig, (env) => {
const headers = {'Content-Type': 'application/jwk-set+json'};
let userErrorStub;
let validator;
let containerElement;

beforeEach(() => {
validator = new CryptographicValidator();
userErrorStub = env.sandbox.stub(user(), 'error');

containerElement = env.win.document.createElement('div');
env.win.document.body.appendChild(containerElement);
});

it('should have AMP validator result', () => {
Expand All @@ -47,7 +51,12 @@ describes.realWin('CryptographicValidator', realWinConfig, (env) => {
verify: () => Promise.resolve(VerificationStatus.OK),
};
return validator
.validate({win: env.win}, utf8Encode(data.reserialized), headers)
.validate(
{win: env.win},
containerElement,
utf8Encode(data.reserialized),
headers
)
.then((validatorOutput) => {
expect(validatorOutput).to.be.ok;
expect(validatorOutput.type).to.equal(ValidatorResult.AMP);
Expand All @@ -66,7 +75,12 @@ describes.realWin('CryptographicValidator', realWinConfig, (env) => {
verify: () => Promise.resolve(VerificationStatus.UNVERIFIED),
};
return validator
.validate({win: env.win}, utf8Encode(data.reserialized), headers)
.validate(
{win: env.win},
containerElement,
utf8Encode(data.reserialized),
headers
)
.then((validatorOutput) => {
expect(validatorOutput).to.be.ok;
expect(validatorOutput.type).to.equal(ValidatorResult.NON_AMP);
Expand All @@ -88,6 +102,7 @@ describes.realWin('CryptographicValidator', realWinConfig, (env) => {
return validator
.validate(
{win: env.win},
containerElement,
utf8Encode(data.reserializedInvalidOffset),
headers
)
Expand Down
19 changes: 11 additions & 8 deletions extensions/amp-a4a/0.1/test/test-template-renderer.js
View file
Expand Up @@ -17,12 +17,13 @@
import {
AMP_TEMPLATED_CREATIVE_HEADER_NAME,
TemplateValidator,
getAmpAdTemplateHelper,
} from '../template-validator';
import {AmpMustache} from '../../../amp-mustache/0.1/amp-mustache';
import {TemplateRenderer} from '../template-renderer';
import {ValidatorResult} from '../amp-ad-type-defs';
import {data} from './testdata/valid_css_at_rules_amp.reserialized';
import {getAmpAdTemplateHelper} from '../amp-ad-template-helper';
import {registerExtendedTemplateForDoc} from '../../../../src/service/template-impl';
import {utf8Encode} from '../../../../src/utils/bytes';

const realWinConfig = {
Expand All @@ -41,7 +42,7 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
},
};

let doc;
let doc, ampdoc;
let containerElement;
let context;
let renderer;
Expand All @@ -50,6 +51,7 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {

beforeEach(() => {
doc = env.win.document;
ampdoc = env.ampdoc;
renderer = new TemplateRenderer();
validator = new TemplateValidator();

Expand Down Expand Up @@ -81,14 +83,15 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
};

env.sandbox
.stub(getAmpAdTemplateHelper(env.win), 'fetch')
.stub(getAmpAdTemplateHelper(ampdoc), 'fetch')
.callsFake((url) => {
expect(url).to.equal(templateUrl);
return Promise.resolve(data.adTemplate);
});

validatorPromise = validator.validate(
context,
containerElement,
utf8Encode(
JSON.stringify({
templateUrl,
Expand All @@ -105,7 +108,7 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
});

it('should append iframe child with correct template values', () => {
env.win.AMP.registerTemplate('amp-mustache', AmpMustache);
registerExtendedTemplateForDoc(ampdoc, 'amp-mustache', AmpMustache);
return validatorPromise.then((validatorOutput) => {
// Sanity check. This behavior is tested in test-template-validator.js.
expect(validatorOutput).to.be.ok;
Expand All @@ -131,7 +134,7 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
});

it('should set correct attributes on the iframe', () => {
env.win.AMP.registerTemplate('amp-mustache', AmpMustache);
registerExtendedTemplateForDoc(ampdoc, 'amp-mustache', AmpMustache);
return validatorPromise.then((validatorOutput) => {
// Sanity check. This behavior is tested in test-template-validator.js.
expect(validatorOutput).to.be.ok;
Expand All @@ -154,7 +157,7 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
});

it('should style body of iframe document to be visible', () => {
env.win.AMP.registerTemplate('amp-mustache', AmpMustache);
registerExtendedTemplateForDoc(ampdoc, 'amp-mustache', AmpMustache);
return validatorPromise.then((validatorOutput) => {
// Sanity check. This behavior is tested in test-template-validator.js.
expect(validatorOutput).to.be.ok;
Expand All @@ -174,9 +177,9 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
});

it('should insert analytics', () => {
env.win.AMP.registerTemplate('amp-mustache', AmpMustache);
registerExtendedTemplateForDoc(ampdoc, 'amp-mustache', AmpMustache);
const insertAnalyticsSpy = env.sandbox.spy(
getAmpAdTemplateHelper(env.win),
getAmpAdTemplateHelper(ampdoc),
'insertAnalytics'
);
return validatorPromise.then((validatorOutput) => {
Expand Down

0 comments on commit a0578f7

Please sign in to comment.