Merge ~tsimonq2/ssh-import-id/+git/add-team-support:master into ssh-import-id:master

[Simon Quigley]

Add team support to the Launchpad interface (LP: #1745538).

This adds team support to the Launchpad interface for ssh-import-id.

This is done in fetch_keys because it's the right middle point between calling the function to fetch the keys and actually doing it.

It logs into Launchpad anonymously, checks if the ID is valid (and errors out if it isn't), and checks to see if it's a team. If it is a team, it recursively gets the SSH key for every member of the team who has an active Launchpad account (which doesn't include other teams, but could be implemented in the future). If it isn't a team, it just does what it normally does.

If it matters, I have signed the Canonical CLA.

Thanks, and I look forward to your feedback.

[Scott Moser (smoser)]

Well, I like the idea of adding group support.

Unfortunately, I don't have a better solution that provides the desired functionality.

Perhaps we could require '--team' or 'lp:team-myteam' or some other way of indicating that the input is a team. Then we could opportunistically import Launchpadlib and take that path for the team. If the import failed give a reasonable error.

What I'm not so happy about in this implementation is:

a.) the existing user case pays a speed penalty
    $ time python3 -m timeit --setup='from launchpadlib.launchpad import Launchpad' --number=3 --repeat=1 'Launchpad.login_anonymously("ssh-import-id", "production", version="devel")'
    3 loops, best of 1: 1.07 sec per loop

b.) added dependency on Launchpadlib is pretty large.
$ apt-cache show python3-launchpadlib | grep Depen
Depends: python3-httplib2 (>= 0.4.0), python3-keyring (>= 0.5), python3-lazr.restfulclient (>= 0.11.2), python3-lazr.uri (>= 1.0.2-4~), python3-simplejson, python3-wadllib, python3:any (>= 3.3.2-2~)

In a standard cloud image, that ends up getting 10 new packages and 2.6M new
disk footprint (in bionic). http://paste.ubuntu.com/p/2sMhtw7K2d/

c.) version='devel'.
I think this is basically saying "give me the latest api". Which is kind of a guarantee that it will be broken at some point. Using a 'devel' or 'latest' version of anything isn't really acceptable for something that is expected to continue to work for any reasonable amount of time (such as that of an Ubuntu LTS).

What do you think?

review: Needs Fixing

[Colin Watson (cjwatson)]

On c), the business of freezing Launchpad APIs never really turned out to work very well in practice, and for most things we generally recommend that people just use 'devel'. Note that there is currently no version of the API other than 'devel' that has an advertised EOL date in the future, so if you're going to take a hard line on this then what you're effectively saying is that people can't write tools that use the Launchpad API, which I don't think is reasonable. In particular, the 'account_status' property that this tool relies on is only exported on devel.

[Scott Moser (smoser)]

@Colin,

I understand that 'devel' is what most (all) tools use for version. But the majority of tools that I've seen that use launchpad are effectively developer scripts. I've written several, if they broke we fix them in version control somewhere and go on with life. If users of ubuntu use 'ssh-import-id mygroup', they expect that to work, and if it regresses then Ubuntu regressed.
It has a dramatically larger scope.

That said, I do understand your point, and what your voice in particular represents here.

If there were only points 'a' and 'b', I'd still suggest we look for another solution.

Am I missing something ? Is it un-advisable for some reason to just read
  https://api.launchpad.net/1.0/~group/members
and then get the list that way?

[Colin Watson (cjwatson)]

Here's the reason that won't work:

    account_status = exported(
        Choice(
            title=_("The status of this person's account"), required=True,
            readonly=True, vocabulary=AccountStatus),
        as_of='devel')

I understand your concerns, and they do align with why Launchpad originally tried to produce stable API versions. On the other hand, in practice, all the interfaces being used here are in fact extremely stable and I have a hard time imagining us changing them; we would certainly attempt to get an idea of whether they were being used in production first, and at that point we'd discover the uses by ssh-import-id. I wouldn't expect this to break.

You make a reasonable argument for avoiding launchpadlib in this particular case. I think it would be OK to start from https://api.launchpad.net/devel/~TEAM/members.

[Scott Moser (smoser)]

@Simon,

I poked a bit here is a diff from master that I think seems a reasonable start.
 http://paste.ubuntu.com/p/jcC53Sp3KJ/

Basically, if the +ssh-keys path failed *and* the url was the default url to launchpad ssh keys, then we will go in looking to see if it is a team also.

The fetch_keys_lp_team can be a normal python function that raises exception on error.
We're hoping to get away from things like 'die' in function calls.

Does that seem a reasonable direction to you?

There are plenty of improvements to be made. I can see that we may well want this to return a tuple of "path" and "keys", where "path" is:
  group1/sub-group2/user
So that the code in import_keys could be adjusted to more clearly indicate how this key got there.

Thoughts?

Unmerged commits

1ee29bb... by Simon Quigley on 2018-06-23

Add team support to ssh-import-id-lp.

diff --git a/ssh_import_id/__init__.py b/ssh_import_id/__init__.py
index d2f845e..6371c4d 100644
--- a/ssh_import_id/__init__.py
+++ b/ssh_import_id/__init__.py
@@ -39,6 +39,7 @@ try:
 except ImportError:
     from urllib import quote_plus
 
+from launchpadlib.launchpad import Launchpad
 
 from .version import VERSION
 
@@ -225,7 +226,19 @@ def fetch_keys(proto, username, useragent):
     Call out to a subcommand to handle the specified protocol and username
     """
     if proto == "lp":
-        return fetch_keys_lp(username, useragent)
+        launchpad = Launchpad.login_anonymously('ssh-import-id', 'production', version='devel')
+        try:
+            lpid = launchpad.people[username]
+        except:
+            die("Launchpad ID not found.")
+        if lpid.is_team:
+            keys = ""
+            for member in lpid.members_details:
+                if member.member.account_status == "Active":
+                    keys = keys + "\n" + fetch_keys_lp(member.member.name, useragent)
+            return keys
+        else:
+            return fetch_keys_lp(username, useragent)
     elif proto == "gh":
         return fetch_keys_gh(username, useragent)