[KAuth] Add support for action details in Polkit1 backend.

Authored by feverfew on Jun 14 2019, 6:12 AM.



KAuth action has a setDetail method. This can be used to override the message shown in the authentication dialog.
However, until now none of the backends actually used it. So this patch;

  • Overloads setDetail method. The overload accepts details in a QMap.
  • Makes use of the details in polkit1 backend.

Diff Detail

R283 KAuth
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Jun 14 2019, 6:12 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 14 2019, 6:12 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Jun 14 2019, 6:12 AM
sitter requested changes to this revision.Jul 21 2019, 10:42 AM
sitter added a subscriber: sitter.

The way details are enumerated needs changing IMO. And this effectively bumps the required polkitqt version through exactly one call, so maybe an #if conditional to not have the forced bump might be good


I didn't check super carefully but at a glance the backend api is not public API so we could probably refactor this right now already.

Also, shouldn't the callerID be a const ref?


left over debugging? either remove or categorize


you want constBegin/End here


looks to me this was only added 4 months ago. so this would probably require a version bump. seeing as we are fairly conservative with frameworks' requirements it may be better to if version>=whatevs the call.


you could just delegate to the new constructor here instead of having two code duplicated ctors.


this seems like super leaky abstraction. you are allowing the caller to set backend specific stuff here. I think it'd be much better to make an enum for detail types, and have this be a QMap of enum,QVariant.

if the caller sets polkit.message then that won't apply to the mac backend even if someone were to implement the relevant functionality there. if it is a general purpose enum key each backend can easily implement or ignore as necessary


Should the old functions maybe be marked deprecated?

This revision now requires changes to proceed.Jul 21 2019, 10:42 AM

@chinmoyr, any word on this?

chinmoyr updated this revision to Diff 63880.Aug 16 2019, 6:22 PM
chinmoyr marked 6 inline comments as done.

Addressed comments.

chinmoyr added inline comments.Aug 16 2019, 6:23 PM

the mac backend modifies callerID later on so I think it was deliberately kept here.


I deprecated it. Except the unit tests this isn't used anywhere in the backend.

chinmoyr added inline comments.Aug 26 2019, 1:38 AM

I am not sure what to do here. The commit didn't really change the version and it is not of KF5 either.

sitter requested changes to this revision.Aug 26 2019, 10:39 AM

Unfortunately I've noticed a huge blocker...
polkit-qt-1 with the checkAuthorizationWithDetails change is not actually released. That needs fixing :| https://community.kde.org/ReleasingSoftware


I wonder if we should change that. Right now every backend gets a QBA copy even when they don't need to modify it. So it sounds to me like it should be const& and the mac backend should make a copy on its stack. Not that it matters a great deal though, so if you disagree that's fine too.


We need to be backwards compatible here. As far as I can tell this is where we call the actual helper binary. The helper binary may have been built with an older version of kauth, so it doesn't necessarily understand the new API.

You could call org.freedesktop.DBus.Introspectable to figure out which method arguments it supports, or possibly the simpler approach is to could with new arguments and if that results in org.freedesktop.DBus.Error.InvalidArgs try again with old arguments before giving up.


What I mean is PolkitQt1::checkAuthorizationWithDetails was only introduced some months ago, so you can't just assume it's available.
You'll want to guard it with something like

#if POLKITQT1_IS_VERSION(0, 113, 0)

We generally do not use get prefixes.


For consistency with the associated getter I would actually call this setDetailsV2 even though technically not necessary.


should say V2

This revision now requires changes to proceed.Aug 26 2019, 10:39 AM
This comment was removed by bcooksley.
bcooksley removed a subscriber: fsitter.

polkit-qt-1-0.113.0 has been released, so I believe now we can move over with this patch.

@chinmoyr now that D21783 has landed, this is all that's left before we can turn on the feature, right?

