Add service module to write PolKit agents #302

Cu3PO42 · 2025-10-11T19:37:16Z

This implements a new service module that enables users to write PolKit agents by integrating with libpolkit and libpolkitagent. A test configuration is added to the source.

Status: I've done everything I have planned.

Original description

This implements a new service module that enables users to write PolKit agents. To this end, I'm integrating with the existing polkit-qt-1 library that is also used by the KDE agent.

A relatively minimal example configuration using this new service can be found here.

There's a minor TODO left around cleanup of the Identity objects that I haven't been able to get quite right due to my inexperience with Qt. I'm hoping you might have a clean solution for this.

Closes #271.

outfoxxed · 2025-10-12T00:45:56Z

While I am not opposed to the addition of a polkit integration, I would rather avoid all KDE dependencies as including them leads to massive dependency trees on most distros, and they're usually pretty replaceable.

polkit-qt-1 appears to be a very thin wrapper around the polkit api that doesn't provide much. I would prefer to avoid it unless you have a good reason to pull it in.

Cu3PO42 · 2025-10-12T08:56:14Z

polkit-qt-1 is indeed a very thin wrapper. The 'good' reason for using it is that it deals with some of the more error-prone aspects of the upstream libraries and doesn't really bring anything that we don't need. While it is maintained by the KDE team, the package doesn't actually pull in anything but Polkit itself and Qt on the distros I checked (Debian, Ubuntu, Arch, Fedora, RHEL, NixOS, or Gentoo). I find it unlikely that others would have wildly different dependencies.

That said, it should also be fairly easy to not use it at the cost of a few hundred LOC, which would have the benefit of me getting to correct one of the limitations of polkit-qt-1.

outfoxxed · 2025-10-12T10:21:05Z

It sounds like one of the more reasonable ones if we had to use it then, though I would personally still prefer not to.

Cu3PO42 · 2025-10-12T10:51:48Z

Alright. I'll see what I can do.

Cu3PO42 · 2025-10-12T21:39:36Z

I removed the dependency on polkit-qt, interacting with the PolKit libraries directly, instead.

outfoxxed

Partial review on the interface.

Before continuing further, I've noticed the polkit agent interface appears to be available directly over dbus. Is there a reason to use any library at all for it? (Especially a gobject library)

src/services/polkit/qml.hpp

Cu3PO42 · 2025-10-13T07:35:47Z

Thanks for the review!

I don't like using a gobject library here either, but yes, there is a good reason for using it instead of interacting directly over DBus. PolKit requires the client that sends the confirmation message to be privileged. To this end, libpolkit has a setuid helper binary that it invokes, which handles the actual PAM conversation, and shuffles all of the data between the parent and the helper.

