Add YubiKey module (show a visible indication that YubiKey is waiting for a touch) #1110

maximbaz · 2017-10-14T13:46:04Z

This module is designed to show a visible indication that YubiKey is waiting for a touch.

Basically it uses a bunch of hacks to solve this issue.
Owners of YubiKey, especially YubiKey 4 Nano would be extremely glad for this module 🙂

How it works:

It requires maximbaz/yubikey-touch-detector to run in the background and subscribes to updates from it via unix sockets.

DEMO:

This module is designed to show a visible indication that YubiKey is waiting for a touch.

lasers

I dunno about thread stuffs. Here's the comments on formatter stuffs though. 👍

lasers · 2017-10-14T14:02:15Z

py3status/modules/yubikey.py

+    state_off: Message when the touch is not needed.
+        (default '')
+    state_on: Message when the YubiKey is waiting for a touch.
+        (default 'Y')


We could remove configs by doing this...

format = '\?if=is_waiting Y' # Default

format = '\?if=is_waiting Y|N'

lasers · 2017-10-14T14:02:56Z

py3status/modules/yubikey.py

+            'cached_until': self.py3.time_in(self.cache_timeout),
+            'full_text': self.py3.safe_format(self.format, {'state': state}),
+            'color': self.py3.COLOR_BAD
+            if pending_touch else self.py3.COLOR_GOOD


We could remove color too.

format = '\?if=is_waiting&color=good Y' # Default

format = '\?if=is_waiting&color=good Y|\?color=bad N'

lasers · 2017-10-14T14:08:00Z

py3status/modules/yubikey.py

+    state_on: Message when the YubiKey is waiting for a touch.
+        (default 'Y')
+    u2f_keys_path: Full path to u2f_keys if you want to monitor sudo access.
+        (default '')


I think (default None) would look more nice... and then do if None: '' thing in post_config_hook. We could add os.path.expanduser(path) in post_config_hook too.

lasers · 2017-10-14T14:45:08Z

py3status/modules/yubikey.py

+        (default '')
+    state_on: Message when the YubiKey is waiting for a touch.
+        (default 'Y')
+    u2f_keys_path: Full path to u2f_keys if you want to monitor sudo access.


This could be generic. key_path.

I'd rather keep it like this, because this is the file name used by pam-u2f and all online tutorials use this name. If I saw key_path option I would never guess that I need to provide path to u2f_keys file

No problem. The pam-u2f link seems to use authfile. If we parse key_path option, then we probably did parse the description too. Also... Using auth_file or auth_key would put it on top of the config list. Just stating things.

I wonder... I see that authfile points to $HOME/.config/Yubico/u2f_keys. Is that your path too? And if it is useful to default $HOME/.config/Yubico/u2f_keys?

Yes I use the default, and thanks for the hint with os.path.expanduser, I thought that it will expand ~ to /root, but no, it expands to my own user - sweet! $HOME is not being expanded, only ~ by the way.

Although they use authfile in their README, the rest of the internet refers to this as u2f_keys, so I'll still keep the configuration name. Thought hopefully most people don't need to configure this anymore :)

lasers · 2017-10-14T17:07:49Z

py3status/modules/yubikey.py

+        (default '~/.config/Yubico/u2f_keys')
+
+SAMPLE OUTPUT
+{'color': '#FF0000', 'full_text': 'Y'}


I think I'd like something more readable like...

Waiting

Key Waiting

Key waiting <-- @lasers's pick 🥇

YubiKey

Key

KEY

KEY!

Yeah, I'm personally always using some fancy icons, so coming up with a default text message was a tough choice...

Not to confuse what key is the key that is waiting, how about a more specific Touch YubiKey?

I feel it's a bit on the nose and I'd like to stay with with subtle approach for QA purposes. Maybe just YubiKey or Key as I believe people will pick this up quicker than our expectations... And I feel that seeing Touch YubiKey repeatedly could be tiresome. It is akin to seeing Check Gmail repeatedly too... when Gmail or Mail would work just fine here.

Same thing with key_path. Make yubikey look clean in the config. (All my QA opinions).

YubiKey it is then. Config name u2f_keys_path will leave for the reasons described in another comment.

lasers · 2017-10-14T17:17:45Z

py3status/modules/yubikey.py

+        self.pending_sudo = False
+        self.sudo_reset_timer = None
+
+        class GpgThread(threading.Thread):


