Add commands to RunCommandPlugin - Desktop part

Authored by nicolasfella on Sep 23 2017, 10:10 PM.


Group Reviewers
KDE Connect
Test Plan

Works in plasma, did not test it with other DEs where notifications might work differently

Diff Detail

R224 KDE Connect
No Linters Available
No Unit Test Coverage
nicolasfella created this revision.Sep 23 2017, 10:10 PM

Let the other device know that it can add commands. Prevents newer Android Clients to show functionality that is not yet there on the desktop

apol added a subscriber: apol.Sep 27 2017, 1:18 PM

I'm unsure about this one. Could it become a security liability?
Having 1 of the systems compromised on the network will escalate on every device very easily with this...

What use-case are you thinking about solving?

I see your point. Right now, the UI to add a command is quite hidden. Most users probably don't even know that there is the Commands Plugin, and without adding commands its pretty useless. Maybe we could ask for the user to confirm the sent command in the desktop. What do you think about this?

A confirmation on the desktop would be a solution... but I also think that otherwise this could be dangerous. As of now, in KDE Connect there is no way to execute arbitrary commands on the other machine, so even if a device is compromised there isn't much it can do to the connected devices. With this patch, we are adding a way to execute anything we want on the other device.

albertvaka requested changes to this revision.Oct 13 2017, 11:01 PM
This revision now requires changes to proceed.Oct 13 2017, 11:01 PM
nicolasfella edited the test plan for this revision. (Show Details)

Ask user for confirmation. Updated android part as well

thomasp added inline comments.

maybe clarify to confirmAddCommand
same for the text below
e.g. "Confirm to add a command sent from a connected device."

nicolasfella edited the summary of this revision. (Show Details)

implement suggestion from @thomasp

thomasp added inline comments.Nov 7 2017, 5:41 PM

is a

return true;

missing here?

thomasp added inline comments.Nov 7 2017, 6:03 PM

config()->set(...) in addCommand already calls sendConfig() through the configChanged signal

nicolasfella edited the summary of this revision. (Show Details)Dec 20 2017, 9:07 PM
nicolasfella updated this revision to Diff 24190.EditedDec 20 2017, 9:27 PM
nicolasfella marked 3 inline comments as done.

Addressed comments

mtijink accepted this revision.Jan 14 2018, 2:11 PM
mtijink added a subscriber: mtijink.

Nice feature! The code looks good to me, and it appear to work well.

nicolasfella planned changes to this revision.Jan 20 2018, 1:35 AM

Something broke, have to figure out what.

@apol @albertvaka @mtijink can somebody please test if this works? It doesn't work for me out of nowhere. Looks like the slot in line 128 is never called.

It works fine for me.

nicolasfella requested review of this revision.Feb 22 2018, 12:17 AM

Issue seems to have gone

apol added a comment.Mar 5 2018, 3:02 PM

Let's discuss this at the sprint. I'm not convinced we want this.

I like it. It can wait until the sprint though.

Rebase on master

nicolasfella abandoned this revision.Apr 18 2018, 9:18 PM