Merge ~woutervb/various/+git/snapstore-proxy-charm:registration_bundle into ~woutervb/various/+git/snapstore-proxy-charm:main

[Wouter van Bommel]

Add the option to provide the charm with a registration bundle

This bundle, created with snapstore-client, is preferred, as it will allow registration with an ubuntu one account that has 2fa enabled.

Unmerged commits

647db14... by Wouter van Bommel on 2022-01-13

Support a registration bundle

When using a registration bundle, it is no longer needed to add an
username or password to the config.
Updated the README to reflex the change
Updated the overlay.yaml.example file to reflex this change

diff --git a/README.md b/README.md
index cd1ef35..817c9d7 100644
--- a/README.md
+++ b/README.md
@@ -19,9 +19,7 @@ Below is a minimal bundle example that can be used to deploy the charm using juj
       snapstore-proxy:
         charm: snapstore-proxy
         options:
-          domain: http://something.not.local
-          username: [e-mail]
-          password: UserPassword
+          registration_bundle: <content of registration file base64 encoded>
 
     relations:
     - - postgresql:db-admin
@@ -37,7 +35,7 @@ The charm supports the following actions:
 
 * **status** - Get the status result of the proxy, use like;
 
-  `juju run-action snapstore-proxy/leader status --wait`
+      juju run-action snapstore-proxy/leader status --wait
 
 # Advanced options
 
@@ -45,9 +43,15 @@ The charm supports the following actions:
 
 There are some mandatory config options that have to be configured for the charm to work, these are:
 
-* **domain** - The domain on which the proxy will listen. This is needed for the registration, and the proxy itself needs to be able to resolve it. Ip addresses don't work. It has to be prepended with the protocol (http).
+* **registration_bundle** - A bundle created with a snap called `snapstore-client`. This tool supports accounts with 2 Factor authentication for Ubuntu One accounts, and contains everything to reproduce the proxy. This is the recommended way to configure the proxy. It is also a required way, if the machine on which the proxy is installed does not have an internet connection to the store. Easiest way to provide this option on the cli is the following:
+
+      juju config snapstore-proxy registration_bundle=$(cat <path to file created with snapstore-client> | base64)
+
 
-* **username** - This is the username of the ubuntu one account that will be used to register the proxy
+Or (deprecated)
+
+* **domain** - The domain on which the proxy will listen. This is needed for the registration, and the proxy itself needs to be able to resolve it. Ip addresses don't work. It has to be prepended with the protocol (http).
+* **username** - This is the username of the Ubuntu One account that will be used to register the proxy
 * **password** - The password of the above username
 
 ## Juju resource usage
@@ -65,9 +69,7 @@ An example bundle to do such a deployment will look like the following.
       snapstore-proxy:
         charm: snapstore-proxy
         options:
-          domain: http://something.not.local
-          username: [e-mail]
-          password: UserPassword
+          registration_bundle: <content of registration file base64 encoded>
         resources:
           snap-store-proxy: ./snap-store-proxy.snap
           core: ./core20.snap
diff --git a/config.yaml b/config.yaml
index eaaa7df..1c131a6 100644
--- a/config.yaml
+++ b/config.yaml
@@ -5,11 +5,16 @@
 # always possible
 
 options:
+  registration_bundle:
+    default: null
+    desciption: |
+      A bundle created via `snapstore-proxy_registration_bundle` added as a base64 
+      encoded string. ie "$(cat <file> | base64)"
+    type: string
   domain:
     default: http://localhost.localdomain
     description: Domain name for the Snap Store Proxy.
     type: string
-
   username:
     default: null
     description: |
diff --git a/juju/overlay.yaml.example b/juju/overlay.yaml.example
index fc9b38d..ddd4f49 100644
--- a/juju/overlay.yaml.example
+++ b/juju/overlay.yaml.example
@@ -1,5 +1,4 @@
 applications:
   snapstore-proxy:
     options:
