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
From 4fb8a85bc055119e91f906268d5863883f311268 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Tue, 8 Mar 2016 15:56:52 +0100
Subject: [PATCH] otptoken-add: improve the robustness of QR code printing

The python-qrcode print_ascii() method does not work in terminals with
non-UTF-8 encoding. When this is the case do not render QR code but print a
warning instead. Also print a warning when the QR code size is greater that
terminal width if the output is a tty.

https://fedorahosted.org/freeipa/ticket/5700
---
 ipalib/messages.py         |  8 ++++++
 ipalib/plugins/otptoken.py | 62 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/ipalib/messages.py b/ipalib/messages.py
index 5d723b2c0ae7be07a5c89757b07ca353f23bf22e..681fc2bda611bca4510add15b74e7f786f5cc182 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -342,6 +342,14 @@ class BrokenTrust(PublicMessage):
                "running 'ipa trust-add' again.")
 
 
+class ResultFormattingError(PublicMessage):
+    """
+    **13019** Unable to correctly format some part of the result
+    """
+    errno = 13019
+    type = "warning"
+
+
 def iter_messages(variables, base):
     """Return a tuple with all subclasses
     """
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 846155dfb3af53ad65f3a4f488629837d10a2bce..849710daf7a8f9273d39aa73cae840897911fe2f 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -18,23 +18,28 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 from __future__ import print_function
+import sys
 
 from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMember
 from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve
 from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext
+from ipalib.messages import add_message, ResultFormattingError
 from ipalib.plugable import Registry
 from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound, ValidationError
 from ipalib.request import context
 from ipalib.frontend import Local
 from ipaplatform.paths import paths
 from ipapython.nsslib import NSSConnection
+from ipapython.version import API_VERSION
 
 import base64
+import locale
 import uuid
 import qrcode
 import os
 
 import six
+from six import StringIO
 from six.moves import urllib
 
 if six.PY3:
@@ -352,16 +357,57 @@ class otptoken_add(LDAPCreate):
 
     def output_for_cli(self, textui, output, *args, **options):
         uri = output['result'].get('uri', None)
-        rv = super(otptoken_add, self).output_for_cli(textui, output, *args, **options)
+
+        if uri is None or options.get('no_qrcode', False):
+            return super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
 
         # Print QR code to terminal if specified
-        if uri and not options.get('no_qrcode', False):
-            print("\n")
-            qr = qrcode.QRCode()
-            qr.add_data(uri)
-            qr.make()
-            qr.print_ascii(tty=True)
-            print("\n")
+        qr_output = StringIO()
+        qr = qrcode.QRCode()
+        qr.add_data(uri)
+        qr.make()
+        qr.print_ascii(out=qr_output, tty=False)
+
+        encoding = getattr(sys.stdout, 'encoding', None)
+        if encoding is None:
+            encoding = locale.getpreferredencoding(False)
+
+        try:
+            qr_code = qr_output.getvalue().decode(encoding)
+        except UnicodeError:
+            add_message(
+                API_VERSION,
+                output,
+                message=ResultFormattingError(
+                    message=_("Unable to display QR code using {encoding} "
+                              "output encoding".format(encoding=encoding))
+                )
+            )
+            return super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
+
+        stdout_is_tty = sys.stdout.isatty()
+        if stdout_is_tty:
+            output_width = self.api.Backend.textui.get_tty_width()
+            qr_code_width = len(qr_code.splitlines()[0])
+            if qr_code_width > output_width:
+                add_message(
+                    API_VERSION,
+                    output,
+                    message=ResultFormattingError(
+                        message=_(
+                            "QR code width is greater than that of the output "
+                            "tty. Please resize your terminal.")
+                    )
+                )
+
+        rv = super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
+
+        print("\n")
+        qr.print_ascii(tty=stdout_is_tty)
+        print("\n")
 
         return rv
 
-- 
2.5.0

