Move the KDE Polkit-Agent into ksmserver
AbandonedPublic

Authored by davidedmundson on Nov 21 2017, 11:56 AM.

Details

Reviewers
None
Group Reviewers
Plasma
Summary

The rationale is two-fold:

  1. It fixes a security issue.

Right now, a rogue app can killall kde-policykit-agent-1 and register a
new authentication agent. Next time a user gets asked for a password, they get
prompted by the rogue app.

If KSMServer quits or doesn't start, the plasma session quits or doesn't
start.

  1. It's a boot speed-up.

Registering a few things in KSMServer is a lot quicker and lighter than
spawning a whole extra QApplication binary.


PolicyKit agent has been imported with the usual:
git filter-branch to move into a subdir, then add as a remote of p-w,
then merge.

Though given it's literally only 3 classes, I'm not sure the mess is
worth it.

Messages.sh file moved with the translation context the same. So it
should be headache free for translators (hopefully!)


Files changed in policykit-agent are polkitkde.* and it no longer
installs a .desktop file into the autostart folder.

Test Plan

Logged in
Didn't run the process
pkexec showed the right dialog
qdbus into ksmserver whilst the dialog is open shows ksmserver isn't blocked

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added a project: Plasma. · View Herald TranscriptNov 21 2017, 11:56 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol added a subscriber: apol.Nov 21 2017, 12:27 PM

+1 for having it all in once place.

This creates a "slight" problem for Wayland as ksmserver is forced to XCB. What do you think about doing it like with kscreenlocker? On X11 ksmserver, on Wayland KWin? Otherwise +1000 to this suggestion.

apol added a comment.Nov 22 2017, 8:20 AM

This creates a "slight" problem for Wayland as ksmserver is forced to XCB. What do you think about doing it like with kscreenlocker? On X11 ksmserver, on Wayland KWin? Otherwise +1000 to this suggestion.

Isn't the solution to stop forcing ksmserver to XCB?

In D8924#170463, @apol wrote:

This creates a "slight" problem for Wayland as ksmserver is forced to XCB. What do you think about doing it like with kscreenlocker? On X11 ksmserver, on Wayland KWin? Otherwise +1000 to this suggestion.

Isn't the solution to stop forcing ksmserver to XCB?

no. ksmserver is an X11 only tool. We have two options: get rid of ksmserver or leave it on X11. Porting to Wayland makes no sense. The main task of ksmserver is being an X11 Session Manager.

On X11 ksmserver, on Wayland KWin?

In principle great. It was the other possible choice.
But this spawns a window, generally kwin is bad at having it's own windows, so it might not be trivial.

I certainly don't want to go down the route of kwin holding the name, then spawning the dialog separately with a socket communicating back passwords.

The main task of ksmserver is being an X11 Session Manager.

Based on the name, sure.
Based on the code...I wouldn't say "main".

The autostart handling is just as important, and just as much code, we don't want that in a process that we consider irrelevant.

Thinking of the lonerg term picture, I see 3 options for handling that.

  1. We could split that out into it's own process and spawn that on both X and wayland.
  2. Autostart could be made into a lib, and added to the ever-growing kwind
  3. Or we could port the X session code to use xcb_connect instead of QX11Info::connection().. would be another option, there's no need for it to match what the QPA is using.

Decide that, and I think we're implicitly deciding what we do with this patch.

I fail to see how option 3 would help. But I'm in favor of option 1.

davidedmundson planned changes to this revision.Nov 29 2017, 9:13 AM

Yeah, I like that too.

davidedmundson abandoned this revision.Nov 30 2017, 11:32 AM

Somehow this went from "lets add a thing to ksmserver" to "lets split ksmserver"

Abandoning for now. Will return with something more extreme. (maybe for 5.13 at this point)