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

[Martin Babinsky]

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.

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