I don't know anything about threading, but I ponder if we need classes here... And if this could be done by using methods alone. @tobes can give you better feedbacks.

I'm more concerned by the approaches that this code takes, these are very hacky tricks... I hope someone from Yubico would give a hint on how to make it better. Especially the check for gpg...

lasers · 2017-10-14T17:20:45Z

py3status/modules/yubikey.py

+            'full_text':
+            self.py3.safe_format(self.format, {'is_waiting': is_waiting})
+        }
+        return response


Do we want to include 'urgent': is_waiting here too? If users does not like it, they can disable this by adding allow_urgent = False in the module config section. This might help with ~~your ADHD (jk)~~ attention too.

Huh, that's an interesting idea, I didn't know about this property. Looks neat, will add:

lasers · 2017-10-14T17:22:30Z

py3status/modules/yubikey.py

+            'cached_until':
+            self.py3.time_in(self.cache_timeout),
+            'full_text':
+            self.py3.safe_format(self.format, {'is_waiting': is_waiting})


(Nit)

return { 'cached_until': self.py3.time_in(self.cache_timeout), 'full_text': self.py3.safe_format( self.format, {'is_waiting': is_waiting}) }

I have yapf automatically formatting the code here (using pep8 rules) - don't want to interfere with it.

Extracted params in a separate variable, this lead yaps to decide to change to a more pleasant styling 😉

lasers · 2017-10-14T19:28:07Z

py3status/modules/yubikey.py

+        (default '~/.config/Yubico/u2f_keys')
+
+SAMPLE OUTPUT
+{'color': '#FF0000', 'full_text': 'Y'}


I feel it's a bit on the nose and I'd like to stay with with subtle approach for QA purposes. Maybe just YubiKey or Key as I believe people will pick this up quicker than our expectations... And I feel that seeing Touch YubiKey repeatedly could be tiresome. It is akin to seeing Check Gmail repeatedly too... when Gmail or Mail would work just fine here.

Same thing with key_path. Make yubikey look clean in the config. (All my QA opinions).

lasers · 2017-10-14T19:36:54Z

py3status/modules/yubikey.py

+            'full_text': self.py3.safe_format(self.format, format_params),
+            'urgent': is_waiting
+        }
+        return response


Okay. Now I know you like this. Can we do this instead?

response = { 'cached_until': self.py3.time_in(self.cache_timeout), 'full_text': self.py3.safe_format(self.format, format_params), } if is_waiting: response['urgent'] = True return response

This prevents us from alway seeing urgent: False in the log / testing. It's all about simple pleasures.

lasers · 2017-10-14T19:41:39Z

py3status/modules/yubikey.py

+        (default '\?if=is_waiting Touch YubiKey')
+    u2f_keys_path: Full path to u2f_keys if you want to monitor sudo access.
+        (default '~/.config/Yubico/u2f_keys')
+


Missing.

Control placeholders: {is_waiting} a boolean based on gpg content and sudo access

maximbaz · 2017-10-15T00:10:47Z

It just got much more performant, but more complicated as well. The long explanation is posted here, where I'm also hoping to get some ideas on how to improve/simplify stuff here.

I'll be testing a lot more during the following days, but so far I like it a lot, it is almost perfect. I'm also listening to this ticket on how to get rid of sudo_reset_timer and truly detect when Yubikey was pressed.

Regardless, this PR doesn't have to wait for any of those tickets, review and merge when you have time, and if I hear anything interesting I'll always make a separate PR.

maximbaz · 2017-10-15T00:48:23Z

py3status/modules/yubikey.py

