search

paolostivanin / OTPClient

Highly secure and easy to use OTP client written in C/GTK3 that supports both TOTP and HOTP
GNU General Public License v3.0
481 stars 47 forks source link

Minimize to tray with libindicator #386

Closed len-foss closed 3 days ago

len-foss commented 3 weeks ago

Hello,

First, thank you very much for this wonderful program. It is very nice to be able to use a secure device for security purposes.

Some sites are very annoying with MFA, and require multiple entries. I tend to prefer having it minimized to tray for these cases. In terms of security, I think it does not really pose a problem since there are good auto lock on system lock and inactive settings that are available already.

Although I think if it were to be accepted, it would make sense to add a setting for it and disable it by default.

Let me know what you think!

paolostivanin commented 3 weeks ago

Hello, thanks a lot for your contribution, very much appreciated :smile: Let's discuss some points:

  1. yes, we would need to add an option to enable such behavior (should be disabled by default)
  2. would it work on other DE too? (e.g. KDE)
  3. would it work on Wayland? If not, we would need to deal with that at runtime
  4. we should use libayatana-appindicator and not libappindicator3. See here.
len-foss commented 3 weeks ago

I agree with point 1. For point 2, it does work on KDE, and since that's what I'm using it will be the most thoroughly tested.

Wayland seems to open a can of worms though. It seems that the best way is to implement it via StatusNotifierItem (SNI), which if I understand would work if the user installs https://github.com/ubuntu/gnome-shell-extension-appindicator.

Should I try to implement it using this standard instead?

paolostivanin commented 3 weeks ago

I'm sorry, I don't have enough knowledge on tray icon standards & co to give a real answer. It does seem that using, for example, https://github.com/AyatanaIndicators/libayatana-appindicator could work.

len-foss commented 3 weeks ago

You're right, thank you! It seems that I read outdated doc on the subject.

I've tested the following on KDE X11 and Wayland, and everything works as expected.

Everything should be ready for review I think.

paolostivanin commented 3 weeks ago

Thank you very much! I'll try to give you a feedback by end of week :smile:

paolostivanin commented 2 weeks ago

additional todo: please rebase your commits and sign the final one :smile: thanks a lot!

len-foss commented 1 week ago

I have added an option to skip the dependency altogether, and a test to run the install without it. To avoid cluttering the code with too much #ifdef, the settings do exist even when the option is not there, they are just unplugged. Because in a GtkGrid it isn't possible to hide child elements individually, it seemed much simpler to remove the row. The other cmake lines referring to APPINDICATOR_LIBRARIES do not seem to matter either, so they are not between if either.

I think this approach keeps the complexity minimal enough.

len-foss commented 1 week ago

By the way, I had a suggestion to update https://github.com/paolostivanin/OTPClient/wiki/Secure-Memory-Limitations#how-to-set-memlock

* then, locate if a file containing the limits exists with `grep -rni memlock /etc/security/`.
If none exists, you can create a file called, for example, `/etc/security/limits.d/memlock.conf`. 
Then add the following text:

It can be the case that there is already a file even if it was not set up.

paolostivanin commented 1 week ago

thanks a lot for your work! I'll test it in the coming days on GNOME and KDE too and then, if all good, I will merge it.

PS wiki updated, thank you

len-foss commented 1 week ago

Thank you for talking the time, and don't hesitate it you want more changes to be done. I think I have approached most of it to be as simple as possible and limit the amount of potential bugs (for instance, destroying and recreating the indicator at each setting change seemed janky, whereas hiding it will basically have the same effect at next restart).

paolostivanin commented 1 week ago

indeed! I really do like your implementation :smile: Hopefully, I will be able to test everything and merge it before end of this week!

paolostivanin commented 4 days ago

@len-foss could you please rebase into a single commit? Thanks :smile:

len-foss commented 4 days ago

Done!

joakim-tjernlund commented 4 days ago

On MATE DE and also like this addition.

paolostivanin commented 3 days ago

Thanks a lot for your contribution! Let's merge this into master :smile: I'll make a new release in a few weeks. I need to tackle some flatpak bugs first.

len-foss commented 3 days ago

Nice! Don't hesitate to ping me if there are any bugs reported when there are more users with different setups.

paolostivanin commented 3 days ago

fantastic! Thanks a lot again :smile: