D-Bus: Send reply on auth failure

[hartwork]

In reaction to comment https://github.com/USBGuard/usbguard/pull/546#issuecomment-1100214103 by @benzea.

The effect can be seen e.g. when running…

$ dbus-send --system --print-reply --dest=org.usbguard1 /org/usbguard1 org.usbguard1.getParameter string:ImplicitPolicyTarget

… (as non-root) and then hitting the Cancel button in the PolKit auth dialog. Previously, dbus-send would continue to run waiting for a reply that is not coming with this pull request applied it shuts down properly saying:

Error org.freedesktop.DBus.Error.AccessDenied: Not authorized.

as expected.

Note that there are no changes to whether interactive authentication is requested from Polkit.

CC @benzea @radosroka @muelli

[hartwork]

@benzea @radosroka @muelli any thoughts? Thank you! :pray:

[benzea]

I would say, two possibilities:

1. Make the function itself return the invocation. As-is, you need to duplicate code for every call, which is just ugly.
2. Exceptions seem quite natural here (and you already have exception handling in handle_method_call of gdbus-server.cpp. So, add an exception that contain the DBus error type and throw that. Then you can just have a call assertAuthorizedByPolikit without any further logic.

[hartwork]

@benzea thanks for your review. Did you get a chance to try out the function aspect of this change — does it work for you as expected? Before I reply to your comment more: I'm happy if someone refactors this function more, later. Personally, I'm aiming for "fix the problem without adding new bugs" here for the moment.

I would say, two possibilities:

1. Make the function itself return the invocation. As-is, you need to duplicate code for every call, which is just ugly.

I considered that option, but pulling that into assertAuthorizedByPolikit would arguable make it do too much and would not longer fit that name; it also has many exit points, so doing more for each exit may even increase duplication. So I did consider it, and picked the lesser evil, or so I thought. PS: I should note that similar calls to g_dbus_method_invocation_return_error_* happen near and outside of assertAuthorizedByPolikit already, so the current additions match that current pattern or "approach to encapsulation", in a way.

2. Exceptions seem quite natural here (and you already have exception handling in handle_method_call of gdbus-server.cpp. So, add an exception that contain the DBus error type and throw that. Then you can just have a call assertAuthorizedByPolikit without any further logic.

The current code in isAuthorizedByPolkit is quite C-like and was merged being as C-like as it is. Moving this code here to exceptions now without adding memory leaks and without introducing smart pointers without significant risk and effort is not something I can offer at the moment. I'm hoping we can fix the bug in the current C-like function in a C-like way, and then someone other than me can refactor this to be super modern C++ after, if desired. That way probably everyone will be happy. Or if an alternative implementation of this that is more C++-like is realistic to have better chances to be merged sooner and with not introducing any more bugs than this version here, please everyone feel encouraged to create an alternative pull request, and I'll be happy if your version gets merged and also fixes the bug and is closer to authentic C++.

What do you think?

[hartwork]

@radosroka any thoughts?

[hartwork]

@radosroka any thoughts?

[hartwork]

@radosroka any thoughts?

[Cropi]

Hello,

I like the idea of introducing an automatic reply on authentication failure. At first, I thought that this was a security measure similar to when you want to change to a different user but enter the wrong password and it lets you wait a few seconds.

[Cropi]

Thanks for the PR!