Merge pull request #882 from cderici/revisit-access-control-levels

#882

#### Description

This is something that we [stumbled upon](#874 (comment)) in one of the integration tests, which initially led me to believe there might be a regression in `ModelManager` facade. However, I realized that the libjuju only deals with the controller level access and doesn't know anything about model or offer access levels. 

There are two main contributions here:
1) Changes the way the controller object gets the `model_uuid`s (i.e. `cmd list-models` on juju). The old way was to call the `ControllerFacade.AllModels()` for admin users and `ModelManager.ListModels()` for anyone else. This changes it by calling `ModelManager.ListModelSummaries` for everyone just like [how `cmd` does it](https://github.com/juju/juju/blob/576c487266443c1b3d788bd1c85c4a45d0f91db0/cmd/juju/controller/listmodels.go#L126).
2) Adds new module `access.py` that has all the access levels for models, controllers and offers that can be used with `user.grant()` and `user.revoke()`.
3) Improves the existing `user.grant()` and `user.revoke()` which together with (2) should give a more fine grained control over permissions for pylibjuju users to test their stuff in setups involving multiple models, relations and users.


#### QA Steps

This revisits the existing integration tests and adds a new one too, so there are two main ways to QA this, manual, and automatic. The automatic way is to run the integration tests:

```
tox -e integration -- tests/integration/test_controller.py::test_grant_revoke_controller_access
```

```
tox -e integration -- tests/integration/test_controller.py::test_grant_revoke_model_access
```

All CI tests need to pass.

The manual way is to fire up a pylibjuju console and :
```python
 $ python -m asyncio
asyncio REPL 3.10.6 (main, May 29 2023, 11:10:38) [GCC 11.3.0] on linux
Use "await" directly instead of "asyncio.run()".
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> from juju import controller;c = controller.Controller(); await c.connect()
>>> user = await c.add_user('foouser')
>>> await c.list_models(username=user.username)
[]
>>> await user.grant('read', model_name='controller')
True
>>> await c.list_models(username=user.username)
['controller']
>>> await user.revoke('read', model_name='controller')
True
>>> await c.list_models(username=user.username)
[]
>>>
```

It is advisable to QA also the offer stuff as well.

Author: jujubot

juju/access.py

@@ -0,0 +1,47 @@
+
+from .errors import JujuNotValid
+
+# No permissions at all
+NO_ACCESS = ""
+
+# Model permissions
+
+READ_ACCESS = "read"
+WRITE_ACCESS = "write"
+CONSUME_ACCESS = "consume"
+ADMIN_ACCESS = "admin"
+MODEL_ACCESS_LEVELS = {READ_ACCESS, WRITE_ACCESS, CONSUME_ACCESS, ADMIN_ACCESS}
+
+# Controller permissions
+
+LOGIN_ACCESS = "login"
+ADD_MODEL_ACCESS = "add-model"
+SUPERUSER_ACCESS = "superuser"
+CONTROLLER_ACCESS_LEVELS = {LOGIN_ACCESS, ADD_MODEL_ACCESS, SUPERUSER_ACCESS}
+
+OFFER_ACCESS_LEVELS = {READ_ACCESS, CONSUME_ACCESS, ADMIN_ACCESS}
+
+ALL_ACCESS_LEVELS = MODEL_ACCESS_LEVELS.union(CONTROLLER_ACCESS_LEVELS)
+
+
+def validate_access_level(access):
+    if access not in ALL_ACCESS_LEVELS:
+        raise JujuNotValid("access level", access)
+
+
+def validate_offer_access(access):
+    validate_access_level()
+    if access not in OFFER_ACCESS_LEVELS:
+        raise JujuNotValid("offer access level", access)
+
+
+def validate_model_access(access):
+    validate_access_level(access)
+    if access not in MODEL_ACCESS_LEVELS:
+        raise JujuNotValid("model access level", access)
+
+
+def validate_controller_access(access):
+    validate_access_level(access)
+    if access not in CONTROLLER_ACCESS_LEVELS:
+        raise JujuNotValid("controller access level", access)

juju/controller.py

@@ -563,31 +563,14 @@ async def model_uuids(self, username=None, all=False):
 
         :returns: {str name : str UUID}
         """
-
-        if all:
-            facade = client.ControllerFacade.from_connection(
-                self.connection())
-        else:
-            facade = client.ModelManagerFacade.from_connection(
-                self.connection())
-            u_name = username if username else self.get_current_username()
-            user = tag.user(u_name)
-
-        for attempt in (1, 2, 3):
-            try:
-                if all:
-                    userModelList = await facade.AllModels()
-                else:
-                    userModelList = await facade.ListModels(tag=user)
-
-                return {um.model.name: um.model.uuid
-                        for um in userModelList.user_models}
-            except errors.JujuAPIError as e:
-                # retry concurrency error until resolved in Juju
-                # see: https://bugs.launchpad.net/juju/+bug/1721786
-                if 'has been removed' not in e.message or attempt == 3:
-                    raise
-                await jasyncio.sleep(attempt)
+        model_manager_facade = client.ModelManagerFacade.from_connection(self.connection())
+        u_name = username if username else self.get_current_username()
+        user = tag.user(u_name)
+
+        user_model_list = await model_manager_facade.ListModelSummaries(user_tag=user, all_=all)
+        model_summaries = [msr.result for msr in user_model_list.results]
+        return {model_summary.name: model_summary.uuid
+                for model_summary in model_summaries}
 
     async def list_models(self, username=None, all=False):
         """Return list of names of the available models on this controller.

