Repository navigation
daemon: Allow clients to cancel requests - #246
dangerousplay wants to merge 1 commit into
Conversation
xdg-desktop-portal calls org.freedesktop.impl.portal.Request.Close() on the handler when a client closes its request or disconnects, but the daemon never exported such an object, so requests could not be cancelled. Accept an optional `request_handle` object path in the CreateCredential and GetCredential options. For the duration of the ceremony, export an org.freedesktop.impl.portal.Request object at that path. Close() is only accepted from the connection that started the ceremony; it cancels a token that is passed down to the flow controller, which cancels the request and dismisses the UI. The cancellation does not need the gateway service lock, which is held for the whole ceremony. The method call then returns response code 1 (cancelled). Fixes linux-credentials#90
|
Thanks for this! I will take a look at this next week :) |
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
Sorry for the delay. I finished reading through the code, and it's looking pretty good! I need to run some tests locally and then I think we can merge this in
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
Hi @dangerousplay, sorry this took so long to get to!
I quite like the ceremony and cancellation token changes you've made.
The handler interface is supposed to be an internal interface between xdg-desktop-portal and credentialsd; it's also deprecated as we shift architecture in xdg-desktop-portal to share have x-d-p call the backend directly.
So I think I would prefer to export a separate org.freedesktop.handler.portal.experimental.Request interface with a Close() method. That would make it more clear that this is not a normal backend request that clients are supposed to call; they should call the Close() method exported by xdg-desktop-portal instead.
Then on the xdg-desktop-portal side, we should hook into the GObject closed signal on the request object, and call the handler's Close() method in the signal handler.
| necessary. Clients can cancel their own requests by passing a `request_handle` | ||
| option: the Gateway exports an `org.freedesktop.impl.portal.Request` object at | ||
| that path for the duration of the request, and calling `Close()` on it cancels | ||
| the request and dismisses the UI. Otherwise, the user has to cancel via the UI. |
There was a problem hiding this comment.
Let's not document this until it's in its own interface, org.freedesktop.handler.portal.experimental.Request
| ## Cancelling Requests | ||
|
|
||
| `CreateCredential()` and `GetCredential()` return a response code and results, | ||
| following the [XDG Desktop Portal `Response` conventions][xdg-request]: | ||
|
|
||
| - `0`: Success. The results contain the credential. | ||
| - `1`: The request was cancelled by the client. The results are empty. | ||
| - `2`: The request failed. The results contain an `error` string. | ||
|
|
||
| A caller that wants to be able to cancel an in-flight request passes the | ||
| optional `request_handle` option, an object path below | ||
| `/org/freedesktop/portal/desktop/request/`. For the duration of the request, the | ||
| gateway exports an object implementing `org.freedesktop.impl.portal.Request` at | ||
| that path: | ||
|
|
||
| ``` | ||
| org.freedesktop.impl.portal.Request { | ||
| Close() | ||
| } | ||
| ``` | ||
|
|
||
| Calling `Close()` cancels the request and dismisses the UI, and the original | ||
| method call returns response code `1`. `Close()` is only accepted from the same | ||
| D-Bus connection that made the `CreateCredential()` or `GetCredential()` call; | ||
| other callers receive `org.freedesktop.DBus.Error.AccessDenied`. The object is | ||
| removed when the request ends, whether it succeeded, failed or was cancelled. | ||
|
|
||
| If the path is already in use or is outside of the prefix above, the request is | ||
| rejected with `TypeError`. If `request_handle` is omitted, the request cannot be | ||
| cancelled. | ||
|
|
||
| xdg-desktop-portal passes the path of the `Request` object it created for the | ||
| client, so that closing that request, or the client disconnecting, cancels the | ||
| ceremony. | ||
|
|
||
| [xdg-request]: https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.Request.html | ||
|
|
There was a problem hiding this comment.
Let's not document this until we have a separate interface
Hello I hope you are having a good day ^^
This pull request fixes #90.
xdg-desktop-portal creates an
org.freedesktop.impl.portal.Requestproxy for everyCredential request at
request->idon the handler's bus name and callsClose()on itwhen the client closes its
org.freedesktop.portal.Requestor disconnects. credentialsdnever learned that path and never exported the object, so client cancellation was a no-op
and the UI stayed open.
This adds an optional
request_handle(o) option to the handler'sCreateCredential/GetCredential. When present, credentialsd exportsorg.freedesktop.impl.portal.Requestat that path for the lifetime of the ceremony:
Close()is only accepted from the bus name that issued the handler call(others get
AccessDenied).CancellationTokenthat is threaded down to the flow controller; it nevertakes the gateway mutex, so it cannot deadlock with the running ceremony. A request
still queued behind another one is cancelled too.
cancel_request()and sendsErrorCancelled, which dismisses the UIthrough the existing path, and the handler call returns response code 1 (Cancelled).
/org/freedesktop/portal/desktop/request/.Without the option, behaviour is unchanged. The matching one-line xdg-desktop-portal change
(passing
request_handle = request->id) is in linux-credentials/xdg-desktop-portal#1.Tested with unit tests (option parsing, response codes, invalid handles), private-bus
integration tests (
gio::TestDBus: foreign Close -> AccessDenied, caller Close -> code 1,object removed), flow-control tests, and end to end from a WebKitGTK client
(AbortController.abort() -> Request.Close -> ceremony cancelled, UI dismissed).