allow firebase for 3p frames #25092

cathyxz · 2019-10-17T01:49:14Z

Added a fortesting option to gulp firebase to enable testing ads on custom domains.

Legacy description

The following are outdated descriptions of this PR.

Take 2

~~This PR does the following:~~
~~1. Re-adds integration.js and f.js to the list of files that we prepend AMP_CONFIG to.~~
~~2. Adds code in firebase.js to add firebase to the list of allowed 3rd party urls and 3rd party frame regexes so that ads can work on firebase.~~

Filed:

PR Deploy Bot doesn't work with ads #25104 to @estherkim for fixing ads testing in PR deploy bot
Integration.js refactoring for local and demo-based testing #25108 to @lannka for integration.js refactoring and any work to make sure that deployed demos still work with ads.

Take 1

~~I wanted to make testing amp-ad work for firebase deployments. They're currently failing here:~~

amphtml/3p/integration.js

Lines 719 to 740 in 3e6457f

    
           export function validateAllowedTypes(window, type, allowedTypes) { 
        
             const thirdPartyHost = parseUrlDeprecated(urls.thirdParty).hostname; 
        
             // Everything allowed in default iframe. 
        
             if (window.location.hostname == thirdPartyHost) { 
        
               return; 
        
             } 
        
             if (urls.thirdPartyFrameRegex.test(window.location.hostname)) { 
        
               return; 
        
             } 
        
             if (window.location.hostname == 'ads.localhost') { 
        
               return; 
        
             } 
        
             if (defaultAllowedTypesInCustomFrame.indexOf(type) != -1) { 
        
               return; 
        
             } 
        
             userAssert( 
        
               allowedTypes && allowedTypes.indexOf(type) != -1, 
        
               'Non-whitelisted 3p type for custom iframe: %s', 
        
               type 
        
             ); 
        
           }

~~The example that I'm using is examples/amp-ads.amp.html.~~

We currently have a section in firebase.js that appends "thirdPartyUrl":location.origin to AMP_CONFIG. My initial approach here was just to add another modification that would append "thirdPartyFrameRegex": "^.*\.firebaseapp\.com$", which would enable it to pass this test:

amphtml/3p/integration.js

Line 726 in 3e6457f