juju/errors.py

@@ -87,6 +87,13 @@ class JujuBackupError(JujuError):
     pass
 
 
+class JujuNotValid(JujuError):
+    def __init__(self, entity_type, entity_name):
+        self.entity_type = entity_type
+        self.entity_name = entity_name
+        super().__init__(f'Invalid {entity_type} : {entity_name}')
+
+
 class JujuConfigError(JujuError):
     """Exception raised during processing a configuration key-value pair
     in a config set for an application.

juju/user.py

@@ -2,7 +2,8 @@
 
 import pyrfc3339
 
-from . import tag
+from . import tag, errors
+from .client import client
 
 log = logging.getLogger(__name__)
 
@@ -31,6 +32,7 @@ def last_connection(self):
 
     @property
     def access(self):
+        """Identifies the controller access levels of this user"""
         return self._user_info.access
 
     @property
@@ -59,19 +61,108 @@ async def set_password(self, password):
         await self.controller.change_user_password(self.username, password)
         self._user_info.password = password
 
-    async def grant(self, acl='login'):
-        """Set access level of this user on the controller.
+    async def modify_model_access(self, acl, action, model_name):
+        """Grants or revokes the given access level for this user for a given model
 
-        :param str acl: Access control ('login', 'add-model', or 'superuser')
+        :param str acl: Model access levels (see access module)
+        :param str action: grant/revoke
+        :param str model_name: Name of the model
+
+        :return bool: True if access changed, Error if user already has it
+        """
+        modelmanager_facade = client.ModelManagerFacade.from_connection(
+            self.controller.connection())
+        models = await self.controller.model_uuids()
+        if model_name not in models:
+            raise errors.JujuError(f'Unable to find model : {model_name}')
+        changes = client.ModifyModelAccess(acl, action, tag.model(models[model_name]), self.tag)
+        await modelmanager_facade.ModifyModelAccess(changes=[changes])
+        return True
+
+    async def modify_controller_access(self, acl, action):
+        """Grants or revokes the given access level for this user on the current controller
+
+        :param str acl: Controller access levels (see access module)
+        :param str action: grant/revoke
+
+        :return bool: True if access changed, Error if user already has it
+        """
+        controller_facade = client.ControllerFacade.from_connection(self.controller.connection())
+        changes = client.ModifyControllerAccess(acl, action, self.tag)
+        await controller_facade.ModifyControllerAccess(changes=[changes])
+
+        new_access = acl
+        if action == 'revoke':
+            new_access = ''
+        self._user_info.access = new_access
+        return True
+
+    async def modify_offer_access(self, acl, action, offer_url):
+        """Grants or revokes the given access level for this user on a given offer
+
+        :param str acl: Controller access levels (see access module)
+        :param str action: grant/revoke
+        :param str offer_url: url for the offer
+
+        :return bool: True if access changed, Error if user already has it
         """
-        if await self.controller.grant(self.username, acl):
-            self._user_info.access = acl
+        application_offers_facade = client.ApplicationOffersFacade.from_connection(
+            self.controller.connection())
+        changes = client.ModifyOfferAccess(acl, action, offer_url, self.tag)
+        await application_offers_facade.ModifyOfferAccess(changes=[changes])
+        return True
+
+    async def grant_or_revoke(self, acl, action, **kwargs):
+        """Grants or revokes the given access level of this user on model, offer or controller,
+        depending on the access level (see the access module)
+
+        :param str acl: Access control level
+        :param str action: 'grant' or 'revoke'
+
+        Depending on the access level, the available keyword parameters are:
+        :param str model_name: name of the model if acl is one of model access levels
+        :param str offer_url: url for the offer if acl is one of offer access levels
+
+        :return: True if access changed, False if user already has it
+        """
+        try:
+            if 'model_name' in kwargs:
+                return await self.modify_model_access(acl, action, kwargs['model_name'])
+            elif 'offer_url' in kwargs:
+                return await self.modify_offer_access(acl, action, kwargs['offer_url'])
+            else:
+                return await self.modify_controller_access(acl, action)
+        except errors.JujuError as e:
+            if 'user already has' in str(e):
+                return False
+            else:
+                raise
+
+    async def grant(self, acl, **kwargs):
+        """Grant the given access level of this user on model, offer or controller, depending on
+        the access level (see the access module)
+
+        :param str acl: Access control level
+
+        Depending on the access level, the available keyword parameters are:
+        :param str model_name: name of the model if acl is one of model access levels
+        :param str offer_url: url for the offer if acl is one of offer access levels
+
+        :return: None or Error
+        """
+        return await self.grant_or_revoke(acl, 'grant', **kwargs)
+
+    async def revoke(self, acl='login', **kwargs):
+        """The opposite of user.grant(). Revokes the given access level of this user on model,
+        offer or controller, depending on the given access level.
+
+        :param str acl: Access control level (see access module)
 
-    async def revoke(self, acl='login'):
-        """Removes all access rights for this user from the controller.
+        Available keyword parameters are:
+        :param str model_name: name of the model if acl is one of model access levels
+        :param str offer_url: url for the offer if acl is one of offer access levels
         """
-        await self.controller.revoke(self.username, acl)
-        self._user_info.access = ''
+        return await self.grant_or_revoke(acl, 'revoke', **kwargs)
 
     async def disable(self):
         """Disable this user.

