Merge ppa-dev-tools:add-ppa-credentials-command into ppa-dev-tools:main

[Bryce Harrington]

This adds a 'ppa credentials' command that dumps your Launchpad creds to a file, that can be passed back in via LP_CREDENTIALS.

This enables a workaround for LP: #2022363, but may be handy for any case where either you can't log in via the website, or the automatically stored credentials aren't getting read for some reason.

This is not a complete solution for LP: #2022363, since the snap should be handling this automatically, but that will require separate investigation.

I imagine this command could benefit from some options, like specifying how to name the credentials file, and it might be nice to be able to refer to it via --credentials-file=<filename> instead of via the env variable. But for this first pass I'm keeping it simple, as I just want the functionality available for the release, which I want to get out the door ASAP.

[Christian Ehrhardt  (paelzer)]

A few minor comments and suggestions to improve before merging as we might never look back to it otherwise.

The "allow argument to specify output file" seems so easy that it should IMHO be added.
I mean any snap magic that needs to be research, yes that is later.
But allowing where to store, that should be so easy that we should not hold back on that.

And TBH a global arg (not per subcommand, but generally available) to pass a filename.
"--use-credential" (if no arg given also using the default path) and "--use-credential foo" which would do not more but load that into the environment variable - that also seems rather close. Like <1h of work.
So again, unless you already released by time pressure, please consider adding that right away.

review: Needs Fixing

629848e... by Bryce Harrington on 2023-09-11

Add a --credentials option to use the stored creds for auth

Modify the Lp class to accept credentials on creation, and refactor the
existing LP_CREDENTIALS envvar handling code to use this instead. This
moves the envvar handling from the ppa.lp module to the ppa script
itself; add a new wrapper create_lp() to encapsulate this and to add
support for loading credentials from a given file, which can be
specified by a newly introduced --credentials global argument.

3fed215... by Bryce Harrington on 2023-09-11

ppa: Include name of the default filename in help text

[Bryce Harrington (bryce)]

Unfortunately this was kind of a "pull a loose thread from a sweater" problem, and ended up actually involving a more extensive amount of work than apparent. The credentials handling is pretty early on in the process (obviously) which made it tricky to tweak without breaking other things, but the good news is that the testsuite is extensive and thorough enough now that it was super helpful in spotting problems.

I've refactored the env var handling to be more general purpose, and layered in functionality to allow it to come from the specified file. I opted to name the option --credentials rather than --use-credentials to match the similar option --config.

I had initially hoped to split the review changes out separately to make it easier for you to check, but due to needing to add more fixes it was simpler to just merge all the rework into the main commits. Hopefully you can see what changed via range-diff, although since so much changed it may be worth just doing a fresh review from scratch.

[Christian Ehrhardt  (paelzer)]

+1 on refactoring the argparse and help text later in a separate PR - just make sure it is not forgotten

This MP LGMT now, the few things that came to my mind were too small to mention or known and can be done in follow up iterations.

review: Approve

[Bryce Harrington (bryce)]

Thanks, change landed to main:

$ git merge --ff-only add-ppa-credentials-command
Updating 06e6152..20533a7
Fast-forward
 ppa/constants.py | 2 ++
 ppa/lp.py | 53 +++++++++++++++++++++++++++++++++++----------------
 scripts/ppa | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 tests/helpers.py | 13 ++++++++++++-
 tests/test_lp.py | 8 ++++----
 tests/test_scripts_ppa.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 185 insertions(+), 31 deletions(-)
$ git push origin
Enumerating objects: 33, done.
Counting objects: 100% (33/33), done.
Delta compression using up to 12 threads
Compressing objects: 100% (20/20), done.
Writing objects: 100% (23/23), 4.83 KiB | 1.21 MiB/s, done.
Total 23 (delta 17), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   06e6152..20533a7 main -> main

diff --git a/ppa/constants.py b/ppa/constants.py
index da05046..4362562 100644
--- a/ppa/constants.py
+++ b/ppa/constants.py
@@ -17,6 +17,8 @@ ARCHES_PPA_ALL = ["amd64", "arm64", "armhf", "i386", "powerpc", "ppc64el", "s390
 ARCHES_PPA_EXTRA = ["riscv64"]
 ARCHES_AUTOPKGTEST = ["amd64", "arm64", "armhf", "i386", "ppc64el", "s390x"]
 
+CREDENTIALS_FILENAME_DEFAULT = "credentials.oauth"
+
 URL_LPAPI = "https://api.launchpad.net/devel"
 URL_AUTOPKGTEST = "https://autopkgtest.ubuntu.com"
 
diff --git a/ppa/lp.py b/ppa/lp.py
index c31d4dc..5ede47a 100644
--- a/ppa/lp.py
+++ b/ppa/lp.py
@@ -11,7 +11,6 @@
 
 """Launchpad Interface."""
 
-import os
 from contextlib import suppress
 from functools import lru_cache
 
@@ -40,10 +39,34 @@ class Lp:
 
     _real_instance = None
 
-    def __init__(self, application_name, service=Launchpad, staging=False):
-        """Create a Launchpad service object."""
+    def __init__(self, application_name, service=Launchpad, staging=False, credentials=None):
+        """Create a Launchpad service object.
+
+        Authentication with Launchpad is done lazily, not at object
+        initialization but at the point it first needs to actually use
+        Launchpad functionality.  This permits adjustment of the
+        object's credentials or other properties as needed.
+
+        If the `$LP_CREDENTIAL` environment variable is defined, its
+        contents will be loaded as the credentials to pass to the
+        Credentials.from_string() function.  Stored credentials must
+        be formatted according to the requirements of launchpadlib's
+        Credentials class.  For more information on this class see:
+            https://git.launchpad.net/launchpadlib/tree/src/launchpadlib/credentials.py
+
+        :param str application_name: The text name of the software using
+            this class.
+        :param Launchpad service: The launchpadlib service class or
+            object to wrapper.
+        :param bool staging: When true, operate against a test instance
+            of Launchpad instead of the real one.
+        :param str credentials: (Optional) Formatted OAuth information
+            to use when authenticating with Launchpad.  If not provided,
+            will automatically login to Launchpad as needed.
+        """
         self._app_name = application_name
         self._service = service
+        self._credentials = credentials
         if staging:
             self._service_root = 'qastaging'
             self.ROOT_URL = 'https://qastaging.launchpad.net/'
@@ -53,18 +76,20 @@ class Lp:
         else:
             self._service_root = 'production'
 
-    def _get_instance_from_env(self) -> 'Launchpad | None':
+    def _get_instance_from_creds(self) -> 'Launchpad | None':
         """
-        Get an instance of _service using `$LP_CREDENTIAL` environment
-        variable if it is defined, else return None
+        Get an instance of _service using stored credentials if defined,
+        else return None.
+
+        For more information on Launchpad credentials-based authentication see
+        https://help.launchpad.net/API/launchpadlib#Authenticated_access_for_website_integration
 
         :rtype: Launchpad | None
-        :returns: Logged in Launchpad instance if LP_CREDENTIALS is in env
+        :returns: Logged in Launchpad instance if credentials available,
             else None
         """
-        lp_cred = os.getenv("LP_CREDENTIALS")
-        if lp_cred:
-            cred = Credentials.from_string(lp_cred)
+        if self._credentials:
+            cred = Credentials.from_string(self._credentials)
             return self._service(
                 cred, None, None,
                 service_root=self._service_root,
@@ -77,11 +102,6 @@ class Lp:
         Prompts the user to authorize the login of a new credential
         or use the cached one if it is available and valid
 
-        Note: This uses Launchpad.login_with if service is left to
-            default, if you have a credential file you can provide
-            the path to it via the LP_CREDENTIALS_FILE environment
-            variable and this call will pass without prompting the
-            user
         :rtype: launchpadlib.launchpad.Launchpad
         :returns: Logged in Launchpad instance
         """
@@ -97,7 +117,8 @@ class Lp:
         """Cache LP object."""
         if not self._real_instance:
             self._real_instance = (
-                self._get_instance_from_env() or self._get_instance_from_login()
+                self._get_instance_from_creds() or
+                self._get_instance_from_login()
             )
         return self._real_instance
 
diff --git a/scripts/ppa b/scripts/ppa
index 9dbb15a..12eab8f 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -71,6 +71,7 @@ from ppa.constants import (
     ARCHES_PPA_ALL,
     ARCHES_PPA_DEFAULT,
     ARCHES_AUTOPKGTEST,
+    CREDENTIALS_FILENAME_DEFAULT,
     LOCAL_REPOSITORY_PATH,
     LOCAL_REPOSITORY_MIRRORING_DIRECTIONS,
 )
@@ -117,9 +118,14 @@ def add_global_options(parser: argparse.ArgumentParser) -> None:
 
     :param argparse.ArgumentParser parser: A parser or subparser.
     """
+    parser.add_argument('-A', '--credentials',
+                        dest='credentials_filename', action='store',
+                        metavar='FILE',
+                        help="Location of oauth credentials file")
     parser.add_argument('-C', '--config',
                         dest='config_filename', action='store',
                         default="~/.config/ppa-dev-tools/config.yml",
+                        metavar="FILE",
                         help="Location of config file")
     parser.add_argument('-D', '--debug',
                         dest='debug', action='store_true',
@@ -263,6 +269,24 @@ def create_arg_parser() -> argparse.ArgumentParser:
                                help="Person or team to create PPA under, if not specified via the ppa address (defaults to current LP user)")
     add_basic_config_options(create_parser)
 
+    # Credentials Command
+    credentials_parser = subparser.add_parser(
+        'credentials',
+        argument_default=argparse.SUPPRESS,
+        help=(
+            "Store the Launchpad credentials to a file for manual use.  "
+            f"(Default: '{CREDENTIALS_FILENAME_DEFAULT}')"
+        ),
+        prog=progname,
+    )
+    add_global_options(credentials_parser)
+    credentials_parser.add_argument(
+        'ppa_name',
+        action='store',
+        nargs='?',
+        default='me',
+        help="Name of the PPA (optional)")
+
     # Desc Command
     desc_parser = subparser.add_parser(
         'desc',
@@ -399,6 +423,27 @@ def create_arg_parser() -> argparse.ArgumentParser:
     return parser
 
 
+def create_lp(app_name: str, args: argparse.Namespace) -> Lp:
+    """Instantiate the Lp Launchpad Interface object.
+
+    Use credentials from the file file specified by --credentials , if
+    provided.  If not, next try to use contents from the LP_CREDENTIALS
+    environment variable.  Last, leave creds undefined, and Lp will
+    handle website login and credentials caching automatically.
+
+    :param argparse.Namespace args: Command line arguments.
+    :rtype: Lp
+    :returns: Interface object for accessing the launchpad service.
+    """
+    if args.credentials_filename:
+        with open(args.credentials_filename, 'r') as f:
+            creds = f.read()
+    else:
+        creds = os.getenv("LP_CREDENTIALS")
+
+    return Lp(app_name, credentials=creds)
+
+
 DEFAULT_CONFIG = {
     'debug': False,
     'ppa_name': None,
@@ -554,6 +599,28 @@ def command_create(lp: Lp, config: dict[str, str]) -> int:
     return 1
 
 
+def command_credentials(lp: Lp, config: dict[str, str]) -> int:
+    """Saves login credentials to a file.
+
+    :param Lp lp: The Launchpad wrapper object.
+    :param dict[str, str] config: Configuration param:value map.
+    :rtype: int
+    :returns: Status code OK (0) on success, non-zero on error.
+    """
+    try:
+        credentials_filename = config.get(
+            'credentials_filename',
+            CREDENTIALS_FILENAME_DEFAULT
+        )
+        lp.credentials.save_to_path(credentials_filename)
+        print(f"Launchpad credentials written to {credentials_filename}")
+        return os.EX_OK
+    except KeyboardInterrupt:
+        return 2
+    print("Unhandled error")
+    return 1
+
+
 def command_desc(lp: Lp, config: dict[str, str]) -> int:
     """Sets the description for a PPA.
 
@@ -962,14 +1029,15 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
 
 
 COMMANDS = {
-    'create':     (command_create, None),
-    'desc':       (command_desc, None),
-    'destroy':    (command_destroy, None),
-    'list':       (command_list, None),
-    'set':        (command_set, None),
-    'show':       (command_show, None),
-    'tests':      (command_tests, None),
-    'wait':       (command_wait, None),
+    'create':        (command_create, None),
+    'credentials':   (command_credentials, None),
+    'desc':          (command_desc, None),
+    'destroy':       (command_destroy, None),
+    'list':          (command_list, None),
+    'set':           (command_set, None),
+    'show':          (command_show, None),
+    'tests':         (command_tests, None),
+    'wait':          (command_wait, None),
     }
 
 
@@ -984,9 +1052,8 @@ def main(args: argparse.Namespace) -> int:
         error("No command given.")
         return os.EX_USAGE
 
-    lp = Lp('ppa-dev-tools')
-
     try:
+        lp = create_lp('ppa-dev-tools', args)
         config = create_config(lp, args)
     except KeyboardInterrupt:
         return 2
diff --git a/tests/helpers.py b/tests/helpers.py
index 3510b35..63b5756 100644
--- a/tests/helpers.py
+++ b/tests/helpers.py
@@ -12,6 +12,11 @@
 
 import os
 import sys
+from functools import lru_cache
+
+from mock import Mock
+
+from launchpadlib.credentials import Credentials
 
 sys.path.insert(0, os.path.realpath(
     os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
@@ -113,8 +118,14 @@ class LpServiceMock:
     BUGS_ROOT_URL = 'https://bugs.mocklaunchpad.net/'
     CODE_ROOT_URL = 'https://code.mocklaunchpad.net/'
 
-    def __init__(self):
+    def __init__(self, credentials=None):
         self.launchpad = LaunchpadMock()
+        self._credentials = credentials
+
+    @property
+    @lru_cache
+    def credentials(self):
+        return Mock(Credentials)
 
     @property
     def me(self):
diff --git a/tests/test_lp.py b/tests/test_lp.py
index b863107..d1f2696 100644
--- a/tests/test_lp.py
+++ b/tests/test_lp.py
@@ -76,15 +76,15 @@ def test_new_connection():
     )
 
 
[e-mail](os.environ, {"LP_CREDENTIALS": "..."})
 @patch("ppa.lp.Credentials")
-def test_new_connection_envvar(credentials_mock):
-    """Verifies the Lp object will login via envvar if provided."""
+def test_new_connection_creds(credentials_mock):
+    """Verifies the Lp object will login if credentials are provided."""
     mock_launchpad = Mock(spec=Launchpad)
 
     lp = Lp(
         application_name=APPNAME,
-        service=mock_launchpad)
+        service=mock_launchpad,
+        credentials='...')
 
     # Cause the lp._instance() internal property to be triggered
     logging.debug(lp.me)
diff --git a/tests/test_scripts_ppa.py b/tests/test_scripts_ppa.py
index 5eb55f5..0f190a3 100644
--- a/tests/test_scripts_ppa.py
+++ b/tests/test_scripts_ppa.py
@@ -117,6 +117,14 @@ def test_create_arg_parser():
     args = parser.parse_args(['status', 'test-ppa'])
     assert args.command == 'status'
 
+    # Check -A, --credentials
+    args = parser.parse_args(['-A', 'my-creds', 'status', 'test-ppa'])
+    assert args.credentials_filename == "my-creds"
+    args.credentials_filename = None
+    args = parser.parse_args(['--credentials', 'my-creds', 'status', 'test-ppa'])
+    assert args.credentials_filename == "my-creds"
+    args.credentials_filename = None
+
     # Check -D, --debug
     args = parser.parse_args(['-D', 'status', 'test-ppa'])
     assert args.debug is True
@@ -156,6 +164,7 @@ def test_create_arg_parser():
             subparsers.append(choice)
     assert subparsers == [
         'create',
+        'credentials',
         'desc',
         'destroy',
         'list',
@@ -485,6 +494,36 @@ def test_create_config_from_args_error(args, expected_exception):
         script.create_config(lp, args)
 
 
+def test_create_lp_via_file(tmp_path):
+    '''Checks loading of credentials provided by --credentials arg.'''
+    expected_credentials = "[1]\na: 1\nb: 2\n"
+    credentials_file = tmp_path / 'x'
+    credentials_file.write_text(expected_credentials)
+
+    lp = LpServiceMock()
+    parser = script.create_arg_parser()
+    args = parser.parse_args([
+        '--credentials', str(credentials_file),
+        'status', 'test-ppa',
+    ])
+    lp = script.create_lp('x', args)
+
+    assert lp._credentials == expected_credentials
+
+
[e-mail](os.environ, {"LP_CREDENTIALS": "[1]\na: 1\nb: 2\n"})
+def test_create_lp_via_envvar(tmp_path):
+    '''Checks loading of credentials provided by $LP_CREDENTIALS.'''
+    lp = LpServiceMock()
+    parser = script.create_arg_parser()
+    args = parser.parse_args([
+        'status', 'test-ppa',
+    ])
+    lp = script.create_lp('x', args)
+
+    assert lp._credentials == os.environ['LP_CREDENTIALS']
+
+
 @pytest.mark.parametrize('stdin, params, expected_ppa_config', [
     # Defaults
     (None, {}, {'description': ''}),
@@ -564,6 +603,20 @@ def test_command_create_with_architectures(monkeypatch, fake_config, architectur
     assert [proc.name for proc in lp_ppa.processors] == expected_processors
 
 
[e-mail]('params, expected_filename', [
+    ({}, 'credentials.oauth'),
+    ({'credentials_filename': 'a'}, 'a'),
+    ({'credentials_filename': '~/.x/creds.oauth'}, '~/.x/creds.oauth'),
+])
+def test_command_credentials(fake_config, params, expected_filename):
+    """Check that credentials command saves LP creds to file."""
+    lp = LpServiceMock()
+    config = {**fake_config, **params}
+
+    assert script.command_credentials(lp, config) == 0
+    lp.credentials.save_to_path.assert_called_once()
+
+
 @pytest.mark.xfail(reason="Unimplemented")
 def test_command_desc(fake_config):
     lp = LpServiceMock()