Repository: SFDO-Tooling/CumulusCI (python)

Issue:
Setting empty service environment variable throws unhelpful error
### Describe the bug

A specific error message that the environment variable is empty would be helpful in understanding this one.  This can impact anyone now that you don't have to set CUMULUSCI_KEYCHAIN_CLASS to have the keychain read env vars.

### Reproduction steps

```
$ export CUMULUSCI_SERVICE_github=
$ cci shell
Error: Expecting value: line 1 column 1 (char 0)
Run this command for more information about debugging errors: cci error --help
$ unset CUMULUSCI_SERVICE_github
$ cci shell
Python 3.10.2 (main, Apr 24 2022, 04:00:28) [Clang 10.0.1 (clang-1001.0.46.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>>
```

### Your CumulusCI and Python versions

CumulusCI version: 3.60.0
Python version: 3.10.2

### Operating System

OS X

### Windows environment

_No response_

### CumulusCI installation method

pipx

### Error Gist

_No response_

### Additional information

Traceback (most recent call last):
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/cli/cci.py", line 91, in main
    cli(args[1:], standalone_mode=False, obj=runtime)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/cli/runtime.py", line 182, in new_func
    runtime._load_keychain()
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/core/runtime.py", line 95, in _load_keychain
    self.keychain = self.keychain_cls(self.project_config, keychain_key)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/core/keychain/base_project_keychain.py", line 44, in __init__
    self._load_keychain()
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/core/keychain/base_project_keychain.py", line 50, in _load_keychain
    self._load_services()
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/core/keychain/encrypted_file_project_keychain.py", line 545, in _load_services
    self._load_services_from_environment()
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/core/keychain/encrypted_file_project_keychain.py", line 632, in _load_services_from_environment
    self._load_service_from_environment(env_var_name, value)
  File "/Users/jlantz/.local/pipx/venvs/cumulusci/lib/python3.10/site-packages/cumulusci/core/keychain/encrypted_file_project_keychain.py", line 637, in _load_service_from_environment
    service_config = ServiceConfig(json.loads(value))
  File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

Pull request:
Add better error handling for empty or invalid org and service env vars
Moved from #3341

Thank you @jlantz !

Adds extra error handling and a new exception, ServiceCannotBeLoaded, to match the exception already there for org load issues.  Should have 100% test coverage.

# Issues Closed
* Fixes #3340 

Patch:
diff --git a/cumulusci/core/exceptions.py b/cumulusci/core/exceptions.py
index 4d8d66f61..1e1dc2c46 100644
--- a/cumulusci/core/exceptions.py
+++ b/cumulusci/core/exceptions.py
@@ -62,6 +62,12 @@ class ServiceNotConfigured(CumulusCIUsageError):
     pass
 
 
+class ServiceCannotBeLoaded(CumulusCIUsageError):
+    """Raised when a service cannot be decrypted or unpickled"""
+
+    pass
+
+
 class ServiceNotValid(CumulusCIUsageError):
     """Raised when no service configuration could be found by a given name in the project configuration"""
 
diff --git a/cumulusci/core/keychain/encrypted_file_project_keychain.py b/cumulusci/core/keychain/encrypted_file_project_keychain.py
index 58167b49e..1d4e3d3a6 100644
--- a/cumulusci/core/keychain/encrypted_file_project_keychain.py
+++ b/cumulusci/core/keychain/encrypted_file_project_keychain.py
@@ -15,6 +15,7 @@ from cumulusci.core.exceptions import (
     KeychainKeyNotFound,
     OrgCannotBeLoaded,
     OrgNotFound,
+    ServiceCannotBeLoaded,
     ServiceNotConfigured,
 )
 from cumulusci.core.keychain import BaseProjectKeychain
@@ -229,7 +230,16 @@ class EncryptedFileProjectKeychain(BaseProjectKeychain):
                 self._load_org_from_environment(env_var_name, value)
 
     def _load_org_from_environment(self, env_var_name, value):
