authority: add support for passing details to polkit
ClosedPublic

Authored by mgerstner on Feb 8 2019, 10:43 AM.

Details

Summary

Currently polkit-qt-1 doesn't support to pass details during the checkAuthorization() call. Details are for example required to employ runtime generated authorization messages that contain placeholders like "authentication required for accessing $(device)". This feature can be helpful to improve the end user experience and security, since the exact context of a privileged operation can be expressed better. Polkit documentation about this details parameter can be found here.

This change adds alternative versions of checkAuthorization() and checkAuthorizationSync() that accepts an additional QMap<QString, QString> details parameter. In the implementation this map is converted into the required polkit level PolkitDetails type and passed to the according polkit functions as necessary.

To conform with the KDE binary compatibility policy I didn't extend or overload the existing checkAuthorization() and checkAuthorizationSync() functions, but introduced newly named functions.

Diff Detail

Repository
R563 Polkit-1 Qt Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgerstner requested review of this revision.Feb 8 2019, 10:43 AM
mgerstner created this revision.
bruns added inline comments.Feb 15 2019, 2:27 AM
core/polkitqt1-authority.cpp
130

nullptr

328

nullptr

332

whitespace

422

nullptr, or just if (pk_details) {

core/polkitqt1-authority.h
226

please add a KF6 TODO, merge with checkAuthorization

mgerstner updated this revision to Diff 51753.Feb 15 2019, 1:29 PM

Incorporated review comments: replaced NULL by nullptr, removed some extra whitespace within parantheses, added KF6 TODO.

mgerstner added inline comments.Feb 15 2019, 1:31 PM
core/polkitqt1-authority.cpp
328

I didn't want to mix styles in the source files. It's adjusted now.

bruns added a comment.Feb 20 2019, 6:55 PM

Does this solve part of T8075?

Does this solve part of T8075?

Part of a part it seems. I am currently working towards T10480. To actually add polkit details to authentication messages further changes in the kauth component are required once this change here is accepted.

mati865 added a subscriber: mati865.Mar 8 2019, 6:42 PM

LGTM to me as well - I'm not familiar with this code at all though.

I would like this patch to land so +1 from me. But I am not familiar with this code so I can't give any meaningful feedback. Sorry.

core/polkitqt1-authority.cpp
336

Nitpick; constData() because the API seems to take const gchar*

Three LGTMs and +1 are pretty good :)

bruns requested changes to this revision.Mar 18 2019, 5:21 PM

Can you address the last nitpick from @chinmoyr ?

This revision now requires changes to proceed.Mar 18 2019, 5:21 PM
mgerstner added inline comments.Wed, Mar 27, 11:07 AM
core/polkitqt1-authority.cpp
336

Strictly spoken it already returns const gchar*, since toUtf8() returns a const QByteArray temporary object and thus the const char* data() const member function should be called here.

Anways I will make it explicit like you suggest.

mgerstner updated this revision to Diff 54931.EditedWed, Mar 27, 12:54 PM

Now using constData() as suggested by chinmoyr.

bruns accepted this revision.Wed, Mar 27, 4:37 PM

Thanks

This revision is now accepted and ready to land.Wed, Mar 27, 4:37 PM

@mgerstner can you provide your email address so we can land this patch with correct authorship information?

@mgerstner can you provide your email address so we can land this patch with correct authorship information?

It's matthias.gerstner@suse.com.

elvisangelaccio added inline comments.
core/polkitqt1-authority.h
226

@bruns The polkit-qt-1 repository is not a framework, so "KF6" could be misleading here.

What about something like

// TODO: merge with checkAuthorization when we decide to break binary compatibility.

?

So what's next for this? Can this land now or do we need to change the comment first?

davidedmundson accepted this revision.Tue, Apr 2, 5:08 PM
This revision was automatically updated to reflect the committed changes.

Landed and changed the comment. @chinmoyr, what's next for T8075: Fix security issues with KAuth support in KIO?