To avoid libpolkit, Quickshell would either need to run as root or we would need our own setuid helper. Besides the implementation overhead, this would break simply running it from your Flake via Nix or installing it via Home-Manager. I imagine (although I don't positively know) many distros might also look more sceptically at any setuid binaries.

Finally, the DBus interface isn't actually stable because it is currently considered an implementation detail. I don't think it has changed significantly in recent years, but I would still prefer the more stable solution.

outfoxxed · 2025-10-13T07:38:02Z

That's horrible on so many levels.

Cu3PO42 · 2025-10-13T07:43:03Z

I... yeah. But that's Polkit for you. There's many things I wish I didn't know about it.

If, given these circumstances, you'd rather not have the code upstream, I'm just going to package it up as a separate library.

outfoxxed · 2025-10-13T07:44:07Z

I'm fine with the gobject code since its an actual requirement, and this is a feature many people want. I'll finish the review tomorrow probably.

Cu3PO42 · 2025-10-13T07:47:15Z

Thank you! I'm not in any rush. Also, fair warning: this is the first 'not insignificant' thing I've done with Qt, so while I tried to do things right, some things might be the way they are because I didn't know any better. I'll happily take any pointers.

outfoxxed · 2025-10-13T08:02:28Z

Thats fine. One further thing that would be helpful would be to add a manual test qml file similar to what exists in other parts of the source. Just cover a reasonable amount of the api for a simple manual regression test

Cu3PO42 · 2025-10-13T21:48:15Z

I got started on some of your comments. Here's an additional thing we might consider:

PolKit can send either 'show-info' or 'show-error'. I am currently storing both into the same property with an isError flag. I.e. an incoming error overwrites a previous info. PolKit itself does not specify the way the UI should look, but many agents handle it this way. However, for some scenarios showing both might make sense, e.g. fprintd will show 'Place your right index finger on the reader' as info and 'Fingerprint did not match' as info. I am considering having both individual properties and a combined one for the latest message so users can choose how to design their dialog.

One thing I'm a bit unsure on is object destruction/cleanup and its behavior in Qt. When an authentication request finishes, we need to clean up all objects, but does this cause the UI to flicker, if we animate a window out, for example?

outfoxxed · 2025-10-14T08:15:42Z

PolKit can send either 'show-info' or 'show-error'. I am currently storing both into the same property with an isError flag. I.e. an incoming error overwrites a previous info. PolKit itself does not specify the way the UI should look, but many agents handle it this way. However, for some scenarios showing both might make sense, e.g. fprintd will show 'Place your right index finger on the reader' as info and 'Fingerprint did not match' as info. I am considering having both individual properties and a combined one for the latest message so users can choose how to design their dialog.

Does the api provide any sort of hint as to if both should be present or not? If it does something like sending a new version of the error message thats an empty string to reset thats a pretty clear signal we could have multiple, but if not I'm less sure. If the messages are coming from pam, our pam module currently replaces the last message with the new one so it probably makes sense to keep that design.

One thing I'm a bit unsure on is object destruction/cleanup and its behavior in Qt. When an authentication request finishes, we need to clean up all objects, but does this cause the UI to flicker, if we animate a window out, for example?

This is a bit of a mess, Quickshell in particular has a hack for it called Retainable which you can see here https://quickshell.org/docs/v0.2.1/types/Quickshell/Retainable/. Retainable objects generally hold their last state and become immutable while retained to make it easier to write reactive UI against them.

Note: I tested the dialog and see you have a lot of DEBUG: PolkitAgent: lines. Use logging categories (qCDebug etc) for this. We hide them by default but they can be as detailed as you want (though you should make an effort to not put passwords in them)

Cu3PO42 · 2025-10-14T19:41:59Z

Does the api provide any sort of hint as to if both should be present or not? If it does something like sending a new version of the error message thats an empty string to reset thats a pretty clear signal we could have multiple, but if not I'm less sure. If the messages are coming from pam, our pam module currently replaces the last message with the new one so it probably makes sense to keep that design.

Unfortunately not. The messages are indeed forwarded from PAM. I am therefore inclined to leave it as is.

This is a bit of a mess, Quickshell in particular has a hack for it called Retainable which you can see here https://quickshell.org/docs/v0.2.1/types/Quickshell/Retainable/. Retainable objects generally hold their last state and become immutable while retained to make it easier to write reactive UI against them.

Thanks! I'll look into that to figure out how best to use it in this scenario.

Note: I tested the dialog and see you have a lot of DEBUG: PolkitAgent: lines. Use logging categories (qCDebug etc) for this. We hide them by default but they can be as detailed as you want (though you should make an effort to not put passwords in them)

That's exactly what I was looking for. I'll migrate to that.

outfoxxed

Not super in depth as I've been busy, but I gave it a look-over.

src/services/polkit/listener.cpp

outfoxxed · 2025-10-15T08:42:48Z

src/services/polkit/qml.cpp

+	// First, destroy all but the first request in the queue, so that the cancel
+	// method doesn't start a new one.


I would think its not unreasonable to handle a request right after cancelling a different one, especially if they're for different programs.

Also, when reloading the graph, preserving the old polkit session may be the most reasonable case, especially if the user wants to do something like edit their auth dialog with a real app running to test with.

Handling one request after another is the default behavior in finishAuthenticationRequest. The cancelAuthenticationRequest method calls finishAuthenticationRequest and therefore would start another request immediately.

However, we are in the destructor of the agent here, so there will be no-one to handle the queued requests after the destructor has run. Therefore I want to reject all currently queued requests and not start processing them. To this end, I start by rejecting all requests after the current one, so that when the cancel method runs, it won't have any more to start.

In the case of reloading, I agree, preservation would be ideal. I'll look into how I can solve that cleanly only for the case of the reload, but still dismiss everything when actually destroying the agent.

As for reloading see my comment below about using an internal singleton. you can wait for the destruction of the last engine generation with a ref to the backing agent to cancel all requests and deregister the agent, or some similar pattern.

outfoxxed · 2025-10-15T08:44:28Z

src/services/polkit/qml.cpp

+		// In case of config reloads new objects are constructed before the old ones
+		// are destroyed. Therefore the new agent cannot take over the path still in
+		// use by the old agent.


It can as long as its negotiated. I think we established it doesn't make sense to have more than one agent, so a single agent per process solves this nicely.

More than one polkit agent object at a time in a single generation however does not make sense and should print an error message and render any agent objects >1 inoperable.

It can as long as its negotiated.

Are you referring to some particular Quickshell interface or is this a general remark? If the former, do you have a pointer? If the latter, this code is the early version of this negotiation. I meant to express 'it cannot take over the path without doing something to support it explicitly'. In the old version using polkit-qt, it wasn't possible to transfer the listener over, so closing the path and then opening it again was necessary. Now it might be possible and cleaner to transfer the listener. I'll look into it.

I'm not sure I fully understand the particular problem here, but singleton resources that live longer than a single engine generation are used in a few places in Quickshell.

Since the qml engine controls the lifecycle of all objects created in it, Quickshell often has an internal singleton object with a thin wrapper object owned by the qml engine instead, so it doesn't neeto die with the generation.

With the restriction that we can only reasonably have one polkit agent at once, it seems logical to me to represent it this way.

What I meant by negotiation was only allowing one qml owner of the polkit agent per generation, but keeping tha actual agent object the same.

I think we agree conceptually, I'm just not sure of the exact API surface that permits me to do this cleanly. I can obviously put the listener itself into a singleton and introduce a thin wrapper. Since we want to permit only one such wrapper per generation, I need to add a check to this effect: If a new PolkitAgent is constructed, I can check this singleton and see one already exists and set an error state.

In the case of a config reload, I will also see the listener is already in use, but in this case I want to take ownership of it. I could attempt to use currentGeneration to detect this, but this seems mildly heuristic. Alternatively, I could extend Reloadable and do it in Reloadable#onReload. Do you have a preference for any of these approaches or is there some other idiomatic way I'm not seeing currently?

Thanks!

Theres a couple differences in the approaches you could take:

Reloadable uses heuristics to try to match an object in the old generation to one in the new generation. This is the system that allows allocated windows to be reused. You could use this if you wanted to only persist the polkit agent if the user placed it in the same spot in the same file across generations.

PostReloadHook fires unconditionally after Reloadable and has no concept of the scene graph. Combining this with findObjectGeneration on the user facing wrapper objects from the current and previous generations will tell you if we're switching generations or the user created a second invalid agent. Avoid currentGeneration.

Either of these interfaces you would implement on the wrapper object and not the internal singleton.

I recommend the PostReloadHook approach but would accept either.

src/services/polkit/qml.cpp

Cu3PO42 · 2025-10-19T21:28:22Z

I performed a fairly significant refactoring to accommodate the changes you recommended.

PolkitAgent is now a very thin wrapper with the business logic living in PolkitAgentImpl.
PostReloadHook is used to handle transitions between generations.
State of the currently active request as well as any queued requests is preserved on reload.
AuthFlow is a new class that houses the actual state of an in-progress authentication. It is Retainable to accommodate animating out dialogs.
The code is just better organized overall.

I'll look at the lint errors soon.

outfoxxed

Re-reviewed the pr as a whole

outfoxxed · 2025-10-19T21:36:27Z

src/services/polkit/flow.hpp

+	/// The icon to present to the user in association with the message.
+	///
+	/// The icon name follows the [FreeDesktop icon naming specification](https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html).
+	Q_PROPERTY(QString iconName READ iconName CONSTANT);


Best to link to @@Quickshell.Quickshell.iconPath() to display it in an image

outfoxxed · 2025-10-19T21:38:13Z

src/services/polkit/flow.hpp

+	/// Setting this will abort any ongoing authentication conversations and start a new one.
+	Q_PROPERTY(Identity* selectedIdentity READ selectedIdentity WRITE setSelectedIdentity NOTIFY selectedIdentityChanged);


"Setting" -> "Changing"

Reactive changes can cause a lot of "Setting" so this wording could be confusing

src/services/polkit/flow.hpp

outfoxxed · 2025-10-19T21:41:27Z

src/services/polkit/flow.cpp

+		emit completedChanged();
+		emit authenticationSucceeded();


this-> on method calls including emit, as well as members. ci should be catching this one if its not

outfoxxed · 2025-10-19T21:42:47Z

src/services/polkit/flow.hpp

+	Identity* mSelectedIdentity = nullptr;
+	bool mIsResponseRequired = false;
+	QString mResponseMessage;
+	bool mResponseVisible = false;
+	QString mExtraMessage;
+	bool mExtraIsError = false;
+	bool mIsCompleted = false;
+	bool mIsSuccessful = false;
+	bool mIsCancelled = false;


While acceptable as-is, properties like this are better handled with QObject bindable properties, which are basically C++-side reactive properties.

outfoxxed · 2025-10-19T22:59:27Z

src/services/polkit/qml.cpp

+void PolkitAgent::classBegin() {
+	// Nothing to do here.
+}


just leave classbegin () {} in the header

outfoxxed · 2025-10-19T22:59:39Z

src/services/polkit/qml.cpp

+namespace qs::service::polkit {
+PolkitAgent::PolkitAgent(QObject* parent): PostReloadHook(parent) {}
+
+PolkitAgent::~PolkitAgent() = default;


this can also go in the header

src/services/polkit/qml.cpp

outfoxxed · 2025-10-19T23:06:36Z

src/services/polkit/qml.cpp

+	emit isRegisteredChanged();
+	emit isActiveChanged();
+	emit flowChanged();


Another good site for bindable properties

Based on my current understanding of bindable properties (at least as far as the Q_OBJECT_BINDABLE_PROPERTY) is concerned, are backed by a field in the object. This is not the case for the properties that are associated with these signals. I am sure they can be hooked up to work anyway, but I'm not sure if that would still be simpler?

I think I've figured it out. Will hopefully push something later.

They are field backed but when used with a binding callback the field acts as more of a cache than anything else. It also compares old/new values to see if anything changed and emit signals or not.

There are cases where it improves readability, generally the more values that depend on eachother the greater the improvement.

Here's an example of it being used with a binding:

quickshell/src/wayland/wlr_layershell/wlr_layershell.cpp

Line 18 in 3e2ce40

this->bcExclusiveZone.setBinding([this]() -> qint32 {

And heres an example of it being used as a field that just so happens to emit signals automatically on prop change:

quickshell/src/services/notifications/notification.cpp

Line 113 in 3e2ce40

Qt::beginPropertyUpdateGroup();

Up to you where you use it but it does generally make things clearer.

src/services/polkit/agentimpl.hpp

Cu3PO42 · 2025-10-20T11:32:51Z

Thank you for the extensive review! I addressed most of your comments. I also introduced a RAII helper GObjectRef to make handling of GObject objects less error prone. I still need to look into bindable properties.

I selectively turned off some lints in specific instances where I felt they were not helpful or actively wrong (e.g. it won't let me forward declare any structs from the Polkit libraries). There should always be an accompanying comment explaining why the specific rule doesn't make sense in that case. Let me know if you disagree with any of my reasons.

Cu3PO42 · 2025-10-20T19:58:12Z

I do love a good reactivity system. I switched over to bindable properties where I figured it makes sense. With that I believe I have addressed everything you pointed out. I appreciate your patience, this has been quite the learning experience.

Cu3PO42 · 2025-10-23T20:49:57Z

I rebased onto current master, squashed and fixed a small bug in the reloading logic.

I have also integrated this into Noctalia in a branch to validate in a more complex config.

Cu3PO42 changed the title ~~scope/polkit: add service module to write PolKit agents~~ Add service module to write PolKit agents Oct 11, 2025

Cu3PO42 force-pushed the polkit branch from 78a68fb to 68ea7e9 Compare October 11, 2025 22:31

outfoxxed requested changes Oct 13, 2025

View reviewed changes

src/services/polkit/qml.hpp Outdated Show resolved Hide resolved

src/services/polkit/qml.hpp Outdated Show resolved Hide resolved

src/services/polkit/qml.hpp Outdated Show resolved Hide resolved

src/services/polkit/qml.hpp Outdated Show resolved Hide resolved

Cu3PO42 force-pushed the polkit branch from da24788 to 3e921f4 Compare October 14, 2025 20:36

outfoxxed requested changes Oct 15, 2025

View reviewed changes

outfoxxed requested changes Oct 20, 2025

View reviewed changes

Cu3PO42 force-pushed the polkit branch from e682d1b to 1a01ccd Compare October 20, 2025 21:24

service/polkit: add service module to write PolKit agents

e87903d

Cu3PO42 force-pushed the polkit branch from 1a01ccd to e87903d Compare October 23, 2025 20:47

		// First, destroy all but the first request in the queue, so that the cancel
		// method doesn't start a new one.

		/// Setting this will abort any ongoing authentication conversations and start a new one.
		Q_PROPERTY(Identity* selectedIdentity READ selectedIdentity WRITE setSelectedIdentity NOTIFY selectedIdentityChanged);

Add service module to write PolKit agents #302

Are you sure you want to change the base?

Add service module to write PolKit agents #302

Conversation

Cu3PO42 commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

outfoxxed commented Oct 12, 2025

Uh oh!

Cu3PO42 commented Oct 12, 2025

Uh oh!

outfoxxed commented Oct 12, 2025

Uh oh!

Cu3PO42 commented Oct 12, 2025

Uh oh!

Cu3PO42 commented Oct 12, 2025

Uh oh!

outfoxxed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cu3PO42 commented Oct 13, 2025

Uh oh!

outfoxxed commented Oct 13, 2025

Uh oh!

Cu3PO42 commented Oct 13, 2025

Uh oh!

outfoxxed commented Oct 13, 2025

Uh oh!

Cu3PO42 commented Oct 13, 2025

Uh oh!

outfoxxed commented Oct 13, 2025

Uh oh!

Cu3PO42 commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

outfoxxed commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cu3PO42 commented Oct 14, 2025

Uh oh!

outfoxxed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cu3PO42 Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

outfoxxed Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

outfoxxed Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cu3PO42 commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

outfoxxed left a comment

Choose a reason for hiding this comment

Cu3PO42 commented Oct 11, 2025 •

edited

Loading

outfoxxed left a comment •

edited

Loading

Cu3PO42 commented Oct 13, 2025 •

edited

Loading

outfoxxed commented Oct 14, 2025 •

edited

Loading

Cu3PO42 Oct 15, 2025 •

edited

Loading

outfoxxed Oct 16, 2025 •

edited

Loading

outfoxxed Oct 17, 2025 •

edited

Loading

Cu3PO42 commented Oct 19, 2025 •

edited

Loading

Cu3PO42 Oct 20, 2025 •

edited

Loading

Cu3PO42 commented Oct 20, 2025 •

edited

Loading

Cu3PO42 commented Oct 23, 2025 •

edited

Loading