Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

First draft of dat RPC implementation #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Gozala
Copy link

@Gozala Gozala commented Sep 19, 2018

Overview of changes

  1. I end up changing lot of code in dat/protocol.js. (I did not wanted to, but I'm afraid I was unable to following the logic, I hope it's ok). I made logic more linear so it's easier to make sense of it.
    • Code that was normalizing filepath was pretty similar to the normalizeFilepath from web-apis/bg/dat-archive.js so I factored it out to dat/util.js & import from both modules.
    • Timeout handling was bit hairy and was getting into the way of linearization. It also appeared to me that timer function used all across web-apis/bg/dat-archive.js was serving the same purpose, so I end up importing and using it into dat/protocol.js as well.

      Note: Former timeout logic ignored time spend on name resolution, but with my changes it no longer does that. If necessary I can change code to do the same, it just appeared to me that former version was not deliberate given that web-apis/bg/dat-archive.js seems to account that time.

  2. Now function match takes request and returns a corresponding handler which makes it a lot easier IMO to figure what route is taken. Former handler ended up being split into two serveEntry and downloadZip handlers. All the other handlers are matched according to REST interface spec.

    Note: I also end up exposing all the READ operations as GET with special query argument, mostly because it allowed me to test REST API by just navigating to URLs. That being said I think there is an actual value in that & I'd suggest to consider altering spec.

  3. All of the REST API handlers are defined in new dat/rpc.js module and are pretty much wrappers for corresponding functions in dat/protocol.js which just take care of translating query arguments, uploadData and URL to corresponding arguments & then translating results back to StreamProtocolResponse objects.

Status

  • I did some manual testing yesterday and fixed bunch of bugs (forgot to push them, but did that just now). From what I can tell all the READ functionality works as expected.
  • For whatever reason watch API does not work and I'll have to look more into it.
  • All the WRITE functionality is broken. Idea was to let web-apis/bg/dat-archive.js do it's normal access control checks using referrer of the request, but it seems that electron does not bother to set it anything other than '' nor I managed to get it to send Origin header.
  • All READ APIs work as expected.
  • All WRITE APis work as expected as long as origin has being previously being granted access to an archive it's trying to write to.
  • Request of write access is not working right now but that problem isn't necessarily specific to this, it's more of UX question of where such request should be displayed if request is coming from: (hidden) iframe, web worker, service worker.

Open questions

  • watch API is tricky in that how does one unsubscribe ? I don't seems to see how one could detect that connection was dropped.
  • As mentioned in above section figuring out how to communicate where request originated from is crucial to making WRITE APIs work. Turns out "null" origin is set for request from same origin and actual Origin is included for requests from other origins.
  • I also have created createArchive \ selectArchive APIs available in dat/rpc.js module, which are not exposed. I think we need to discuss exposing them as well, because without them WRITE APIs are not going to be all that useful. On the other hand though it brings up UX question - how does beaker communicate which app is requesting write permission.

I think this has potential to unlock

@Gozala
Copy link
Author

Gozala commented Sep 19, 2018

After some researching it appears that lack of referrer / Origin is a known issue that is commonly mitigated via WebRequest which allows setting headers before request is passed to a protocol handler. @pfrazee Would it be acceptable way to overcome described limitation ?

@pfrazee
Copy link
Member

pfrazee commented Sep 19, 2018

@Gozala That's how I figured this would be solved. I haven't dug in yet to see if we can get exactly what we want, but I'd start there.

@Gozala
Copy link
Author

Gozala commented Sep 20, 2018

I did bunch of fixes. All WRITE APIs except writeFile seem to work at least in self mutable dat scenario. I was unable to get writeFile / configure to work because request passed to protocol handler does not seem to have uploadData field which is supposed to contain posted data.

@Gozala
Copy link
Author

Gozala commented Sep 20, 2018

I should note that RPC API only works with XMLHttpRequest and not with fetch as later does not seem to set Origin header while former does, which is required for access right verification.

@Gozala
Copy link
Author

