Merge lp:~sinzui/launchpad/newline-removal into lp:launchpad

[Curtis Hovey]

This is my branch to strip line breaks from listteammembers sshkeys.
This branch elaborates on the solution suggested by Nick Moffitt. Nick's
approach broke the existing tests, so I wrote a helper function that is
easy to test.

    lp:~sinzui/launchpad/newline-removal
    Diff size: 175 (50% lint)
    Launchpad bug: https://bugs.launchpad.net/bugs/556525
    Test command: ./bin/test -vv -t test_listteammembers
    Pre-implementation: no one
    Target release: 10.04

Strip line breaks from listteammembers sshkeys
-----------------------------------------------

The current report generation script preserves newlines in ssh key data as
input by a user. Often users will mis-paste their key from a terminal display
that inserted hard newlines (such as some configurations of less under certain
terminal types) and this will result in a key formatted such that it cannot be
used in an authorized_keys file.

In addition, any user can create a key such as the one at
https://launchpad.net/~lp-test-outflux/+sshkeys and create a false entry for
another user. This is the more troublesome aspect of this behavior, and
constitutes a security risk for systems that make use of this ssh key data.

Rules
-----

    * Using Nick's branch as a base, provide a test to verify that '\r\n\f'
      characters are removed from sshkeys.

QA
--

    * On staging, create a teeam named goat-boys
      and add ~lp-test-outflux as a member.
    * Run scripts/list-team-members -s goat-boys
    * Verify that ~lp-test-outflux's key is on one line.

Lint
----

Lint hated these files.

Linting changed files:
  lib/lp/registry/scripts/listteammembers.py
  lib/lp/registry/tests/test_listteammembers.py

Test
----

    * lib/lp/registry/tests/test_listteammembers.py
      * Added a test for the new function.

Implementation
--------------

    * lib/lp/registry/scripts/listteammembers.py
      * Extracted the sshkey formatting to a function and added a rule to
        remove '[\r\n\f]'

[Nick Moffitt (nick-moffitt)]

Is your multi-line break-up of the key on line 108 meant to be part of the test for this functionality? It would appear to me that no newlines would be inserted using that syntax.

review: Needs Fixing

[Nick Moffitt (nick-moffitt)]

Also, pawing through the whitespace changes and all, perhaps "bad_ssh_patters" could be written as "bad_ssh_patterns"?

Oh, I just spotted your u'123badKeysMight\r\nContain\fBadCharacters' line. It's worth also testing for newlines in the comment of a key as well.

[Curtis Hovey (sinzui)]

Hi Nick.

Our lint checker hated the two files I edited. It reported that I had to wrap the long lines and fix the commas before submitt my changes for review. So all the line wrapping was done just to quiet lint.

I renamed bad_ssh_pattern and I added bad characters to the sshkey comment.

{{{
=== modified file 'lib/lp/registry/scripts/listteammembers.py'
--- lib/lp/registry/scripts/listteammembers.py 2010-04-06 21:43:55 +0000
+++ lib/lp/registry/scripts/listteammembers.py 2010-04-07 13:07:27 +0000
@@ -27,14 +27,14 @@
     """Used if non-existent team name is specified."""

-bad_ssh_patters = re.compile('[\r\n\f]')
+bad_ssh_pattern = re.compile('[\r\n\f]')

 def make_sshkey_params(member, type_name, key):
     sshkey = "%s %s %s" % (
         type_name,
- bad_ssh_patters.sub('', key.keytext),
- bad_ssh_patters.sub('', key.comment).strip())
+ bad_ssh_pattern.sub('', key.keytext),
+ bad_ssh_pattern.sub('', key.comment).strip())
     return dict(name=member.name, sshkey=sshkey)

=== modified file 'lib/lp/registry/tests/test_listteammembers.py'
--- lib/lp/registry/tests/test_listteammembers.py 2010-04-06 21:43:55 +0000
+++ lib/lp/registry/tests/test_listteammembers.py 2010-04-07 13:07:27 +0000
@@ -98,7 +98,7 @@
         team.addMember(member, reviewer=team.teamowner)
         sshkey = self.factory.makeSSHKey(member)
         sshkey.keytext = u'123badKeysMight\r\nContain\fBadCharacters'
- sshkey.comment = 'comment'
+ sshkey.comment = 'co\rmm\ne\f\fnt'
         expected = dict(
             name=u'biggles',
             sshkey=u'ssh-rsa 123badKeysMightContainBadCharacters comment')

}}}