+        is_waiting = self.pending_gpg or self.pending_sudo
+        format_params = {'is_waiting': is_waiting}
+        response = {
+            'cached_until': self.py3.time_in(self.cache_timeout),


@tobes, @lasers: I have to put a small number here, 1 or even smaller, but most of the time it would be wasting cycles and just redrawing the same output over and over again. I would really love to put here self.py3.CACHE_FOREVER, and instead just manually force module refresh when at least one checks is triggered in other threads - is this possible to achieve?

In other words, is there some kind of function self.py3.force_redraw(<module_name>)?

Maybe you're looking for self.py3.update()? scratchpad_async and window_title_async might be interesting to you as both modules uses the function and the threading.

Exactly, thanks! Now it is instantly reacting only when the value changes - awesome!!

maximbaz · 2017-10-16T12:45:09Z

I did not get a support from folks at pam-u2f (because that project is being rewritten), but I made my own fork that adds this functionality.

Now I'm fully happy with the code, ready to be merged. I'll revisit the approaches here if I figure out better ways to run a detection.

lasers · 2017-10-16T13:18:42Z

py3status/modules/yubikey.py

+        if len(self.gpg_check_watch_paths) > 0:
+            GpgThread().start()
+
+        if len(self.sudo_check_watch_paths) > 0:


(Nit). if len(self.sudo_check_watch_paths): would work okay here too.

yeah, but I like it explicit

lasers · 2017-10-16T13:28:35Z

py3status/modules/yubikey.py

+        self.pending_gpg = False
+        self.pending_sudo = False
+
+        class GpgThread(threading.Thread):


(Useful?) If you look at volume_status, you'll find several classes outside of class Py3status. If you are able to do the same here, then you could reclaim some indentations... and some things might be more readable... I recall we have to use parent for py3 stuffs... eg self.parent.py3.update(). Hopefully that's useful.

Something to try if we don't like how it's sitting inside post_config_hook instead of a line or two. EDIT: Another example is sysdata. EDIT: Maybe we wait.

It started as two very simple classes with just few lines, but since then they grew... Now it is time to extract them indeed, makes total sense to me.

lasers · 2017-10-17T05:23:40Z

py3status/modules/yubikey.py

+        (default '\?if=is_waiting YubiKey')
+    gpg_check_timeout: Time to wait for gpg check response.
+        Use value as small as possible that doesn't introduce false positives.
+        (default 0.1)


(Q) Is it useful for users to have gpg_check_timeout config if the default is 0.1? The message also say Use value as small as possible that doesn't introduce false positives. Another thing... I see a hardcoded time.sleep(0.25) too... right after that config too... so what can they do with this config?

There is a value of keeping it configurable, the default 0.1 works well on my laptop, but I can imagine that slower machines would not be able to do the detection within 0.1 seconds and those people would have to start slowly increasing the value until false positives disappear. The larger value you put, the larger would be a time delay between Yubikey begins to ask for touch and this module waking up. With the default value of 0.1 it is almost instant, which I like 🙂

time.sleep(0.25) has no value for being configurable, this is an internal value to the algorithm to prevent false positives and I cannot imagine why would someone want to change them. The same goes for a constant 20 nearby. These are just two very good values that work well.

lasers · 2017-10-17T05:37:53Z

py3status/modules/yubikey.py

+    gpg: to check for pending gpg access request
+    inotify-simple: to check for pending gpg and u2f requests
+    github.com/maximbaz/pam-u2f: a fork that adds watch capability to pam-u2f module
+    subprocess32: if after all these years you are still using python2


Modules uses Requires: and this gpg line might not be needed (installed everywhere).

Not sure if we can use (optional) thing like this.

python-inotify-simple: a simple wrapper around inotify github.com/maximbaz/pam-u2f: (is this optional?) a fork that adds... python2-subprocess32: (optional) backported subprocess module for python2

good ideas:

Yubikey is bought primarily for gpg operations, so gpg is definitely installed on the system 🙂

only patched pam-u2f is optional, because that's the only dependency for u2f, will mark it so.

lasers · 2017-10-17T05:43:51Z

py3status/modules/yubikey.py

+if os.name == 'posix' and sys.version_info[0] < 3:
+    import subprocess32 as subprocess
+else:
+    import subprocess


We import sys for this. Wanting to see if we can live w/o sys.

try: import subprocess # py3 except: import subprocess32 as subprocess # py2

Not really, because subprocess exists in both py2 and py3, it's just that the module in py2 doesn't contain timeout functionality that was introduced in py3, and I need it :)

Who is using py2 anyway, and when will py3status deprecate py2? It's not called py2status anyway 😉

Flip the import subprocess lines could be sufficent to omit sys... then I wonder if we should explicitly throw an exception on subprocess32 not imported and is python2... yubikey: subprocess32 not installed. py3status support python2 until or after https://pythonclock.org and I think py3status is a wordplay on i3wm which is also a wordplay on wmii. ;-)

The exception will happen with current implementation, right? Python3 will import subprocess and nothing else, python2 will import subprocess32 and throw an exception if it cannot do so.

The exception will happen with current implementation, right?

Yup. Sharing the findings.

Packages: inotify_simple, subprocess32