Gozala commented Sep 21, 2018

Submitted following issues to an electron electron/electron#14720 electron/electron#14746 electron/electron#14747

@Gozala
Copy link
Author

Gozala commented Sep 22, 2018

After some more digging on pointers from electron/electron#7931 it appears to me that Origin is set to 'null' whenever it matches request URLs origin (which I find very counter intuitive & would love some verification / confirmation).

I was also able to verify that whenever request is coming from different origin Origin header is actually set to reflect the origin of the source which is illustrated by following example:
https://github.com/Gozala/electron-quick-start/blob/0146bbd03b1f49e6fb7d06b079a861fde962ec1c/bug/script.js#L50-L54

In fact such request causes a preflight OPTIONS request to the protocol handler and only lets actual request to go through if response to it allows requested method, origin, headers (here is the illustration https://github.com/Gozala/electron-quick-start/blob/origin-header/protocol.js#L33-L44)

What is unclear to me - I don't believe beaker handles such requests (I have not seen evidence of it at least) but somehow cross-origin request still seem to succeed. Which makes me wonder if there is a difference in scheme configuration here is one used in the example
https://github.com/Gozala/electron-quick-start/blob/0146bbd03b1f49e6fb7d06b079a861fde962ec1c/webview.js#L2-L8

I am also uncertain what would happen if source dat://a attempt so write into dat://b specifically as far as I can tell if archives are different that invokes some perms API:

var allowed = await globals.permsAPI.queryPermission(perm, sender)
if (allowed) return true
// ask the user
allowed = await globals.permsAPI.requestPermission(perm, sender, { title: details.title })
if (!allowed) throw new UserDeniedError()

That I don't actually know what it does but I suspect it requires sender that has more than just getURL which is sadly the case here. I'll try to find some answers but any shortcuts / pointers are welcome.

@Gozala
Copy link
Author

Gozala commented Sep 22, 2018

I guess queryPermissions is actually not using anything but getURL so this looks good:
https://github.com/beakerbrowser/beaker/blob/c3c75df24be851274a0bca3cfd2be63126bf3af8/app/background-process/ui/permissions.js#L54

@Gozala
Copy link
Author

Gozala commented Sep 22, 2018

As of requestPermission unfortunately my fear is confirmed:

https://github.com/beakerbrowser/beaker/blob/c3c75df24be851274a0bca3cfd2be63126bf3af8/app/background-process/ui/permissions.js#L27-L29

https://github.com/beakerbrowser/beaker/blob/c3c75df24be851274a0bca3cfd2be63126bf3af8/app/background-process/ui/permissions.js#L100-L104

https://github.com/beakerbrowser/beaker/blob/c3c75df24be851274a0bca3cfd2be63126bf3af8/app/background-process/ui/permissions.js#L162-L164

API needs to figure out what window does sender corresponds to in order to I presume do user prompt. Which does also raises a more general question unrelated to this

Assuming we eventually have DatArchive API in ServiceWorker / Worker / (hidden) iframe where would that user prompt be displayed in that case ?

@pfrazee I presume answer to that question would also dictate solution for this specific limitation.

@pfrazee
Copy link
Member

pfrazee commented Sep 24, 2018

Assuming we eventually have DatArchive API in ServiceWorker / Worker / (hidden) iframe where would that user prompt be displayed in that case ?

That's a really tough call. Iframes could mislead the user if they prompt within the parent frame, and service workers may be running without a frame active (I believe).

@Gozala
Copy link
Author

Gozala commented Sep 25, 2018

service workers may be running without a frame active (I believe)

Yes. In fact fetch to a url handled by registered service worker should activate it unless it already is active

@Gozala
Copy link
Author

Gozala commented Sep 25, 2018

After seating on this for a day I think there are two options:

  1. Require a page to be open for requesting an archive acess. Unfortunately it would be somewhat limiting and harm UX in some instances but presumably this would be an uncommon case. Probably it would be worst only first time around.

  2. Revamp current permission request UI so that it doesn’t imply request coming from the page you’re on. For instance request could reflect who’s requesting what. For example if app 🎹 is interacting with app 📨that interacts with app 📔requesting writable dat, permission request UI could clearly display the whole request chain like:

    🎹➡️📨➡️📔

    I’ve seen pretty versions of that (although with just two nodes) for oauth flows. It is also an option to hide nodes between by default for straightforward visual but make digging into whole chain possible.

    Although admittedly apps would need to have clear names and distinct icons to make sense.

@Gozala
Copy link
Author

Gozala commented Sep 25, 2018

One more thought - If app is talking to other app in iframe or a service worker which is asking for permission it might be reasonable to display permission prompt ancored to a visible tab that is initiating request flow (it would be still nice to clarify via icon / url the site permission is being granred to). In worst case user may mistake that for a request from initiating page, but that’s in a way not a mistake, that page is requesting mediated permission. Also in worst case scenario BAD app may get access to some dat but on the other can app considered to be GOOD if it’s doing exchange with BAD one ? I’m inclined to say no.

@Gozala
Copy link
Author

Gozala commented Sep 25, 2018

I propose to extend DatArchive API as follows:

Add optional owner field to opts of the following methods:

  • DatArchive.create
  • DatArchive.fork
  • DatArchive.selectArchive

That would allow one app to request access on another apps behalf. Which IMO would provide reasonable way for collaborative apps to request desired permission.

For example - app A is interacting with app B’s service worker, which needs to create / select / fork an archive. B can respond to A asking it to request corresponding permission on it’s behalf. App A then could call corresponding DatArchive API method with owner set to B. If user grants permission A will get DatArchive instance back for which it’s not an owner, but it can pass archive’s URL back to B which will be able to load the archive and complete requested task.

With this in place it would be fairly reasonable to allow permission prompts only from the top level documents. And most interactions could provide reasonable UX (as long as prompts reflect designated owner)

Only unfortunate consequence is that app performing request on others behalf will learn the URL of the dat that user granted permission for which may be undesired.
That being said B could slways ask A to open window with it’s URL and prompt User there in which case A won’t be able to learn a URL. More complex alternative could instead issue opaque DatArchive with URL that only owner can load / resolve but that might be an overkill (although desire for Dat URLs that provide replication capability without read capabilities is frequently reoccurring)

@Gozala
Copy link
Author

Gozala commented Sep 25, 2018

I have misunderstood DatArhive API. Namely I was under impression that selectArchive API was granting write access to a requestor, but turns out it does not. Instead user is prompted on first write.

This means that my proposal as described in previous comment (which I started implementing) is not going to work. Because even if provider app (hidden iframe / worker / service worker) app may ask user to select archive through a consumer app (displayed by an active tab) mediation, provider will still fail to write as there will be no place to display user prompt.

Only solution I can think of right now is to extend one more method DatArchive.load(url, {writable:true, owner:providerURL}) to let provider request a write access through consumer mediation. Sadly it starts to feel awkward & provider now needs to track whether it has write access or not.

P.S.: I do like that provider and consumer apps need to cooperate in order to request access from a user. IMO it is much better than say allow provider to request access on it's own, because cooperation makes it impossible for provider to trick user into thinking that consumer requested permission, instead it needs a to convince consumer to actually do that on it's behalf.

@Gozala
Copy link
Author

Gozala commented Oct 17, 2018

Hey @pfrazee any feedback on the proposed "mediated permissions" solution ? And more generally this pull request ? Implementation already started to rot, so I'd like to have some closure which is either:

  1. By getting this into a shape that could land
  2. Abandoning as failed exploration

I would argue that even with the following limitations:

  1. Inability to write into archives that have not being permitted yet.
  2. Inability to request additional permissions without a top-level window. (In fact former is just specialized case of this)

This changes provide enough value to justify them. It would allow use of dat archive APIs in both iframes and workers. And lack of permission requests could be overcome by opening top level window to request them if necessary.

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

Successfully merging this pull request may close these issues.

2 participants