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.

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

Add explicit permission call to allow payment app to handle payments #94

Closed
adamroach opened this issue Jan 23, 2017 · 19 comments
Closed
Milestone

Comments

@adamroach
Copy link
Contributor

In the current document, the request for permission to install an app occurs implicitly as part of the "setManifest()" processing. This is similar to the way permissions are handled in a host of existing web technologies, such as geolocation and getUserMedia.

The more recent pattern for these kinds of permissions requests (e.g., for push notifications) involves an explicit call to request permission before the action that requires permission is performed.

In @marcoscaceres' payment-request-handler doc, he proposes that the paymentAppManager object include an explicit requestPermission() method rather than this being an implicit step in setting up a payment app. I think this is an improvement, and would support adding it to the payment-app specification.

@jakearchibald
Copy link

A pattern more like marcoscaceres/payment-request-handler#2 would be in line with other service worker APIs.

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 27, 2017

Ok, so considering all feedback thus far, I want to avoid asking for permissions on multiple instances (because it leads to redundant code, like):

// Only the first one matters... so second line is just code duplication.
Promise.all([
  sw1.paymentManager.requestPermission(),
  sw2.paymentManager.requestPermission(),
]).then()

Also, I don't know if the app should be allowed to revoke its permission (through some kind on .unregister()). That's been really controversial, and more of less rejected in permission API discussions (I had to pref that off in Gecko).

So how about...

Permission can only be triggered via user interaction:

screenshot 2017-01-27 17 20 55

The user MUST activate the element, in this case a button.

Getting permission

So, "Let's get permission"... but this this time, using PaymentManager.requestPermission() - similarly how Web Notifications does things.

button.addEventListener("click", async () => {
  let permCheck;
  try {
    permCheck = await navigator.permissions.query({ name: "payments" });
  } catch (err) {
    return; // not supported
  }
  const canProceed = await permissionsCheck(permCheck);
  if (!canProceed) {
    return; // we got denied.
  }
  weCanDoStuff(); // Yay!
}, { once: true });

async function permissionsCheck({ state }) {
  switch (state) {
    case "prompt":
      const result = await PaymentManager.requestPermission();
      return await permissionsCheck(Promise.resolve({ state: result }));
    case "granted":
      return true;
    default:
      return false;
  }
}

Proposed IDL

interface PaymentManager {
  static Promise<PermissionState> requestPermission();
};

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 27, 2017

Here is a solution using Permission API that works nicely with multiple tabs - so if one tab disables the permission, the other tab picks it up. The solution assumes a there is a HTMLButtonElement with id "request-permission" on the page.

(async () => {
  let permissionObj;
  try {
    permissionObj = await navigator.permissions.query({ name: "payments" });
  } catch (err) {
    return; // not supported, bail out!
  }
  const button = document.getElementById("request-permission");
  const clickListener = () => {
    window.PaymentManager.requestPermission();
  }
  const changeListener = () => {
    // Monitor other windows/in case the permission changes
    if (permissionObj.state === "prompt") {
      button.disabled = false;
      button.addEventListener("click", clickListener, { once: true });
      return;
    }
    button.disabled = true;
    button.removeEventListener("click", clickListener, { once: true });
  };
  changeListener(); // Show/hid button, based on the state of the permission
  permissionObj.addEventListener("change", changeListener);
})();

@ianbjacobs
Copy link
Contributor

@travisleithead, would love to have some input from you on this! (Feel free to ping me for more background.)

Ian

@adrianhopebailie
Copy link
Contributor

@marcoscaceres can you explain where this code would run and what it does?

I am confused about why there is a PaymentManager interface on window

@dlongley
Copy link
Contributor

dlongley commented Mar 7, 2017

@adrianhopebailie,

can you explain where this code would run and what it does?

Not to speak for @marcoscaceres, but I would expect this code to run on a page on the site that wants to handle payments. It's not something that runs in a service worker from that site, if that's what you're getting at. As for PaymentManager being on window, that looks quite similar (but not exactly) to the approach the Notifications API takes to permissions, which is to expose a Notifications interface on window with a requestPermission method. But I do understand that it would be odd to expose the entire PaymentManager API there, given how it is meant to be bound to a service worker registration.

The purpose of this code is so that when a user visits a website that can potentially handle payments, the website may call Something.requestPermission() to ask the user to grant that origin the ability to handle payments. By granting this permission, the website may now register payment handlers in service workers to receive payment requests.