tests/integration/test_controller.py

@@ -2,9 +2,10 @@
 import uuid
 import hvac
 
+from juju import access
 from juju.client.connection import Connection
 from juju.client import client
-from juju.errors import JujuAPIError
+from juju.errors import JujuAPIError, JujuError
 
 import pytest
 
@@ -105,26 +106,6 @@ async def test_reset_user_password(event_loop):
                 raise AssertionError()
 
 
-@base.bootstrapped
-@pytest.mark.asyncio
-async def test_grant_revoke(event_loop):
-    async with base.CleanController() as controller:
-        username = 'test-grant{}'.format(uuid.uuid4())
-        user = await controller.add_user(username)
-        await user.grant('superuser')
-        assert user.access == 'superuser'
-        fresh = await controller.get_user(username)  # fetch fresh copy
-        assert fresh.access == 'superuser'
-        await user.grant('login')  # already has 'superuser', so no-op
-        assert user.access == 'superuser'
-        fresh = await controller.get_user(username)  # fetch fresh copy
-        assert fresh.access == 'superuser'
-        await user.revoke()
-        assert user.access == ''
-        fresh = await controller.get_user(username)  # fetch fresh copy
-        assert fresh.access == ''
-
-
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_list_models(event_loop):
@@ -134,25 +115,6 @@ async def test_list_models(event_loop):
             assert model.name in result
 
 
-@base.bootstrapped
-@pytest.mark.asyncio
-async def test_list_models_user_access(event_loop):
-    async with base.CleanController() as controller:
-        username = 'test-grant{}'.format(uuid.uuid4())
-        user = await controller.add_user(username)
-        await user.grant(acl='superuser')
-        assert user.access == 'superuser'
-        models1 = await controller.list_models(username)
-        await user.revoke(acl='superuser')
-        models2 = await controller.list_models(username)
-        assert len(models1) > len(models2)
-
-        # testing all flag
-        await user.grant(acl='superuser')
-        models_all = await controller.list_models(username, all=True)
-        assert len(models_all) > len(models2)
-
-
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_get_model(event_loop):
@@ -314,3 +276,75 @@ async def test_secrets_backend_lifecycle(event_loop):
         list_after = await controller.list_secret_backends()
         assert len(list_after["results"]) == 1
         assert list_after["results"][0]["result"].name == "internal"
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_grant_revoke_controller_access(event_loop):
+    async with base.CleanController() as controller:
+        username = 'test-grant{}'.format(uuid.uuid4())
+        user = await controller.add_user(username)
+        await user.grant('superuser')
+        assert user.access == 'superuser'
+        fresh = await controller.get_user(username)  # fetch fresh copy
+        assert fresh.access == 'superuser'
+        await user.grant('login')  # already has 'superuser', so no-op
+        assert user.access == 'superuser'
+        fresh = await controller.get_user(username)  # fetch fresh copy
+        assert fresh.access == 'superuser'
+        await user.revoke()
+        assert user.access == ''
+        fresh = await controller.get_user(username)  # fetch fresh copy
+        assert fresh.access == ''
+        try:
+            # try removing the created user
+            await controller.remove_user(username)
+        except JujuError as e:
+            if 'state changing too quickly' in str(e):
+                pass
+            else:
+                raise
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_grant_revoke_model_access(event_loop):
+    async with base.CleanController() as controller:
+        username = 'test-grant{}'.format(uuid.uuid4())
+        user = await controller.add_user(username)
+
+        model_name = 'test-{}'.format(uuid.uuid4())
+        model = await controller.add_model(model_name)
+
+        with pytest.raises(JujuError):
+            # superuser is a controller access level, i.e. not a valid model acl
+            await user.grant('superuser', model_name=model_name)
+
+        models1 = await controller.list_models(username)
+        assert models1 == []
+
+        # grant user the access to see the model
+        await user.grant(access.READ_ACCESS, model_name=model_name)
+        models2 = await controller.list_models(username)
+
+        # assert that the user sees the model
+        assert model_name in models2
+
+        # now let's revoke the read access
+        await user.revoke(access.READ_ACCESS, model_name=model_name)
+        models3 = await controller.list_models(username)
+
+        # user shouldn't be able to see the model
+        assert models3 == []
+
+        # cleanup
+        await model.disconnect()
+        await controller.destroy_model(model_name)
+        try:
+            # try removing the created user
+            await controller.remove_user(username)
+        except JujuError as e:
+            if 'state changing too quickly' in str(e):
+                pass
+            else:
+                raise