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

[Martin Babinsky]

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.

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