launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:oci-creds-store-unencrypted-fields into launchpad:master.

Commit message:
Avoiding to encrypt "region" on IOCIRegistryCredentials

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/394703
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:oci-creds-store-unencrypted-fields into launchpad:master.

diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index b3a43d7..c5385ed 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -363,8 +363,7 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
                         __name__=self._getFieldName('url', elem.id),
                         default=elem.registry_credentials.url,
                         required=True, readonly=True))
-                region = elem.registry_credentials.getCredentialsValue(
-                    'region')
+                region = elem.registry_credentials.region
                 region_fields.append(
                     TextLine(
                         __name__=self._getFieldName('region', elem.id),
diff --git a/lib/lp/oci/interfaces/ociregistrycredentials.py b/lib/lp/oci/interfaces/ociregistrycredentials.py
index f55f274..7d3085b 100644
--- a/lib/lp/oci/interfaces/ociregistrycredentials.py
+++ b/lib/lp/oci/interfaces/ociregistrycredentials.py
@@ -57,15 +57,18 @@ class IOCIRegistryCredentialsView(Interface):
     def getCredentials():
         """Get the saved credentials."""
 
-    def getCredentialsValue(key):
-        """Gets the credential value for a specific key."""
-
     username = TextLine(
         title=_("Username"),
         description=_("The username for the credentials, if available."),
         required=True,
         readonly=True)
 
+    region = TextLine(
+        title=_("Region"),
+        description=_("The registry region, if available."),
+        required=False,
+        readonly=True)
+
 
 class IOCIRegistryCredentialsEditableAttributes(IHasOwner):
 
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 4ae7f16..b7d41c6 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -645,7 +645,7 @@ class AWSAuthenticatorMixin:
     def _getRegion(self):
         """Returns the region from the push URL domain."""
         push_rule = self.push_rule
-        region = push_rule.registry_credentials.getCredentialsValue("region")
+        region = push_rule.registry_credentials.region
         if region is not None:
             return region
         # Try to guess from the domain. The format should be something like
diff --git a/lib/lp/oci/model/ociregistrycredentials.py b/lib/lp/oci/model/ociregistrycredentials.py
index 377dded..9980f48 100644
--- a/lib/lp/oci/model/ociregistrycredentials.py
+++ b/lib/lp/oci/model/ociregistrycredentials.py
@@ -87,6 +87,10 @@ class OCIRegistryCredentials(Storm):
 
     _credentials = JSON(name="credentials", allow_none=True)
 
+    # The list of dict keys that should not be encrypted when storing
+    # _credentials attribute.
+    UNENCRYPTED_CREDENTIALS_FIELDS = ['username', 'region']
+
     def __init__(self, owner, url, credentials):
         self.owner = owner
         self.url = url
@@ -111,17 +115,21 @@ class OCIRegistryCredentials(Storm):
     def setCredentials(self, value):
         container = getUtility(IEncryptedContainer, "oci-registry-secrets")
         copy = value.copy()
-        username = copy.pop("username", None)
+        # Remove fields that should not be encrypted.
+        unencrypted_fields = {}
+        for field in self.UNENCRYPTED_CREDENTIALS_FIELDS:
+            unencrypted_fields[field] = copy.pop(field, None)
+        # Encrypt the rest of the dict.
         data = {
             "credentials_encrypted": removeSecurityProxy(
                 container.encrypt(json.dumps(copy).encode('UTF-8')))}
-        if username is not None:
-            data["username"] = username
+        # Put back the fields that shouldn't be encrypted.
+        for field in self.UNENCRYPTED_CREDENTIALS_FIELDS:
+            value = unencrypted_fields[field]
+            if value is not None:
+                data[field] = value
         self._credentials = data
 
-    def getCredentialsValue(self, key):
-        return self.getCredentials().get(key)
-
     @property
     def username(self):
         return self._credentials.get('username')
@@ -130,6 +138,14 @@ class OCIRegistryCredentials(Storm):
     def username(self, value):
         self._credentials['username'] = value
 
+    @property
+    def region(self):
+        return self._credentials.get('region')
+
+    @region.setter
+    def region(self, value):
+        self._credentials['region'] = value
+
     def destroySelf(self):
         """See `IOCIRegistryCredentials`."""
         IStore(OCIRegistryCredentials).remove(self)
diff --git a/lib/lp/oci/tests/test_ociregistrycredentials.py b/lib/lp/oci/tests/test_ociregistrycredentials.py
index 5f43d60..971f3cc 100644
--- a/lib/lp/oci/tests/test_ociregistrycredentials.py
+++ b/lib/lp/oci/tests/test_ociregistrycredentials.py
@@ -48,13 +48,15 @@ class TestOCIRegistryCredentials(OCIConfigHelperMixin, TestCaseWithFactory):
         self.assertProvides(oci_credentials, IOCIRegistryCredentials)
 
     def test_credentials_are_encrypted(self):
-        credentials = {'username': 'foo', 'password': 'bar'}
+        credentials = {
+            'username': 'foo', 'password': 'bar', 'region': 'br-101'}
         oci_credentials = removeSecurityProxy(
             self.factory.makeOCIRegistryCredentials(
                 credentials=credentials))
         container = getUtility(IEncryptedContainer, "oci-registry-secrets")
         self.assertThat(oci_credentials._credentials, MatchesDict({
             "username": Equals("foo"),
+            "region": Equals("br-101"),
             "credentials_encrypted": AfterPreprocessing(
                 lambda value: json.loads(
                     container.decrypt(value).decode("UTF-8")),
@@ -66,12 +68,14 @@ class TestOCIRegistryCredentials(OCIConfigHelperMixin, TestCaseWithFactory):
         oci_credentials = self.factory.makeOCIRegistryCredentials(
             registrant=owner, owner=owner,
             url='http://example.org',
-            credentials={'username': 'foo', 'password': 'bar'})
+            credentials={
+                'username': 'foo', 'password': 'bar', 'region': 'br-101'})
 
         with person_logged_in(owner):
             self.assertThat(oci_credentials.getCredentials(), MatchesDict({
                 "username": Equals("foo"),
-                "password": Equals("bar")}))
+                "password": Equals("bar"),
+                "region": Equals("br-101")}))
 
     def test_credentials_set_empty(self):
         owner = self.factory.makePerson()
diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index 55596e9..af8f2e3 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -3740,7 +3740,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
 
         region = TextLine(
             __name__=self._getFieldName('region', id),
-            default=credentials.getCredentialsValue("region"),
+            default=credentials.region,
             required=False, readonly=False)
 
         delete = Bool(
@@ -3918,11 +3918,8 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             credentials.url = parsed_credentials["url"]
         elif parsed_credentials["url"] != credentials.url:
             credentials.url = parsed_credentials["url"]
-        if credentials.getCredentialsValue('region') != region:
-            credentials_data = removeSecurityProxy(
-                credentials.getCredentials())
-            credentials_data["region"] = region
-            credentials.setCredentials(credentials_data)
+        if credentials.region != region:
+            removeSecurityProxy(credentials).region = region
         if owner != credentials.owner:
             credentials.owner = owner