-      password: my super password
-      username: my ubuntu one email
\ No newline at end of file
+      registration_bundle: `cat <registation file> | base64`
\ No newline at end of file
diff --git a/juju/test-bundle.yaml b/juju/test-bundle.yaml
index 39c1f08..a2be469 100644
--- a/juju/test-bundle.yaml
+++ b/juju/test-bundle.yaml
@@ -13,8 +13,6 @@ applications:
   snapstore-proxy:
     charm: ../snapstore-proxy_ubuntu-20.04-amd64.charm
     num_units: 1
-    options:
-      domain: http://something.not.local
 
 
 relations:
diff --git a/src/charm.py b/src/charm.py
index 591965f..002cebb 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -5,7 +5,9 @@
 # Learn more at: https://juju.is/docs/sdk
 #
 
+import base64
 import hashlib
+import json
 import logging
 from urllib.parse import urlparse
 
@@ -121,8 +123,9 @@ class SnapstoreProxyCharm(CharmBase):
         # Then configure it, and only then we can attempt to configure it
         self.handle_config()
 
-        self.unit.status = MaintenanceStatus("Registering proxy")
-        self.handle_registration(event)
+        if not getattr(self._stored, "registered", False):
+            self.unit.status = MaintenanceStatus("Registering proxy")
+            self.handle_registration(event)
 
         self.evaluate_status()
 
@@ -207,6 +210,11 @@ class SnapstoreProxyCharm(CharmBase):
         A different method, as it will contain mappings between items and the
         actual snap entries"""
 
+        has_registration_bundle = False
+        for item in updates:
+            has_registration_bundle |= item["name"] == "registration_bundle"
+
+        # If the has_registration_bundle is true, we omit changes to the domain
         for item in updates:
             if item["name"] == "domain":
                 logger.info("Configuring the domain")
@@ -222,6 +230,57 @@ class SnapstoreProxyCharm(CharmBase):
             setattr(self._stored, item["name"], item["value"])
             setattr(self._stored, f"{item['name']}_md5", item["value"])
 
+        if has_registration_bundle:
+            # If this config option is set, we basically ignore the username and
+            # password and domain settings. And we take all from this field.
+            for item in updates:
+                if item["name"] == "registration_bundle":
+                    break
+            if item["value"]:
+                # A bit harsh, but this way we ensure that only this bundle is used
+                self._stored.registered = True
+            else:
+                # The value is removed / reset, so act accordingly
+                self._stored.registered = False
+                return
+
+            data = json.loads(base64.b64decode(item["value"]))
+            for entries in [
+                "domain",
+                "private_key",
+                "public_key",
+                "store_id",
+                # "store_assertion", needed once airgap mode is implemented
+            ]:
+                # see if any of these keys are missing
+                if entries not in data.keys():
+                    self.errors.append(
+                        f"Missing key {entries} in the registration bundle"
+                    )
+
+            if self.errors:
+                # Incomplete bundle(?) so stop doing anything
+                return
+
+            # Now make 2 lists, option names and values where the indexes
+            # correspond with eachother
+            options = []
+            values = []
+            for entry in data.keys():
+                if entry == "domain":
+                    options.append("proxy.domain")
+                    values.append(data[entry])
+                elif entry == "public_key":
+                    options.append("proxy.key.public")
+                    values.append(data[entry])
+                elif entry == "private_key":
+                    options.append("proxy.key.private")
+                    values.append(data[entry])
+                elif entry == "store_id":
+                    options.append("internal.store.id")
+                    values.append(data[entry])
+            configure_proxy(options, values, force=True)
+
     def handle_registration(self, event):
         """Try to register the store.
 
diff --git a/src/helpers.py b/src/helpers.py
index 0575279..07cf19c 100644
--- a/src/helpers.py
+++ b/src/helpers.py
@@ -10,10 +10,23 @@ import yaml
 logger = logging.getLogger(__name__)
 
 
-def configure_proxy(option, value):
+def configure_proxy(option, value, force=False):
     """Simple wrapper around calling snap settings"""