@marcoscaceres
Copy link
Member

So, @dlongley gave a good summary of what I was thinking 👍 My code example would run in a normal Window/page (i.e., not in a service worker).

The window.PaymentManager is the thing that allows a developer to request the permission and to manage the payment instruments: so, once a developer has permission from the end-user, they can .add(), .remove() payment instruments.

@adrianhopebailie
Copy link
Contributor

Okay, thanks for the confirmation at @marcoscaceres and @dlongley

Would it still be possible to explicitly request permission from within a Service Worker registration?

i.e. How does window.PaymentManager relate to ServiceWorkerRegistration.PaymentAppManager? Do we want a requestPermission method on that interface too?

p.s. I think we should consider changing PaymentAppManager to PaymentManager. WDYT? @adamroach , @tommythorsen , @ianbjacobs , @jnormore

@marcoscaceres
Copy link
Member

Would it still be possible to explicitly request permission from within a Service Worker registration?

yeah, that should be ok, I think. My proposed model might not have had that, is all... I don't remember anymore :)

@dlongley
Copy link
Contributor

dlongley commented Mar 8, 2017

Would it still be possible to explicitly request permission from within a Service Worker registration?

I'm not sure how that would work, actually -- at least it's not the model other APIs follow. The Notifications interface isn't exposed in service workers, only on window Notifications Spec.

It seems to me that the main (only?) use case for calling requestPermission here would be when a user visits a page on an origin and wants to grant that origin the ability to handle payments for them. I may be misunderstanding, but what's the use case for requesting permission in a service worker instead of on the page directly -- when the user has it in front of them?

As the service worker runs in the background, when would it run requestPermission? It would have to be triggered by an event that the service worker can respond to (install, activate, message?). If the service worker detected it needed permission from the user, couldn't it open a client window (or focus one) and tell it to request permission via postMessage? Perhaps having a page from the origin in front of the user should even be a requirement for getting permission as it helps them understand what's going on.

@adrianhopebailie
Copy link
Contributor

@dlongley all good points. I was really just probing. I can imagine many use cases where a payment app has no UI but I think you re right that they registration of these would likely still require the user to browse to a page that would expose window.

@dlongley
Copy link
Contributor

dlongley commented Mar 8, 2017

I can imagine many use cases where a payment app has no UI but I think you re right that they registration of these would likely still require the user to browse to a page that would expose window.

Agreed. It's not really the payment app asking for permission anyway, it's the origin asking for permission to provide payment apps to begin with.

@marcoscaceres
Copy link
Member

Agreed. It's not really the payment app asking for permission anyway, it's the origin asking for permission to provide payment apps to begin with.

Exactly. The permission grant has to happen first through some visible tab/window.

@ianbjacobs
Copy link
Contributor

@adrianhopebailie asked:

"p.s. I think we should consider changing PaymentAppManager to PaymentManager. WDYT? @adamroach , @tommythorsen , @ianbjacobs , @jnormore"

Name harmonization sounds good...unless it would cause confusion. (I don't know here.)
I defer to those with more API design experience.

Ian

@adrianhopebailie
Copy link
Contributor

Unless there are a lot of pending edits I'll make a quick pass over the whole doc to try and align with our latest thinking around terminology.

@ianbjacobs
Copy link
Contributor

@adrianhopebailie,

That's fine, but if you could do that in the next 4 hours that would be preferred. I had planned to dive in later today with other edits.

Ian

@ianbjacobs ianbjacobs added this to the Mark in FPWD milestone Apr 4, 2017
@rsolomakhin
Copy link
Collaborator

@romandev cc

@ianbjacobs
Copy link
Contributor

We discussed this briefly on 20 June 2017 and are looking forward to a pull request from Samsung implementers who are experimenting with approaches.

romandev added a commit to romandev/payment-handler that referenced this issue Jun 21, 2017
In the current spec, there is no definition of the requestPermission() method.
This change is fixing w3c#94 issue.
ianbjacobs pushed a commit that referenced this issue Jun 23, 2017
* Define requestPermission() behavior.

In the current spec, there is no definition of the requestPermission() method.
This change is fixing #94 issue.

* Addressed all review comments.

* Fixed link error of NotAllowedError.

* Addressed @ianbjcobs's comment.
@ianbjacobs
Copy link
Contributor

Closed since PR 180 merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants