Repository: planetlabs/planet-client-python

Issue:
Clarify auth source with CLI only sourcing from config file and additional log messages
**Is your feature request related to a problem? Please describe.**
Users and maintainers troubleshooting with the users alike have been stuck wondering why their Planet SDK authentication was not recognized when they had just performed the recommended `planet auth init` ([example](https://github.com/planetlabs/planet-client-python/issues/643)). Eventually they determined that the user had the PL_API_KEY environment variable set, which was not updated when `planet auth init` was run (aka, configuration file), but takes priority over the value stored with `planet auth init`.

Use #699 as inspiration for how to implement CLI-specific auth.

**Describe the solution you'd like**

Change the CLI authentication source to differ from the API source in the following way: the CLI source is only the configuration file, environment variables are ignored. Further, add `planet auth set` to allow an alternative to `planet auth login` for storing auth information. `planet auth value` would always report the CLI auth value, which is from the configuration file.

To support the user in understanding and maintaining auth sources, do the following:
* When CLI login (or set) is run, determine if the environment variable exists and, if so, raise a warning with a suggestion for how to update the environment variable from the CLI configuration (i.e. using `planet auth value`)
* Clarify the Python library auth source (which could be environment variable or configuration file) by adding an INFO log specifying the source.

This pattern is inspired by, and closely follows, the gcloud authorization pattern. That is, the CLI exclusively uses the authentication information managed by the CLI. The Python API first looks for an environment variable then falls back to authentication information managed by the CLI.

**Describe alternatives you've considered**

The primary alternative to this solution is to have the CLI obtain authentication information from an environment variable first, then the configuration file, and to have the CLI report the  authentication that would actually be used (aka, if t[here](https://github.com/planetlabs/planet-client-python/pull/699) is an environment variable, report that, if not, report what is in the configuration file). This solution is documented here. This alternative solution has the benefit of already having a draft PR. It has the additional benefit of being similar to the AWS authentication scheme. It has the drawback that it is a different authentication scheme than gcloud and maxar (though this is inevitable with aws and google/maxar having different schemes). It has the additional drawback that, although it is close to AWS, it differs from AWS in ways that impact usage. 

While AWS has the CLI use authentication information from the environment variable first, then the configuration, it only reports what is in the configuration file. The AWS command is `aws configure get` which makes it pretty clear it is configuration specific, while our CLI command is `planet auth value` which is more auth-specific. The alternative solution also has the CLI use authentication information from the environment variable first, but differs in that it reports the primary authentication information, which will be the environment variable if it is set. This approach makes it difficult to update the environment variable if it exists (a likely scenario when using a docker environment e.g. for notebooks). The currently-recommended approach is: Set configuration with `planet auth init`, set environment variable with `PL_API_KEY=(planet auth value)` then share the environment variable with docker. When `planet auth value` reports the environment variable, that workflow simply sets the environment variable to the value of the environment variable. One would need to delete the environment variable to allow for updating the value. Alternatively, we could add a CLI command for reporting what is in the configuration similar to `aws configure get`.

Compared to the alternative solution, the solution proposed here has the benefit that it would remove potentially confusing functionality (multiple sources of auth for the CLI), it would allow for ease and clarity in setting the environment variable using the CLI authentication functionality, and would be aligned with the authentication scheme of gcloud and maxar, making it easier for new users to orient. It has the drawback that, when the environment variable is set, the CLI and the SDK will potentially have two different auth values.

**Additional context**

The three major SDKs in the Planet SDK sphere handle authentication in the following ways:
* Gcloud [CLI](https://cloud.google.com/sdk/docs/authorizing) does not use environment variables, only configuration file. [SDK](https://cloud.google.com/docs/authentication/application-default-credentials) uses environment variables before looking at the (different from CLI) configuration file.
* Maxar [CLI](https://ard.maxar.com/docs/sdk/cli/api/) does not use environment variables, only configuration file. [SDK](https://ard.maxar.com/docs/sdk/sdk/authentication/) uses environment variables before looking at the configuration file.
* AWS [CLI](https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html) uses environment variables before looking at the configuration file and boto3 ([SDK](https://boto3.amazonaws.com/v1/documentation/api/latest/guide/configuration.html)) does the same. When users run `[aws configure get](https://docs.aws.amazon.com/cli/latest/reference/configure/get.html)`, they always get the value in the configuration file, which does not consider other configurations e.g. environment variable.



Discussion:

Patch:
diff --git a/docs/get-started/quick-start-guide.md b/docs/get-started/quick-start-guide.md
index 28e64a5..368ee40 100644
--- a/docs/get-started/quick-start-guide.md
+++ b/docs/get-started/quick-start-guide.md
@@ -83,8 +83,8 @@ And you can see that the value was stored successfully as an environment variabl
 echo $PL_API_KEY
 ```
 
-!!!note "The API Key environment variable is always used first in the Planet SDK"
-    If you do create a `PL_API_KEY` environment variable, the SDK will use this value. `PL_API_KEY` overrides the value that was retrieved using your Planet login with a call to `planet auth init`. The `planet auth value` call currently does not reflect that `PL_API_KEY` overrides the `auth init` value (this should be fixed in 2.0-beta.1 with [issue 643](https://github.com/planetlabs/planet-client-python/issues/643))
+!!!note "The API Key environment variable is ignored by the CLI but used by the Python library"
+    If you do create a `PL_API_KEY` environment variable, the CLI will be unaffected but the Planet library will use this as the source for authorization instead of the value stored in `planet auth init`.
 
 ## Step 5: Search for Planet Imagery
 
diff --git a/docs/python/sdk-guide.md b/docs/python/sdk-guide.md
index fb1e7dc..40e71f4 100644
--- a/docs/python/sdk-guide.md
+++ b/docs/python/sdk-guide.md
@@ -46,15 +46,18 @@ asyncio.run(main())
 
 ## Authenticate with Planet services
 
-There are two steps to managing authentication information, obtaining the
-authentication information from Planet and then managing it for local retrieval
-for authentication purposes.
+A `Session` requires authentication to communicate with Planet services. This
+authentication information is retrieved when a `Session` is created. By default,
+a `Session` obtains authorization from the following sources with order
+indicating priority:
 
-The recommended method for obtaining authentication information is through
-logging in, using `Auth.from_login()` (note: using something like the `getpass`
-module is recommended to ensure your password remains secure). Alternatively,
-the api key can be obtained directly from the Planet account site and then used
-in `Auth.from_key()`.
+1. The environment variable `PL_API_KEY`
+2. The secret file
+
+The SDK provides the `auth.Auth` class for managing authentication information.
+This module can be used to obtain authentication information from username
+and password with `Auth.from_login()`. Additionally, it can be created with
+the API key obtained directly from the Planet account site with `Auth.from_key(<API_KEY>)`.
 
 Once the authentication information is obtained, the most convenient way of
 managing it for local use is to write it to a secret file using `Auth.write()`.
@@ -69,16 +72,15 @@ from planet import Auth
 
 pw = getpass.getpass()
 auth = Auth.from_login('user', 'pw')
-auth.write()
+auth.store()
 
 ```
 
-When a `Session` is created, by default, authentication is read from the secret
-file created with `Auth.write()`. This behavior can be modified by specifying
+The default authentication behavior of the `Session` can be modified by specifying
 `Auth` explicitly using the methods `Auth.from_file()` and `Auth.from_env()`.
 While `Auth.from_key()` and `Auth.from_login` can be used, it is recommended
 that those functions be used in authentication initialization and the
-authentication information be stored using `Auth.write()`.
+authentication information be stored using `Auth.store()`.
 
 The file and environment variable read from can be customized in the
 respective functions. For example, authentication can be read from a custom
diff --git a/planet/auth.py b/planet/auth.py
index 5a64ef3..e50f1af 100644
--- a/planet/auth.py
+++ b/planet/auth.py
@@ -25,14 +25,13 @@ import httpx
 import jwt
 
 from . import http
-from .constants import PLANET_BASE_URL, SECRET_FILE_PATH
+from .constants import ENV_API_KEY, PLANET_BASE_URL, SECRET_FILE_PATH
 from .exceptions import AuthException
 from typing import Optional
 
 LOGGER = logging.getLogger(__name__)
 
 BASE_URL = f'{PLANET_BASE_URL}/v0/auth'
-ENV_API_KEY = 'PL_API_KEY'
 
 AuthType = httpx.Auth
 
@@ -135,9 +134,9 @@ class Auth(metaclass=abc.ABCMeta):
     def to_dict(self) -> dict:
         pass
 
-    def write(self,
+    def store(self,
               filename: Optional[typing.Union[str, pathlib.Path]] = None):
-        '''Write authentication information.
+        '''Store authentication information in secret file.
 
         Parameters:
             filename: Alternate path for the planet secret file.
diff --git a/planet/cli/auth.py b/planet/cli/auth.py
index 49e0bc9..22d0ebc 100644
--- a/planet/cli/auth.py
+++ b/planet/cli/auth.py
@@ -13,10 +13,12 @@
 # limitations under the License.
 """Auth API CLI"""
 import logging
+import os
 
 import click
 
 import planet
+from planet.constants import ENV_API_KEY
 from .cmds import translate_exceptions
 
 LOGGER = logging.getLogger(__name__)
@@ -48,8 +50,12 @@ def init(ctx, email, password):
     '''Obtain and store authentication information'''
     base_url = ctx.obj['BASE_URL']
     plauth = planet.Auth.from_login(email, password, base_url=base_url)
-    plauth.write()
+    plauth.store()
     click.echo('Initialized')
+    if os.getenv(ENV_API_KEY):
+        click.echo(f'Warning - Environment variable {ENV_API_KEY} already '
+                   'exists. To update, with the new value, use the following:')
+        click.echo(f'export {ENV_API_KEY}=$(planet auth value)')
 
 
 @auth.command()
@@ -57,3 +63,19 @@ def init(ctx, email, password):
 def value():
     '''Print the stored authentication information'''
     click.echo(planet.Auth.from_file().value)
+
+
+@auth.command()
+@translate_exceptions
+@click.argument('key')
+def store(key):
+    '''Store authentication information'''
+    plauth = planet.Auth.from_key(key)
+    if click.confirm('This overrides the stored value. Continue?'):
+        plauth.store()
+        click.echo('Updated')
+        if os.getenv(ENV_API_KEY):
+            click.echo(f'Warning - Environment variable {ENV_API_KEY} already '
+                       'exists. To update, with the new value, use the '
+                       'following:')
+            click.echo(f'export {ENV_API_KEY}=$(planet auth value)')
diff --git a/planet/cli/data.py b/planet/cli/data.py
index 61688d6..14fccf3 100644
--- a/planet/cli/data.py
+++ b/planet/cli/data.py
@@ -37,10 +37,8 @@ valid_item_string = "Valid entries for ITEM_TYPES: " + "|".join(ALL_ITEM_TYPES)
 
 @asynccontextmanager
 async def data_client(ctx):
-    auth = ctx.obj['AUTH']
-    base_url = ctx.obj['BASE_URL']
-    async with CliSession(auth=auth) as sess:
-        cl = DataClient(sess, base_url=base_url)
+    async with CliSession() as sess:
+        cl = DataClient(sess, base_url=ctx.obj['BASE_URL'])
         yield cl
 
 
@@ -52,7 +50,6 @@ async def data_client(ctx):
               help='Assign custom base Orders API URL.')
 def data(ctx, base_url):
     '''Commands for interacting with the Data API'''
-    ctx.obj['AUTH'] = None
     ctx.obj['BASE_URL'] = base_url
 
 
diff --git a/planet/cli/orders.py b/planet/cli/orders.py
index 998168e..43e7ed5 100644
--- a/planet/cli/orders.py
+++ b/planet/cli/orders.py
@@ -31,9 +31,8 @@ LOGGER = logging.getLogger(__name__)
 
 @asynccontextmanager
 async def orders_client(ctx):
-    auth = ctx.obj['AUTH']
     base_url = ctx.obj['BASE_URL']
-    async with CliSession(auth=auth) as sess:
+    async with CliSession() as sess:
         cl = OrdersClient(sess, base_url=base_url)
         yield cl
 
@@ -46,7 +45,6 @@ async def orders_client(ctx):
               help='Assign custom base Orders API URL.')
 def orders(ctx, base_url):
     '''Commands for interacting with the Orders API'''
-    ctx.obj['AUTH'] = None
     ctx.obj['BASE_URL'] = base_url
 
 
diff --git a/planet/cli/session.py b/planet/cli/session.py
index fa3537f..a3b28b4 100644
--- a/planet/cli/session.py
+++ b/planet/cli/session.py
@@ -1,12 +1,12 @@
 """CLI HTTP/auth sessions."""
 
-from planet.auth import AuthType
+from planet.auth import Auth
 from planet.http import Session
-from typing import Optional
 
 
 class CliSession(Session):
+    """Session with CLI-specific auth and identifying header"""
 
-    def __init__(self, auth: Optional[AuthType] = None):
-        super().__init__(auth)
+    def __init__(self):
+        super().__init__(Auth.from_file())
         self._client.headers.update({'X-Planet-App': 'python-cli'})
diff --git a/planet/cli/subscriptions.py b/planet/cli/subscriptions.py
index 41d2283..84e339f 100644
--- a/planet/cli/subscriptions.py
+++ b/planet/cli/subscriptions.py
@@ -14,9 +14,7 @@ from planet.clients.subscriptions import SubscriptionsClient
 @click.pass_context
 def subscriptions(ctx):
     '''Commands for interacting with the Subscriptions API'''
-    # None means that order of precedence is 1) environment variable,
-    # 2) secret file.
-    ctx.obj['AUTH'] = None
+    pass
 
 
 # We want our command to be known as "list" on the command line but
@@ -40,7 +38,7 @@ def subscriptions(ctx):
 @coro
 async def list_subscriptions_cmd(ctx, status, limit, pretty):
     """Prints a sequence of JSON-encoded Subscription descriptions."""
-    async with CliSession(auth=ctx.obj['AUTH']) as session:
+    async with CliSession() as session:
         client = SubscriptionsClient(session)
         subs_aiter = client.list_subscriptions_aiter(status=status,
                                                      limit=limit)
@@ -77,7 +75,7 @@ def parse_request(ctx, param, value: str) -> dict:
 @coro
 async def create_subscription_cmd(ctx, request, pretty):
     """Submits a subscription request and prints the API response."""
-    async with CliSession(auth=ctx.obj['AUTH']) as session:
+    async with CliSession() as session:
         client = SubscriptionsClient(session)
         sub = await client.create_subscription(request)
         echo_json(sub, pretty)
@@ -91,7 +89,7 @@ async def create_subscription_cmd(ctx, request, pretty):
 @coro
 async def cancel_subscription_cmd(ctx, subscription_id, pretty):
     """Cancels a subscription and prints the API response."""
-    async with CliSession(auth=ctx.obj['AUTH']) as session:
+    async with CliSession() as session:
         client = SubscriptionsClient(session)
         sub = await client.cancel_subscription(subscription_id)
         echo_json(sub, pretty)
@@ -106,7 +104,7 @@ async def cancel_subscription_cmd(ctx, subscription_id, pretty):
 @coro
 async def update_subscription_cmd(ctx, subscription_id, request, pretty):
     """Updates a subscription and prints the API response."""
-    async with CliSession(auth=ctx.obj['AUTH']) as session:
+    async with CliSession() as session:
         client = SubscriptionsClient(session)
         sub = await client.update_subscription(subscription_id, request)
         echo_json(sub, pretty)
@@ -120,7 +118,7 @@ async def update_subscription_cmd(ctx, subscription_id, request, pretty):
 @coro
 async def describe_subscription_cmd(ctx, subscription_id, pretty):
     """Gets the description of a subscription and prints the API response."""
-    async with CliSession(auth=ctx.obj['AUTH']) as session:
+    async with CliSession() as session:
         client = SubscriptionsClient(session)
         sub = await client.get_subscription(subscription_id)
         echo_json(sub, pretty)
@@ -156,7 +154,7 @@ async def list_subscription_results_cmd(ctx,
                                         status,
                                         limit):
     """Gets results of a subscription and prints the API response."""
-    async with CliSession(auth=ctx.obj['AUTH']) as session:
+    async with CliSession() as session:
         client = SubscriptionsClient(session)
         results_aiter = client.get_results_aiter(subscription_id,
                                                  status=status,
diff --git a/planet/constants.py b/planet/constants.py
index 8cafd02..3eedc80 100644
--- a/planet/constants.py
+++ b/planet/constants.py
@@ -20,6 +20,8 @@ from pathlib import Path
 
 DATA_DIR = Path(os.path.dirname(__file__)) / 'data'
 
+ENV_API_KEY = 'PL_API_KEY'
+
 PLANET_BASE_URL = 'https://api.planet.com'
 
 SECRET_FILE_PATH = Path(os.path.expanduser('~')) / '.planet.json'


Test patch:
diff --git a/tests/integration/test_auth_cli.py b/tests/integration/test_auth_cli.py
index 357128b..e9490c2 100644
--- a/tests/integration/test_auth_cli.py
+++ b/tests/integration/test_auth_cli.py
@@ -13,6 +13,7 @@
 # the License.
 from http import HTTPStatus
 import json
+import os
 
 from click.testing import CliRunner
 import httpx
@@ -104,3 +105,19 @@ def test_cli_auth_value_failure(redirect_secretfile):
     assert result.exception
     assert 'Error: Auth information does not exist or is corrupted.' \
         in result.output
+
+
+def test_cli_auth_store_cancel(redirect_secretfile):
+    result = CliRunner().invoke(cli.main, ['auth', 'store', 'setval'],
+                                input='')
+    assert not result.exception
+    assert not os.path.isfile(redirect_secretfile)
+
+
+def test_cli_auth_store_confirm(redirect_secretfile):
+    result = CliRunner().invoke(cli.main, ['auth', 'store', 'setval'],
+                                input='y')
+    assert not result.exception
+
+    with open(redirect_secretfile, 'r') as f:
+        assert json.load(f) == {'key': 'setval'}
diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py
index 13851de..511b516 100644
--- a/tests/unit/test_auth.py
+++ b/tests/unit/test_auth.py
@@ -118,23 +118,23 @@ def test_Auth_from_login(monkeypatch):
     assert test_auth.value == auth_data
 
 
-def test_Auth_write_doesnotexist(tmp_path):
+def test_Auth_store_doesnotexist(tmp_path):
     test_auth = auth.Auth.from_key('test')
     secret_path = str(tmp_path / '.test')
-    test_auth.write(secret_path)
+    test_auth.store(secret_path)
 
     with open(secret_path, 'r') as fp:
         assert json.loads(fp.read()) == {"key": "test"}
 
 
-def test_Auth_write_exists(tmp_path):
+def test_Auth_store_exists(tmp_path):
     secret_path = str(tmp_path / '.test')
 
     with open(secret_path, 'w') as fp:
         fp.write('{"existing": "exists"}')
 
     test_auth = auth.Auth.from_key('test')
-    test_auth.write(secret_path)
+    test_auth.store(secret_path)
 
     with open(secret_path, 'r') as fp:
         assert json.loads(fp.read()) == {"key": "test", "existing": "exists"}
diff --git a/tests/unit/test_cli_session.py b/tests/unit/test_cli_session.py
new file mode 100644
index 0000000..4ea75b2
--- /dev/null
+++ b/tests/unit/test_cli_session.py
@@ -0,0 +1,100 @@
+# Copyright 2022 Planet Labs PBC.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+import base64
+from http import HTTPStatus
+import json
+
+import httpx
+import respx
+
+import pytest
+
+# from planet.auth import _SecretFile
+from planet import auth
+from planet.cli import session
+from planet.exceptions import AuthException
+
+TEST_URL = 'mock://mock.com'
+
+
+# skip the global mock of _SecretFile.read
+# for this module
+@pytest.fixture(autouse=True, scope='module')
+def test_secretfile_read():
+    return
+
+
+@pytest.fixture()
+def test_valid_secretfile(tmp_path, monkeypatch):
+    # write our own secret file
+    secret_path = f'{tmp_path}/secret.test'
+    monkeypatch.setattr(auth, 'SECRET_FILE_PATH', secret_path)
+    with open(secret_path, 'w') as fp:
+        json.dump({'key': 'clisessiontest'}, fp)
+
+
+@respx.mock
+@pytest.mark.asyncio
+async def test_CliSession_headers(test_valid_secretfile):
+    async with session.CliSession() as sess:
+        route = respx.get(TEST_URL)
+        route.return_value = httpx.Response(HTTPStatus.OK)
+
+        await sess.request(method='GET', url=TEST_URL)
+
+        # the proper headers are included and they have the expected values
+        received_request = route.calls.last.request
+        assert received_request.headers['x-planet-app'] == 'python-cli'
+        assert 'planet-client-python/' in received_request.headers[
+            'user-agent']
+
+
+@respx.mock
+@pytest.mark.asyncio
+async def test_CliSession_auth_valid(test_valid_secretfile):
+    async with session.CliSession() as sess:
+        route = respx.get(TEST_URL)
+        route.return_value = httpx.Response(HTTPStatus.OK)
+
+        await sess.request(method='GET', url=TEST_URL)
+
+        # the proper headers are included and they have the expected values
+        received_request = route.calls.last.request
+        credentials = received_request.headers['authorization'].strip(
+            'Authorization: Basic ')
+        assert base64.b64decode(credentials) == b'clisessiontest:'
+
+
+@respx.mock
+@pytest.mark.asyncio
+async def test_CliSession_auth_invalid(tmp_path, monkeypatch):
+    # write invalid secret file
+    secret_path = f'{tmp_path}/secret.test'
+    monkeypatch.setattr(auth, 'SECRET_FILE_PATH', secret_path)
+    with open(secret_path, 'w') as fp:
+        json.dump({'invalidkey': 'clisessiontest'}, fp)
+
+    with pytest.raises(AuthException):
+        session.CliSession()
+
+
+@respx.mock
+@pytest.mark.asyncio
+async def test_CliSession_auth_nofile(tmp_path, monkeypatch):
+    # point to non-existant file
+    secret_path = f'{tmp_path}/doesnotexist.test'
+    monkeypatch.setattr(auth, 'SECRET_FILE_PATH', secret_path)
+
+    with pytest.raises(AuthException):
+        session.CliSession()