keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.07k stars 1.42k forks source link

WIP: Wayland autotype implementation (using xdg-desktop-portal) #10905

Open TheConfuZzledDude opened 3 weeks ago

TheConfuZzledDude commented 3 weeks ago

Fixes #2281

This is a work-in-progress attempt to implement auto-type on Wayland, using the RemoteDesktop portal. Caveats currently include:

All testing so far I've done on KDE Plasma 6.1, and the core functionality seems to work great there. Testing other platforms is a bit annoying to do, so I'd appreciate people trying it and giving feedback

Screenshots

Testing strategy

Type of change

droidmonkey commented 3 weeks ago

Geez sure is nice when you don't have to muck with emulated key presses! I wonder if we could use this same method for X11 as well, do you know?

TheConfuZzledDude commented 3 weeks ago

Geez sure is nice when you don't have to muck with emulated key presses! I wonder if we could use this same method for X11 as well, do you know?

Technically it should, as long as there's a desktop portal running that works on it, so I have a feeling KDE and GNOME would, but I don't think there's a generic portal that supports RemoteDesktop for other X11 WMs.

Also, it is very nice I can just chuck keysyms at it without worrying about reversing keymaps and whatnot :)

droidmonkey commented 3 weeks ago

Worth a try to see if that fixes some of our awkward issues with international keyboard layout.

hifi commented 3 weeks ago

Very nice, I will give this a spin!

hifi commented 3 weeks ago

Applied a small patch to build on Debian bookworm and to fix some assumptions about a window title existing and the shortcut crash and to disable minimize behavior if the main window is inactive, didn't test X11 still works: https://gist.github.com/hifi/e32c03f7b131cfc9e2d38c1ca11764fe

I registered this command with KDE to perform globally: dbus-send --session --type=method_call --dest=org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.requestGlobalAutoType

Good work! I will stay on Wayland for now as this implements everything I need. Just needs some cleanups.

Zahrun commented 3 weeks ago

Thanks @TheConfuZzledDude for the PR Thanks @droidmonkey for all the work on KeepassXC Thanks @hifi for the patch

I was able to build this branch on my Tuxedo OS 3 (Plasma 6.0.5) after applying @hifi ’s patch.

If I perform AutoType from the KeePassXC window, that one does not get minimized. That is due to the && qApp->activeWindow()) added by @hifi . Works fine once reverted that line.

Assigned a global keyboard shortcut to dbus-send --session --type=method_call --dest=org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.requestGlobalAutoType in Plasma, that’s pretty sick, just need to press the shortcut, type the name of the entry, select it, and it will autotype.

The only issue I’m having is that when I switch keyboard layout, the autotype changes. On my default fr oss layout it types correctly. Changing to fr bepo_afnor or us intl types wrong characters. It is actually pressing the same keys on the keyboard, but now they have different characters.

hifi commented 3 weeks ago

My bad about minimizing not working as I didn't test it properly. The intention was to not minimize the main window on global auto-type if the main window isn't the active window (aka entry auto-type).

Also it's a bit funny keyboard layouts are still an issue as they are a nightmare on X11.

TheConfuZzledDude commented 2 weeks ago

Thanks @TheConfuZzledDude for the PR Thanks @droidmonkey for all the work on KeepassXC Thanks @hifi for the patch

I was able to build this branch on my Tuxedo OS 3 (Plasma 6.0.5) after applying @hifi ’s patch.

If I perform AutoType from the KeePassXC window, that one does not get minimized. That is due to the && qApp->activeWindow()) added by @hifi . Works fine once reverted that line.

Assigned a global keyboard shortcut to dbus-send --session --type=method_call --dest=org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.requestGlobalAutoType in Plasma, that’s pretty sick, just need to press the shortcut, type the name of the entry, select it, and it will autotype.

The only issue I’m having is that when I switch keyboard layout, the autotype changes. On my default fr oss layout it types correctly. Changing to fr bepo_afnor or us intl types wrong characters. It is actually pressing the same keys on the keyboard, but now they have different characters.

