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
From 2a40bdba87756a7ea5a16111ddbafe14c493516e 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 | 81 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 82 insertions(+), 7 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..07476543fb242d5cfe10ef270daba2613e4d1382 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:
@@ -350,17 +355,79 @@ class otptoken_add(LDAPCreate):
         _convert_owner(self.api.Object.user, entry_attrs, options)
         return super(otptoken_add, self).post_callback(ldap, dn, entry_attrs, *keys, **options)
 
+    def _get_qrcode(self, output, uri, version):
+        # Print QR code to terminal if specified
+        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(
+                version,
+                output,
+                message=ResultFormattingError(
+                    message=_("Unable to display QR code using the configured "
+                              "output encoding")
+                )
+            )
+            add_message(
+                version,
+                output,
+                message=ResultFormattingError(
+                    message=_(
+                        "Please use the token URI to configure you OTP device"
+                    )
+                )
+            )
+            return None
+
+        if sys.stdout.isatty():
+            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(
+                    version,
+                    output,
+                    message=ResultFormattingError(
+                        message=_(
+                            "QR code width is greater than that of the output "
+                            "tty. Please resize your terminal.")
+                    )
+                )
+
+        return qr
+
     def output_for_cli(self, textui, output, *args, **options):
+        # copy-pasted from ipalib/Frontend.__do_call()
+        # because option handling is broken on client-side
+        if 'version' in options:
+            pass
+        elif self.api.env.skip_version_check:
+            options['version'] = u'2.0'
+        else:
+            options['version'] = API_VERSION
+
         uri = output['result'].get('uri', None)
-        rv = 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):
+        if uri is not None and not options.get('no_qrcode', False):
+            qr = self._get_qrcode(output, uri, options['version'])
+        else:
+            qr = None
+
+        rv = super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
+
+        if qr is not None:
             print("\n")
-            qr = qrcode.QRCode()
-            qr.add_data(uri)
-            qr.make()
-            qr.print_ascii(tty=True)
+            qr.print_ascii(tty=sys.stdout.isatty())
             print("\n")
 
         return rv
-- 
2.5.0

From 1af64f7809b5e1545927aa60d7b587d5d5fc60fd 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 | 81 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/ipalib/messages.py b/ipalib/messages.py
index 8926e953d783414dffefc3802d64516ae3734b30..2c9d0660d7364840c04b9ad4933371629e896590 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..a9bb5ae34f63206777d4a786573a1ccf8c8b1940 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:
@@ -354,17 +359,79 @@ class otptoken_add(LDAPCreate):
         _convert_owner(self.api.Object.user, entry_attrs, options)
         return super(otptoken_add, self).post_callback(ldap, dn, entry_attrs, *keys, **options)
 
+    def _get_qrcode(self, output, uri, version):
+        # Print QR code to terminal if specified
+        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(
+                version,
+                output,
+                message=ResultFormattingError(
+                    message=_("Unable to display QR code using the configured "
+                              "output encoding")
+                )
+            )
+            add_message(
+                version,
+                output,
+                message=ResultFormattingError(
+                    message=_(
+                        "Please use the token URI to configure you OTP device"
+                    )
+                )
+            )
+            return None
+
+        if sys.stdout.isatty():
+            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(
+                    version,
+                    output,
+                    message=ResultFormattingError(
+                        message=_(
+                            "QR code width is greater than that of the output "
+                            "tty. Please resize your terminal.")
+                    )
+                )
+
+        return qr
+
     def output_for_cli(self, textui, output, *args, **options):
+        # copy-pasted from ipalib/Frontend.__do_call()
+        # because option handling is broken on client-side
+        if 'version' in options:
+            pass
+        elif self.api.env.skip_version_check:
+            options['version'] = u'2.0'
+        else:
+            options['version'] = API_VERSION
+
         uri = output['result'].get('uri', None)
-        rv = 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):
+        if uri is not None and not options.get('no_qrcode', False):
+            qr = self._get_qrcode(output, uri, options['version'])
+        else:
+            qr = None
+
+        rv = super(otptoken_add, self).output_for_cli(
+                textui, output, *args, **options)
+
+        if qr is not None:
             print("\n")
-            qr = qrcode.QRCode()
-            qr.add_data(uri)
-            qr.make()
-            qr.print_ascii(tty=True)
+            qr.print_ascii(tty=sys.stdout.isatty())
             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