[Freeipa-devel] [PATCH 0139] otptoken-add: improve the robustness of QR code printing to tty

[Nathaniel McCallum]

On Tue, 2016-03-15 at 17:54 +0100, Martin Babinsky wrote:
> On 03/15/2016 03:36 PM, Martin Babinsky wrote:
> > 
> > On 03/09/2016 07:06 AM, Jan Cholasta wrote:
> > > 
> > > On 8.3.2016 17:45, Martin Babinsky wrote:
> > > > 
> > > > On 03/08/2016 05:35 PM, Jan Cholasta wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > On 8.3.2016 16:21, Martin Babinsky wrote:
> > > > > > 
> > > > > > https://fedorahosted.org/freeipa/ticket/5700
> > > > > 1) Instead of checking for utf-8 in particular, I would
> > > > > prefer a more
> > > > > robust approach:
> > > > > 
> > > > > try:
> > > > >      qr = qrcode.QRCode()
> > > > >      qr.add_data('test')
> > > > >      qr.make()
> > > > >      qr.print_ascii(tty=True)
> > > > > except UnicodeError:
> > > > >      # it is not printable
> > > > > else:
> > > > >      # it is printable
> > > > > 
> > > > Now you mean the check in the _check_qrcode_capability() or the
> > > > _print_qrcode() method itself?
> > > _check_qrcode_capability() of course.
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 2) There is no os.isatty() check to see if stdout is actually
> > > > > a tty.
> > > > > 
> > > > This check is performed inside both print_ascii() and
> > > > print_tty()
> > > > methods of QRCode object, but you probably mean that I should
> > > > put the
> > > > check also into _check_qrcode_capability() method, right?
> > > Yes. If stdout is not a tty, we should at least not tty=True in
> > > print_ascii().
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > Honza
> > > > > 
> > > > 
> > > 
> > Attaching updated patch. After the discussion with other developers
> > we
> > decided to just print warnings when non-UTF-8 encoding is used and
> > tty
> > width is smaller that the QR code size.
> > 
> > 
> > 
> Found some minor errors in the patch, attaching updated version.

NACK

This patch has the major problem that tokens are added but then
unusable because they can't be provisioned to the devices. You need to
check if qrcode output is possible before the token is added to LDAP.