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

🐛 Post-based identity bug fix #21533

Merged
merged 9 commits into from
Apr 2, 2019
Merged

🐛 Post-based identity bug fix #21533

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
401bbfc
Force request POST transform with presence of crossorigin attribute.
hellokoji Mar 21, 2019
9ce46e5
Remove unecessary win argument from xhrUtil function
hellokoji Mar 21, 2019
0749f83
add getViewerAuthTokenIfAvailable tests to test-xhr-utils
hellokoji Mar 21, 2019
c3fd7b3
remove previous identity token test
hellokoji Mar 21, 2019
40cfdf5
Restrict crossorigin attribute check to xhrUtils.
hellokoji Mar 25, 2019
7b06f03
space
hellokoji Mar 25, 2019
2e88d73
fix types
hellokoji Mar 25, 2019
6dd2736
Merge remote-tracking branch 'upstream/master' into POST-identity-bug…
hellokoji Mar 27, 2019
aafe8c4
Address nit PR comments
hellokoji Apr 1, 2019
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
7 changes: 2 additions & 5 deletions src/batched-json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,13 @@ export function batchFetchJsonFor(
const xhr = Services.batchedXhrFor(ampdoc.win);
return requestForBatchFetch(element, opt_urlReplacement, opt_refresh)
.then(data => {
const crossOriginAttr = element.getAttribute('cross-origin') ||
element.getAttribute('crossorigin');
if (crossOriginAttr &&
crossOriginAttr.trim() === 'amp-viewer-auth-token-via-post') {
if (opt_token !== undefined) {
data.fetchOpt['method'] = 'POST';
data.fetchOpt['headers'] = {
'Content-Type': 'application/x-www-form-urlencoded',
};
data.fetchOpt['body'] = {
'ampViewerAuthToken': opt_token || '',
'ampViewerAuthToken': opt_token,
};
}
return xhr.fetchJson(data.xhrUrl, data.fetchOpt);
Expand Down
21 changes: 11 additions & 10 deletions src/utils/xhr-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,15 +420,16 @@ export function assertSuccess(response) {
export function getViewerAuthTokenIfAvailable(element) {
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const crossOriginAttr = element.getAttribute('cross-origin') ||
element.getAttribute('crossorigin');
if (crossOriginAttr &&
crossOriginAttr.trim() === 'amp-viewer-auth-token-via-post') {
return Services.viewerAssistanceForDocOrNull(element)
.then(viewerAssistance => {
userAssert(viewerAssistance,
'crossorigin="amp-viewer-auth-token-post" ' +
'requires amp-viewer-assistance extension.');
return viewerAssistance.getIdTokenPromise();
});
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 attr is present, resolve with token or empty string.
.then(token => token || '').catch(() => '');
}
return Promise.resolve();
// If crossorigin attribute is missing, always resolve with undefined.
return Promise.resolve(undefined);
}
35 changes: 22 additions & 13 deletions test/unit/test-batched-json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,9 @@ describe('batchFetchJsonFor', () => {
});
});

describe('POST based identity with crossorigin attribute', () => {
it('should send POST request with auth token if attribute ' +
'crossorigin=amp-viewer-auth-token-via-post is present', () => {
describe('POST based identity', () => {
it('should send POST request with auth token is present', () => {
const el = element('https://data.com');
el.setAttribute('crossorigin', 'amp-viewer-auth-token-via-post');
const all = UrlReplacementPolicy.ALL;

urlReplacements.expandUrlAsync
Expand All @@ -134,26 +132,37 @@ describe('batchFetchJsonFor', () => {
});
});

it('should send POST request with crossorigin attribute present with no' +
' identity token', () => {
it('should send POST request with empty, defined identity token', () => {
const el = element('https://data.com');
const all = UrlReplacementPolicy.ALL;

urlReplacements.expandUrlAsync
.withArgs('https://data.com')
.returns(Promise.resolve('https://data.com'));

return batchFetchJsonFor(ampdoc, el, null, all, false, '')
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
.then(() => {
const fetchArgs = fetchJson['firstCall']['args'];
expect(fetchArgs[1]['body']['ampViewerAuthToken'])
.to.equal('');
expect(fetchArgs[1]['method'])
.to.equal('POST');
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
});
});

it('should not transform the request with an undefined token', () => {
const el = element('https://data.com');
el.setAttribute('crossorigin', 'amp-viewer-auth-token-via-post');
const all = UrlReplacementPolicy.ALL;

urlReplacements.expandUrlAsync
.withArgs('https://data.com')
.returns(Promise.resolve('https://data.com'));

const expectedRequest = {
'body': {'ampViewerAuthToken': ''},
'headers': {
'Content-Type': 'application/x-www-form-urlencoded',
},
'method': 'POST',
'requireAmpResponseSourceOrigin': false,
};

return batchFetchJsonFor(ampdoc, el, null, all, false, '')
return batchFetchJsonFor(ampdoc, el, null, all, false)
.then(() => {
expect(fetchJson).to.be.calledWithExactly(
'https://data.com', expectedRequest);
Expand Down
25 changes: 25 additions & 0 deletions test/unit/utils/test-xhr-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,31 @@ describes.sandboxed('utils/xhr-utils', {}, env => {
expect(token).to.equal('idToken');
});
});
it('should return an empty auth token if there is not one present', () => {
sandbox.stub(Services, 'viewerAssistanceForDocOrNull').returns(
Promise.resolve({
getIdTokenPromise: (() => Promise.resolve(undefined)),
}));
const el = document.createElement('html');
el.setAttribute('crossorigin', 'amp-viewer-auth-token-via-post');
return getViewerAuthTokenIfAvailable(el)
.then(token => {
expect(token).to.equal('');
});
});
it('should return an empty auth token if there is an issue retrieving ' +
'the identity token', () => {
sandbox.stub(Services, 'viewerAssistanceForDocOrNull').returns(
Promise.reject({
getIdTokenPromise: (() => Promise.reject()),
}));
const el = document.createElement('html');
el.setAttribute('crossorigin', 'amp-viewer-auth-token-via-post');
return getViewerAuthTokenIfAvailable(el)
.then(token => {
expect(token).to.equal('');
});
});
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
it('should assert that amp-viewer-assistance extension is present', () => {
sandbox.stub(Services, 'viewerAssistanceForDocOrNull').returns(
Promise.resolve());
Expand Down