jack-ullery / AppAnvil

Graphical user interface for the AppArmor security module (in-progress)
GNU General Public License v3.0
14 stars 12 forks source link

Prevent multiple command failures #70

Closed Skeevert closed 11 months ago

Skeevert commented 11 months ago

What kinda annoyed me is that if I reject authorization, it keeps asking me for it multiple times. This PR should prevent running any further commands in case of a failure. If this approach is not applicable (i.e. you prefer to avoid throwing exceptions), please let me know and I'll fix it.

jack-ullery commented 11 months ago

I totally agree with you that the current behavior is annoying and undesirable. If the user cancels authorization, we should not spam them until they accept. This behavior is a side-affect of how "ConsoleThread" was implemented.

I think your implementation is mostly good, but I would've preferred to brainstorm the expected behavior in an issue first. This would help you and ensure that you don't waste time re-implementing things.

Some thoughts about possible expected behavior

It would be preferable if we could find a solution that does not kill the thread. If the user cancels authorization for aa-caller, that should not affect other things.

Furthermore, if they cancel the authorization, there should be some sort of button to re-enable it later. Currently, if the authorization to aa-caller is canceled immediately, then the application is permanently useless until it is restarted.

Alternatively, we can consider changing the TIME_WAIT variable to be longer so that this is less annoying, or removing polling altogether and only asking for authentication once, but that doesn't really solve the problem IMO.

Notes about implementation

First note: One problem with the proposed implementation is that if the user attempts to change a profile's status and cancels authentication, then the CommandCaller thread quits and the user cannot change any other profile status.

It might be beneficial to modify CommandCaller::get_status() instead of CommandCaller::call_command().

Second note: Also I'm not sure about these lines:

https://github.com/Skeevert/AppAnvil/blob/30833e5900c4e01394e52778ad5e31ca8502de7a/src/console_thread.cc#L150-L151

command_caller() should be running on the second thread (when we construct the asynchronous_thread field). So calling, send_quit_message() will do nothing as we've already exited the main while-loop in that function. Similarly, calling asynchronous_thread.wait() tells the thread to wait for itself, which might cause the thread to hang.

The difference with ~CommandCaller() is that the destructor is on the parent thread.

Conclusion

Wow, I wrote an essay. Thanks for bringing up this bug and making a PR. I think it's a good first start and we should probably discuss the expected behavior more (either here or in a new issue)

Skeevert commented 11 months ago

Thanks for such a detailed answer. As you're the owner of this repository, I think that it's up to you whether to continue discussion here or move it to the separate issue. I think that the optimal solution will be to either close the app (which was my intention when I cancelled authorization) or to add an authorization button, but I'm not very proficient with Gtk in general and Gtkmm in particular. I think that if this application is meant to be used exclusively with superuser privileges and there are no plans for adding any functionality which will not require it, the most sensible option would be to quit. If such functionality is planned (or even already implemented), then the button would be more suitable for this case.

jack-ullery commented 11 months ago

I think I'll probably implement the button then and use your code as a base.

I intended the GUI to run without super-user permissions, requesting them through aa-caller when needed. You can alternatively run sudo appanvil or pkexec appanvil if you don't want to see these prompts

Skeevert commented 11 months ago

Sure. So should I close this PR?

jack-ullery commented 11 months ago

I merged it into a staging branch to work on. Thanks again