launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-826846 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #826846 in Launchpad itself: "AttributeError: 'NoneType' object has no attribute 'split' adding a ssh key"
  https://bugs.launchpad.net/launchpad/+bug/826846

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-826846/+merge/71769

= Summary =

Attempting to add an SSH key somehow passed None to the model method
new() when it really have been u''.  This data mismatch caused
an OOPS.

== Proposed fix ==

Make the model catch AttributeError and transform it to the expected
custom error.

== Pre-implementation notes ==

None.

== Tests ==

bin/test -vvt sshkey.txt

== Demo and Q/A ==

Cannot reproduce.  Tried multiple browsers.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:

Will fix lint.

  lib/lp/registry/doc/sshkey.txt
  lib/lp/registry/model/person.py

./lib/lp/registry/doc/sshkey.txt
       1: narrative uses a moin header.
       7: 'SSHKeyType' imported but unused
-- 
https://code.launchpad.net/~bac/launchpad/bug-826846/+merge/71769
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-826846 into lp:launchpad.

=== modified file 'lib/lp/registry/doc/sshkey.txt'
--- lib/lp/registry/doc/sshkey.txt	2010-10-19 18:44:31 +0000
+++ lib/lp/registry/doc/sshkey.txt	2011-08-16 19:58:24 +0000
@@ -33,6 +33,20 @@
     >>> key, key.keytext
     (<SSHKey at ...>, u'zzzNOT-EITHER')
 
+Bad keys raise a SSHKeyAdditionError.
+
+    >>> sshkeyset.new(foobar, None)
+    Traceback (most recent call last):
+      ...
+    SSHKeyAdditionError
+
+    >>> bad_key = "thiskeyhasnospaces"
+    >>> sshkeyset.new(foobar, bad_key)
+    Traceback (most recent call last):
+      ...
+    SSHKeyAdditionError
+
+
 There's also a convenience method for fetching multiple SSH keys at
 once:
 
@@ -42,4 +56,3 @@
     [(u'name12', <DBItem SSHKeyType.RSA, (1) RSA>, u'zzzNOT-EITHER'),
      (u'name12', <DBItem SSHKeyType.DSA, (2) DSA>, u'AAAAB3...vz9SG1gBOiI='),
      (u'name16', <DBItem SSHKeyType.RSA, (1) RSA>, u'zzzNOT-REALLY')]
-

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-08-11 20:37:16 +0000
+++ lib/lp/registry/model/person.py	2011-08-16 19:58:24 +0000
@@ -4419,7 +4419,7 @@
     def new(self, person, sshkey):
         try:
             kind, keytext, comment = sshkey.split(' ', 2)
-        except ValueError:
+        except (ValueError, AttributeError):
             raise SSHKeyAdditionError
 
         if not (kind and keytext and comment):