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.

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

Viewer must be trusted for ssr templates to be supported #25520

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions build-system/tasks/presubmit-checks.js
Expand Up @@ -427,15 +427,16 @@ const forbiddenTerms = {
message: requiresReviewPrivacy,
whitelist: [
'extensions/amp-bind/0.1/bind-impl.js',
'extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js',
'src/error.js',
'src/utils/xhr-utils.js',
'src/service/viewer-impl.js',
'src/service/viewer-interface.js',
'src/service/viewer-cid-api.js',
'src/impression.js',
'src/inabox/inabox-viewer.js',
'src/service/cid-impl.js',
'src/impression.js',
'extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js',
'src/service/viewer-cid-api.js',
'src/service/viewer-impl.js',
'src/service/viewer-interface.js',
'src/ssr-template-helper.js',
'src/utils/xhr-utils.js',
],
},
'prerenderSafe': {
Expand Down
79 changes: 52 additions & 27 deletions extensions/amp-form/0.1/amp-form.js
Expand Up @@ -385,7 +385,8 @@ export class AmpForm {
);

// Form verification is not supported when SSRing templates is enabled.
if (!this.ssrTemplateHelper_.isSupported()) {
this.ssrTemplateHelper_.isSupported().then(isSSRSupported => {
if (!isSSRSupported) {
this.form_.addEventListener('change', e => {
this.verifier_.onCommit().then(({updatedElements, errors}) => {
updatedElements.forEach(checkUserValidityAfterInteraction_);
Expand All @@ -397,9 +398,11 @@ export class AmpForm {
if (this.state_ === FormState.VERIFYING) {
if (errors.length) {
this.setState_(FormState.VERIFY_ERROR);
this.renderTemplate_(dict({'verifyErrors': errors})).then(() => {
this.renderTemplate_(dict({'verifyErrors': errors})).then(
() => {
this.triggerAction_(FormEvents.VERIFY_ERROR, errors);
});
}
);
} else {
this.setState_(FormState.INITIAL);
}
Expand All @@ -412,6 +415,7 @@ export class AmpForm {
checkUserValidityAfterInteraction_(dev().assertElement(e.target));
this.validator_.onInput(e);
});
});
}

/** @private */
Expand All @@ -431,18 +435,21 @@ export class AmpForm {
* @private
*/
triggerFormSubmitInAnalytics_(eventType) {
this.assertSsrTemplate_(false, 'Form analytics not supported');
this.assertCsrTemplate_('Form analytics not supported').then(() => {
const formDataForAnalytics = dict({});
const formObject = this.getFormAsObject_();

for (const k in formObject) {
if (Object.prototype.hasOwnProperty.call(formObject, k)) {
formDataForAnalytics['formFields[' + k + ']'] = formObject[k].join(',');
formDataForAnalytics['formFields[' + k + ']'] = formObject[k].join(
','
);
}
}
formDataForAnalytics['formId'] = this.form_.id;

this.analyticsEvent_(eventType, formDataForAnalytics);
});
}

/**
Expand Down Expand Up @@ -548,14 +555,20 @@ export class AmpForm {

this.dirtinessHandler_.onSubmitting();

const nonXhrGet = !this.xhrAction_ && this.method_ === 'GET';
return Promise.resolve()
.then(() => {
// Do any assertions we may need to do
// For NonXhrGET
// That way we can determine if
// we can submit synchronously
if (!this.xhrAction_ && this.method_ == 'GET') {
this.assertSsrTemplate_(false, 'Non-XHR GETs not supported.');
if (nonXhrGet) {
this.assertNoSensitiveFields_();

return this.assertCsrTemplate_('Non-XHR GETs not supported.');
}
})
.then(() => {
if (nonXhrGet) {
// If we have no async inputs, we can just submit synchronously
if (asyncInputs.length === 0) {
for (let i = 0; i < varSubsFields.length; i++) {
Expand All @@ -570,14 +583,18 @@ export class AmpForm {

this.handleNonXhrGet_(shouldSubmitFormElement);
this.dirtinessHandler_.onSubmitSuccess();
return Promise.resolve();
return Promise.resolve({earlyReturn: true});
}

if (event) {
event.preventDefault();
}
}

})
.then(({earlyReturn = false} = {}) => {
if (earlyReturn) {
return;
}
// Set ourselves to the SUBMITTING State
this.setState_(FormState.SUBMITTING);

Expand All @@ -589,7 +606,9 @@ export class AmpForm {
iterateCursor(asyncInputs, asyncInput => {
const asyncCall = this.getValueForAsyncInput_(asyncInput);
if (
asyncInput.classList.contains(AsyncInputClasses.ASYNC_REQUIRED_ACTION)
asyncInput.classList.contains(
AsyncInputClasses.ASYNC_REQUIRED_ACTION
)
) {
requiredActionPromises.push(asyncCall);
} else {
Expand All @@ -609,6 +628,7 @@ export class AmpForm {
},
error => this.handlePresubmitError_(error)
);
});
}

/**
Expand Down Expand Up @@ -674,8 +694,9 @@ export class AmpForm {
* @return {!Promise}
*/
handleXhrSubmit_(trust) {
return this.ssrTemplateHelper_.isSupported().then(isSSRSupported => {
let p;
if (this.ssrTemplateHelper_.isSupported()) {
if (isSSRSupported) {
p = this.handleSsrTemplate_(trust);
} else {
this.submittingWithTrust_(trust);
Expand All @@ -688,6 +709,7 @@ export class AmpForm {
this.xhrSubmitPromise_ = p;
}
return p;
});
}

/**
Expand Down Expand Up @@ -893,13 +915,16 @@ export class AmpForm {
* @private
*/
doXhr_(url, method, opt_extraFields, opt_fieldBlacklist) {
this.assertSsrTemplate_(false, 'XHRs should be proxied.');
return this.requestForFormFetch(
return this.assertCsrTemplate_('XHRs should be proxied.')
.then(() =>
this.requestForFormFetch(
url,
method,
opt_extraFields,
opt_fieldBlacklist
).then(request => this.xhr_.fetch(request.xhrUrl, request.fetchOpt));
)
)
.then(request => this.xhr_.fetch(request.xhrUrl, request.fetchOpt));
}

/**
Expand All @@ -911,7 +936,7 @@ export class AmpForm {
const json = /** @type {!Promise<!JsonObject>} */ (response.json());
return this.handleSubmitSuccess_(json).then(() => {
this.triggerFormSubmitInAnalytics_('amp-form-submit-success');
this.maybeHandleRedirect_(response);
return this.maybeHandleRedirect_(response);
});
}

Expand Down Expand Up @@ -1001,18 +1026,16 @@ export class AmpForm {
}

/**
* Asserts that SSR support is the same as value.
* @param {boolean} value
* @param {string} msg
* Asserts that standard client side rendering is being used as opposed to SSR.
*
* @private
* @param {string} msg
* @return {!Promise}
*/
assertSsrTemplate_(value, msg) {
const supported = this.ssrTemplateHelper_.isSupported();
userAssert(
supported === value,
'[amp-form]: viewerRenderTemplate | %s',
msg
);
assertCsrTemplate_(msg) {
return this.ssrTemplateHelper_.isSupported().then(isSSR => {
userAssert(!isSSR, '[amp-form]: viewerRenderTemplate | %s', msg);
});
}

/**
Expand Down Expand Up @@ -1052,10 +1075,11 @@ export class AmpForm {
* Handles response redirect throught the AMP-Redirect-To response header.
* Not applicable if viewer can render templates.
* @param {?Response} response
* @return {!Promise}
* @private
*/
maybeHandleRedirect_(response) {
this.assertSsrTemplate_(false, 'Redirects not supported.');
return this.assertCsrTemplate_('Redirects not supported.').then(() => {
if (!response || !response.headers) {
return;
}
Expand All @@ -1082,6 +1106,7 @@ export class AmpForm {
const navigator = Services.navigationForDoc(this.form_);
navigator.navigateTo(this.win_, redirectTo, REDIRECT_TO_HEADER);
}
});
}

/**
Expand Down