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

[Martin Babinsky]

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.

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