🐛 Post-based identity bug fix

[hellokoji]

Bug fix

Fix a bug with POST based identity request using batched-xhr. In Pull Request (#21107), I mistakenly removed the crossorigin attribute check in the batchFetchJsonFor in favor of checking for the identity token. See this discussion.

Expected behavior: Adding the crossorigin="amp-viewer-auth-token-via-post" attribute to amp-state, amp-list & amp-form should transform the request to POST and attach the viewer identity token.

Current behavior: Without an identity token, amp-state and amp-list will send a GET request regardless of the presence of the crossorigin attribute.

Proposed solution: When the crossorigin attribute is present on the remote xhr extension, the request should always be transformed into a POST request. Without an identity token, an empty token will be sent instead.

Additional Clean Up

• Added tests for getViewerAuthTokenIfAvailable in the new xhr-utils tests.
• Removed the window argument from getViewerAuthTokenIfAvailable as requested here.

cc @choumx

[dreamofabear]

Ah, sorry didn't realize the full implications of that comment. Thanks for the fix!

[dreamofabear]

An alternative fix is to resolve with empty string instead of undefined, and then change the check in batchFetchJsonFor to compare against undefined instead of truthy. I.e.

return Services.viewerAssistanceForDocOrNull(element).then(va => {
  userAssert(va, 'crossorigin="amp-viewer-auth-token-post" requires amp-viewer-assistance extension.');
  return va.getIdTokenPromise();
}).then(token => token || '').catch(() => ''); // Return empty string if token is not available.

This preserves the current semantics by keeping the "crossorigin" attribute encapsulated in this helper function. WDYT?

[hellokoji]

I am okay with having the token return an empty string instead of undefined without a token, but it's important to still transform the request without a token based on the presence of the crossorigin attribute -- not the identity token.

If I'm understanding your proposal correctly, the token would only be undefined in cases where getViewerAuthTokenIfAvailable isn't called at all. However, getViewerAuthTokenIfAvailable will always be called in amp-state and amp-list before being passed to the batched-json call, so it will always transform the request into a POST because the opt_token !== undefined.

[dreamofabear]

Here's a clearer snippet of what I'm suggesting:

if (crossOriginAttr 
    && crossOriginAttr.trim() === 'amp-viewer-auth-token-via-post') {
  return Services.viewerAssistanceForDocOrNull(element).then(va => {
    userAssert(va, 'crossorigin="amp-viewer-auth-token-post" '
        + 'requires amp-viewer-assistance extension.');
    return va.getIdTokenPromise();
  })
  // If crossorigin attribute is present, resolve with token or empty string.
  .then(token => token || '').catch(() => ''); 
}
// If crossorigin attribute is missing, always resolve with undefined.
return Promise.resolve(undefined);

Would that work?

[hellokoji]

Ah yes, that makes sense. This solution should work! Thanks.

[hellokoji]

Just made the changes and added some tests. Thanks for the suggestion!

[dreamofabear]

Looks great.