-    logger.debug(f"Running config for option {option} with value {value}")
-    run(["snap-store-proxy", "config", f"{option}={value}"])
+
+    if isinstance(option, list):
+        # Make sure that we set multiple values in one go
+        combined = []
+        for count, entry in enumerate(option):
+            combined.append(f'{entry}="{value[count]}"')
+        command = ["snap-store-proxy", "config"] + combined
+    else:
+        logger.debug(f"Running config for option {option} with value {value}")
+        command = ["snap-store-proxy", "config", f"{option}={value}"]
+
+    if force:
+        command.append("--force")
+
+    run(command)
 
 
 def config_options():
@@ -36,6 +49,8 @@ def config_options():
             type_dict.update({item: "url"})
         elif item == "username":
             type_dict.update({item: "email"})
+        elif item == "registration_bundle":
+            type_dict.update({item: "base64+json"})
         else:
             type_dict.update({item: config["options"][item]["type"]})
 
diff --git a/src/optionvalidation.py b/src/optionvalidation.py
index 0ef81d7..2e8efae 100644
--- a/src/optionvalidation.py
+++ b/src/optionvalidation.py
@@ -2,10 +2,15 @@
 # See LICENSE file for licensing details.
 #
 
+import base64
+import json
 import re
+from logging import Logger
 
 from exceptions import InvalidTypeException, UnknownTypeException
 
+logger = Logger(__name__)
+
 
 class OptionValidation:
     @staticmethod
@@ -16,12 +21,15 @@ class OptionValidation:
             return OptionValidationURL()
         if test_type == "email":
             return OptionValidationEmail()
+        if test_type == "base64+json":
+            return OptionValidationBase64Json()
         raise UnknownTypeException(test_type)
 
 
 class OptionValidationString(OptionValidation):
     @staticmethod
     def validate(config_name, value):
+        logger.debug("Validating the type string for {config_name}")
         if not isinstance(value, str):
             raise InvalidTypeException(config_name, "string", value)
 
@@ -29,6 +37,7 @@ class OptionValidationString(OptionValidation):
 class OptionValidationURL(OptionValidation):
     @staticmethod
     def validate(config_name, value):