-        org_config = json.loads(value)
+        if not value:
+            raise OrgCannotBeLoaded(
+                f"Org env var {env_var_name} cannot be loaded because it is empty. Either set {env_var_name} to a json string or unset it from the environment."
+            )
+        try:
+            org_config = json.loads(value)
+        except Exception as e:
+            raise OrgCannotBeLoaded(
+                f"Could not parse {env_var_name} as JSON becase {e}"
+            )
         org_name = env_var_name[len(self.env_org_var_prefix) :].lower()
         if org_config.get("scratch"):
             org_config = scratch_org_factory(
@@ -655,6 +665,16 @@ class EncryptedFileProjectKeychain(BaseProjectKeychain):
     def _load_service_from_environment(self, env_var_name, value):
         """Given a valid name/value pair, load the
         service from the environment on to the keychain"""
+        if not value:
+            raise ServiceCannotBeLoaded(
+                f"Service env var {env_var_name} cannot be loaded because it is empty. Either set {env_var_name} to a json string or unset it from the environment."
+            )
+        try:
+            service_config = json.loads(value)
+        except Exception as e:
+            raise ServiceCannotBeLoaded(
+                f"Could not parse {env_var_name} as JSON because {e}"
+            )
         service_config = ServiceConfig(json.loads(value))
         service_type, service_name = self._get_env_service_type_and_name(env_var_name)
         self.set_service(service_type, service_name, service_config, save=False)


Test patch:
diff --git a/cumulusci/core/keychain/tests/test_encrypted_file_project_keychain.py b/cumulusci/core/keychain/tests/test_encrypted_file_project_keychain.py
index 4366077d1..9360f7162 100644
--- a/cumulusci/core/keychain/tests/test_encrypted_file_project_keychain.py
+++ b/cumulusci/core/keychain/tests/test_encrypted_file_project_keychain.py
@@ -26,7 +26,9 @@ from cumulusci.core.exceptions import (
     CumulusCIException,
     CumulusCIUsageError,
     KeychainKeyNotFound,
+    OrgCannotBeLoaded,
     OrgNotFound,
+    ServiceCannotBeLoaded,
     ServiceNotConfigured,
     ServiceNotValid,
 )
@@ -296,6 +298,28 @@ class TestEncryptedFileProjectKeychain:
         actual_config = keychain.get_org("devhub")
         assert _simplify_config(actual_config.config) == org_config.config
 
+    def test_load_orgs_from_environment__empty_throws_error(self, keychain, org_config):
+        env = EnvironmentVarGuard()
+        with EnvironmentVarGuard() as env:
+            env.set(
+                f"{keychain.env_org_var_prefix}dev",
+                "",
+            )
+            with pytest.raises(OrgCannotBeLoaded):
+                keychain._load_orgs_from_environment()
+
+    def test_load_orgs_from_environment__invalid_json_throws_error(
+        self, keychain, org_config
+    ):
+        env = EnvironmentVarGuard()
+        with EnvironmentVarGuard() as env:
+            env.set(
+                f"{keychain.env_org_var_prefix}dev",
+                "['foo',]",
+            )
+            with pytest.raises(OrgCannotBeLoaded):
+                keychain._load_orgs_from_environment()
+
     #######################################
     #              Services               #
     #######################################
@@ -358,6 +382,28 @@ class TestEncryptedFileProjectKeychain:
 
         assert 1 == 1
 
+    def test_load_services_from_env__empty_throws_error(self, keychain):
+        service_prefix = EncryptedFileProjectKeychain.env_service_var_prefix
+        env = EnvironmentVarGuard()
+        with EnvironmentVarGuard() as env:
+            env.set(
+                f"{service_prefix}github",
+                "",
+            )
+            with pytest.raises(ServiceCannotBeLoaded):
+                keychain._load_services_from_environment()
+
+    def test_load_services_from_env__invalid_json_throws_error(self, keychain):
+        service_prefix = EncryptedFileProjectKeychain.env_service_var_prefix
+        env = EnvironmentVarGuard()
+        with EnvironmentVarGuard() as env:
+            env.set(
+                f"{service_prefix}github",
+                "['foo',]",
+            )
+            with pytest.raises(ServiceCannotBeLoaded):
+                keychain._load_services_from_environment()
+
     def test_get_service__built_in_connected_app(self, keychain):
         built_in_connected_app = keychain.get_service("connected_app")
         assert built_in_connected_app is DEFAULT_CONNECTED_APP