2017-10-17 INFO loading module "yubikey" from .../yubikey.py 2017-10-17 INFO Module `yubikey` could not be loaded

No exceptions... The bar will report:

yubikey // yubikey: Import Error, No module named 'inotify_simple' yubikey // yubikey: Import Error, No module named 'subprocess32'

It's just a matter of if we should need sys... and/or if we should make this nice like module: not installed... eg yubikey: subprocess32 not imported or yubikey: inotify_simple not imported. Anyhow, I leave this to others. Same with the formatting. :-)

lasers · 2017-10-17T05:55:55Z

py3status/modules/yubikey.py

+
+    def yubikey(self):
+        is_waiting = self.pending_gpg or self.pending_u2f
+        format_params = {'is_waiting': is_waiting}


Do more with formatting tricks by replacing is_waiting with gpg and u2f, eg...

format = '[\?color=#fa0 YubiKey [\?if=gpg&color=#aaf GPG][\?if=u2f&color=#faf U2F]]' format = '[\?if=gpg&color=degraded YubiKey GPG][\?if=u2f&color=#fa0 YubiKey U2F]]' format = '[\?color=good YubiKey [\?if=gpg GPG][\?if=u2f U2F]]' format = '[\?color=good [\?if=gpg GPG][\?if=u2f U2F]]' format = '[\?color=good&u2f U2F]' # ignore GPG

EDIT: Add some examples in the docs too.

This is interesting thought, but in practice this would be just too much. Users don't care whether the module is flashing because of gpg or u2f, once it flashes for whatever reason they need to figure out manually what caused this and decide on the spot, touch or not touch.

What could have been interesting is to see the process name that requests the touch, e.g. I can configure using both gpg or u2f technology that ssh'ing somewhere will ask me to touch the device for confirmation, and in real life I don't care which technology is being used, instead I want to know which process wants this confirmation. But I don't know how to detect that from the module, we will leave this to a different time 🙂

Users don't care whether the module is flashing because of gpg or u2f, once it flashes for whatever reason they need to figure out manually what caused this and decide on the spot, touch or not touch.

Possibly all the more reasons to do... as GPG is related to security, keys, encryption, logins, credentials, etc. Some users may like that option or may use nice icons to differentiate.

once it flashes for whatever reason they need to figure out manually what caused this

It at least differentiates Something to do with GPG. Hopefully it will grow more from there... to support {process_name}, {pid}, Idk what else.

maximbaz (in another thread): Of course it would be awesome if one of the yubikey CLI tools provided such information out of the box, it is a really useful feature in my mind...

If that happen, then we should use it to expose everything for users... just like cmus, moc and coin_market as they all have placeholders the users may not care about. We included them all because the data is there. :o

Let's first find at least one person who needs that 🙂

ultrabug · 2017-10-18T15:35:10Z

I'm just quickly coming around this PR to say big thanks to you @maximbaz since I'm also a Yubikey lover and user and I dreamed of this very module ! 👍

I'll do my best to try it asap

maximbaz · 2017-10-18T16:47:34Z

Looking forward 🙂

maximbaz · 2017-10-25T19:53:40Z

@ultrabug when you get to testing the module, let me know what you think of the default gpg_check_timeout value and whether you have touch policy for gpg operations enabled on your device (e.g. whether you have to touch Yubikey to confirm the gpg --sign operation). I have the touch policy enabled and the value of 0.1 is just perfect, but by default the touch policy is disabled on Yubikey, and in such cases on my laptop the values below 1 produce false positives. So I'm considering to increasing the default to 1.

ultrabug · 2017-11-03T13:00:38Z

@maximbaz I wanted to try this out and got stuck in the setup process. What are those options supposed to refer to and are used for?

    gpg_check_watch_paths: A list of files to watch, start gpg check if any one was opened.
        (default ['~/.gnupg/pubring.kbx', '~/.ssh/known_hosts'])
    u2f_check_watch_paths: A list of files to watch, toggle u2f check if any one was opened.
        (default ['~/.config/Yubico/u2f_keys'])

The GPG ones I got, the u2f_keys file, I don't. What should be in there?

maximbaz · 2017-11-03T13:05:04Z

The defaults will work just work fine, if you don't have the U2F you probably don't use pam-u2f, the code will ignore all missing files.

Why these files are needed to be configured I'll explain a bit later.

ultrabug · 2017-11-03T13:21:56Z