[Nick Moffitt (nick-moffitt)]

Many thanks!

review: Approve

[Gavin Panella (allenap)]

Some of the tests that you've folded the lines for are (result, expected) rather than (expected, result), so drive-by those if that knowledge now bothers you. Other than that, all's cool.

review: Approve

[Edwin Grubbs (edwin-grubbs)]

Hi Curtis,

This branch looks good. I just have a couple of minor comments below.

merge-conditional

-Edwin

>=== modified file 'lib/lp/registry/tests/test_listteammembers.py'
>--- lib/lp/registry/tests/test_listteammembers.py 2009-08-14 12:59:56 +0000
>+++ lib/lp/registry/tests/test_listteammembers.py 2010-04-07 13:59:25 +0000
>@@ -48,14 +52,22 @@
> ])
>
> ubuntuteam_sshkeys = [
>- u'mark: ssh-dss AAAAB3NzaC1kc3MAAABBAL5VoWG5sy3CnLYeOw47L8m9A15hA/PzdX2u0B7c2Z1ktFPcEaEuKbLqKVSkXpYm7YwKj9y88A9Qm61CdvI0c50AAAAVAKGY0YON9dEFH3DzeVYHVEBGFGfVAAAAQCoe0RhBcefm4YiyQVwMAxwTlgySTk7FSk6GZ95EZ5Q8/OTdViTaalvGXaRIsBdaQamHEBB+Vek/VpnF1UGGm8YAAABAaCXDl0r1k93JhnMdF0ap4UJQ2/NnqCyoE8Xd5KdUWWwqwGdMzqB1NOeKN6ladIAXRggLc2E00UsnUXh3GE3Rgw== Private key in lib/lp/codehosting/tests/id_dsa',
>+ u'mark: ssh-dss AAAAB3NzaC1kc3MAAABBAL5VoWG5sy3CnLYeOw47L8m9A15hA/PzdX2u0'
>+ u'B7c2Z1ktFPcEaEuKbLqKVSkXpYm7YwKj9y88A9Qm61CdvI0c50AAAAVAKGY0YON9dEFH3Dz'
>+ u'eVYHVEBGFGfVAAAAQCoe0RhBcefm4YiyQVwMAxwTlgySTk7FSk6GZ95EZ5Q8/OTdViTaalv'
>+ u'GXaRIsBdaQamHEBB+Vek/VpnF1UGGm8YAAABAaCXDl0r1k93JhnMdF0ap4UJQ2/NnqCyoE8'
>+ u'Xd5KdUWWwqwGdMzqB1NOeKN6ladIAXRggLc2E00UsnUXh3GE3Rgw== Private key in '
>+ u'lib/lp/codehosting/tests/id_dsa',
> ]
>
>
>-class ListTeamMembersTestCase(unittest.TestCase):
>+class ListTeamMembersTestCase(TestCaseWithFactory):
> """Test listing team members."""
> layer = LaunchpadZopelessLayer
>
>+ def setUp(self):
>+ super(ListTeamMembersTestCase, self).setUp()

Why is this necessary?

> def test_listteammembers_default_list(self):
> """Test the default option."""
> self.assertEqual(
>@@ -64,22 +76,40 @@
> def test_listteammembers_email_only(self):
> """Test the email only option."""
> self.assertEqual(
>- listteammembers.process_team('ubuntu-team', 'email'), ubuntuteam_email)
>+ listteammembers.process_team('ubuntu-team', 'email'),
>+ ubuntuteam_email)
>
> def test_listteammembers_full_details(self):
> """Test the full details option."""
> self.assertEqual(
>- listteammembers.process_team('ubuntu-team', 'full'), ubuntuteam_full)
>+ listteammembers.process_team('ubuntu-team', 'full'),
>+ ubuntuteam_full)
>
> def test_listteammembers_sshkeys(self):
> """Test the ssh keys option."""
> self.assertEqual(
>- listteammembers.process_team('ubuntu-team', 'sshkeys'), ubuntuteam_sshkeys)
>+ listteammembers.process_team('ubuntu-team', 'sshkeys'),
>+ ubuntuteam_sshkeys)
>+
>+ def test_make_sshkey_params(self):
>+ """Test that ssh keys ."""

Incomplete docstring.

