Use consistent method of checking for presence of info in connection (#27694)

Fixes #27652

Ensure that mirror's to_dict function returns a syaml_dict object for all code
paths.

Switch to using the .get function for accessing the potential information from
the S3 mirror objects.  If the key is not there, it will gracefully return
None instead of failing with a KeyError

Additionally, check that the connection object is a dictionary before trying
to "get" from it.

Add a test for the capturing of the new S3 information.

3 files changed, 62 insertions, 29 deletions

diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index f6e5d38091..9b6fd5ba3c 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -90,7 +90,9 @@ class Mirror(object):
 
     def to_dict(self):
         if self._push_url is None:
-            return self._fetch_url
+            return syaml_dict([
+                ('fetch', self._fetch_url),
+                ('push', self._fetch_url)])
         else:
             return syaml_dict([
                 ('fetch', self._fetch_url),
@@ -105,12 +107,12 @@ class Mirror(object):
 
     def display(self, max_len=0):
         if self._push_url is None:
-            _display_mirror_entry(max_len, self._name, self._fetch_url)
+            _display_mirror_entry(max_len, self._name, self.fetch_url)
         else:
             _display_mirror_entry(
-                max_len, self._name, self._fetch_url, "fetch")
+                max_len, self._name, self.fetch_url, "fetch")
             _display_mirror_entry(
-                max_len, self._name, self._push_url, "push")
+                max_len, self._name, self.push_url, "push")
 
     def __str__(self):
         name = self._name
@@ -145,8 +147,8 @@ class Mirror(object):
     def get_profile(self, url_type):
         if isinstance(self._fetch_url, dict):
             if url_type == "push":
-                return self._push_url['profile']
-            return self._fetch_url['profile']
+                return self._push_url.get('profile', None)
+            return self._fetch_url.get('profile', None)
         else:
             return None
 
@@ -159,8 +161,8 @@ class Mirror(object):
     def get_access_pair(self, url_type):
         if isinstance(self._fetch_url, dict):
             if url_type == "push":
-                return self._push_url['access_pair']
-            return self._fetch_url['access_pair']
+                return self._push_url.get('access_pair', None)
+            return self._fetch_url.get('access_pair', None)
         else:
             return None
 
@@ -173,8 +175,8 @@ class Mirror(object):
     def get_endpoint_url(self, url_type):
         if isinstance(self._fetch_url, dict):
             if url_type == "push":
-                return self._push_url['endpoint_url']
-            return self._fetch_url['endpoint_url']
+                return self._push_url.get('endpoint_url', None)
+            return self._fetch_url.get('endpoint_url', None)
         else:
             return None
 
@@ -187,8 +189,8 @@ class Mirror(object):
     def get_access_token(self, url_type):
         if isinstance(self._fetch_url, dict):
             if url_type == "push":
-                return self._push_url['access_token']
-            return self._fetch_url['access_token']
+                return self._push_url.get('access_token', None)
+            return self._fetch_url.get('access_token', None)
         else:
             return None
 
diff --git a/lib/spack/spack/test/web.py b/lib/spack/spack/test/web.py
index 4ac0da162b..41aa1e6121 100644
--- a/lib/spack/spack/test/web.py
+++ b/lib/spack/spack/test/web.py
@@ -246,6 +246,29 @@ class MockS3Client(object):
         raise self.ClientError
 
 
+def test_gather_s3_information(monkeypatch, capfd):
+    mock_connection_data = {"access_token": "AAAAAAA",
+                            "profile": "SPacKDeV",
+                            "access_pair": ("SPA", "CK"),
+                            "endpoint_url": "https://127.0.0.1:8888"}
+
+    session_args, client_args = spack.util.s3.get_mirror_s3_connection_info(mock_connection_data)  # noqa: E501
+
+    # Session args are used to create the S3 Session object
+    assert "aws_session_token" in session_args
+    assert session_args.get("aws_session_token") == "AAAAAAA"
+    assert "aws_access_key_id" in session_args
+    assert session_args.get("aws_access_key_id") == "SPA"
+    assert "aws_secret_access_key" in session_args
+    assert session_args.get("aws_secret_access_key") == "CK"
+    assert "profile_name" in session_args
+    assert session_args.get("profile_name") == "SPacKDeV"
+
+    # In addition to the session object, use the client_args to create the s3
+    # Client object
+    assert "endpoint_url" in client_args
+
+
 def test_remove_s3_url(monkeypatch, capfd):
     fake_s3_url = 's3://my-bucket/subdirectory/mirror'
 
diff --git a/lib/spack/spack/util/s3.py b/lib/spack/spack/util/s3.py
index c120b4d5c5..82d51eb72e 100644
--- a/lib/spack/spack/util/s3.py
+++ b/lib/spack/spack/util/s3.py
@@ -27,6 +27,30 @@ def _parse_s3_endpoint_url(endpoint_url):
     return endpoint_url
 
 
+def get_mirror_s3_connection_info(connection):
+    s3_connection = {}
+
+    s3_connection_is_dict = connection and isinstance(connection, dict)
+    if s3_connection_is_dict:
+        if connection.get("access_token"):
+            s3_connection["aws_session_token"] = connection["access_token"]
+        if connection.get("access_pair"):
+            s3_connection["aws_access_key_id"] = connection["access_pair"][0]
+            s3_connection["aws_secret_access_key"] = connection["access_pair"][1]
+        if connection.get("profile"):
+            s3_connection["profile_name"] = connection["profile"]
+
+    s3_client_args = {"use_ssl": spack.config.get('config:verify_ssl')}
+
+    endpoint_url = os.environ.get('S3_ENDPOINT_URL')
+    if endpoint_url:
+        s3_client_args['endpoint_url'] = _parse_s3_endpoint_url(endpoint_url)
+    elif s3_connection_is_dict and connection.get("endpoint_url"):
+        s3_client_args["endpoint_url"] = _parse_s3_endpoint_url(connection["endpoint_url"])  # noqa: E501
+
+    return (s3_connection, s3_client_args)
+
+
 def create_s3_session(url, connection={}):
     url = url_util.parse(url)
     if url.scheme != 's3':
@@ -40,25 +64,9 @@ def create_s3_session(url, connection={}):
     from boto3 import Session
     from botocore.exceptions import ClientError
 
-    s3_connection = {}
-
-    if connection:
-        if connection['access_token']:
-            s3_connection["aws_session_token"] = connection["access_token"]
-        if connection["access_pair"][0]:
-            s3_connection["aws_access_key_id"] = connection["access_pair"][0]
-            s3_connection["aws_secret_access_key"] = connection["access_pair"][1]
-        if connection["profile"]:
-            s3_connection["profile_name"] = connection["profile"]
+    s3_connection, s3_client_args = get_mirror_s3_connection_info(connection)
 
     session = Session(**s3_connection)
-    s3_client_args = {"use_ssl": spack.config.get('config:verify_ssl')}
-
-    endpoint_url = os.environ.get('S3_ENDPOINT_URL')
-    if endpoint_url:
-        s3_client_args['endpoint_url'] = _parse_s3_endpoint_url(endpoint_url)
-    elif connection and 'endpoint_url' in connection:
-        s3_client_args["endpoint_url"] = _parse_s3_endpoint_url(connection["endpoint_url"])  # noqa: E501
     # if no access credentials provided above, then access anonymously
     if not session.get_credentials():
         from botocore import UNSIGNED