if (urls.thirdPartyFrameRegex.test(window.location.hostname)) {

However, by inspecting the output of urls in integration.js and urls in config.js I'm noticing a race condition. I'm observing that sometimes the urls object reflects the contents of the AMP_CONFIG appended to amp.js, and sometimes it does not (no localDev: "true", no thirdPartyUrl, no thirdPartyFrameRegex). It seems like integration.js is trying to access urls before config.js has had a chance to read from the AMP_CONFIG object. I'm wondering if there's a race condition somewhere.

~~I also noted that in enableLocalDev, there's the option to specify a TEST_HOST with env.AMP_TEST_HOST. Is there a documented way to use this to enable ads on a demo url?~~

amphtml/build-system/tasks/prepend-global/index.js

Lines 181 to 205 in f515760

    
           function enableLocalDev(config, target, configJson) { 
        
             let LOCAL_DEV_AMP_CONFIG = {localDev: true}; 
        
             const TESTING_HOST = process.env.AMP_TESTING_HOST; 
        
             if (typeof TESTING_HOST == 'string') { 
        
               const TESTING_HOST_FULL_URL = TESTING_HOST.match(/^https?:\/\//) 
        
                 ? TESTING_HOST 
        
                 : 'http://' + TESTING_HOST; 
        
               const TESTING_HOST_NO_PROTOCOL = TESTING_HOST.replace(/^https?:\/\//, ''); 
        
               LOCAL_DEV_AMP_CONFIG = Object.assign(LOCAL_DEV_AMP_CONFIG, { 
        
                 thirdPartyUrl: TESTING_HOST_FULL_URL, 
        
                 thirdPartyFrameHost: TESTING_HOST_NO_PROTOCOL, 
        
                 thirdPartyFrameRegex: TESTING_HOST_NO_PROTOCOL, 
        
               }); 
        
               log( 
        
                 'Set', 
        
                 cyan('TESTING_HOST'), 
        
                 'to', 
        
                 cyan(TESTING_HOST), 
        
                 'in', 
        
                 cyan(target) 
        
               ); 
        
             } 
        
             return Object.assign(LOCAL_DEV_AMP_CONFIG, configJson); 
        
           }

cathyxz · 2019-10-17T01:51:06Z

Hi @rsimha and @lannka ! Apologies for the unconventional way of filing what is actually a feature request. I tried to make ads work on firebase demos by prepending a thirdPartyFrameRegex to AMP_CONFIG, but this isn't working. Do you have any idea why, or what alternative approaches I could perhaps look into?

rsimha · 2019-10-17T18:15:24Z

Hi @rsimha and @lannka ! Apologies for the unconventional way of filing what is actually a feature request.

Here's a high level question: What does gulp firebase do that the PR deploy solution introduced recently by @estherkim dos not do? I recognize that there may be good reasons for both solutions to exist, but it'd be good to eliminate duplication if they are functionally equivalent.

I tried to make ads work on firebase demos by prepending a thirdPartyFrameRegex to AMP_CONFIG, but this isn't working. Do you have any idea why, or what alternative approaches I could perhaps look into?

I think this is due to #24471, where @lannka removed the code that automatically adds AMP_CONFIG to integration.js (unminified) / f.js (minified). IIRC, adding the config to the 3p frame and the main runtime binary was undesirable. Perhaps it needs to be reinstated to support your use case?

cathyxz · 2019-10-17T18:26:46Z

I forgot about the PR deploy bot! Maybe we should update our documentation in TESTING.md to make the PR Deploy Bot more discoverable, so we can start deprecating the firebase demo deployment once it's gotten more usage across the board. We can start by adding a line referencing it under the Firebase documentation encouraging people to try it out instead.

@leafsy is currently using this for iterating on a bug bash. I guess the reality of our current use case is that we may push a lot of small updates to a demo, but not wish to have spam an in Review PR with a lot of small commits and force-pushes. If we start using PR deploy bot for this use case, our repo might begin to have a lot of demo Draft PRs that are not intended for merge.

I tried the PR Deploy Bot on this PR, and it seems to suffer from a different issue for ads. None of the ads display on: https://storage.googleapis.com/amp-test-website-1/amp_dist_65323/examples/ads.amp.html
@estherkim do you know what we'd need to do to make ads work on PR deploy bot?

cathyxz · 2019-10-17T20:57:14Z

build-system/tasks/helpers.js

@@ -75,6 +75,7 @@ const UNMINIFIED_TARGETS = [
  'amp-inabox.js',
  'amp-shadow.js',
  'amp.js',
+  'integration.js',


This is a partial revert of #24471. I'm not sure why we removed it initially, but per offline discussion with @lannka, this seems ok to reinstate. Filed #25108 to track any follow up refactoring.

@cathyxz / @lannka: Are we sure we don't want to add this for minified builds as well for consistency between gulp build and gulp dist? (The minified equivalent is f.js.)

I ask because several of our tests are able to switch between --compiled mode and regular mode, and I'm concerned that this might break some assumptions in code (not sure what they might be).

Got it. I'll add in f.js for consistency.

This list is now different from the one used by our internal release scripts. (See cl/269449844.)

@lannka @erwinmombay @jridgewell @danielrozenberg Do we want to change that list too? (I don't fully understand how AMP_CONFIG is used by the 3p frame.)

cathyxz · 2019-10-17T21:21:03Z

Updated the PR description. Could @estherkim, @rsimha and @lannka please take a look?

rsimha

Couple comments. I'm approving to unblock.

rsimha · 2019-10-17T21:25:29Z

build-system/tasks/firebase.js

  const result = data.replace(
    'self.AMP_CONFIG={',
-    'self.AMP_CONFIG={"thirdPartyUrl":location.origin,'
+    // eslint-disable-next-line prettier/prettier
+    'self.AMP_CONFIG={"thirdPartyUrl":location.origin,"thirdPartyFrameRegex": "^.*\.firebaseapp\.com$",'


I believe you need a double-backslash here for a single-backslash to appear in the AMP_CONFIG that's written to file.

'self.AMP_CONFIG={"thirdPartyUrl":location.origin,"thirdPartyFrameRegex": "^.*\\.firebaseapp\\.com$",'

If that works, the eslint-disable-next-line prettier/prettier in the line above will no longer be necessary.

Based on a new approach, I've actually removed this code altogether. Let me know if this approach works better.

rsimha · 2019-10-17T21:28:22Z

build-system/tasks/helpers.js

@@ -75,6 +75,7 @@ const UNMINIFIED_TARGETS = [
  'amp-inabox.js',
  'amp-shadow.js',
  'amp.js',
+  'integration.js',


This list is now different from the one used by our internal release scripts. (See cl/269449844.)

@lannka @erwinmombay @jridgewell @danielrozenberg Do we want to change that list too? (I don't fully understand how AMP_CONFIG is used by the 3p frame.)

cathyxz · 2019-10-17T23:03:15Z

After some more digging, I found a way that does not need us to modify the prepend targets in our build process. We can just call enableLocalTesting from firebase.js. I've added code and documentation to that effect. @rsimha @lannka could you take a look? I think this might be a better approach that won't have these side effects.

rsimha

I think this might be a better approach that won't have these side effects.

It certainly is! LGTM! (Can you update the description to reflect the new approach?)

cathyxz · 2019-10-18T20:43:24Z

I think Owners Bot requires @mrjoro to approve the documentation changes. PTAL?

cathyxz · 2019-10-18T20:48:44Z

Updated the PR description and crossed out all the legacy stuff. Pending an LGTM from @mrjoro for documentation changes.

mrjoro

Doc changed look good!

cathyxz · 2019-10-18T22:47:43Z

Thank you!

* allow firebase for 3p frames * Prepend AMP_CONFIG to integration.js * Add comments * Add f.js for consistency * localize changes to firebase only * Wait for async function * Add documentation

googlebot added the cla: yes label Oct 17, 2019

amp-owners-bot bot requested a review from rcebulko October 17, 2019 01:49

cathyxz requested a review from rsimha October 17, 2019 01:49

cathyxz requested a review from lannka October 17, 2019 01:51

rsimha requested a review from estherkim October 17, 2019 18:18

cathyxz mentioned this pull request Oct 17, 2019

PR Deploy Bot doesn't work with ads #25104

Closed

cathyxz added 2 commits October 17, 2019 13:49

allow firebase for 3p frames

4b6e4a2

Prepend AMP_CONFIG to integration.js

4f7e6f4

cathyxz mentioned this pull request Oct 17, 2019

Integration.js refactoring for local and demo-based testing #25108

Closed

cathyxz force-pushed the firebase/3pframe branch from 9a7e034 to 4f7e6f4 Compare October 17, 2019 20:54

cathyxz commented Oct 17, 2019

View reviewed changes

cathyxz added 2 commits October 17, 2019 14:00

Add comments

5c8de1b

Add f.js for consistency

9626496

cathyxz changed the title ~~[DO NOT MERGE] allow firebase for 3p frames~~ allow firebase for 3p frames Oct 17, 2019

rsimha approved these changes Oct 17, 2019

View reviewed changes

rcebulko removed their request for review October 17, 2019 21:35

cathyxz added 3 commits October 17, 2019 15:46

localize changes to firebase only

3c91d59

Wait for async function

36e3b3d

Add documentation

af09516

rsimha approved these changes Oct 18, 2019

View reviewed changes

cathyxz requested a review from mrjoro October 18, 2019 20:43

mrjoro approved these changes Oct 18, 2019

View reviewed changes

cathyxz merged commit 4d7b2dd into ampproject:master Oct 18, 2019

cathyxz deleted the firebase/3pframe branch October 18, 2019 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow firebase for 3p frames #25092

allow firebase for 3p frames #25092

cathyxz commented Oct 17, 2019 •

edited

cathyxz commented Oct 17, 2019 •

edited

rsimha commented Oct 17, 2019 •

edited

cathyxz commented Oct 17, 2019

cathyxz Oct 17, 2019

rsimha Oct 17, 2019 •

edited

cathyxz Oct 17, 2019

rsimha Oct 17, 2019

cathyxz commented Oct 17, 2019

rsimha left a comment

rsimha Oct 17, 2019 •

edited

cathyxz Oct 17, 2019

rsimha Oct 17, 2019

cathyxz commented Oct 17, 2019

rsimha left a comment •

edited

cathyxz commented Oct 18, 2019

cathyxz commented Oct 18, 2019

mrjoro left a comment

cathyxz commented Oct 18, 2019

	export function validateAllowedTypes(window, type, allowedTypes) {
	const thirdPartyHost = parseUrlDeprecated(urls.thirdParty).hostname;

	// Everything allowed in default iframe.
	if (window.location.hostname == thirdPartyHost) {
	return;
	}
	if (urls.thirdPartyFrameRegex.test(window.location.hostname)) {
	return;
	}
	if (window.location.hostname == 'ads.localhost') {
	return;
	}
	if (defaultAllowedTypesInCustomFrame.indexOf(type) != -1) {
	return;
	}
	userAssert(
	allowedTypes && allowedTypes.indexOf(type) != -1,
	'Non-whitelisted 3p type for custom iframe: %s',
	type
	);
	}

	function enableLocalDev(config, target, configJson) {
	let LOCAL_DEV_AMP_CONFIG = {localDev: true};
	const TESTING_HOST = process.env.AMP_TESTING_HOST;
	if (typeof TESTING_HOST == 'string') {
	const TESTING_HOST_FULL_URL = TESTING_HOST.match(/^https?:\/\//)
	? TESTING_HOST
	: 'http://' + TESTING_HOST;
	const TESTING_HOST_NO_PROTOCOL = TESTING_HOST.replace(/^https?:\/\//, '');

	LOCAL_DEV_AMP_CONFIG = Object.assign(LOCAL_DEV_AMP_CONFIG, {
	thirdPartyUrl: TESTING_HOST_FULL_URL,
	thirdPartyFrameHost: TESTING_HOST_NO_PROTOCOL,
	thirdPartyFrameRegex: TESTING_HOST_NO_PROTOCOL,
	});
	log(
	'Set',
	cyan('TESTING_HOST'),
	'to',
	cyan(TESTING_HOST),
	'in',
	cyan(target)
	);
	}
	return Object.assign(LOCAL_DEV_AMP_CONFIG, configJson);
	}

allow firebase for 3p frames #25092

allow firebase for 3p frames #25092

Conversation

cathyxz commented Oct 17, 2019 • edited

Legacy description

Take 2

Take 1

cathyxz commented Oct 17, 2019 • edited

rsimha commented Oct 17, 2019 • edited

cathyxz commented Oct 17, 2019

cathyxz Oct 17, 2019

Choose a reason for hiding this comment

rsimha Oct 17, 2019 • edited

Choose a reason for hiding this comment

cathyxz Oct 17, 2019

Choose a reason for hiding this comment

rsimha Oct 17, 2019

Choose a reason for hiding this comment

cathyxz commented Oct 17, 2019

rsimha left a comment

Choose a reason for hiding this comment

rsimha Oct 17, 2019 • edited

Choose a reason for hiding this comment

cathyxz Oct 17, 2019

Choose a reason for hiding this comment

rsimha Oct 17, 2019

Choose a reason for hiding this comment

cathyxz commented Oct 17, 2019

rsimha left a comment • edited

Choose a reason for hiding this comment

cathyxz commented Oct 18, 2019

cathyxz commented Oct 18, 2019

mrjoro left a comment

Choose a reason for hiding this comment

cathyxz commented Oct 18, 2019

cathyxz commented Oct 17, 2019 •

edited

cathyxz commented Oct 17, 2019 •

edited

rsimha commented Oct 17, 2019 •

edited

rsimha Oct 17, 2019 •

edited

rsimha Oct 17, 2019 •

edited

rsimha left a comment •

edited