From 59a9f277ba8ea888e3deaa2134af588fcf8c5c9b Mon Sep 17 00:00:00 2001
From: Dima Voytenko <dvoytenko@google.com>
Date: Fri, 15 Jan 2016 17:34:49 -0800
Subject: [PATCH] Add AUTH var substitutions to pingback/login URLs.

---
 extensions/amp-access/0.1/amp-access.js       | 40 +++++++++++++++----
 .../amp-access/0.1/test/test-amp-access.js    | 35 ++++++++++++----
 extensions/amp-access/amp-access.md           | 13 ++++--
 src/url-replacements.js                       |  4 +-
 test/functional/test-url-replacements.js      | 40 +++++++++++++++++++
 5 files changed, 110 insertions(+), 22 deletions(-)

diff --git a/extensions/amp-access/0.1/amp-access.js b/extensions/amp-access/0.1/amp-access.js
index 5801da62b247..b46457bbf452 100644
--- a/extensions/amp-access/0.1/amp-access.js
+++ b/extensions/amp-access/0.1/amp-access.js
@@ -144,6 +144,9 @@ export class AccessService {
     /** @private {?Promise<string>} */
     this.readerIdPromise_ = null;
 
+    /** @private {?JSONObject} */
+    this.authResponse_ = null;
+
     /** @private {!Promise} */
     this.firstAuthorizationPromise_ = new Promise(resolve => {
       /** @private {!Promise} */
@@ -277,14 +280,24 @@ export class AccessService {
 
   /**
    * @param {string} url
+   * @param {boolean} useAuthData Allows `AUTH(field)` URL var substitutions.
    * @return {!Promise<string>}
    * @private
    */
-  buildUrl_(url) {
+  buildUrl_(url, useAuthData) {
     return this.getReaderId_().then(readerId => {
-      return this.urlReplacements_.expand(url, {
+      const vars = {
         'READER_ID': readerId
-      });
+      };
+      if (useAuthData) {
+        vars['AUTHDATA'] = field => {
+          if (this.authResponse_) {
+            return this.authResponse_[field];
+          }
+          return undefined;
+        };
+      }
+      return this.urlReplacements_.expand(url, vars);
     });
   }
 
@@ -301,12 +314,14 @@ export class AccessService {
 
     log.fine(TAG, 'Start authorization via ', this.config_.authorization);
     this.toggleTopClass_('amp-access-loading', true);
-    return this.buildUrl_(this.config_.authorization).then(url => {
+    const promise = this.buildUrl_(
+        this.config_.authorization, /* useAuthData */ false);
+    return promise.then(url => {
       log.fine(TAG, 'Authorization URL: ', url);
       return this.xhr_.fetchJson(url, {credentials: 'include'});
     }).then(response => {
       log.fine(TAG, 'Authorization response: ', response);
-      this.firstAuthorizationResolver_();
+      this.setAuthResponse_(response);
       this.toggleTopClass_('amp-access-loading', false);
       return new Promise((resolve, reject) => {
         onDocumentReady(this.win.document, () => {
@@ -319,6 +334,15 @@ export class AccessService {
     });
   }
 
+  /**
+   * @param {!JSONObject} authResponse
+   * @private
+   */
+  setAuthResponse_(authResponse) {
+    this.authResponse_ = authResponse;
+    this.firstAuthorizationResolver_();
+  }
+
   /**
    * @param {!JSONObjectDef} response
    * @return {!Promise}
@@ -515,7 +539,9 @@ export class AccessService {
       log.fine(TAG, 'Ignore pingback');
       return Promise.resolve();
     }
-    return this.buildUrl_(this.config_.pingback).then(url => {
+    const promise = this.buildUrl_(
+        this.config_.pingback, /* useAuthData */ true);
+    return promise.then(url => {
       log.fine(TAG, 'Pingback URL: ', url);
       return this.xhr_.sendSignal(url, {
         method: 'POST',
@@ -567,7 +593,7 @@ export class AccessService {
 
     log.fine(TAG, 'Start login');
     const urlPromise = this.buildUrl_(assert(this.config_.login,
-        'Login URL is not configured'));
+        'Login URL is not configured'), /* useAuthData */ true);
     this.loginPromise_ = this.openLoginDialog_(urlPromise).then(result => {
       log.fine(TAG, 'Login dialog completed: ', result);
       this.loginPromise_ = null;
diff --git a/extensions/amp-access/0.1/test/test-amp-access.js b/extensions/amp-access/0.1/test/test-amp-access.js
index 27be63ae95cb..e973f3e055ea 100644
--- a/extensions/amp-access/0.1/test/test-amp-access.js
+++ b/extensions/amp-access/0.1/test/test-amp-access.js
@@ -298,6 +298,8 @@ describe('AccessService authorization', () => {
       expect(document.documentElement).not.to.have.class('amp-access-loading');
       expect(elementOn).not.to.have.attribute('amp-access-hide');
       expect(elementOff).to.have.attribute('amp-access-hide');
+      expect(service.authResponse_).to.exist;
+      expect(service.authResponse_.access).to.be.true;
     });
   });
 
@@ -536,7 +538,7 @@ describe('AccessService pingback', () => {
     configElement.setAttribute('type', 'application/json');
     configElement.textContent = JSON.stringify({
       'authorization': 'https://acme.com/a?rid=READER_ID',
-      'pingback': 'https://acme.com/p?rid=READER_ID',
+      'pingback': 'https://acme.com/p?rid=READER_ID&type=AUTHDATA(type)',
       'login': 'https://acme.com/l?rid=READER_ID'
     });
     document.body.appendChild(configElement);
@@ -724,13 +726,30 @@ describe('AccessService pingback', () => {
   it('should send POST pingback', () => {
     expectGetReaderId('reader1');
     xhrMock.expects('sendSignal')
-        .withExactArgs('https://acme.com/p?rid=reader1', sinon.match(init => {
-          return (init.method == 'POST' &&
-              init.credentials == 'include' &&
-              init.body == '' &&
-              init.headers['Content-Type'] ==
-                  'application/x-www-form-urlencoded');
-        }))
+        .withExactArgs('https://acme.com/p?rid=reader1&type=',
+            sinon.match(init => {
+              return (init.method == 'POST' &&
+                  init.credentials == 'include' &&
+                  init.body == '' &&
+                  init.headers['Content-Type'] ==
+                      'application/x-www-form-urlencoded');
+            }))
+        .returns(Promise.resolve())
+        .once();
+    return service.reportViewToServer_().then(() => {
+      return 'SUCCESS';
+    }, error => {
+      return 'ERROR ' + error;
+    }).then(result => {
+      expect(result).to.equal('SUCCESS');
+    });
+  });
+
+  it('should resolve AUTH vars in POST pingback', () => {
+    expectGetReaderId('reader1');
+    service.setAuthResponse_({type: 'premium'});
+    xhrMock.expects('sendSignal')
+        .withArgs('https://acme.com/p?rid=reader1&type=premium')
         .returns(Promise.resolve())
         .once();
     return service.reportViewToServer_().then(() => {
diff --git a/extensions/amp-access/amp-access.md b/extensions/amp-access/amp-access.md
index c9583b334a31..7fec9bebc6fc 100644
--- a/extensions/amp-access/amp-access.md
+++ b/extensions/amp-access/amp-access.md
@@ -101,7 +101,7 @@ Here’s an example of the AMP Access configuration:
 ```html
 <script id="amp-access" type="application/json">
 {
-  "authorization": 
+  "authorization":
       "https://pub.com/amp-access?rid={READER_ID}&url={AMPDOC_URL}",
   "pingback":
       "https://pub.com/amp-ping?rid={READER_ID}&url={AMPDOC_URL}",
@@ -117,6 +117,7 @@ When configuring the URLs for various endpoints, the Publisher can use substitut
 Var               | Description
 ----------------- | -----------
 READER_ID         | The AMP Reader ID.
+AUTHDATA(field)   | The value of the field in the authorization response.
 AMPDOC_URL        | The URL of this AMP Document.
 CANONICAL_URL     | The canonical URL of this AMP Document.
 DOCUMENT_REFERRER | The Referrer URL.
@@ -131,9 +132,13 @@ https://pub.com/access?
   &ref={DOCUMENT_REFERRER}
   &_={RANDOM}
 ```
+
+AUTHDATA variable is availbale to Pingback and Login URLs. It allows passing any field in the authorization
+response as an URL parameter. E.g. `AUTHDATA(isSubscriber)`.
+
 ##Access Content Markup
 
-Access Content Markup describes which sections are visible or hidden. It is comprised of two AMP attributes: ```amp-access``` and ```amp-access-hide``` that can be placed on any HTML element. 
+Access Content Markup describes which sections are visible or hidden. It is comprised of two AMP attributes: ```amp-access``` and ```amp-access-hide``` that can be placed on any HTML element.
 
 The ```amp-access``` attribute provides the expression that yields true or false based on the authorization response returned by the Authorization endpoint. The resulting value indicates whether or not the element and its contents are visible.
 
@@ -252,7 +257,7 @@ The publisher may choose to use the pingback as:
  - One of the main purposes for pingback is to count down meter when it is used.
  - As a credentialed CORS endpoint it may contain publisher cookies. Thus it can be used to map AMP Reader ID to the Publisher’s identity.
 
-The request format is:  
+The request format is:
 ```
 https://publisher.com/amp-pingback?
    rid={READER_ID}
@@ -323,7 +328,7 @@ PROPERTY ::= [a-zA-Z][a-zA-Z0-9\_\-]*
 VALUE ::= QUOTE [a-zA-Z0-9\_\-]* QUOTE
 ```
 
-Notice that ```amp-access``` expressions are evaluated by the AMP Runtime and AMP Cache. This is NOT part of the specification that the Publisher needs to implement. It is here simply for informational properties. 
+Notice that ```amp-access``` expressions are evaluated by the AMP Runtime and AMP Cache. This is NOT part of the specification that the Publisher needs to implement. It is here simply for informational properties.
 
 #Detailed Discussion
 This section will cover a detailed explanation of the design underlying the amp-access spec, and clarify design choices. Coming soon
diff --git a/src/url-replacements.js b/src/url-replacements.js
index aca843b6745f..d67b25df99f4 100644
--- a/src/url-replacements.js
+++ b/src/url-replacements.js
@@ -172,9 +172,7 @@ class UrlReplacements {
     const expr = this.getExpr_(opt_bindings);
     let replacementPromise;
     const encodeValue = val => {
-      // Value 0 is specialcased because the numeric 0 is a valid substitution
-      // value.
-      if (!val && val !== 0) {
+      if (val == null) {
         val = '';
       }
       return encodeURIComponent(val);
diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js
index 06d68a7bc259..c0f85f8d1459 100644
--- a/test/functional/test-url-replacements.js
+++ b/test/functional/test-url-replacements.js
@@ -239,4 +239,44 @@ describe('UrlReplacements', () => {
       expect(res).to.match(/rid=func_abc\?$/);
     });
   });
+
+  it('should expand null as empty string', () => {
+    return expand('v=VALUE', false, {
+      'VALUE': null
+    }).then(res => {
+      expect(res).to.equal('v=');
+    });
+  });
+
+  it('should expand undefined as empty string', () => {
+    return expand('v=VALUE', false, {
+      'VALUE': undefined
+    }).then(res => {
+      expect(res).to.equal('v=');
+    });
+  });
+
+  it('should expand empty string as empty string', () => {
+    return expand('v=VALUE', false, {
+      'VALUE': ''
+    }).then(res => {
+      expect(res).to.equal('v=');
+    });
+  });
+
+  it('should expand zero as zero', () => {
+    return expand('v=VALUE', false, {
+      'VALUE': 0
+    }).then(res => {
+      expect(res).to.equal('v=0');
+    });
+  });
+
+  it('should expand false as false', () => {
+    return expand('v=VALUE', false, {
+      'VALUE': false
+    }).then(res => {
+      expect(res).to.equal('v=false');
+    });
+  });
 });