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

Promise resolution in "determine the storage access policy" seems wrong? #152

Closed
domfarolino opened this issue Dec 23, 2022 · 6 comments
Closed

Comments

@domfarolino
Copy link

Pre-requisite context: #151.

First point

Assuming my analysis of #151 is correct, then I think we should be passing |p| from https://privacycg.github.io/storage-access/#dom-document-hasstorageaccess into the https://privacycg.github.io/storage-access/#determine-the-storage-access-policy algorithm. That promise resolves to a boolean, but inside https://privacycg.github.io/storage-access/#determine-the-storage-access-policy it only ever gets "resolved", not resolved with a particular value. Rejections seem to be handled correctly from a syntactic point of view.

Second point

But semantically but I was surprised to see that:

If implicitly denied is true, queue a global task on the permission task source given global to reject p with a "NotAllowedError" DOMException, and return p.

I don't know much about the storage access spec, but as per https://w3ctag.github.io/promises-guide/#rejections-should-be-exceptional it seems that maybe if implicitly denied is true, we should just be resolving the promise with the "false" value, since rejections are supposed to be reserved for real exceptional errors, and not just a true/false answer? Again I'm not super familiar with this spec in general so I could be off the mark, but let me know what you think.

@cfredric
Copy link
Contributor

Re: the first point, this will be fixed as part of #141.

Re: the second point: as prior art, we do have Notification.requestPermission() which returns a Promise that resolves to a 3-valued string, rather than a Promise<bool> which sometimes rejects. (Although the spec seems to disagree with MDN, and say that it returns a Promise that resolves to a 2-valued string ("granted" or "denied")...)

On the other hand, we also have MediaDevices.getUserMedia() which returns a promise that rejects if a MediaStream could not be provided for any reason, including if permission was denied.

I don't really have an opinion either way on this, though.

@domfarolino
Copy link
Author

Sometimes rejecting is OK, but making it the primary method by which the "no" answer is communicated seems like bad practice, so if there is precedent for it, I think we shouldn't follow it.

@annevk
Copy link
Collaborator

annevk commented Jan 9, 2023

I think it's too late at this point to change it into a boolean.

I agree with you that it would have been better that all promises pending an end user decision would never reject as it's very plausible for the user to say no, but unfortunately there's a number of APIs that errored on that front.

@johannhof
Copy link
Member

Yeah +1 to what Anne said, it seems a bit too late. I think you're making a good point @domfarolino and this may have been the ideal design from the start, but at the same time (IMO) this isn't dramatically inconsistent with the web platform enough to warrant breaking the existing API surface.

@domfarolino
Copy link
Author

Makes sense, I wasn't sure what the shipping status of this API was so I had a morsel of hope that we could still change it, but that's alright.

@johannhof
Copy link
Member

Okay, I think I'll close this given that we still have #151?

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

4 participants