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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-3-mbabinsk-0139.3-otptoken-add-improve-the-robustness-of-QR-code-print.patch
Type: text/x-patch
Size: 4912 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160316/5e45bca0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0139.3-otptoken-add-improve-the-robustness-of-QR-code-print.patch
Type: text/x-patch
Size: 4878 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160316/5e45bca0/attachment-0001.bin>