launchpad-reviewers team mailing list archive

[Maximiliano Bertacchini <maximiliano.bertacchini@xxxxxxxxxxxxx>]

Maximiliano Bertacchini has proposed merging lp:~maxiberta/launchpad/sshkeyadditionerror-msg-format into lp:launchpad.

Commit message:
Add SSHKeyAdditionError constructor to try to prevent a mismatch with SSO regex matching.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1777507 in Canonical SSO provider: "UnknownLaunchpadError in what should be a LaunchpadAPIError causes user-visible oops (adding SSH key)"
  https://bugs.launchpad.net/canonical-identity-provider/+bug/1777507

For more details, see:
https://code.launchpad.net/~maxiberta/launchpad/sshkeyadditionerror-msg-format/+merge/348230

Add SSHKeyAdditionError constructor to try to prevent a mismatch with SSO regex matching. It's already tested in lib/lp/registry/tests/test_ssh.py.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~maxiberta/launchpad/sshkeyadditionerror-msg-format into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/ssh.py'
--- lib/lp/registry/interfaces/ssh.py	2018-02-24 09:11:39 +0000
+++ lib/lp/registry/interfaces/ssh.py	2018-06-19 14:47:10 +0000
@@ -124,4 +124,21 @@
 
 @error_status(httplib.BAD_REQUEST)
 class SSHKeyAdditionError(Exception):
-    """Raised when the SSH public key is invalid."""
+    """Raised when the SSH public key is invalid.
+
+    WARNING: Be careful when changing the message format as
+    SSO uses a regex to recognize it.
+    """
+
+    def __init__(self, *args, **kwargs):
+        msg = ""
+        if 'key' in kwargs:
+            key = kwargs.pop('key')
+            msg = "Invalid SSH key data: '%s'" % key
+        if 'kind' in kwargs:
+            kind = kwargs.pop('kind')
+            msg = "Invalid SSH key type: '%s'" % kind
+        if 'exception' in kwargs:
+            exception = kwargs.pop('exception')
+            msg = "%s (%s)" % (msg, exception)
+        super(SSHKeyAdditionError, self).__init__(msg, *args, **kwargs)

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2018-05-14 10:59:35 +0000
+++ lib/lp/registry/model/person.py	2018-06-19 14:47:10 +0000
@@ -4137,8 +4137,7 @@
             try:
                 Key.fromString(sshkey)
             except Exception as e:
-                raise SSHKeyAdditionError(
-                    "Invalid SSH key data: '%s' (%s)" % (sshkey, e))
+                raise SSHKeyAdditionError(key=sshkey, exception=e)
 
         if send_notification:
             person.security_field_changed(
@@ -4171,15 +4170,14 @@
         try:
             kind, keytext, comment = sshkey.split(' ', 2)
         except (ValueError, AttributeError):
-            raise SSHKeyAdditionError("Invalid SSH key data: '%s'" % sshkey)
+            raise SSHKeyAdditionError(key=sshkey)
 
         if not (kind and keytext and comment):
-            raise SSHKeyAdditionError("Invalid SSH key data: '%s'" % sshkey)
+            raise SSHKeyAdditionError(key=sshkey)
 
         keytype = SSH_TEXT_TO_KEY_TYPE.get(kind)
         if keytype is None:
-            raise SSHKeyAdditionError(
-                "Invalid SSH key type: '%s'" % kind)
+            raise SSHKeyAdditionError(kind=kind)
         return keytype, keytext, comment