launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28508
[Merge] ~lgp171188/launchpad:allow-ssh-keys-without-comment into launchpad:master
Guruprasad has proposed merging ~lgp171188/launchpad:allow-ssh-keys-without-comment into launchpad:master.
Commit message:
Allow SSH keys without a comment to be uploaded
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/423518
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:allow-ssh-keys-without-comment into launchpad:master.
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 6a9eea7..5f061aa 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -4374,9 +4374,18 @@ class SSHKey(SQLBase):
if send_notification:
# For security reasons we want to notify the preferred email
# address that this sshkey has been removed.
+ if self.comment:
+ change_description = (
+ "The SSH key %s was removed from your account."
+ ) % (self.comment)
+ else:
+ change_description = (
+ "An SSH key of type '%s' was removed from your "
+ "account." % self.getFullKeyText().split(" ", 2)[0]
+ )
self.person.security_field_changed(
- "SSH Key removed from your Launchpad account.",
- "The SSH Key %s was removed from your account." % self.comment)
+ "SSH key removed from your Launchpad account.",
+ change_description)
super().destroySelf()
def getFullKeyText(self):
@@ -4397,7 +4406,10 @@ class SSHKey(SQLBase):
raise ValueError(
"SSH key of type %s has invalid data '%s'" %
(self.keytype, self.keytext))
- return "%s %s %s" % (ssh_keytype, self.keytext, self.comment)
+ key_text = "%s %s" % (ssh_keytype, self.keytext)
+ if self.comment:
+ key_text = "%s %s" % (key_text, self.comment)
+ return key_text
@implementer(ISSHKeySet)
@@ -4418,9 +4430,18 @@ class SSHKeySet:
raise SSHKeyAdditionError(type_mismatch=(kind, keydatatype))
if send_notification:
+ if comment:
+ change_description = (
+ "The SSH key '%s' has been added to your account."
+ ) % comment
+ else:
+ change_description = (
+ "An SSH key of type '%s' has been added "
+ "to your account." % keydatatype
+ )
person.security_field_changed(
"New SSH key added to your account.",
- "The SSH key '%s' has been added to your account." % comment)
+ change_description)
if not dry_run:
return SSHKey(person=person, keytype=keytype, keytext=keytext,
@@ -4446,12 +4467,17 @@ class SSHKeySet:
def _extract_ssh_key_components(self, sshkey):
try:
- kind, keytext, comment = sshkey.split(' ', 2)
- except (ValueError, AttributeError):
+ ssh_key_components = sshkey.split(' ', 2)
+ except AttributeError:
raise SSHKeyAdditionError(key=sshkey)
- if not (kind and keytext and comment):
+ if len(ssh_key_components) < 2:
raise SSHKeyAdditionError(key=sshkey)
+ elif len(ssh_key_components) == 2:
+ kind, keytext = ssh_key_components
+ comment = ''
+ else:
+ kind, keytext, comment = ssh_key_components
keytype = SSH_TEXT_TO_KEY_TYPE.get(kind)
if keytype is None:
diff --git a/lib/lp/registry/stories/person/xx-add-sshkey.txt b/lib/lp/registry/stories/person/xx-add-sshkey.txt
index a9ab70c..dde5b7f 100644
--- a/lib/lp/registry/stories/person/xx-add-sshkey.txt
+++ b/lib/lp/registry/stories/person/xx-add-sshkey.txt
@@ -173,6 +173,16 @@ format.
... print(tag.decode_contents())
SSH public key added.
+ >>> sshkey_without_comment = (
+ ... "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIGoDIdJaOkdr0wn0egyWEOtGxhe"
+ ... "3x0Iz9um6CEOFaqIw")
+ >>> browser.getControl(name='sshkey').value = sshkey_without_comment
+ >>> browser.getControl('Import Public Key').click()
+ >>> soup = find_main_content(browser.contents)
+ >>> for tag in soup('p', 'informational message'):
+ ... print(tag.decode_contents())
+ SSH public key added.
+
Launchpad administrators are not allowed to poke at other user's ssh keys.
>>> login(ANONYMOUS)
@@ -198,6 +208,7 @@ to edit his keys is on the page.
salgado@canario
salgado@canario
salgado@canario
+ ED25519 key
>>> browser.getLink('Update SSH keys').click()
>>> print(browser.title)
Change your SSH keys...
@@ -245,7 +256,7 @@ that the key has been removed.
>>> to_addr
['guilherme.salgado@xxxxxxxxxxxxx']
>>> payload = email.message_from_bytes(msg).get_payload()
- >>> assert payload.startswith('The SSH Key')
+ >>> assert payload.startswith('The SSH key')
Keys containing non-ASCII comments
diff --git a/lib/lp/registry/templates/person-portlet-contact-details.pt b/lib/lp/registry/templates/person-portlet-contact-details.pt
index a1e4de6..8650e7b 100644
--- a/lib/lp/registry/templates/person-portlet-contact-details.pt
+++ b/lib/lp/registry/templates/person-portlet-contact-details.pt
@@ -157,7 +157,10 @@
</dt>
<dd tal:define="sshkeys context/sshkeys">
<tal:keys repeat="sshkey sshkeys">
- <a href="+sshkeys" tal:content="sshkey/comment">foo@bar</a>
+ <a href="+sshkeys" tal:content="sshkey/comment" tal:condition="sshkey/comment">foo@bar</a>
+ <a href="+sshkeys" tal:condition="not: sshkey/comment">
+ <span tal:condition="not: sshkey/comment" tal:content="sshkey/keytype/title">RSA</span> key
+ </a>
<br />
</tal:keys>
<div tal:condition="not: context/sshkeys">
diff --git a/lib/lp/registry/tests/test_ssh.py b/lib/lp/registry/tests/test_ssh.py
index c85baad..07a4e30 100644
--- a/lib/lp/registry/tests/test_ssh.py
+++ b/lib/lp/registry/tests/test_ssh.py
@@ -68,6 +68,13 @@ class TestSSHKey(TestCaseWithFactory):
expected = "ssh-ed25519 %s %s" % (key.keytext, key.comment)
self.assertEqual(expected, key.getFullKeyText())
+ def test_getFullKeyText_for_key_without_comment(self):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ key = self.factory.makeSSHKey(person, "ssh-rsa", comment="")
+ expected = "ssh-rsa %s" % (key.keytext)
+ self.assertEqual(expected, key.getFullKeyText())
+
def test_getFullKeyText_for_corrupt_key(self):
# If the key text is corrupt, the type from the database is used
# instead of the one decoded from the text.
@@ -88,7 +95,7 @@ class TestSSHKey(TestCaseWithFactory):
[email] = pop_notifications()
self.assertEqual(
email['Subject'],
- "SSH Key removed from your Launchpad account.")
+ "SSH key removed from your Launchpad account.")
self.assertThat(
email.get_payload(),
StartsWith(
@@ -96,6 +103,27 @@ class TestSSHKey(TestCaseWithFactory):
% key.comment)
)
+ def test_destroySelf_notification_empty_comment(self):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ key = self.factory.makeSSHKey(
+ person, send_notification=False, comment=""
+ )
+ key.destroySelf()
+ [email] = pop_notifications()
+ self.assertEqual(
+ "SSH key removed from your Launchpad account.",
+ email['Subject']
+ )
+ keytype = key.getFullKeyText().split(" ", 1)[0]
+ self.assertThat(
+ email.get_payload(),
+ StartsWith(
+ "An SSH key of type '%s' was removed "
+ "from your account." % keytype
+ )
+ )
+
def test_destroySelf_notifications_can_be_suppressed(self):
person = self.factory.makePerson()
with person_logged_in(person):
@@ -124,6 +152,25 @@ class TestSSHKeySet(TestCaseWithFactory):
StartsWith("The SSH key 'comment' has been added to your account.")
)
+ def test_notification_add_ssh_key_comment_empty(self):
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ key = getUtility(ISSHKeySet).new(
+ person,
+ self.factory.makeSSHKeyText(comment="")
+ )
+ [email] = pop_notifications()
+ self.assertEqual(
+ "New SSH key added to your account.", email['Subject']
+ )
+ keytype = key.getFullKeyText().split(" ", 1)[0]
+ self.assertThat(
+ email.get_payload(),
+ StartsWith(
+ "An SSH key of type '%s' has been added "
+ "to your account." % keytype)
+ )
+
def test_does_not_send_notifications(self):
person = self.factory.makePerson()
with person_logged_in(person):
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 29c6059..6a5e459 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -4548,7 +4548,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
return "%s %s %s" % (key_type, key_text, comment)
def makeSSHKey(self, person=None, key_type="ssh-rsa",
- send_notification=True):
+ send_notification=True, comment=None):
"""Create a new SSHKey.
:param person: If specified, the person to attach the key to. If
@@ -4561,7 +4561,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
"""
if person is None:
person = self.makePerson()
- public_key = self.makeSSHKeyText(key_type=key_type)
+ public_key = self.makeSSHKeyText(key_type=key_type, comment=comment)
return getUtility(ISSHKeySet).new(
person, public_key, send_notification=send_notification)
Follow ups
