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

[Jan Cholasta]

On 24.3.2016 14:13, Martin Babinsky wrote:
> On 03/24/2016 01:47 PM, Martin Babinsky wrote:
>> On 03/22/2016 12:28 PM, Martin Babinsky wrote:
>>> On 03/16/2016 02:17 PM, Martin Babinsky wrote:
>>>> On 03/16/2016 01:35 PM, Nathaniel McCallum wrote:
>>>>> 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.
>>>>>
>>>>
>>>> Thank you Nathaniel.
>>>>
>>>> Jan had some offline comments to the patch. Attaching updated version.
>>>>
>>>>
>>>>
>>> Attaching updated patches.
>>>
>>>
>>>
>> I fixed the warning message when the QR code can not be rendered.
>>
>> Attaching updated patches.
>>
>>
>>
> 6th time the charm.

Thanks, ACK.

Pushed to:
master: 7febe569cede47b50a0ee1b19968627716ddbc0d
ipa-4-3: 77e9d31c75f7514f076662ac4e3ffcf66915880f

-- 
Jan Cholasta