>+ member = self.factory.makePerson(name='biggles')
>+ team = self.factory.makeTeam(name='squadron')
>+ team.addMember(member, reviewer=team.teamowner)
>+ sshkey = self.factory.makeSSHKey(member)
>+ sshkey.keytext = u'123badKeysMight\r\nContain\fBadCharacters'
>+ sshkey.comment = 'co\rmm\ne\f\fnt'
>+ expected = dict(
>+ name=u'biggles',
>+ sshkey=u'ssh-rsa 123badKeysMightContainBadCharacters comment')
>+ ...

review: Approve

[Curtis Hovey (sinzui)]

On Wed, 2010-04-07 at 15:50 +0000, Gavin Panella wrote:
> Review: Approve
> Some of the tests that you've folded the lines for are (result,
> expected) rather than (expected, result), so drive-by those if that
> knowledge now bothers you. Other than that, all's cool.

Oh that is so cruel. How can I sleep knowing this? I cannot. Fixed.

On Wed, 2010-04-07 at 16:04 +0000, Edwin Grubbs wrote:
> Review: Approve
> Hi Curtis,
>
> This branch looks good. I just have a couple of minor comments below.
>
> merge-conditional
...
> >+class ListTeamMembersTestCase(TestCaseWithFactory):
> > """Test listing team members."""
> > layer = LaunchpadZopelessLayer
> >
> >+ def setUp(self):
> >+ super(ListTeamMembersTestCase, self).setUp()
>
>
> Why is this necessary?

It is not. I assumed for a moment that I needs to do some setup for
everything. I Removed this.

...

> >+ def test_make_sshkey_params(self):
> >+ """Test that ssh keys ."""
>
>
> Incomplete docstring.

Indeed:
    """Test that ssh keys are rendered as a single line."""

{{{

=== modified file 'lib/lp/registry/tests/test_listteammembers.py'
--- lib/lp/registry/tests/test_listteammembers.py 2010-04-07 13:07:27 +0000
+++ lib/lp/registry/tests/test_listteammembers.py 2010-04-07 20:15:44 +0000
@@ -65,34 +65,31 @@
     """Test listing team members."""
     layer = LaunchpadZopelessLayer

- def setUp(self):
- super(ListTeamMembersTestCase, self).setUp()
-
     def test_listteammembers_default_list(self):
         """Test the default option."""
         self.assertEqual(
- listteammembers.process_team('ubuntu-team'), ubuntuteam_default)
+ ubuntuteam_default, listteammembers.process_team('ubuntu-team'))

     def test_listteammembers_email_only(self):
         """Test the email only option."""
         self.assertEqual(
- listteammembers.process_team('ubuntu-team', 'email'),
- ubuntuteam_email)
+ ubuntuteam_email,
+ listteammembers.process_team('ubuntu-team', 'email'))

     def test_listteammembers_full_details(self):
         """Test the full details option."""
         self.assertEqual(
- listteammembers.process_team('ubuntu-team', 'full'),
- ubuntuteam_full)
+ ubuntuteam_full,
+ listteammembers.process_team('ubuntu-team', 'full'))

     def test_listteammembers_sshkeys(self):
         """Test the ssh keys option."""
         self.assertEqual(
- listteammembers.process_team('ubuntu-team', 'sshkeys'),
- ubuntuteam_sshkeys)
+ ubuntuteam_sshkeys,
+ listteammembers.process_team('ubuntu-team', 'sshkeys'))

     def test_make_sshkey_params(self):
- """Test that ssh keys ."""
+ """Test that ssh keys are rendered as a single line."""
         member = self.factory.makePerson(name='biggles')
         team = self.factory.makeTeam(name='squadron')
         team.addMember(member, reviewer=team.teamowner)

}}}
--
__Curtis C. Hovey_________
http://launchpad.net/

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2010-03-29 16:44:37 +0000
+++ lib/lp/registry/interfaces/distribution.py	2010-04-07 20:59:30 +0000
@@ -25,11 +25,12 @@
 
 from lazr.restful.fields import CollectionField, Reference
 from lazr.restful.declarations import (
-   collection_default_content, copy_field, export_as_webservice_collection,
+   collection_default_content, export_as_webservice_collection,
    export_as_webservice_entry, export_operation_as,
    export_read_operation, exported, operation_parameters,
    operation_returns_collection_of, operation_returns_entry,
    rename_parameters_as)