+        logger.debug("Validating the type url for {config_name}")
         regex = re.compile(
             r"^(?:http)://"  # http:// or https://
             r"(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+(?:[A-Z]{2,6}\.?|"
@@ -45,6 +54,23 @@ class OptionValidationURL(OptionValidation):
 class OptionValidationEmail(OptionValidation):
     @staticmethod
     def validate(config_name, value):
+        logger.debug("Validating the type email for {config_name}")
         regex = r"\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b"
         if (not isinstance(value, str)) or (not re.fullmatch(regex, value)):
             raise InvalidTypeException(config_name, "email", value)
+
+
+class OptionValidationBase64Json(OptionValidation):
+    @staticmethod
+    def validate(config_name, value):
+        logger.debug("Validating the type base64+json for {config_name}")
+        try:
+            result = base64.b64decode(value)
+            print(result)
+        except Exception:
+            raise InvalidTypeException(config_name, "base64", value)
+
+        try:
+            json.loads(result)
+        except Exception:
+            raise InvalidTypeException(config_name, "json", value)
diff --git a/tests/test_charm.py b/tests/test_charm.py
index a224ba7..6dc62f1 100644
--- a/tests/test_charm.py
+++ b/tests/test_charm.py
@@ -4,6 +4,7 @@
 # Learn more about testing at: https://juju.is/docs/sdk/testing
 #
 
+import base64
 import logging
 import subprocess
 from pathlib import Path
@@ -197,6 +198,146 @@ class TestConfig(CharmTest):
         patched_proxy.assert_called_once_with("postgresql://fake/database")
 
 
+class TestRegistrationBundle(CharmTest):
+    # Split off to ensure that tests are specific for what they do
+    # data that is passed to do_snap_updates has alrady been validated
+    # via the optionvalidation functions, so no need to test invalid data
+    def test_no_bundle(self):
+        assert not hasattr(self.harness.charm._stored, "registered")
+
+        self.harness.charm.do_snap_updates({})
+
+        # If we start without the registered key, and provide no bundle, no
+        # key will be added
+        assert not hasattr(self.harness.charm._stored, "registered")
+
+    def test_registration_removed(self):
+        setattr(self.harness.charm._stored, "registered", True)
+
+        self.harness.charm.do_snap_updates(
+            [{"name": "registration_bundle", "value": None}]
+        )
+
+        # We started with a key, so it will stay with us
+        assert not getattr(self.harness.charm._stored, "registered")
+
+    def test_has_incomplete_bundle_missing_all(self):
+        to_update = [
+            {
+                "name": "registration_bundle",
+                "value": base64.b64encode("{}".encode("UTF-8")),
+            }
+        ]
+
+        self.harness.charm.do_snap_updates(to_update)
+        assert len(self.harness.charm.errors) == 4
+        assert (
+            self.harness.charm.errors[0]
+            == "Missing key domain in the registration bundle"
+        )
+
+    def test_has_incomplete_bundle_added_domain(self):
+        to_update = [
+            {
+                "name": "registration_bundle",
+                "value": base64.b64encode('{"domain": "sdfsd"}'.encode("UTF-8")),
+            }
+        ]
+
+        self.harness.charm.do_snap_updates(to_update)
+        assert len(self.harness.charm.errors) == 3
+        assert (
+            self.harness.charm.errors[0]
+            == "Missing key private_key in the registration bundle"
+        )
+
+    def test_has_incomplete_bundle_added_private_key(self):
+        to_update = [
+            {
+                "name": "registration_bundle",
+                "value": base64.b64encode(
+                    '{"domain": "sdfsd",' '"private_key": "12"}'.encode("UTF-8")
+                ),
+            }
+        ]
+
+        self.harness.charm.do_snap_updates(to_update)
+        assert len(self.harness.charm.errors) == 2
+        assert (
+            self.harness.charm.errors[0]
+            == "Missing key public_key in the registration bundle"
+        )
+
+    def test_has_incomplete_bundle_added_public_key(self):
+        to_update = [
+            {
+                "name": "registration_bundle",
+                "value": base64.b64encode(
+                    '{"domain": "sdfsd",'
+                    '"public_key": "12",'
+                    '"private_key": "12"}'.encode("UTF-8")
+                ),
+            }
+        ]
+
+        self.harness.charm.do_snap_updates(to_update)
+        assert len(self.harness.charm.errors) == 1
+        assert (
+            self.harness.charm.errors[0]
+            == "Missing key store_id in the registration bundle"
+        )
+
+    # Possibly needed once airgap mode has been implemented
+    # def test_has_incomplete_bundle_added_store_id(self):
+    #     to_update = [
+    #         {
+    #             "name": "registration_bundle",
+    #             "value": base64.b64encode(
+    #                 '{"domain": "sdfsd",'
+    #                 '"public_key": "12",'
+    #                 '"store_id": "12",'
+    #                 '"private_key": "12"}'.encode("UTF-8")
+    #             ),
+    #         }
+    #     ]
+
+    #     self.harness.charm.do_snap_updates(to_update)
+    #     assert len(self.harness.charm.errors) == 1
+    #     assert (
+    #         self.harness.charm.errors[0]
+    #         == "Missing key store_assertion in the registration bundle"
+    #     )
+
+    def test_has_complete_bundle(self):
+        to_update = [
+            {
+                "name": "registration_bundle",
+                "value": base64.b64encode(
+                    '{"domain": "sdfsd",'
+                    '"public_key": "10",'
+                    '"store_id": "11",'
+                    '"store_assertion": "0",'
+                    '"private_key": "12"}'.encode("UTF-8")
+                ),
+            }
+        ]
+        with patch("charm.configure_proxy") as patched_configure:
+            self.harness.charm.do_snap_updates(to_update)
+
+        patched_configure.assert_called_once_with(
+            [
+                "proxy.domain",
+                "proxy.key.public",
+                "internal.store.id",
+                "proxy.key.private",
+            ],
+            ["sdfsd", "10", "11", "12"],
+            force=True,
+        )
+
+        assert getattr(self.harness.charm._stored, "registered", None)
+
+
 class TestStatus(CharmTest):
     def test_status_missing_db_uri(self):
         self.harness.charm._stored.db_uri = None
@@ -367,7 +508,7 @@ class TestEvents(CharmTest):
 
         patched_get_status.assert_called_once_with()
         event.fail.assert_called_once_with(
-            "Failed to get status, result String, errorcode 1"
+            "Failed to get status, errorcode 1\n, result String"
         )
 
 
diff --git a/tests/test_helpers.py b/tests/test_helpers.py
index 730f683..1e61b15 100644
--- a/tests/test_helpers.py
+++ b/tests/test_helpers.py
@@ -13,6 +13,40 @@ def test_configure_proxy():
     )
 
 
