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

Authored by feverfew 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.Aug 26 2019, 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.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

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.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?

src/AuthBackend.h
63

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

src/backends/dbus/DBusHelperProxy.cpp
102

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.

LGTM.
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.
src/backends/polkit-1/Polkit1Backend.cpp
4

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

74

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

177

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

234

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?

src/kauthaction.cpp
23

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

53

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
src/backends/polkit-1/Polkit1Backend.cpp
234

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.

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

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.

src/backends/dbus/DBusHelperProxy.cpp
120

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

src/kauthaction.h
108

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)