feverfew commandeered this revision.Feb 20 2020, 10:32 PM
feverfew added a reviewer: chinmoyr.
feverfew updated this revision to Diff 76071.Feb 20 2020, 10:33 PM
feverfew marked 4 inline comments as done.
  • Rebase
  • Respond to sitter's comments
feverfew updated this revision to Diff 76073.Feb 20 2020, 10:43 PM
  • add const &

Ok I've commandeered this on request, as we're so close to getting this done. I've simply addressed sitter's comments here seeming as they're simple enough to do so without understanding the code that well. I haven't actually tested this in any capacity, as again, I'm not too familiar with this code and what it's trying to accomplish. If someone could point me in the correct direction I'll test as well.

Also the way Phab has parsed this diff looks odd to me. It claims there's been a copy between two of the files but looking locally this hasn't happened AFAICT (and I definitely didn't intend so). I also did a git diff before arc diff and I didn't notice anything odd there. Could someone do a sanity check for me in that regard?


Looking at the code your intuition was correct, i.e. none of the QBAs were modified, so I've switched to const &


Done as requested. Note three's a blocking call in the slot, but I'm assuming (for simplicity's sake) that's ok. LMK if otherwise.

I do wonder if the new deprecation annotations should be used for the the deprecation though: https://api.kde.org/ecm/module/ECMGenerateExportHeader.html

feverfew updated this revision to Diff 76419.Feb 25 2020, 8:35 PM
  • Fix the diff
sitter requested changes to this revision.Feb 26 2020, 11:07 AM
sitter added inline comments.

According to the diff of the diff you seem to have lost David's copyright


Is there a reason you use stringy connection syntax instead of &member syntax?


action has & on the wrong size of the space, same for details ;)


Mhhhh. Mhhhhhhhh. I don't really have a suggestion here, but this is an incredibly dangerous change. Nested event loops can cause all sorts of negative effects. That's why the isValid had a note in the documentation, to tell the frontend dev to be careful. By adding a new loop in existing api we add a pit to fall into. I really don't know what can be done about it though. Could we get by with 1 second maybe?


I think that is deprecated, or at least not recommended for use anymore. You'll want QRegularExpression


Mh, minor annoyance. valid and timeout should get their defaults set here as a more modern best practice and also because parent is already set for consistency. That default ctor is kinda pointless then and should be dropped IMHO.

This revision now requires changes to proceed.Feb 26 2020, 11:07 AM
dfaure added inline comments.Feb 26 2020, 11:26 AM

This code is FULL of nested event loops.

Does it run in a GUI process, or in some sort of separate backend process?

feverfew planned changes to this revision.Feb 26 2020, 2:08 PM

Ok, I see what's going on here. Earlier I mucked up the diff a bit and had to go back to different diff id and reapply my changes. In the process I forgot to rebase onto master. Once I do that David's copyright will be back in (and the event loop goes with it).

feverfew updated this revision to Diff 76497.Feb 26 2020, 9:42 PM
  • Merge branch 'master' into arcpatch-D21795_1
  • Update version

I believe most of your (@sitter) comments (apart from the misaligned & probably were caused by me forgetting to rebase), lmk if otherwise.


This line wasn't changed?

sitter accepted this revision.Feb 28 2020, 1:07 PM

Much better. Since nobody has input on the deprecations I guess they must be alright. Do have a look into using QDBusError::InvalidArgs before landing though.


Probably better to us the abstraction enum if (watcher->error().type() == QDBusError::InvalidArgs) {


consider enum class

This revision is now accepted and ready to land.Feb 28 2020, 1:07 PM
feverfew updated this revision to Diff 76739.Mar 1 2020, 5:56 PM
  • Fix &
  • Use enum class
  • Use enum instead of string comparison
This revision was automatically updated to reflect the committed changes.
ngraham edited the summary of this revision. (Show Details)May 12 2020, 7:48 PM
ngraham edited the summary of this revision. (Show Details)