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

2020-05-12 Thread Nathaniel Graham
ngraham edited the summary of this revision.

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, bruns


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

2020-05-12 Thread Nathaniel Graham
ngraham edited the summary of this revision.

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, bruns


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

2020-03-01 Thread Alexander Saoutkin
This revision was automatically updated to reflect the committed changes.
Closed by commit R283:f53d6a29a049: [KAuth] Add support for action details in 
Polkit1 backend. (authored by feverfew).

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=76739=76740

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-03-01 Thread Alexander Saoutkin
feverfew updated this revision to Diff 76739.
feverfew added a comment.


  - Fix &
  - Use enum class
  - Use enum instead of string comparison

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=76497=76739

BRANCH
  arcpatch-D21795_2

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-28 Thread Harald Sitter
sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  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.

INLINE COMMENTS

> DBusHelperProxy.cpp:120
>  if (reply.type() == QDBusMessage::ErrorMessage) {
> +if (reply.errorName() == 
> QStringLiteral("org.freedesktop.DBus.Error.InvalidArgs")) {
> +// For backwards compatibility if helper binary was built 
> with older KAuth version.

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

> kauthaction.h:108
> + */
> +enum AuthDetail {
> +DetailOther = 0,

consider enum class

REPOSITORY
  R283 KAuth

BRANCH
  arcpatch-D21795_1

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-26 Thread Alexander Saoutkin
feverfew added a comment.


  I believe most of your (@sitter) comments (apart from the misaligned `&` 
probably were caused by me forgetting to rebase), lmk if otherwise.

INLINE COMMENTS

> sitter wrote in Polkit1Backend.cpp:75
> Is there a reason you use stringy connection syntax instead of  syntax?

This line wasn't changed?

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-26 Thread Alexander Saoutkin
feverfew updated this revision to Diff 76497.
feverfew added a comment.


  - Merge branch 'master' into arcpatch-D21795_1
  - Update version

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=76419=76497

BRANCH
  arcpatch-D21795_1

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-26 Thread Alexander Saoutkin
feverfew planned changes to this revision.
feverfew added a comment.


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

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-26 Thread David Faure
dfaure added inline comments.

INLINE COMMENTS

> sitter wrote in Polkit1Backend.cpp:239
> M. M. 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?

This code is FULL of nested event loops.

Does it run in a GUI process, or in some sort of separate backend process?

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-26 Thread Harald Sitter
sitter requested changes to this revision.
sitter added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> Polkit1Backend.cpp:4
>  *   Copyright (C) 2009 Radek Novacek 
>  *   Copyright (C) 2009-2010 Dario Freddi 
>  *

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

> Polkit1Backend.cpp:75
>  connect(PolkitQt1::Authority::instance(), 
> SIGNAL(enumerateActionsFinished(PolkitQt1::ActionDescription::List)),
>  this, 
> SLOT(updateCachedActions(PolkitQt1::ActionDescription::List)));
>  

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

> Polkit1Backend.cpp:178
>  
> -bool Polkit1Backend::isCallerAuthorized(const QString , QByteArray 
> callerID)
> +bool Polkit1Backend::isCallerAuthorized(const QString& action, const 
> QByteArray , const QVariantMap& details)
>  {

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

> Polkit1Backend.cpp:239
>  // Wait max 2 seconds
>  QEventLoop e;
>  QTimer::singleShot(200, , SLOT(quit()));

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

> kauthaction.cpp:23
>  #include 
>  #include 
>  

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

> kauthaction.cpp:53
>  QVariantMap args;
>  bool valid;
>  QWidget *parent = nullptr;

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.

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-25 Thread Alexander Saoutkin
feverfew updated this revision to Diff 76419.
feverfew added a comment.


  - Fix the diff

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=76073=76419

BRANCH
  arcpatch-D21795_1

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/policykit/PolicyKitBackend.cpp
  src/backends/policykit/PolicyKitBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-21 Thread Harald Sitter
sitter added a comment.


  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

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-20 Thread Alexander Saoutkin
feverfew added a comment.


  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?

INLINE COMMENTS

> sitter wrote in AuthBackend.h:61
> 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.

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

> sitter wrote in 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.

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.

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-20 Thread Alexander Saoutkin
feverfew updated this revision to Diff 76073.
feverfew added a comment.


  - add const &

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=76071=76073

BRANCH
  arcpatch-D21795

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/policykit/PolicyKitBackend.cpp
  src/backends/policykit/PolicyKitBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-20 Thread Alexander Saoutkin
feverfew updated this revision to Diff 76071.
feverfew marked 4 inline comments as done.
feverfew added a comment.


  - Rebase
  - Respond to sitter's comments

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=63880=76071

BRANCH
  arcpatch-D21795

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/policykit/PolicyKitBackend.cpp
  src/backends/policykit/PolicyKitBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2020-02-20 Thread Alexander Saoutkin
feverfew commandeered this revision.
feverfew added a reviewer: chinmoyr.

REPOSITORY
  R283 KAuth

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

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, 
chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns


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

2019-12-22 Thread Nathaniel Graham
ngraham added a comment.


  @chinmoyr now that D21783  has landed, 
this is all that's left before we can turn on the feature, right?

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, bruns


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

2019-10-23 Thread Nathaniel Graham
ngraham added a task: T8075: Fix security issues with KAuth support in KIO.

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, bruns


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

2019-09-13 Thread Elvis Angelaccio
elvisangelaccio added a comment.


  polkit-qt-1-0.113.0 has been released, so I believe now we can move over with 
this patch.

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, bruns


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

2019-08-29 Thread Ben Cooksley
bcooksley removed a subscriber: fsitter.

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns, fsitter


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

2019-08-29 Thread Fuk Sitter
fsitter added a comment.


  You think sending your minions to insult me and then disabling my account 
will solve the issue? what will you do next? Disable registration so no one 
points out your hypocrisy? Which overlord made the decision to disable my 
account and for what?

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: fsitter, ngraham, sitter, mreeves, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns


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

2019-08-26 Thread 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 ) override;
> +QVariantMap getBackendDetails(const DetailsMap ) override;
>  

We generally do not use get prefixes.

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

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


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

2019-08-25 Thread Chinmoy Ranjan Pradhan
chinmoyr added inline comments.

INLINE COMMENTS

> sitter wrote in Polkit1Backend.cpp:193
> 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.

I am not sure what to do here. The commit didn't really change the version and 
it is not of KF5 either.

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


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

2019-08-16 Thread Chinmoy Ranjan Pradhan
chinmoyr added inline comments.

INLINE COMMENTS

> sitter wrote in AuthBackend.h:61
> 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?

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

> sitter wrote in kauthaction.h:246
> Should the old functions maybe be marked deprecated?

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

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, michaelh, bruns


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

2019-08-16 Thread Chinmoy Ranjan Pradhan
chinmoyr updated this revision to Diff 63880.
chinmoyr marked 6 inline comments as done.
chinmoyr added a comment.


  Addressed comments.

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21795?vs=59775=63880

BRANCH
  master

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.cpp
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/policykit/PolicyKitBackend.cpp
  src/backends/policykit/PolicyKitBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: ngraham, sitter, mreeves, kde-frameworks-devel, LeGast00n, michaelh, bruns


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

2019-08-13 Thread Nathaniel Graham
ngraham added a comment.


  @chinmoyr, any word on  this?

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, michaelh, bruns


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

2019-07-21 Thread 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 , QByteArray 
> callerID) = 0;
> +virtual bool isCallerAuthorized(const QString , const DetailsMap 
> , QByteArray callerID) = 0; // KF6 TODO Merge
>  virtual bool actionExists(const QString );

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 args;

left over debugging? either remove or categorize

> Polkit1Backend.cpp:187
> +QMap 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
>  , 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 , const QString )
>  : 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 );
> +

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


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

2019-07-11 Thread Nathaniel Graham
ngraham added reviewers: dfaure, cfeck.

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck
Cc: mreeves, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


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

2019-06-21 Thread Nathaniel Graham
ngraham added a reviewer: Frameworks.

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks
Cc: mreeves, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


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

2019-06-14 Thread Chinmoy Ranjan Pradhan
chinmoyr created this revision.
chinmoyr added reviewers: apol, bruns, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
chinmoyr requested review of this revision.

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

REPOSITORY
  R283 KAuth

BRANCH
  arcpatch-D18845

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

AFFECTED FILES
  autotests/TestBackend.cpp
  autotests/TestBackend.h
  src/AuthBackend.h
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fake/FakeBackend.cpp
  src/backends/fake/FakeBackend.h
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/backends/mac/AuthServicesBackend.cpp
  src/backends/mac/AuthServicesBackend.h
  src/backends/policykit/PolicyKitBackend.cpp
  src/backends/policykit/PolicyKitBackend.h
  src/backends/polkit-1/Polkit1Backend.cpp
  src/backends/polkit-1/Polkit1Backend.h
  src/kauthaction.cpp
  src/kauthaction.h
  src/kauthexecutejob.cpp

To: chinmoyr, apol, bruns, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns