launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/move-ssh-key-creation into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Firstly, I had a pre-implementation chat with Robert Collins about the best way to effect this, and we both agree that the way I've done it is good as a clean-up.

This branch moves validation of SSH keys from browser code to model code, along with fixing the tests that are testing creation and deletion of same.
-- 
https://code.launchpad.net/~stevenk/launchpad/move-ssh-key-creation/+merge/29798
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/move-ssh-key-creation into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-07-08 21:14:06 +0000
+++ lib/lp/registry/browser/person.py	2010-07-13 14:26:10 +0000
@@ -84,7 +84,6 @@
 import copy
 import itertools
 import pytz
-import subprocess
 import urllib
 
 from datetime import datetime, timedelta
@@ -165,7 +164,8 @@
     IPerson, IPersonClaim, IPersonSet, ITeam, ITeamReassignment,
     PersonVisibility, TeamMembershipRenewalPolicy, TeamSubscriptionPolicy)
 from lp.registry.interfaces.poll import IPollSet, IPollSubset
-from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType
+from lp.registry.interfaces.ssh import (
+    ISSHKeySet, SSHKeyType, SSHKeyAdditionError)
 from lp.registry.interfaces.teammembership import (
     DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT, ITeamMembership,
     ITeamMembershipSet, TeamMembershipStatus)
@@ -3598,43 +3598,13 @@
         return canonical_url(self.context, view_name="+edit")
 
     def add_ssh(self):
-        # XXX: JonathanLange 2010-05-13: This should hella not be in browser
-        # code. Move this to ISSHKeySet (bonus! tests become easier to write).
         sshkey = self.request.form.get('sshkey')
-        try:
-            kind, keytext, comment = sshkey.split(' ', 2)
-        except ValueError:
-            self.error_message = structured('Invalid public key')
-            return
-
-        if not (kind and keytext and comment):
-            self.error_message = structured('Invalid public key')
-            return
-
-        process = subprocess.Popen(
-            '/usr/bin/ssh-vulnkey -', shell=True, stdin=subprocess.PIPE,
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        (out, err) = process.communicate(sshkey.encode('utf-8'))
-        if 'compromised' in out.lower():
-            self.error_message = structured(
-                'This key is known to be compromised due to a security flaw '
-                'in the software used to generate it, so it will not be '
-                'accepted by Launchpad. See the full '
-                '<a href="http://www.ubuntu.com/usn/usn-612-2";>Security '
-                'Notice</a> for further information and instructions on how '
-                'to generate another key.')
-            return
-
-        if kind == 'ssh-rsa':
-            keytype = SSHKeyType.RSA
-        elif kind == 'ssh-dss':
-            keytype = SSHKeyType.DSA
+	try:
+      	    getUtility(ISSHKeySet).new(self.user, sshkey)
+        except SSHKeyAdditionError, e:
+            self.error_message = structured(e)
         else:
-            self.error_message = structured('Invalid public key')
-            return
-
-        getUtility(ISSHKeySet).new(self.user, keytype, keytext, comment)
-        self.info_message = structured('SSH public key added.')
+            self.info_message = structured('SSH public key added.')
 
     def remove_ssh(self):
         key_id = self.request.form.get('key')

=== modified file 'lib/lp/registry/doc/sshkey.txt'
--- lib/lp/registry/doc/sshkey.txt	2009-04-17 10:32:16 +0000
+++ lib/lp/registry/doc/sshkey.txt	2010-07-13 14:26:10 +0000
@@ -20,13 +20,13 @@
 Adding new keys is pretty easy:
 
     >>> foobar = personset.getByName('name16')
-    >>> key = sshkeyset.new(foobar, SSHKeyType.RSA, "zzzNOT-REALLY",
-    ...     "This is just a test key.")
+    >>> key = sshkeyset.new(
+    ...     foobar, "ssh-rsa zzzNOT-REALLY This is just a test key")
     >>> key, key.keytext
     (<SSHKey at ...>, u'zzzNOT-REALLY')
 
-    >>> key = sshkeyset.new(name12, SSHKeyType.RSA, "zzzNOT-EITHER",
-    ...     "This is just a test key.")
+    >>> key = sshkeyset.new(
+    ...     name12, "ssh-rsa zzzNOT-EITHER This is just a test key.")
     >>> key, key.keytext
     (<SSHKey at ...>, u'zzzNOT-EITHER')
 

=== modified file 'lib/lp/registry/interfaces/ssh.py'
--- lib/lp/registry/interfaces/ssh.py	2010-03-24 20:19:52 +0000
+++ lib/lp/registry/interfaces/ssh.py	2010-07-13 14:26:10 +0000
@@ -10,6 +10,7 @@
 __all__ = [
     'ISSHKey',
     'ISSHKeySet',
+    'SSHKeyAdditionError',
     'SSHKeyType',
     ]
 
@@ -63,7 +64,7 @@
 class ISSHKeySet(Interface):
     """The set of SSHKeys."""
 
-    def new(person, keytype, keytext, comment):
+    def new(person, sshkey):
         """Create a new SSHKey pointing to the given Person."""
 
     def getByID(id, default=None):
@@ -75,3 +76,8 @@
     def getByPeople(people):
         """Return SSHKey object associated to the people provided."""
 
+
+class SSHKeyAdditionError(Exception):
+    """I get raised when something went wrong with adding an SSH key."""
+
+

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-07-09 14:58:42 +0000
+++ lib/lp/registry/model/person.py	2010-07-13 14:26:10 +0000
@@ -31,6 +31,7 @@
 import pytz
 import random
 import re
+import subprocess
 import time
 import weakref
 
@@ -125,7 +126,8 @@
     SpecificationDefinitionStatus, SpecificationFilter,
     SpecificationImplementationStatus, SpecificationSort)
 from canonical.launchpad.interfaces.lpstorm import IStore
-from lp.registry.interfaces.ssh import ISSHKey, ISSHKeySet, SSHKeyType
+from lp.registry.interfaces.ssh import (
+    ISSHKey, ISSHKeySet, SSHKeyType, SSHKeyAdditionError)
 from lp.registry.interfaces.teammembership import (
     TeamMembershipStatus)
 from lp.registry.interfaces.wikiname import IWikiName, IWikiNameSet
@@ -3746,7 +3748,35 @@
 class SSHKeySet:
     implements(ISSHKeySet)
 
-    def new(self, person, keytype, keytext, comment):
+    def new(self, person, sshkey):
+        try:
+            kind, keytext, comment = sshkey.split(' ', 2)
+        except ValueError:
+            raise SSHKeyAdditionError, 'Invalid public key'
+
+        if not (kind and keytext and comment):
+            raise SSHKeyAdditionError, 'Invalid public key'
+
+        process = subprocess.Popen(
+            '/usr/bin/ssh-vulnkey -', shell=True, stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        (out, err) = process.communicate(sshkey.encode('utf-8'))
+        if 'compromised' in out.lower():
+            raise SSHKeyAdditionError, (
+                'This key is known to be compromised due to a security flaw '
+                'in the software used to generate it, so it will not be '
+                'accepted by Launchpad. See the full '
+                '<a href="http://www.ubuntu.com/usn/usn-612-2";>Security '
+                'Notice</a> for further information and instructions on how '
+                'to generate another key.')
+
+        if kind == 'ssh-rsa':
+            keytype = SSHKeyType.RSA
+        elif kind == 'ssh-dss':
+            keytype = SSHKeyType.DSA
+        else:
+            raise SSHKeyAdditionError, 'Invalid public key'
+
         return SSHKey(person=person, keytype=keytype, keytext=keytext,
                       comment=comment)