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.


  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

INLINE COMMENTS

> chinmoyr wrote in AuthBackend.h:61
> the mac backend modifies `callerID` later on so I think it was deliberately kept here.

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.

> DBusHelperProxy.cpp:102
>      QDBusMessage message;
>      message = QDBusMessage::createMethodCall(helperID, QLatin1String("/"), QLatin1String("org.kde.kf5auth"), QLatin1String("performAction"));
>  

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.

> chinmoyr wrote in Polkit1Backend.cpp:193
> I am not sure what to do here. The commit didn't really change the version and it is not of KF5 either.

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

> Polkit1Backend.h:55
>      bool actionExists(const QString &action) override;
> +    QVariantMap getBackendDetails(const DetailsMap &details) override;
>  

We generally do not use get prefixes.

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

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

> kauthaction.h:262
> +     * @return The action's details
> +     * @deprecated since 5.62, use details() with DetailsMap.
>       */

should say V2

REPOSITORY
  R283 KAuth

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

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