+from lazr.restful.interface import copy_field
 
 from canonical.launchpad import _
 from canonical.launchpad.fields import (

=== modified file 'lib/lp/registry/scripts/listteammembers.py'
--- lib/lp/registry/scripts/listteammembers.py	2009-06-25 04:06:00 +0000
+++ lib/lp/registry/scripts/listteammembers.py	2010-04-07 20:59:30 +0000
@@ -6,6 +6,8 @@
 __metaclass__ = type
 __all__ = ['process_team']
 
+import re
+
 from zope.component import getUtility
 
 from lp.registry.interfaces.person import IPersonSet
@@ -13,17 +15,29 @@
 
 
 OUTPUT_TEMPLATES = {
-   'simple': '%(name)s, %(email)s',
-   'email': '%(email)s',
-   'full': '%(teamname)s|%(id)s|%(name)s|%(email)s|%(displayname)s|%(ubuntite)s',
-   'sshkeys': '%(name)s: %(sshkey)s',
-   }
+    'simple': '%(name)s, %(email)s',
+    'email': '%(email)s',
+    'full': '%(teamname)s|%(id)s|%(name)s|%(email)s|'
+            '%(displayname)s|%(ubuntite)s',
+    'sshkeys': '%(name)s: %(sshkey)s',
+    }
 
 
 class NoSuchTeamError(Exception):
     """Used if non-existent team name is specified."""
 
 
+bad_ssh_pattern = re.compile('[\r\n\f]')
+
+
+def make_sshkey_params(member, type_name, key):
+    sshkey = "%s %s %s" % (
+        type_name,
+        bad_ssh_pattern.sub('', key.keytext),
+        bad_ssh_pattern.sub('', key.comment).strip())
+    return dict(name=member.name, sshkey=sshkey)
+
+
 def process_team(teamname, display_option='simple'):
     output = []
     people = getUtility(IPersonSet)
@@ -54,11 +68,7 @@
                     type_name = 'ssh-rsa'
                 else:
                     type_name = 'Unknown key type'
-                params = dict(
-                    name=member.name,
-                    sshkey="%s %s %s" % (type_name, key.keytext, 
-                        key.comment.strip())
-                    )
+                params = make_sshkey_params(member, type_name, key)
                 output.append(template % params)
         # Ubuntite
         ubuntite = "no"
@@ -74,7 +84,7 @@
             id=member.id,
             displayname=member.displayname.encode("ascii", "replace"),
             ubuntite=ubuntite,
-            sshkey=sshkey
+            sshkey=sshkey,
             )
         output.append(template % params)
     # If we're only looking at email, remove --none-- entries
@@ -86,4 +96,3 @@
     if display_option == 'sshkeys':
         output = [line for line in output if line[-8:] != '--none--']
     return sorted(output)
-

=== modified file 'lib/lp/registry/tests/test_listteammembers.py'
--- lib/lp/registry/tests/test_listteammembers.py	2009-08-14 12:59:56 +0000
+++ lib/lp/registry/tests/test_listteammembers.py	2010-04-07 20:59:30 +0000
@@ -6,6 +6,8 @@
 from canonical.testing import LaunchpadZopelessLayer
 
 from lp.registry.scripts import listteammembers
