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

[Harald Sitter]

sitter requested changes to this revision.
sitter added a comment.
This revision now requires changes to proceed.


  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

INLINE COMMENTS

> AuthBackend.h:61
>      virtual bool isCallerAuthorized(const QString &action, QByteArray callerID) = 0;
> +    virtual bool isCallerAuthorized(const QString &action, const DetailsMap &details, QByteArray callerID) = 0; // KF6 TODO Merge
>      virtual bool actionExists(const QString &action);

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?

> DBusHelperProxy.cpp:109
>  
> +    qDebug() << details;
>      QList<QVariant> args;

left over debugging? either remove or categorize

> Polkit1Backend.cpp:187
> +    QMap<QString, QString> polkit1Details;
> +    for (auto it = details.begin(); it != details.end(); ++it) {
> +        polkit1Details.insert(it.key(), it.value().toString());

you want constBegin/End here

> Polkit1Backend.cpp:193
>              &e, SLOT(requestQuit(PolkitQt1::Authority::Result)));
> -    authority->checkAuthorization(action, subject, PolkitQt1::Authority::AllowUserInteraction);
> +    authority->checkAuthorizationWithDetails(action, subject, PolkitQt1::Authority::AllowUserInteraction, polkit1Details);
>      e.exec();

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.

> kauthaction.cpp:77
>  Action::Action(const QString &name, const QString &details)
>      : d(new ActionData())
>  {

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

> kauthaction.h:236
> +     */
> +    void setDetails(const DetailsMap &details);
> +

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

> kauthaction.h:246
> +     */
> +    QString details() const; // KF6 TODO: Remove
>  

Should the old functions maybe be marked deprecated?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D21795

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: sitter, mreeves, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190721/d3356dd8/attachment-0001.html>