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

[Jan Cholasta]

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?

-- 
Jan Cholasta