+from lp.testing import TestCaseWithFactory
+
 
 ubuntuteam_default = sorted([
     u'cprov, [e-mail]',
@@ -36,11 +38,13 @@
 
 ubuntuteam_full = sorted([
     u'ubuntu-team|10|limi|[e-mail]|Alexander Limi|no',
-    u'ubuntu-team|11|stevea|[e-mail]|Steve Alexander|no',
+    u'ubuntu-team|11|stevea|[e-mail]'
+        u'|Steve Alexander|no',
     u'ubuntu-team|16|name16|[e-mail]|Foo Bar|yes',
     u'ubuntu-team|19|warty-gnome|--none--|Warty Gnome Team|no',
     u'ubuntu-team|1|mark|[e-mail]|Mark Shuttleworth|no',
-    u'ubuntu-team|26|kinnison|[e-mail]|Daniel Silverstone|no',
+    u'ubuntu-team|26|kinnison|[e-mail]'
+        u'|Daniel Silverstone|no',
     u'ubuntu-team|28|cprov|[e-mail]|Celso Providelo|no',
     u'ubuntu-team|33|edgar|[e-mail]|Edgar Bursic|no',
     u'ubuntu-team|4|kamion|[e-mail]|Colin Watson|no',
@@ -48,38 +52,61 @@
     ])
 
 ubuntuteam_sshkeys = [
-    u'mark: ssh-dss AAAAB3NzaC1kc3MAAABBAL5VoWG5sy3CnLYeOw47L8m9A15hA/PzdX2u0B7c2Z1ktFPcEaEuKbLqKVSkXpYm7YwKj9y88A9Qm61CdvI0c50AAAAVAKGY0YON9dEFH3DzeVYHVEBGFGfVAAAAQCoe0RhBcefm4YiyQVwMAxwTlgySTk7FSk6GZ95EZ5Q8/OTdViTaalvGXaRIsBdaQamHEBB+Vek/VpnF1UGGm8YAAABAaCXDl0r1k93JhnMdF0ap4UJQ2/NnqCyoE8Xd5KdUWWwqwGdMzqB1NOeKN6ladIAXRggLc2E00UsnUXh3GE3Rgw== Private key in lib/lp/codehosting/tests/id_dsa',
+    u'mark: ssh-dss AAAAB3NzaC1kc3MAAABBAL5VoWG5sy3CnLYeOw47L8m9A15hA/PzdX2u0'
+    u'B7c2Z1ktFPcEaEuKbLqKVSkXpYm7YwKj9y88A9Qm61CdvI0c50AAAAVAKGY0YON9dEFH3Dz'
+    u'eVYHVEBGFGfVAAAAQCoe0RhBcefm4YiyQVwMAxwTlgySTk7FSk6GZ95EZ5Q8/OTdViTaalv'
+    u'GXaRIsBdaQamHEBB+Vek/VpnF1UGGm8YAAABAaCXDl0r1k93JhnMdF0ap4UJQ2/NnqCyoE8'
+    u'Xd5KdUWWwqwGdMzqB1NOeKN6ladIAXRggLc2E00UsnUXh3GE3Rgw== Private key in '
+    u'lib/lp/codehosting/tests/id_dsa',
     ]
 
 
-class ListTeamMembersTestCase(unittest.TestCase):
+class ListTeamMembersTestCase(TestCaseWithFactory):
     """Test listing team members."""
     layer = LaunchpadZopelessLayer
 
     def test_listteammembers_default_list(self):
         """Test the default option."""
         self.assertEqual(
-            listteammembers.process_team('ubuntu-team'), ubuntuteam_default)
+            ubuntuteam_default, listteammembers.process_team('ubuntu-team'))
 
     def test_listteammembers_email_only(self):
         """Test the email only option."""
         self.assertEqual(
-            listteammembers.process_team('ubuntu-team', 'email'), ubuntuteam_email)
+            ubuntuteam_email,
+            listteammembers.process_team('ubuntu-team', 'email'))
 
     def test_listteammembers_full_details(self):
         """Test the full details option."""
         self.assertEqual(
-            listteammembers.process_team('ubuntu-team', 'full'), ubuntuteam_full)
+            ubuntuteam_full,
+            listteammembers.process_team('ubuntu-team', 'full'))
 
     def test_listteammembers_sshkeys(self):
         """Test the ssh keys option."""
         self.assertEqual(
-            listteammembers.process_team('ubuntu-team', 'sshkeys'), ubuntuteam_sshkeys)
+            ubuntuteam_sshkeys,
+            listteammembers.process_team('ubuntu-team', 'sshkeys'))
+
+    def test_make_sshkey_params(self):
+        """Test that ssh keys are rendered as a single line."""
+        member = self.factory.makePerson(name='biggles')
+        team = self.factory.makeTeam(name='squadron')
+        team.addMember(member, reviewer=team.teamowner)
+        sshkey = self.factory.makeSSHKey(member)
+        sshkey.keytext = u'123badKeysMight\r\nContain\fBadCharacters'
+        sshkey.comment = 'co\rmm\ne\f\fnt'
+        expected = dict(
+            name=u'biggles',
+            sshkey=u'ssh-rsa 123badKeysMightContainBadCharacters comment')
+        result = listteammembers.make_sshkey_params(member, 'ssh-rsa', sshkey)
+        self.assertEqual(expected, result)
 
     def test_listteammembers_unknown_team(self):
         """Test unknown team."""
         self.assertRaises(
-            listteammembers.NoSuchTeamError, listteammembers.process_team, 'nosuchteam-matey')
+            listteammembers.NoSuchTeamError, listteammembers.process_team,
+            'nosuchteam-matey')
 
 
 def test_suite():