I understand, but your patch is applied upon v1.0.5 of pam_u2f which has not been released so it's hard for users to get your commit into their pam_u2f lib.

Can you provide a patch on v1.0.4 ?

ultrabug · 2017-11-04T08:12:06Z

I think this is a sensible approach indeed. Unix sockets look the right way to move forward yes (at least that's what comes to my mind as well).

Thanks!

maximbaz · 2017-11-05T17:54:13Z

@ultrabug it's time to give it another try 😉 I extracted the tool into maximbaz/yubikey-touch-detector and made this module subscribe to notifications from it via unix sockets. The message is always of a fixed length of 5 bytes, so the parsing code here is very simple.

This module is quite resilient, it will detect if the yubikey-touch-detector is not running and will retry the connection every minute (useful for testing, upgrading yubikey-touch-detector without restarting i3, etc).

Finally I also added the is_waiting_gpg and is_waiting_u2f to the list of control placeholders, this is something @lasers suggested long time ago 🙂 Now it's just super easy because the host app is sending separate messages for GPG and U2F.

lasers

👌

lasers · 2017-11-06T09:22:25Z

py3status/modules/yubikey.py

+Control placeholders:
+    {is_waiting} a boolean, True if YubiKey is waiting for a touch for any reason.
+    {is_waiting_gpg} a boolean, True if YubiKey is waiting for a touch due to a gpg operation.
+    {is_waiting_u2f} a boolean, True if YubiKey is waiting for a touch due to a pam-u2f request.


Then you need to be very careful about adding things you may later want to remove from @tobes

This is akin to me supporting both markets = 'btc, eth' and markets = ['btc', eth'] in coin_market when you said that a list alone is sufficent. We could do same thing here with two {is_gpg} and {is_u2f} and have them both in default format?

The exception I made so far is with numbers... In new apt_updates module, I combined {new} packages and {upgrade} packages to make {count} packages to take advantage of color threshold. That and the formatter does not do any basic mathematics.

I like that you killed off several config params above. 🥂

you mean to remove the is_waiting and have the default format be something like this?

format = '\?if=is_waiting_gpg OR is_waiting_u2f YubiKey'

what's the correct syntax for this?

format = '[\?if=is_gpg KEY][\?is_u2f KEY]'

@lasers just stumbled upon an issue with this format, it is possible that there are two commands issued for Yubikey at the same time (e.g. pacman is updating packages and requesting sudo via u2f, while at the same time on a different tab you ssh somewhere and this triggers gpg check). This results in module output "YubiKeyYubiKey", because is_gpg and is_u2f are both active at the same time. I want a true OR, not two separate ifs - how can we achieve that?

I recall it isn't possible. Pinging @tobes just in case. Maybe we could do this?

format = '[YubiKey [\?if=is_gpg GPG][\?soft ,][\?if=is_u2f U2F]]' * * YubiKey GPG * YubiKey U2F * YubiKey GPG,U2F

I need this for a case when I don't want to see GPG or U2F, I just want to see YubiKey (or in my case, I use a symbol of key).

But using your example I think I solved it, this seems to work like a true OR:

format = '[[\?if=is_gpg ][\?if=is_u2f ] Yubikey]'

Nice. format = '[YubiKey[\?if=is_gpg ][\?if=is_u2f ]]' seems to work too.

Indeed, I'll use your way as a default, it's easy to understand how to go from it to additionally showing GPG or U2F.

lasers · 2017-11-06T09:24:46Z

py3status/modules/yubikey.py

+    {is_waiting_u2f} a boolean, True if YubiKey is waiting for a touch due to a pam-u2f request.
+
+SAMPLE OUTPUT
+{'color': '#00FF00', 'full_text': 'YubiKey'}


{'full_text': 'YubiKey', 'urgent': True}

lasers · 2017-11-06T09:51:28Z

py3status/modules/yubikey.py

@@ -0,0 +1,121 @@
+# -*- coding: utf-8 -*-
+"""
+Show an indicator when YubiKey is waiting for a touch.


(Tossing it out) Display an indicator for YubiKey ?

This way it's not very obvious, let's have it as it is to spell out what exactly this indicator is doing.

lasers · 2017-11-06T10:09:09Z

py3status/modules/yubikey.py

+
+        self.killed = threading.Event()
+
+        self.waiting_gpg = 0


(Nit) I see 3 blank lines. See if we can kill one or two.

lasers · 2017-11-09T17:54:04Z

py3status/modules/yubikey.py

+    {is_gpg} a boolean, True if YubiKey is waiting for a touch due to a gpg operation.
+    {is_u2f} a boolean, True if YubiKey is waiting for a touch due to a pam-u2f request.
+
+SAMPLE OUTPUT


This should come after the author.

this should be part of travis check

lasers · 2017-11-09T17:59:39Z

py3status/modules/yubikey.py

+                elif data == b"U2F_1":
+                    self.parent.is_waiting_u2f = True
+                elif data == b"U2F_0":
+                    self.parent.is_waiting_u2f = False


(Nit) Maybe we get rid of all is_waiting. I barely can see the difference, eg self.parent.is_u2f.

ultrabug · 2017-11-09T21:04:28Z

Awesome @maximbaz I will try tomorrow 👍

lasers · 2017-11-10T14:51:29Z

py3status/modules/yubikey.py

+    def yubikey(self):
+        format_params = {
+            'is_gpg': self.is_gpg,
+            'is_u2f': self.is_u2f,


(Suggestion). We can make self.operation = {'is_gpg': None, 'is_u2f' None} in post_config_hook then use self.operation['is_gpg'] = True to update the values. No need to make new dict on loops. This removes few lines too.

lasers · 2017-11-10T14:52:55Z

py3status/modules/yubikey.py

+    {is_u2f} a boolean, True if YubiKey is waiting for a touch due to a pam-u2f request.
+
+Requires:
+    github.com/maximbaz/yubikey-touch-detector: tool to detect when YubiKey is waiting for a touch


(Nit) Three lines here are over column 80. I don't have 4K monitors. ;__;

the line length is enforced by Travis, it was long but I compressed it enough so that Travis is green and happy now.

you should by a 4K screen, and a yubikey 😜

maximbaz · 2017-11-20T17:00:58Z

A friendly ping 😜

You should consider giving @lasers push access to this repo, he is very active at making improvements and code reviews 😄

lasers · 2017-11-20T20:01:08Z

Maybe let's not. I'm a noob. Having more eyes on the code is always good. I enjoy the efforts of fixing and/or improving things, but not so much in the deprecation department. If they're occupied with things, then I guess we wait patiently since they're not noobs. :-)

lasers

🦃

lasers · 2017-11-30T04:36:48Z

py3status/modules/yubikey.py

+            if self.socket is None:
+                # Socket is not available, try again soon
+                time.sleep(60)
+                continue


(Thought) Do we need time.sleep(60) here?

Would the thread not keep going and failing until it's finally made?

What happen if ssh or gpg is made, but we're in this time.sleep period? I think it could not show up... or even could show up several seconds late! 😡

It's not a good idea to completely avoid sleep, if the socket connection cannot be established, there is likely an issue with the backend service and manual intervention is needed, and until then connection will probably fail next time as well. Trying to reconnect too often will only result if wasting lots of cycles. One minute is a sweet spot that I hardcode on purpose, not too often and yet fast enough for an automatic recovery - in real life YubiKey is used much rarely than once per minute.

However your question lead me to the following thought, since manual intervention is likely needed, perhaps silently sleeping is not enough, and we need to show an error message in red color. I saw other modules do this when they fail. Could you point me to how to achieve this?

It will not show up indeed, whether the problem is on the backend side (not publishing events) or on the module side (not listening to the events), no connection means no events received and no messages shown. However the notification will not show up several seconds late, when connection is finally established, it is starting from the clean state, only next even will trigger a message to be shown.

self.py3.error. I think it'll keep going behind the scene until #1114 gets addressed. @tobes also said something about extending the self.py3.error exceptions to threads.

./bluetooth.py:130: self.py3.error(STRING_NOT_STARTED) ./yandexdisk_status.py:58: self.py3.error(STRING_ERROR) ./coin_market.py:170: self.py3.error('bad markets') ./external_script.py:76: self.py3.error(output)

I don't know this ~~well~~ at all, but quick stab...

diff --git a/py3status/modules/yubikey.py b/py3status/modules/yubikey.py index 621a3c90..ba088d52 100644 --- a/py3status/modules/yubikey.py +++ b/py3status/modules/yubikey.py @@ -22,7 +22,6 @@ SAMPLE OUTPUT {'full_text': 'YubiKey', 'urgent': True} """ -import time import os import socket import threading @@ -41,17 +40,15 @@ class YubiKeyTouchDetectorListener(threading.Thread): try: self.socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) self.socket.connect(self.parent.socket_path) - except: - self.socket = None + except Exception as e: + self.error = e def run(self): while not self.parent.killed.is_set(): self._connect_socket() - if self.socket is None: - # Socket is not available, try again soon - time.sleep(60) - continue + if self.error: + self.parent.py3.update() while not self.parent.killed.is_set(): data = self.socket.recv(5) @@ -77,6 +74,7 @@ class Py3status: socket_path = '$XDG_RUNTIME_DIR/yubikey-touch-detector.socket' def post_config_hook(self): + self.error = None self.socket_path = os.path.expanduser(self.socket_path) self.socket_path = os.path.expandvars(self.socket_path) @@ -89,6 +87,8 @@ class Py3status: YubiKeyTouchDetectorListener(self).start() def yubikey(self): + if self.error: + raise self.error response = { 'cached_until': self.py3.CACHE_FOREVER, 'full_text': self.py3.safe_format(self.format, self.status),

awesome, thanks, it is very beautiful and descriptive now, clear to the user when this happens and what to investigate 🙂

lasers · 2017-11-30T04:40:37Z

py3status/modules/yubikey.py

+
+    def run(self):
+        while not self.parent.killed.is_set():
+            self._connect_socket()


We could put everything here for better readability. Less lines and we skip = None too.

I don't think we can skip = None even if we inline the code here, the exception is likely happening during the connect(), and I want to cleanup the dangling socket immediately in that case. I like it as it is.

lasers · 2017-11-30T04:43:32Z

py3status/modules/yubikey.py

+        self.status = {
+            'is_gpg': False,
+            'is_u2f': False,
+        }


(Nit) Maybe just self.status = {'is_gpg': False, 'is_u2f': False} here and kill a line?

for me it is more readable as it is.

Readability is better than compactness every time

lasers · 2017-12-12T03:23:45Z

py3status/modules/yubikey.py

+
+    def yubikey(self):
+        if self.error:
+            raise self.error


Maybe we use self.py3.error(str(self.err), self.py3.CACHE_FOREVER) here instead due to py3ness... and If I understand this okay... that we don't need timeout anymore too due to error? How come we don't use str(self.err) -- a potentially better exception message(s) via except Exception as err-- over a hardcoded self.error message?

ultrabug · 2017-12-12T11:29:17Z

Ok I finally tested the module fully and it works good 👍

As for the error report that @lasers is pointing out, I think @maximbaz did this as-is because py3status will be handling the error report on the bar. The drawback is that the user will have to click on the red yubikey string that will appear to display the error message. It could be misleading because the actual behaviour of the module is to display a red yubikey string to alert of a touch requirement...

So I'd rather the module outputs the error itself in the full_text field than relying on py3status to do so, a bit like @lasers proposes.

ultrabug · 2017-12-12T11:30:02Z

py3status/modules/yubikey.py

+    socket_path = '$XDG_RUNTIME_DIR/yubikey-touch-detector.socket'
+
+    def post_config_hook(self):
+        self.socket_path = os.path.expanduser(self.socket_path)


isn't this overridden by the (re)definition just below on line 85?

yep, on purpose - first expanduser expands ~ in the path if it is present, then expandvars expands environment variables (again, if they are present). We need a full path, and this allows to define the path both as $XDG_RUNTIME_DIR/bla and ~/bla.

maximbaz · 2017-12-12T11:36:21Z

I will play with py3.error, CACHE_FOREVER for it definitely makes sense, as well as reducing one click to see the error 🙂

The actual exception is very cryptic, I would never guess what it means, that's why I put my custom one - at least people will know what to investigate next if they see it.

lasers · 2017-12-12T12:10:47Z

So I'd rather the module outputs the error itself in the full_text field than relying on py3status to do so,

That old thing? We switched away from that... to this new thing. I'd suggest we stay with the new thing for QA / consistency. We also could just make error message much much shorter... like broken socket, no socket, or no connection

We should see if it's possible to expand the exceptions right away using global / module config. Maybe make it a default too for the users. That way, we can keep ours toggled off... or even disappear... on account of switching modules non-stop. Just a thought.

maximbaz · 2017-12-12T15:42:42Z

I used py3.error to refresh module less often, this is cool. Visually it looks exactly the same, you see "yubikey" in red, you click on it and see full error message.

I like to keep the error message as it is to make it clear what to investigate, "no socket" or "no connection" aren't explanatory enough to me.

lasers

Nice job, good module. Maybe a last-minute addition of showing how to turn off urgent and adding GPG/U2F strings in one example would be nice/helpful for users. Let's get this puppy in. 🐩

ultrabug · 2017-12-13T20:35:17Z

Impressive work @maximbaz congratulations and thank you! I'm using it 👍

Add YubiKey module

862f11f

This module is designed to show a visible indication that YubiKey is waiting for a touch.

lasers reviewed Oct 14, 2017

View reviewed changes

Simplify module configuration

b2ccc01

lasers reviewed Oct 14, 2017

View reviewed changes

Add urgent flag, use more explanatory default text

dd547f9

lasers reviewed Oct 14, 2017

View reviewed changes

maximbaz added 3 commits October 14, 2017 23:21

Small improvements

9e03b08

Improve detection of when touch happened for GPG operation

a6f249b

Drastically improve gpg detection performance

9aad8af

Unify configuration options for both checks

894c70f

maximbaz commented Oct 15, 2017

View reviewed changes

maximbaz added 2 commits October 15, 2017 12:46

Refresh module only when needed

fcebca5

Consume watch capability from a forked pam-u2f

4806073

lasers reviewed Oct 16, 2017

View reviewed changes

Extract nested classes, rename sudo check to u2f check

2e4f8d7

lasers reviewed Oct 17, 2017

View reviewed changes

Update requirements section

6e949de

tobes assigned tobes and ultrabug and unassigned tobes Nov 1, 2017

tobes added the new module ✨ I am proposing a new module label Nov 1, 2017

Use external tool for detection

92464d4

maximbaz force-pushed the add_yubikey_module branch from c17fdd6 to 92464d4 Compare November 5, 2017 17:55

lasers reviewed Nov 6, 2017

View reviewed changes

maximbaz added 3 commits November 7, 2017 14:12

Get rid of the precomputed 'is_waiting'

ac0f117

Improve default format value

3fec37e

Simplify is_waiting logic, no need to count numbers

9273320

lasers reviewed Nov 9, 2017

View reviewed changes

Simplify naming

7f2fe2f

lasers reviewed Nov 10, 2017

View reviewed changes

maximbaz added 3 commits November 11, 2017 17:09

Store request statuses in a dictionary

426a9dd

Naming

d5b4c0f

Simplify condition

736a637

lasers mentioned this pull request Nov 29, 2017

IMAP IDLE: Completely redone #1175

Merged

lasers reviewed Nov 30, 2017

View reviewed changes

Show error if cannot connect to the socket

c34879b

lasers reviewed Dec 12, 2017

View reviewed changes

ultrabug reviewed Dec 12, 2017

View reviewed changes

Use py3.error with CACHE_FOREVER

cc23b67

lasers approved these changes Dec 12, 2017

View reviewed changes

ultrabug merged commit 2b2fc67 into ultrabug:master Dec 13, 2017

Add YubiKey module (show a visible indication that YubiKey is waiting for a touch) #1110

Add YubiKey module (show a visible indication that YubiKey is waiting for a touch) #1110

Conversation

maximbaz commented Oct 14, 2017 • edited

lasers left a comment

Choose a reason for hiding this comment

lasers Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz commented Oct 15, 2017

maximbaz Oct 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Oct 17, 2017 • edited

Choose a reason for hiding this comment

lasers Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ultrabug commented Oct 18, 2017

maximbaz commented Oct 18, 2017

maximbaz commented Oct 25, 2017 • edited

ultrabug commented Nov 3, 2017

maximbaz commented Nov 3, 2017

ultrabug commented Nov 3, 2017

ultrabug commented Nov 4, 2017

maximbaz commented Nov 5, 2017

lasers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz Nov 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz commented Oct 14, 2017 •

edited

lasers Oct 14, 2017 •

edited

lasers Oct 14, 2017 •

edited

lasers Oct 14, 2017 •

edited

maximbaz Oct 14, 2017 •

edited

maximbaz Oct 15, 2017 •

edited

lasers Oct 16, 2017 •

edited

maximbaz Oct 17, 2017 •

edited

lasers Oct 17, 2017 •

edited

lasers Oct 17, 2017 •

edited

lasers Oct 17, 2017 •

edited

maximbaz commented Oct 25, 2017 •

edited

maximbaz Nov 6, 2017 •

edited