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

[Nathaniel McCallum]

On Wed, 2016-03-16 at 07:25 +0100, Jan Cholasta wrote:
> On 15.3.2016 22:22, Nathaniel McCallum wrote:
> > 
> > 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.
> We discussed this on the IPA devel meeting and the decision was that 
> since the otpauth URI is always displayed, a warning is sufficient
> when 
> the QR code cannot be printed.
> 
> If you disagree, could you explain why the URI is not sufficient for 
> provisioning the token?

I guess that is okay.