Introduce Credentials portal (experimental) - #1889
Conversation
swick
left a comment
There was a problem hiding this comment.
Went through the frontend interface and added comments for everything that stood out to me. It does look very reasonable!
| <!-- | ||
| CreateCredential: | ||
| @parent_window: Identifier for the application window, see :doc:`window-identifiers` | ||
| @origin: The origin of the request. If specified, must be a valid HTTPS origin; otherwise, the app ID will be used as the origin. |
There was a problem hiding this comment.
Isn't the origin something with a scheme? how would the app id origin get formed?
There was a problem hiding this comment.
Right now, I'm just prefixing the app ID with app: in the backend. (This should probably be moved to the frontend though to require less coordination if it ever changes.)
I'm somewhat deliberately making the app ID format unspecified. Right now, we have not specified how app IDs should map to HTTPS origins for sharing—we need the equivalent of Android's Digital Asset Links or Apple App Site Association. That would allow us to define, for example that app:com.discordapp.Discord is allowed to request credentials for https://discord.com and https://discordapp.com while still being considered "same origin."
We may make it more specific in the future, e.g. flatpak:com.discordapp.Discord and snap:com.discordapp.Discord, or even more specifically, flathub:com.discordapp.Discord and snapcraft:com.discordapp.Discord. I think that eventually these app IDs should be bound to the app verification process, which probably means it would be namespaced by distributor. (Unless some collaboration process forms between Linux app distributors that unifies application builds but allows separate bundle formats... I think something like that would be beneficial for the ecosystem, but is unlikely.)
But that's a much larger conversation and I think should be considered outside scope for now.
There was a problem hiding this comment.
I went back on this and decided to just use HTTPS origins for apps too, and we'll gate access control locally on the machine. (In the original scheme above, this would have required every website to add support for all the Linux app IDs for their app, which most would never comply, so sending the HTTPS origin just makes it seem like a browser.)
|
Okay, skimmed the impl, and it looks how I would expect it to look for the corresponding frontend. Also skimmed the code, and I would very much want to avoid adding more threaded code, so I think this should be rebased on top of the libdex support PR. The other thing that is a bit unusual about this portal is that the impl here isn't a desktop specific component, but e.g. credentialsd. This needs to be spelled out somewhere. This also needs to link to the interface that desktops have to implement for credentialsd! I would like to take a look at that as well, but don't know where it is. Overall this looks good; most issues are just details and documentation issues. |
| --> | ||
| <node name="/" xmlns:doc="http://www.freedesktop.org/dbus/1.0/doc.dtd"> | ||
| <!-- | ||
| org.freedesktop.impl.portal.CredentialsX: |
There was a problem hiding this comment.
why is this named CredentialsX?
There was a problem hiding this comment.
We had discussed previously adding a suffix while it was still experimental. But @swick said we should use an environment variable as a feature flag instead, so I'll do that.
There was a problem hiding this comment.
We would like to merge this in an experimental state first. But I agree that adding just an X at the end is not ideal :)
There was a problem hiding this comment.
I changed the "impl" to be called "handler" instead, since it's not a backend impl. I'm fine with changing nomenclature, but wanted that to be clearer while we talk about it.
| <method name="CreateCredential"> | ||
| <arg type="o" name="handle" direction="in"/> | ||
| <arg type="s" name="app_id" direction="in"/> | ||
| <arg type="s" name="app_display_name" direction="in"/> |
There was a problem hiding this comment.
Can't we derive this from the app_id?
There was a problem hiding this comment.
Yeah. In the current implementation, the frontend looks up the display name by app ID and forwards to the backend. That way backend doesn't have to reimplement the display name lookup if it trusts the frontend.
I'm ignorant of other considerations for doing the lookup from the backend; do you have more?
There was a problem hiding this comment.
it's a bit of an issue because the app id doesn't always map properly to a desktop file (e.g. in the snap case), but it's true that we currently only give backends the app id and pretend that you can get the corresponding desktop file.
There was a problem hiding this comment.
Backends usually use something like https://docs.gtk.org/gio-unix/ctor.DesktopAppInfo.new.html. I'm unsure what rust provides here.
There was a problem hiding this comment.
Removed this from the backend interface and used DesktopAppInfo::display_name() in the reference implementation. Let me know if this is the right direction we should go.
|
As for the interface name, let's call it something like I also noticed that there is no code here which determines if access to an origin is allowed or not. I believe that xdp is the right place to handle it. Here we have access to the app metadata and the permission store. From quickly skimming the credentialsd code I believe this is currently hardcoded? |
Sounds good. I noticed the other portals have singular names: should we do that here too?
Yeah, that was hardcoded for testing purposes. I think the best place to allow this permission would be at distribution. The privileged credential request capability should be specified in the app manifest, which is reviewed and signed by the distributor. My concern with doing this in xdp is that I can't think of a clear way to communicate the permission. It's kind of nuanced: they're giving the app permission to request credentials, not giving direct access to the credentials, but that's hard to communicate if this just pops up in the middle of a browsing session. And if any app is able to prompt the user for this capability, then I think it'll either scare away users from legitimate apps or cause click-throughs to malicious apps. In that case, I wonder if it'd be better for it to be unrestricted until we can review the permissions on behalf of the user. If we did want to do these prompts. Maybe something like this could work? Is this supported by permissions portal? The general behavior would be:
Right, we talked about this two ways. Summarizing discussion elsewhere: it would make sense to make credentialsd a soft dependency for xdg-desktop-portal. In this way, the xdg-desktop-portal becomes the implementor, and credentialsd is an implementation detail. It requires a bit more complexity in xdp, but is more natural to how the rest of the portals are configured. The Flow Control and UI Control APIs are documented here. The desktop would need to implement the UI Control interface, and use the Flow Control methods and signals to guide the user through the ceremony. This fundamentally changes the backend API here. We should review the Flow Control and UI Control APIs to see if we need anymore tweaks. |
|
We discussed things on matrix, and the conclusion is that we to depend on credentialsd, so icredentialsd is not the impl. Instead, the API that credentialsd calls to create the UI should be the impl. |
559343c to
5615cc1
Compare
|
I pushed some updates, but this isn't ready for a full review still since I haven't addressed the other comments from Matrix, namely:
(I'm mostly pushing so that I can paste a reference point for the work in progress on the credentialsd side.) |
65f21d3 to
a158685
Compare
| */ | ||
|
|
||
|
|
||
| #include <gio/gunixfdlist.h> |
| if (results) | ||
| g_object_set_data_full (G_OBJECT (request), "results", g_variant_ref (results), (GDestroyNotify)g_variant_unref); | ||
|
|
||
| task = g_task_new (NULL, NULL, NULL, NULL); |
| if (results) | ||
| g_object_set_data_full (G_OBJECT (request), "results", g_variant_ref (results), (GDestroyNotify)g_variant_unref); | ||
|
|
||
| task = g_task_new (NULL, NULL, NULL, NULL); |
|
|
||
| // TODO: define a syntax for this var | ||
| char *experimental_flags = getenv("XDG_DESKTOP_PORTAL_ENABLE_EXPERIMENTAL"); | ||
| if (experimental_flags != NULL && strcmp (experimental_flags, "credential") == 0) { |
spec review@swick, @bilelmoussaoui, I think I've either addressed or responded to all the review comments above, and the spec portion is ready for re-review. (I'd be happy to split the implementation into a separate PR if that helps. If so, I can split this and unmark this as draft.) implementation prereqsAs far as libdex, it looks like that's still a bit in flux? (I'm looking at #2036, which was opened recently) It looks like entitlements (#1924) is in a similar state. If I cleaned up the code to fix the goblint errors, would it be acceptable to move toward merging this with callback-style code and rewrite with libdex and add the entitlements feature in a subsequent PR? |
|
I finished up both libdex and entitlements. I'll look at this PR in the coming days, try to get it running and port it over. |
|
The history here is a big of a mess. I tried my best to get the relevant changes on top of my branches with libdex and entitlements. It builds but I might have missed some changes. https://github.com/swick/xdg-desktop-portal/tree/wip/credentials-portal |
bb1ebb0 to
87cf2c7
Compare
|
Oh, yeah, that's bogus. Sorry about that. I was avoiding rebases to preserve GitHub comments, but I jacked up that merge commit. I squashed and rebased. I confirmed that all of the changes from this branch are present in your branch. Edit: the |
|
This patch on top of your branch seems to do the trick. I think I did it right. With the async/await pattern and the extra macros, it does simplify the code quite nicely. :) |
|
Not a full review by any means, but I looked at the API and it's smaller than I expected from previous conversations. That's good, but it also means that I'm questioning the design decision to let credentialsd call into the impls. It looks like we could just do what we do in any other portal and have credentials talk with the frontend, and the frontend then talks to the impl. It would also make credentialsd entirely independent of xdp again. What are your thoughts on that? |
|
I guess I have two concerns:
I'm willing to try if you think that's the right way to go though. Also, I had noticed that the docs in your other PR dropped one of the pages that documents the signals sent from the backend to frontend. We did simplify some things, so you might be seeing that effort paying off, but just wanted to make sure we're looking at the right docs |
87cf2c7 to
138c125
Compare
f7270fe to
d6083e6
Compare
|
I updated the backend spec to use the Session pattern used in other portals. I think this follows the same convention, but let me know if anything needs to be tweaked. Once linux-credentials/credentialsd#194 lands, the credentialsd project will implement the frontend and backend APIs according to this spec. I think this is ready for review to be merged behind a feature flag, but the first few commits are a placeholder for another solution for #2066, and this will need be rebased on that solution. |
Wires up formatting to allow merging portals in experimental status, requiring a feature flag, supplied by a comma-separated list of feature flag names in the environment variable XDG_DESKTOP_PORTAL_ENABLE_EXPERIMENTAL. Documentation files for exported interfaces also get an experimental icon in the documentation site. This will eventually be split into a separate PR.
In the upcoming credential portal, we will be delegating much of the frontend request processing to an external service on the host. We are calling this a "handler". This patch allows code generation via gdbus-codegen for handlers. These handler interfaces are not user-facing, whether for app or DE developers; it is an internal dependency. Because of that, they should not appear in documentation.
The Credential portal allows applications to retrieve credentials for web services. It is highly inspired by W3C APIs including the Web Credential Management API and WebAuthn. The bulk of the processing is deferred to credentialsd. In this current form, xdg-desktop-portal is responsible for determining the App ID of the caller, and credentialsd is responsible for granting access to specific origins for that App ID, as well as interacting with the backend for user interaction. In future versions, xdg-desktop-portal will take responsibility for more permissions checking via entitlements and will send requests to the backend, acting as a proxy for credentialsd.
d6083e6 to
f688402
Compare
|
Updated formatting and ported to libdex |
This seems to work so far. Some work to be done:
DiscoverCredential(services as)method to frontend and backend.