You know I really would hope that the desktop portal would be able to query the keymap correctly, but I guess not. I think that using libei would allow getting the actual current keymap (from reading through the KWin code, it seems like it would there at least). I have a branch locally where I started implementing it with EI, but it's sooooo much more of a hassle, and KDE doesn't actually support it until 6.1 is released (they say they implement v2 of the protocol but they don't :P), and I ran into issues where the keymap FD they were providing was perpetually invalid after the first time.

So I'd rather stick with this and maybe drop a question to the portal maintainers about whether they should be querying the current keymap when we request a keysym (or if we should do the exact same reverse mapping just with 200% extra effort)

droidmonkey commented 2 weeks ago

We should absolutely be pushing this onto the portal maintainers to handle. Direct query of the keymap or have them align the key maps accordingly between apps. I say keep current implementation and caveat that it only applies to keymaps that are largely US layout based.

TheConfuZzledDude commented 2 weeks ago

@Zahrun If you'd like a workaround for now, you can set up keyboard shortcuts in the KDE settings to switch layout to a default pc105 keyboard layout, and one to switch to the previous layout. Then, set the default auto-type sequence for your group to press the respective shortcuts

hifi commented 2 weeks ago

I've been using this for a couple of days now and it's been working very well in practice with the known caveats.

@TheConfuZzledDude Regarding keymap, does it make a difference if you start the remote desktop portal at the time of starting Auto-Type? If it helps then we could make it do the initial authorization for remote desktop on startup but immediately close the portal so that it can reopen it on demand to use the current keymap.

Zahrun commented 2 weeks ago

@droidmonkey I also think that the layout management issue should be raised on the desktop portal side.

@TheConfuZzledDude thank you for the workaround suggestion. I have a lot of custom auto-type sequences, so I prefer to switch layout, then do the auto-type, and then switch back manually. I'm actually currently using a script to do that with KPUInput in KeePass2 in mono. I'll adjust it to work with your implementation in KeePassXC. I hope one day the autotype will work with any layout configuration.

@hifi In my experience, it does not matter in which layout I am when starting the portal. It seems to be always using fr oss, which is the layout I have in SDDM and in the ttys. It's not even the first layout in the layout setting list inside Plasma.

EDIT: I am able to reproduce the keyboard layout issue with KDE Connect when typing from my Android device to the Plasma desktop, through xdg-desktop-portal, the keyboard layout issue is also happening.

Zahrun commented 2 weeks ago

I created an issue on the KDE side for xdg-desktop-portal-kde at https://bugs.kde.org/show_bug.cgi?id=489021

TheConfuZzledDude commented 1 week ago

I created an issue on the KDE side for xdg-desktop-portal-kde at https://bugs.kde.org/show_bug.cgi?id=489021

Thanks! I was thinking of doing that myself, but I think I might post it on https://invent.kde.org/plasma/xdg-desktop-portal-kde/-/issues because it seems more active for the desktop portal

Regardless, I'm going to try and clean this up a bit in the next week

Zahrun commented 1 week ago

I opened the issue on bugs.kde.org due to the following guideline.

Please note that all bug reports and feature requests should be filed on [bugs.kde.org](https://bugs.kde.org/) and should never be raised here.
For support with KDE software, it is recommended to post on [KDE Discuss](https://discuss.kde.org/) instead.

Issues on KDE Invent are used for tracking ongoing work and are for the use of contributors and developers only.

Users are asked to consult with a developer or other contributor prior to opening issues here, and where in doubt recommended to open them on bugs.kde.org instead.

Apart from that, I have a suggestion for the feature. Currently, when we open KeePassXC, it directly asks for Remote Control permission. New users might get confused by that. I think it would be good to display a message "We will ask for permission, this is for autotype feature". Or even give the option yes/no. Or probably best, ask for permission only if the user asks for an autotype to be done. And then keep the token.

hifi commented 1 week ago

Apart from that, I have a suggestion for the feature. Currently, when we open KeePassXC, it directly asks for Remote Control permission. New users might get confused by that. I think it would be good to display a message "We will ask for permission, this is for autotype feature". Or even give the option yes/no. Or probably best, ask for permission only if the user asks for an autotype to be done. And then keep the token.

It could just request the remote desktop portal when being triggered and then keep it open after that until next restart. There's no need to open the portal at all if the user never uses auto-type. That's very likely for a lot of people who mostly only use the browser integration.

michaelk83 commented 1 week ago

@TheConfuZzledDude I think your input is needed at https://bugs.kde.org/show_bug.cgi?id=489021 :

Can you show where the bug is in our portal implementation? And maybe also update the title to describe the problem more fully?

michaelk83 commented 1 week ago

Btw, does the desktop portal specification specify how it should deal with different keyboard mappings? If not, then this should be brought up with them as well. Otherwise, you'll have to chase each DE to get them to do what is needed.

TheConfuZzledDude commented 1 week ago

@TheConfuZzledDude I think your input is needed at https://bugs.kde.org/show_bug.cgi?id=489021 :

Can you show where the bug is in our portal implementation? And maybe also update the title to describe the problem more fully?

Just commented, found where the issue was, depending on how fast I get through the rest of this, I may go fix it myself if it's not by the time I'm done

Btw, does the desktop portal specification specify how it should deal with different keyboard mappings? If not, then this should be brought up with them as well. Otherwise, you'll have to chase each DE to get them to do what is needed.

Not really, the remote desktop documentation is kinda sparse apart from what's implied from the method name. I did some research, and it seems like the protocol was defined for/based on Mutter's remote desktop portal, so I think that's probably the "canonical" behaviour

Apart from that, I have a suggestion for the feature. Currently, when we open KeePassXC, it directly asks for Remote Control permission. New users might get confused by that. I think it would be good to display a message "We will ask for permission, this is for autotype feature". Or even give the option yes/no. Or probably best, ask for permission only if the user asks for an autotype to be done. And then keep the token.

It could just request the remote desktop portal when being triggered and then keep it open after that until next restart. There's no need to open the portal at all if the user never uses auto-type. That's very likely for a lot of people who mostly only use the browser integration.

That is the plan, but there's a few issues to work around, and might require changing the autotype plugin behaviour in general. When you start a remote desktop session you get a restore token that can be used to restore the session without prompt in the future, and this could be retained between launches as well. The question for that then is whether you should only be prompted the first time ever, and if so, where should the restore token be stored? I guess it should be kept secure since another program could steal it and silently start controlling your inputs or something, but that means probably storing it in the database which seems kinda meh. Also, starting the session only when the user requests an autotype has some issues too, since the way it works, as soon as they close the dialog it'll start typing, regardless of what window is now active, or how much time has passed. So I guess database unlock/auto-type being enabled would be more appropriate, but I'll need to add those to the plugin interface

droidmonkey commented 1 week ago

I like the idea of one-time ask (after first database unlock maybe) and store the token in the database. A new database requires a new token. This would emulate the behavior of the browser plugin.

The downside would be if you used your database on multiple Linux wayland boxes. So you may need to add the computer name to the token storage key.