+def test_configure_proxy_list():
+    with mock.patch("helpers.run") as mocked_run:
+        options = ["one", "two"]
+        values = ["value_one", "value_two"]
+        helpers.configure_proxy(options, values)
+
+    assert mocked_run.called
+    mocked_run.assert_called_once_with(
+        ["snap-store-proxy", "config", 'one="value_one"', 'two="value_two"']
+    )
+
+
+def test_configure_proxy_forced():
+    with mock.patch("helpers.run") as mocked_run:
+        helpers.configure_proxy("myvalue", "myoption", True)
+
+    assert mocked_run.called
+    mocked_run.assert_called_once_with(
+        ["snap-store-proxy", "config", "myvalue=myoption", "--force"]
+    )
+
+
+def test_configure_proxy_list_forced():
+    with mock.patch("helpers.run") as mocked_run:
+        options = ["one", "two"]
+        values = ["value_one", "value_two"]
+        helpers.configure_proxy(options, values, True)
+
+    assert mocked_run.called
+    mocked_run.assert_called_once_with(
+        ["snap-store-proxy", "config", 'one="value_one"', 'two="value_two"', "--force"]
+    )
+
+
 def test_config_options():
     # Not sure how to do a clean test here, as this function is made
     # in an attempt to prevent double registration of config items
diff --git a/tests/test_optionvalidation.py b/tests/test_optionvalidation.py
index f95ffed..4144c79 100644
--- a/tests/test_optionvalidation.py
+++ b/tests/test_optionvalidation.py
@@ -1,8 +1,11 @@
+import base64
+
 import pytest
 
 from exceptions import InvalidTypeException, UnknownTypeException
 from optionvalidation import (
     OptionValidation,
+    OptionValidationBase64Json,
     OptionValidationEmail,
     OptionValidationString,
     OptionValidationURL,
@@ -21,6 +24,10 @@ def test_email_validation_type():
     assert isinstance(OptionValidation.new("email"), OptionValidationEmail)
 
 
+def test_base64_json_validation_type():
+    assert isinstance(OptionValidation.new("base64+json"), OptionValidationBase64Json)
+
+
 def test_validation_raises_on_unknown():
     with pytest.raises(UnknownTypeException):
         OptionValidation.new("clown")
@@ -102,7 +109,28 @@ def test_email_valid(email):
         "user@domain",
     ],
 )
-def test_email_imvalid(email):
+def test_email_invalid(email):
     with pytest.raises(InvalidTypeException) as exc:
         OptionValidation.new("email").validate("option", email)
     assert email in str(exc.value)
+
+
+def test_base64_json_valid():
+    value = base64.b64encode("{}".encode("UTF-8"))
+    OptionValidation.new("base64+json").validate("option", value)
+
+
+def test_base64_json_both_invalid():
+    value = "+_=-"
+    with pytest.raises(InvalidTypeException) as exc:
+        OptionValidation.new("base64+json").validate("option", value)
+    assert value in str(exc.value)
+    assert "base64" in str(exc.value)
+
+
+def test_base64_json_invalid():
+    value = base64.b64encode("{".encode("UTF-8"))
+    with pytest.raises(InvalidTypeException) as exc:
+        OptionValidation.new("base64+json").validate("option", value)
+    assert value.decode("UTF-8") in str(exc.value)
+    assert "json" in str(exc.value)