Expose login url building to amp access service

[trodrigues]

@dvoytenko this is the PR I mentioned in the email I sent you recently.

This is also mostly an experiment for now just to check if this is an approach we could take. I'll fix docs/tests later on.

The reasoning for this is the following: our "login urls" are retrieved from our authorization endpoint response.

All the information necessary for those login urls (including the return url) is baked into them by our internal service (I believe I had explained how we build those urls when we built the initial implementation of amp-access-laterpay and it's very likely you don't remember, so I can provide a better explanation of this if you'd like).

For that reason, we need to send the RETURN_URL at the time of contacting the authorization endpoint.

The code changes in this PR are able to achieve that.

Do you think this is an acceptable approach? Do you see any reason as to why we shouldn't do this and take a different approach?

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[googlebot]

CLAs look good, thanks!

[trodrigues]

Pushed again with a different email in the commit to be recognized by the CLA bot.

[dvoytenko]

Hi! All looks good. One question though.

[dvoytenko]

Are you sure we need to remove it? Usually this is a somewhat important piece of data, e.g. when more than one origin is supported by the same authorization services.

[trodrigues]

Oh, I only removed this because AMP gives me a deprecation warning:

I looked for info on this ampCors property but couldn't find anything. So then I looked for a similar similar call on amp-access and just followed the same pattern so I just assumed this was the default now.

[dvoytenko]

Got it. DIdn't know it was deprecated, but makes sense.

[trodrigues]

@dvoytenko I was looking into this again and I thought I'd ask about this comment, in the buildLoginUrl function:

  // RETURN_URL has to arrive here unreplaced by UrlReplacements for two
  // reasons: (1) sync replacement and (2) if we need to propagate this
  // replacement to the viewer.

Would the code changes I made here be a problem for this? As in, if we don't expose a RETURN_URL to the viewer, could that be problematic in some situations?

Part of the reason why I came across this again is because I was looking into a way of not having buildLoginUrl append the return parameter again, because we had already replaced it before. I haven't yet found a good clean way of doing this.

[trodrigues]

So I think I've found a way to prevent that additional return parameter from being appended and pushed a possible solution for this.

[trodrigues]

I fixed the tests locally but I don't entirely understand the reason why they are failing in CI. I didn't make any config related changes (which is what the error message is about).

[jridgewell]

You'll need to rebase off of latest master to fix Travis.

[dvoytenko]

You can't have non-default params after default. You have to move it up. Could you, though, explain why don't you need return URL?

[trodrigues]

Sure! So going back a bit to the details of how our backend service works:

• we first send a request to our authorization endpoint with the CANONICAL_URL and READER_ID (and now, RETURN_URL too)
• our authorization endpoint returns the "login urls" which in our case are essentially our purchase URLs for the different options (single article, time passes, subscriptions, etc). Here's an example of one from our development environment:

https://web.dev.laterpaytest.net/dialog/add?
  use_dialog_api=False&
  title=Lorem+Ipsum&
  cp=laterpay&
  pricing=EUR145&
  url=http%3A%2F%2Flocalhost%3A8081%2Fexamples%2Farticle-access-laterpay.html&
  ts=1495534548&
  return_url=http%3A%2F%2Flocalhost%3A8081%2Fextensions%2Famp-access%2F0.1%2Famp-login-done.html%3Furl%3Dhttp%3A%2F%2Flocalhost%3A8081%2Fexamples%2Farticle-access-laterpay.amp.html%23success%3Dtrue&
  article_id=premiumcontent%3A9aa68278-173a-48ec-be04-6b135654bfa9&
  muid=amp-Irryr4n4MTYt8tgAClLDre8_g_LzB5_7nTx9Q8sWUPSeuoAb-rJ6UkMfuk_I6kI0&
  hmac=5243e6e9f6bec6b08151c7d3efe37f7844159e2d840e4b435b2281b6

• Those urls already have the return URL baked in. That's why we need to send it on our initial authorization request.
• Because those urls are signed, if anything is changed in them, they won't be recognized as a valid URL by our payment dialog system

This is why we need to make sure that additional stuff isn't being added to the URL after we get it back from the authorization endpoint.

Does that make sense?

Also this was my suggestion of how to handle this particular issue but we're definitely open to suggestions of a better way of handling this problem (I'll be honest, I don't particularly like the way I found of handling this but it was the best I found without having to duplicate too much of the existing code on our component).

[dvoytenko]

I see. Here's how this now works. Let's see if it's possible at all to reconcile.

1. We try to open login dialog ourselves as a popup. Return URL points to the "login-done" page that messages back to the parent page to refresh.
2. (1) is an ideal scenario, but, in many cases, that doesn't work - the browser cannot or would not open a popup. E.g. a WebView or CCT. Instead, the browser would simply top-navigate to the login dialog. In this case, our "login-done" page further redirects back to the original page.
3. We allow a parent web or native app to assist to show the login dialog. This is somewhat common for a webview. What happens in this case, we message back to the parent app with loginUrl?return=RETURN_URL. The parent app substitutes RETURN_URL to whatever it can work with. It'd often be something like my-native-app-protocol:some-return-code. Then everything works as before and the native app then messages back success/error code to the AMP runtime.

All three cases are completely equivalent from the point of view of the login dialog. But obviously, given the "signing" needs on your side that's not the case.

I see two ways forward:

a. You could whitelist ?return=RETURN_URL query parameter to be excluded from signing. E.g. we could always force it at the end of the URL and you'd chop it off before verifying signature. That would, of course, work if you don't see any security issue with the return redirect, which in turn would normally depend on what you pass back. But if you only pass back #success or such - probably not a security problem?
b. So far, I believe 1-2-3 above are simply variations on trying to do best effort to show login dialog better. We could force the login for you into options 1 or 2. But I need to check this. This might have been added for cases where launching login dialog via parent app was required for some purpose, other than just "enhancement".

What do you think about the option (a)?

[trodrigues]

Ok, so after the discussion we had on the other comments (which I now can't really find probably because I force pushed the branch over those changes), we managed to get our purchase dialog urls to ignore that extra return parameter, and I removed that second commit with the noReturnUrl change.

I've cleaned up the tests and type hints on the first commit and made a couple of other fixes.

Once this is merged I have another PR with a few other smaller unrelated fixes for the component, and once our backend fixes are all in production and we've made a proper test, if there's no objection I'll make a PR to remove the experimental flag.

[dvoytenko]

Just one request, but otherwise LGTM

[dvoytenko]

Could you please create an internal function and use it both here and in openLoginDialog. It'd look something like this:

/**
 * @param ...
 * @return {!WebLoginDialog|!ViewerLoginDialog}
 */
function createLoginDialog(ampdoc, urlOrPromise) {
  const viewer = viewerForDoc(ampdoc);
  const overrideDialog = parseInt(viewer.getParam('dialog'), 10);
  if (overrideDialog) {
    return new ViewerLoginDialog(viewer, urlOrPromise);
  }
  return new WebLoginDialog(ampdoc.win, viewer, urlOrPromise);
}

export function openLoginDialog(ampdoc, urlOrPromise) {
  return createLoginDialog(ampdoc, urlOrPromise).open();
}
export function getLoginUrl(ampdoc, urlOrPromise) {
  return createLoginDialog(ampdoc, urlOrPromise).getLoginUrl();
}

[trodrigues]

@dvoytenko made the requested fix. Also created a separate PR with some other smaller fixes I had worked on in the meanwhile: #9633