Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Ads RTC: support amp-script fetching #33872

Merged
merged 4 commits into from
Apr 21, 2021
Merged

✨ Ads RTC: support amp-script fetching #33872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7d412c5
Ads RTC: support amp-script fetching.
samouri Apr 16, 2021
f4b0500
flesh out example better.
samouri Apr 20, 2021
4a70e3a
revert unecessary diff
samouri Apr 20, 2021
48ae33d
feedback from caleb
samouri Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions examples/amp-ad/doubleclick-rtc.amp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
}
</style>
<script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js"></script>
<script async custom-element="amp-script" src="https://cdn.ampproject.org/v0/amp-script-0.1.js"></script>
<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
<body>
Expand Down Expand Up @@ -42,5 +43,20 @@ <h2>Doubleclick with RTC</h2>
}
}'>
</amp-ad>

<amp-ad width="320" height="50"
type="doubleclick"
data-slot="/4119129/mobile_ad_banner"
rtc-config='{"urls": ["amp-script:targetingFns.getTargeting"]}'
</amp-ad>
<amp-script nodom data-ampdevmode id="targetingFns" script="targetingFnsScript"></amp-script>
<script id="targetingFnsScript" type="text/plain" target="amp-script">
exportFunction("getTargeting", () => {
return {
targeting: {food: ["chicken", "beans"]},
categoryExclusions: ["sports", "food", "fun"]
};
});
</script>
</body>
</html>
111 changes: 72 additions & 39 deletions src/service/real-time-config/real-time-config-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ import {RTC_VENDORS} from './callout-vendors';
import {Services} from '../../services';
import {dev, user, userAssert} from '../../log';
import {getMode} from '../../mode';
import {isAmpScriptUri} from '../../../src/url';
import {isArray, isObject} from '../../core/types';
import {isCancellation} from '../../error-reporting';

import {registerServiceBuilderForDoc} from '../../service';
import {tryParseJson} from '../../json';

