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

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

Details

Summary

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

Repository
R283 KAuth
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15192
Build 15210: arc lint + arc unit
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

src/AuthBackend.h
63

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?

src/backends/dbus/DBusHelperProxy.cpp
104

left over debugging? either remove or categorize

src/backends/polkit-1/Polkit1Backend.cpp
182

you want constBegin/End here

188

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.

src/kauthaction.cpp
84

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

src/kauthaction.h
278

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

288

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
src/AuthBackend.h
63

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

src/kauthaction.h
288

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

chinmoyr added inline comments.Mon, Aug 26, 1:38 AM
src/backends/polkit-1/Polkit1Backend.cpp
188

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.Mon, Aug 26, 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

src/AuthBackend.h
63

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.

src/backends/dbus/DBusHelperProxy.cpp
102

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.

src/backends/polkit-1/Polkit1Backend.cpp
188

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)
    authority->checkAuthorizationWithDetails(...
#else
    authority->checkAuthorization(...
#endif
src/backends/polkit-1/Polkit1Backend.h
55

We generally do not use get prefixes.

src/kauthaction.h
253

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

262

should say V2

This revision now requires changes to proceed.Mon, Aug 26, 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.