From 3614459544e25293de2e80b04decf3ed0926badb Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Tue, 8 Mar 2016 15:56:52 +0100
Subject: [PATCH] otptoken-add: improve the robustness of QR code printing

The python-qrcode print_ascii() method does not work in terminals with
non-UTF-8 encoding. When this is the case do not render QR code but print a
warning instead. Also print a warning when the QR code size is greater that
terminal width if the output is a tty.

https://fedorahosted.org/freeipa/ticket/5700
---
 ipalib/messages.py         |  8 ++++++
 ipalib/plugins/otptoken.py | 62 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/ipalib/messages.py b/ipalib/messages.py
index dbbc34ab14e0ed339bda93db379d70d95e7336ff..5cd0ea1769920c076c62729ed5fe359cd1680723 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -352,6 +352,14 @@ class BrokenTrust(PublicMessage):
                "running 'ipa trust-add' again.")
 
 
+class ResultFormattingError(PublicMessage):
+    """
+    **13019** Unable to correctly format some part of the result
+    """
+    errno = 13019
+    type = "warning"
+
+
 def iter_messages(variables, base):
     """Return a tuple with all subclasses
     """
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 2d7e99d06f4f64319bbd2f8f13a60e5391a57d77..020b428aeab9ea4392d25b9088f335c3d0e08ea0 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -18,10 +18,12 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 from __future__ import print_function
+import sys
 
 from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMember
 from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve
 from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext
+from ipalib.messages import add_message, ResultFormattingError
 from ipalib.plugable import Registry
 from ipalib.errors import (
     PasswordMismatch,
@@ -32,13 +34,16 @@ from ipalib.request import context
 from ipalib.frontend import Local
 from ipaplatform.paths import paths
 from ipapython.nsslib import NSSConnection
+from ipapython.version import API_VERSION
 
 import base64
+import locale
 import uuid
 import qrcode
 import os
 
 import six
+from six import StringIO
 from six.moves import urllib
 
 if six.PY3:
@@ -356,16 +361,57 @@ class otptoken_add(LDAPCreate):
 
     def output_for_cli(self, textui, output, *args, **options):
         uri = output['result'].get('uri', None)
-        rv = super(otptoken_add, self).output_for_cli(textui, output, *args, **options)
+
+        if uri is None or options.get('no_qrcode', False):
+            return super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
 
         # Print QR code to terminal if specified
-        if uri and not options.get('no_qrcode', False):
-            print("\n")
-            qr = qrcode.QRCode()
-            qr.add_data(uri)
-            qr.make()
-            qr.print_ascii(tty=True)
-            print("\n")
+        qr_output = StringIO()
+        qr = qrcode.QRCode()
+        qr.add_data(uri)
+        qr.make()
+        qr.print_ascii(out=qr_output, tty=False)
+
+        encoding = getattr(sys.stdout, 'encoding', None)
+        if encoding is None:
+            encoding = locale.getpreferredencoding(False)
+
+        try:
+            qr_code = qr_output.getvalue().decode(encoding)
+        except UnicodeError:
+            add_message(
+                API_VERSION,
+                output,
+                message=ResultFormattingError(
+                    message=_("Unable to display QR code using {encoding} "
+                              "output encoding".format(encoding=encoding))
+                )
+            )
+            return super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
+
+        stdout_is_tty = sys.stdout.isatty()
+        if stdout_is_tty:
+            output_width = self.api.Backend.textui.get_tty_width()
+            qr_code_width = len(qr_code.splitlines()[0])
+            if qr_code_width > output_width:
+                add_message(
+                    API_VERSION,
+                    output,
+                    message=ResultFormattingError(
+                        message=_(
+                            "QR code width is greater than that of the output "
+                            "tty. Please resize your terminal.")
+                    )
+                )
+
+        rv = super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
+
+        print("\n")
+        qr.print_ascii(tty=stdout_is_tty)
+        print("\n")
 
         return rv
 
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to