Expand Down Expand Up @@ -313,8 +313,9 @@ export class RealTimeConfigManager {
* Manages sending the RTC callouts for the Custom URLs.
* @param {!Object<string, !../../../src/service/variable-source.AsyncResolverDef>} customMacros The ad-network specified macro
* @param {!Function} checkStillCurrent
* @param {!Element} element
*/
handleRtcForCustomUrls(customMacros, checkStillCurrent) {
handleRtcForCustomUrls(customMacros, checkStillCurrent, element) {
// For each publisher defined URL, inflate the url using the macros,
// and send the RTC request.
(this.rtcConfig_.urls || []).forEach((urlObj) => {
Expand All @@ -331,7 +332,9 @@ export class RealTimeConfigManager {
url,
customMacros,
errorReportingUrl,
checkStillCurrent
checkStillCurrent,
/* opt_vendor */ undefined,
element
);
});
}
Expand Down Expand Up @@ -395,14 +398,16 @@ export class RealTimeConfigManager {
* @param {string} errorReportingUrl
* @param {!Function} checkStillCurrent
* @param {string=} opt_vendor
* @param {!Element=} opt_element
* @private
*/
inflateAndSendRtc_(
url,
macros,
errorReportingUrl,
checkStillCurrent,
opt_vendor
opt_vendor,
opt_element
) {
let {timeoutMillis} = this.rtcConfig_;
const callout = opt_vendor || this.getCalloutParam_(url);
Expand All @@ -422,7 +427,10 @@ export class RealTimeConfigManager {
errorReportingUrl
);
}
if (!Services.urlForDoc(this.ampDoc_).isSecure(url)) {
if (
!Services.urlForDoc(this.ampDoc_).isSecure(url) &&
!isAmpScriptUri(url)
) {
return this.buildErrorResponse_(
RTC_ERROR_ENUM.INSECURE_URL,
callout,
Expand All @@ -440,12 +448,14 @@ export class RealTimeConfigManager {
if (url.length > MAX_URL_LENGTH) {
url = this.truncUrl_(url);
}

return this.sendRtcCallout_(
url,
timeoutMillis,
callout,
checkStillCurrent,
errorReportingUrl
errorReportingUrl,
opt_element
);
};

Expand Down Expand Up @@ -494,6 +504,7 @@ export class RealTimeConfigManager {
* @param {string} callout
* @param {!Function} checkStillCurrent
* @param {string} errorReportingUrl
* @param {!Element=} opt_element
* @return {!Promise<!rtcResponseDef>}
* @private
*/
Expand All @@ -502,45 +513,67 @@ export class RealTimeConfigManager {
timeoutMillis,
callout,
checkStillCurrent,
errorReportingUrl
errorReportingUrl,
opt_element
) {
let rtcFetch;
if (isAmpScriptUri(url)) {
rtcFetch = Services.scriptForDocOrNull(opt_element)
.then((service) => {
userAssert(service, 'AMP-SCRIPT is not installed.');
return service.fetch(url);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably me not understanding how amp script works, but how to we ensure they are using the no-dom attr?

Copy link
Member Author

@samouri samouri Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a security/ux standpoint, I don't think we actually need it to be nodom.
I.e. some pubs may want to have their amp-script perform both functions:

  1. As a normal amp-script
  2. As something that exports a fn for RTC

All the usual amp-script restrictions apply (no dom mutation without user activity, etc)

Copy link
Member Author

@samouri samouri Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to followup on this to see if sandboxed should imply nodom

})
.then((json) => {
checkStillCurrent();
const rtcTime = Date.now() - this.rtcStartTime_;
if (typeof json !== 'object') {
return this.buildErrorResponse_(
RTC_ERROR_ENUM.MALFORMED_JSON_RESPONSE,
callout,
errorReportingUrl,
rtcTime
);
}
return {response: json, rtcTime, callout};
});
} else {
rtcFetch = Services.xhrFor(this.win_)
.fetchJson(
// NOTE(bradfrizzell): we could include ampCors:false allowing
// the request to be cached across sites but for now assume that
// is not a required feature.
url,
{credentials: 'include'}
)
.then((res) => {
checkStillCurrent();
return res.text().then((text) => {
checkStillCurrent();
const rtcTime = Date.now() - this.rtcStartTime_;
// An empty text response is allowed, not an error.
if (!text) {
return {rtcTime, callout};
}
const response = tryParseJson(text);
return response
? {response, rtcTime, callout}
: this.buildErrorResponse_(
RTC_ERROR_ENUM.MALFORMED_JSON_RESPONSE,
callout,
errorReportingUrl,
rtcTime
);
});
});
}

/**
* Note: Timeout is enforced by timerFor, not the value of
* rtcTime. There are situations where rtcTime could thus
* end up being greater than timeoutMillis.
*/
return Services.timerFor(this.win_)
.timeoutPromise(
timeoutMillis,
Services.xhrFor(this.win_)
.fetchJson(
// NOTE(bradfrizzell): we could include ampCors:false allowing
// the request to be cached across sites but for now assume that
// is not a required feature.
url,
{credentials: 'include'}
)
.then((res) => {
checkStillCurrent();
return res.text().then((text) => {
checkStillCurrent();
const rtcTime = Date.now() - this.rtcStartTime_;
// An empty text response is allowed, not an error.
if (!text) {
return {rtcTime, callout};
}
const response = tryParseJson(text);
return response
? {response, rtcTime, callout}
: this.buildErrorResponse_(
RTC_ERROR_ENUM.MALFORMED_JSON_RESPONSE,
callout,
errorReportingUrl,
rtcTime
);
});
})
)
.timeoutPromise(timeoutMillis, rtcFetch)
.catch((error) => {
return isCancellation(error)
? undefined
Expand Down Expand Up @@ -589,7 +622,7 @@ export class RealTimeConfigManager {
this.modifyRtcConfigForConsentStateSettings();
customMacros = this.assignMacros(customMacros);
this.rtcStartTime_ = Date.now();
this.handleRtcForCustomUrls(customMacros, checkStillCurrent);
this.handleRtcForCustomUrls(customMacros, checkStillCurrent, element);
this.handleRtcForVendorUrls(customMacros, checkStillCurrent);
return Promise.all(this.promiseArray_);
}
Expand Down
24 changes: 24 additions & 0 deletions test/unit/test-real-time-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,30 @@ describes.realWin('real-time-config service', {amp: true}, (env) => {
expectedRtcArray,
});
});

it('should fetch RTC from amp-script URIs', async () => {
const ampScriptFetch = env.sandbox.stub();
ampScriptFetch.returns(Promise.resolve({targeting: ['sports']}));
env.sandbox
.stub(Services, 'scriptForDocOrNull')
.returns(Promise.resolve({fetch: ampScriptFetch}));

const urls = ['amp-script:scriptId.functionName'];
setRtcConfig({urls, vendors: {}, timeoutMillis: 500});
const rtcResponse = await execute_(
element,
/* customMacros */ {},
/* consentState */ undefined,
/* consentString */ undefined,
/* consentMetadata */ undefined,
() => {}
);
expect(ampScriptFetch).calledWithExactly(
'amp-script:scriptId.functionName'
);
expect(rtcResponse[0].response).deep.equal({targeting: ['sports']});
});

it('should send RTC callouts to inflated vendor URLs', () => {
const vendors = {
'fAkeVeNdOR': {SLOT_ID: 1, PAGE_ID: 2},
Expand Down