From f7f0a4e8117496f8efbdef53de0c402515b75955 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 8 Nov 2018 12:57:00 +0100 Subject: [PATCH] System groups (#18303) * Add read only and admin policies * Migrate to 2 system groups * Add system groups * Add system groups admin & read only * Dont' mutate parameters * Fix types --- homeassistant/auth/__init__.py | 6 +- homeassistant/auth/auth_store.py | 142 ++++++++++++++---- homeassistant/auth/const.py | 3 + homeassistant/auth/models.py | 1 + homeassistant/auth/permissions/__init__.py | 11 +- homeassistant/auth/permissions/const.py | 7 + homeassistant/auth/permissions/entities.py | 8 +- homeassistant/auth/permissions/merge.py | 2 +- .../auth/permissions/system_policies.py | 14 ++ .../auth/permissions/{common.py => types.py} | 2 - tests/auth/permissions/test_init.py | 12 -- .../auth/permissions/test_system_policies.py | 25 +++ tests/auth/test_auth_store.py | 141 ++++++++++++++++- tests/common.py | 3 +- 14 files changed, 309 insertions(+), 68 deletions(-) create mode 100644 homeassistant/auth/permissions/const.py create mode 100644 homeassistant/auth/permissions/system_policies.py rename homeassistant/auth/permissions/{common.py => types.py} (97%) create mode 100644 tests/auth/permissions/test_system_policies.py diff --git a/homeassistant/auth/__init__.py b/homeassistant/auth/__init__.py index 9fd9bf3fa50..0011c98ce73 100644 --- a/homeassistant/auth/__init__.py +++ b/homeassistant/auth/__init__.py @@ -13,6 +13,7 @@ from homeassistant.core import callback, HomeAssistant from homeassistant.util import dt as dt_util from . import auth_store, models +from .const import GROUP_ID_ADMIN from .mfa_modules import auth_mfa_module_from_config, MultiFactorAuthModule from .providers import auth_provider_from_config, AuthProvider, LoginFlow @@ -133,7 +134,7 @@ class AuthManager: name=name, system_generated=True, is_active=True, - groups=[], + group_ids=[], ) self.hass.bus.async_fire(EVENT_USER_ADDED, { @@ -144,11 +145,10 @@ class AuthManager: async def async_create_user(self, name: str) -> models.User: """Create a user.""" - group = (await self._store.async_get_groups())[0] kwargs = { 'name': name, 'is_active': True, - 'groups': [group] + 'group_ids': [GROUP_ID_ADMIN] } # type: Dict[str, Any] if await self._user_should_be_owner(): diff --git a/homeassistant/auth/auth_store.py b/homeassistant/auth/auth_store.py index 8c328bfe13e..ab233489db0 100644 --- a/homeassistant/auth/auth_store.py +++ b/homeassistant/auth/auth_store.py @@ -10,11 +10,14 @@ from homeassistant.core import HomeAssistant, callback from homeassistant.util import dt as dt_util from . import models -from .permissions import DEFAULT_POLICY +from .const import GROUP_ID_ADMIN, GROUP_ID_READ_ONLY +from .permissions import system_policies +from .permissions.types import PolicyType # noqa: F401 STORAGE_VERSION = 1 STORAGE_KEY = 'auth' -INITIAL_GROUP_NAME = 'All Access' +GROUP_NAME_ADMIN = 'Administrators' +GROUP_NAME_READ_ONLY = 'Read Only' class AuthStore: @@ -63,7 +66,7 @@ class AuthStore: is_active: Optional[bool] = None, system_generated: Optional[bool] = None, credentials: Optional[models.Credentials] = None, - groups: Optional[List[models.Group]] = None) -> models.User: + group_ids: Optional[List[str]] = None) -> models.User: """Create a new user.""" if self._users is None: await self._async_load() @@ -71,11 +74,18 @@ class AuthStore: assert self._users is not None assert self._groups is not None + groups = [] + for group_id in (group_ids or []): + group = self._groups.get(group_id) + if group is None: + raise ValueError('Invalid group specified {}'.format(group_id)) + groups.append(group) + kwargs = { 'name': name, # Until we get group management, we just put everyone in the # same group. - 'groups': groups or [], + 'groups': groups, } # type: Dict[str, Any] if is_owner is not None: @@ -238,38 +248,98 @@ class AuthStore: users = OrderedDict() # type: Dict[str, models.User] groups = OrderedDict() # type: Dict[str, models.Group] - # When creating objects we mention each attribute explicetely. This + # Soft-migrating data as we load. We are going to make sure we have a + # read only group and an admin group. There are two states that we can + # migrate from: + # 1. Data from a recent version which has a single group without policy + # 2. Data from old version which has no groups + has_admin_group = False + has_read_only_group = False + group_without_policy = None + + # When creating objects we mention each attribute explicitly. This # prevents crashing if user rolls back HA version after a new property # was added. for group_dict in data.get('groups', []): + policy = None # type: Optional[PolicyType] + + if group_dict['id'] == GROUP_ID_ADMIN: + has_admin_group = True + + name = GROUP_NAME_ADMIN + policy = system_policies.ADMIN_POLICY + system_generated = True + + elif group_dict['id'] == GROUP_ID_READ_ONLY: + has_read_only_group = True + + name = GROUP_NAME_READ_ONLY + policy = system_policies.READ_ONLY_POLICY + system_generated = True + + else: + name = group_dict['name'] + policy = group_dict.get('policy') + system_generated = False + + # We don't want groups without a policy that are not system groups + # This is part of migrating from state 1 + if policy is None: + group_without_policy = group_dict['id'] + continue + groups[group_dict['id']] = models.Group( - name=group_dict['name'], id=group_dict['id'], - policy=group_dict.get('policy', DEFAULT_POLICY), + name=name, + policy=policy, + system_generated=system_generated, ) - migrate_group = None + # If there are no groups, add all existing users to the admin group. + # This is part of migrating from state 2 + migrate_users_to_admin_group = (not groups and + group_without_policy is None) - if not groups: - migrate_group = models.Group( - name=INITIAL_GROUP_NAME, - policy=DEFAULT_POLICY - ) - groups[migrate_group.id] = migrate_group + # If we find a no_policy_group, we need to migrate all users to the + # admin group. We only do this if there are no other groups, as is + # the expected state. If not expected state, not marking people admin. + # This is part of migrating from state 1 + if groups and group_without_policy is not None: + group_without_policy = None + + # This is part of migrating from state 1 and 2 + if not has_admin_group: + admin_group = _system_admin_group() + groups[admin_group.id] = admin_group + + # This is part of migrating from state 1 and 2 + if not has_read_only_group: + read_only_group = _system_read_only_group() + groups[read_only_group.id] = read_only_group for user_dict in data['users']: + # Collect the users group. + user_groups = [] + for group_id in user_dict.get('group_ids', []): + # This is part of migrating from state 1 + if group_id == group_without_policy: + group_id = GROUP_ID_ADMIN + user_groups.append(groups[group_id]) + + # This is part of migrating from state 2 + if (not user_dict['system_generated'] and + migrate_users_to_admin_group): + user_groups.append(groups[GROUP_ID_ADMIN]) + users[user_dict['id']] = models.User( name=user_dict['name'], - groups=[groups[group_id] for group_id - in user_dict.get('group_ids', [])], + groups=user_groups, id=user_dict['id'], is_owner=user_dict['is_owner'], is_active=user_dict['is_active'], system_generated=user_dict['system_generated'], ) - if migrate_group is not None and not user_dict['system_generated']: - users[user_dict['id']].groups = [migrate_group] for cred_dict in data['credentials']: users[cred_dict['user_id']].credentials.append(models.Credentials( @@ -356,11 +426,11 @@ class AuthStore: groups = [] for group in self._groups.values(): g_dict = { - 'name': group.name, 'id': group.id, } # type: Dict[str, Any] - if group.policy is not DEFAULT_POLICY: + if group.id not in (GROUP_ID_READ_ONLY, GROUP_ID_ADMIN): + g_dict['name'] = group.name g_dict['policy'] = group.policy groups.append(g_dict) @@ -410,13 +480,29 @@ class AuthStore: """Set default values for auth store.""" self._users = OrderedDict() # type: Dict[str, models.User] - # Add default group - all_access_group = models.Group( - name=INITIAL_GROUP_NAME, - policy=DEFAULT_POLICY, - ) - groups = OrderedDict() # type: Dict[str, models.Group] - groups[all_access_group.id] = all_access_group - + admin_group = _system_admin_group() + groups[admin_group.id] = admin_group + read_only_group = _system_read_only_group() + groups[read_only_group.id] = read_only_group self._groups = groups + + +def _system_admin_group() -> models.Group: + """Create system admin group.""" + return models.Group( + name=GROUP_NAME_ADMIN, + id=GROUP_ID_ADMIN, + policy=system_policies.ADMIN_POLICY, + system_generated=True, + ) + + +def _system_read_only_group() -> models.Group: + """Create read only group.""" + return models.Group( + name=GROUP_NAME_READ_ONLY, + id=GROUP_ID_READ_ONLY, + policy=system_policies.READ_ONLY_POLICY, + system_generated=True, + ) diff --git a/homeassistant/auth/const.py b/homeassistant/auth/const.py index 2e57986958c..519669ead85 100644 --- a/homeassistant/auth/const.py +++ b/homeassistant/auth/const.py @@ -3,3 +3,6 @@ from datetime import timedelta ACCESS_TOKEN_EXPIRATION = timedelta(minutes=30) MFA_SESSION_EXPIRATION = timedelta(minutes=5) + +GROUP_ID_ADMIN = 'system-admin' +GROUP_ID_READ_ONLY = 'system-read-only' diff --git a/homeassistant/auth/models.py b/homeassistant/auth/models.py index fc35f1398db..cefaabe7521 100644 --- a/homeassistant/auth/models.py +++ b/homeassistant/auth/models.py @@ -22,6 +22,7 @@ class Group: name = attr.ib(type=str) # type: Optional[str] policy = attr.ib(type=perm_mdl.PolicyType) id = attr.ib(type=str, factory=lambda: uuid.uuid4().hex) + system_generated = attr.ib(type=bool, default=False) @attr.s(slots=True) diff --git a/homeassistant/auth/permissions/__init__.py b/homeassistant/auth/permissions/__init__.py index ee0d3af0c54..fd3cf81f029 100644 --- a/homeassistant/auth/permissions/__init__.py +++ b/homeassistant/auth/permissions/__init__.py @@ -7,18 +7,11 @@ import voluptuous as vol from homeassistant.core import State -from .common import CategoryType, PolicyType +from .const import CAT_ENTITIES +from .types import CategoryType, PolicyType from .entities import ENTITY_POLICY_SCHEMA, compile_entities from .merge import merge_policies # noqa - -# Default policy if group has no policy applied. -DEFAULT_POLICY = { - "entities": True -} # type: PolicyType - -CAT_ENTITIES = 'entities' - POLICY_SCHEMA = vol.Schema({ vol.Optional(CAT_ENTITIES): ENTITY_POLICY_SCHEMA }) diff --git a/homeassistant/auth/permissions/const.py b/homeassistant/auth/permissions/const.py new file mode 100644 index 00000000000..e60879881c1 --- /dev/null +++ b/homeassistant/auth/permissions/const.py @@ -0,0 +1,7 @@ +"""Permission constants.""" +CAT_ENTITIES = 'entities' +SUBCAT_ALL = 'all' + +POLICY_READ = 'read' +POLICY_CONTROL = 'control' +POLICY_EDIT = 'edit' diff --git a/homeassistant/auth/permissions/entities.py b/homeassistant/auth/permissions/entities.py index b38600fe130..89b9398628c 100644 --- a/homeassistant/auth/permissions/entities.py +++ b/homeassistant/auth/permissions/entities.py @@ -5,12 +5,8 @@ from typing import ( # noqa: F401 import voluptuous as vol -from .common import CategoryType, ValueType, SUBCAT_ALL - - -POLICY_READ = 'read' -POLICY_CONTROL = 'control' -POLICY_EDIT = 'edit' +from .const import SUBCAT_ALL, POLICY_READ, POLICY_CONTROL, POLICY_EDIT +from .types import CategoryType, ValueType SINGLE_ENTITY_SCHEMA = vol.Any(True, vol.Schema({ vol.Optional(POLICY_READ): True, diff --git a/homeassistant/auth/permissions/merge.py b/homeassistant/auth/permissions/merge.py index 32cbfefcf1c..ec6375a0e3d 100644 --- a/homeassistant/auth/permissions/merge.py +++ b/homeassistant/auth/permissions/merge.py @@ -2,7 +2,7 @@ from typing import ( # noqa: F401 cast, Dict, List, Set) -from .common import PolicyType, CategoryType +from .types import PolicyType, CategoryType def merge_policies(policies: List[PolicyType]) -> PolicyType: diff --git a/homeassistant/auth/permissions/system_policies.py b/homeassistant/auth/permissions/system_policies.py new file mode 100644 index 00000000000..78da68c0d11 --- /dev/null +++ b/homeassistant/auth/permissions/system_policies.py @@ -0,0 +1,14 @@ +"""System policies.""" +from .const import CAT_ENTITIES, SUBCAT_ALL, POLICY_READ + +ADMIN_POLICY = { + CAT_ENTITIES: True, +} + +READ_ONLY_POLICY = { + CAT_ENTITIES: { + SUBCAT_ALL: { + POLICY_READ: True + } + } +} diff --git a/homeassistant/auth/permissions/common.py b/homeassistant/auth/permissions/types.py similarity index 97% rename from homeassistant/auth/permissions/common.py rename to homeassistant/auth/permissions/types.py index f87f9d70ddf..1871861f291 100644 --- a/homeassistant/auth/permissions/common.py +++ b/homeassistant/auth/permissions/types.py @@ -29,5 +29,3 @@ CategoryType = Union[ # Example: { entities: … } PolicyType = Mapping[str, CategoryType] - -SUBCAT_ALL = 'all' diff --git a/tests/auth/permissions/test_init.py b/tests/auth/permissions/test_init.py index 60ec3cb4314..fdc5440a9d5 100644 --- a/tests/auth/permissions/test_init.py +++ b/tests/auth/permissions/test_init.py @@ -32,15 +32,3 @@ def test_owner_permissions(): State('light.balcony', 'on'), ] assert permissions.OwnerPermissions.filter_states(states) == states - - -def test_default_policy_allow_all(): - """Test that the default policy is to allow all entity actions.""" - perm = permissions.PolicyPermissions(permissions.DEFAULT_POLICY) - assert perm.check_entity('light.kitchen', 'read') - states = [ - State('light.kitchen', 'on'), - State('light.living_room', 'off'), - State('light.balcony', 'on'), - ] - assert perm.filter_states(states) == states diff --git a/tests/auth/permissions/test_system_policies.py b/tests/auth/permissions/test_system_policies.py new file mode 100644 index 00000000000..ba6fe214146 --- /dev/null +++ b/tests/auth/permissions/test_system_policies.py @@ -0,0 +1,25 @@ +"""Test system policies.""" +from homeassistant.auth.permissions import ( + PolicyPermissions, system_policies, POLICY_SCHEMA) + + +def test_admin_policy(): + """Test admin policy works.""" + # Make sure it's valid + POLICY_SCHEMA(system_policies.ADMIN_POLICY) + + perms = PolicyPermissions(system_policies.ADMIN_POLICY) + assert perms.check_entity('light.kitchen', 'read') + assert perms.check_entity('light.kitchen', 'control') + assert perms.check_entity('light.kitchen', 'edit') + + +def test_read_only_policy(): + """Test read only policy works.""" + # Make sure it's valid + POLICY_SCHEMA(system_policies.READ_ONLY_POLICY) + + perms = PolicyPermissions(system_policies.READ_ONLY_POLICY) + assert perms.check_entity('light.kitchen', 'read') + assert not perms.check_entity('light.kitchen', 'control') + assert not perms.check_entity('light.kitchen', 'edit') diff --git a/tests/auth/test_auth_store.py b/tests/auth/test_auth_store.py index a3bdbab93d7..b76d68fbeac 100644 --- a/tests/auth/test_auth_store.py +++ b/tests/auth/test_auth_store.py @@ -2,8 +2,8 @@ from homeassistant.auth import auth_store -async def test_loading_old_data_format(hass, hass_storage): - """Test we correctly load an old data format.""" +async def test_loading_no_group_data_format(hass, hass_storage): + """Test we correctly load old data without any groups.""" hass_storage[auth_store.STORAGE_KEY] = { 'version': 1, 'data': { @@ -60,9 +60,15 @@ async def test_loading_old_data_format(hass, hass_storage): store = auth_store.AuthStore(hass) groups = await store.async_get_groups() - assert len(groups) == 1 - group = groups[0] - assert group.name == "All Access" + assert len(groups) == 2 + admin_group = groups[0] + assert admin_group.name == auth_store.GROUP_NAME_ADMIN + assert admin_group.system_generated + assert admin_group.id == auth_store.GROUP_ID_ADMIN + read_group = groups[1] + assert read_group.name == auth_store.GROUP_NAME_READ_ONLY + assert read_group.system_generated + assert read_group.id == auth_store.GROUP_ID_READ_ONLY users = await store.async_get_users() assert len(users) == 2 @@ -70,7 +76,7 @@ async def test_loading_old_data_format(hass, hass_storage): owner, system = users assert owner.system_generated is False - assert owner.groups == [group] + assert owner.groups == [admin_group] assert len(owner.refresh_tokens) == 1 owner_token = list(owner.refresh_tokens.values())[0] assert owner_token.id == 'user-token-id' @@ -80,3 +86,126 @@ async def test_loading_old_data_format(hass, hass_storage): assert len(system.refresh_tokens) == 1 system_token = list(system.refresh_tokens.values())[0] assert system_token.id == 'system-token-id' + + +async def test_loading_all_access_group_data_format(hass, hass_storage): + """Test we correctly load old data with single group.""" + hass_storage[auth_store.STORAGE_KEY] = { + 'version': 1, + 'data': { + 'credentials': [], + 'users': [ + { + "id": "user-id", + "is_active": True, + "is_owner": True, + "name": "Paulus", + "system_generated": False, + 'group_ids': ['abcd-all-access'] + }, + { + "id": "system-id", + "is_active": True, + "is_owner": True, + "name": "Hass.io", + "system_generated": True, + } + ], + "groups": [ + { + "id": "abcd-all-access", + "name": "All Access", + } + ], + "refresh_tokens": [ + { + "access_token_expiration": 1800.0, + "client_id": "http://localhost:8123/", + "created_at": "2018-10-03T13:43:19.774637+00:00", + "id": "user-token-id", + "jwt_key": "some-key", + "last_used_at": "2018-10-03T13:43:19.774712+00:00", + "token": "some-token", + "user_id": "user-id" + }, + { + "access_token_expiration": 1800.0, + "client_id": None, + "created_at": "2018-10-03T13:43:19.774637+00:00", + "id": "system-token-id", + "jwt_key": "some-key", + "last_used_at": "2018-10-03T13:43:19.774712+00:00", + "token": "some-token", + "user_id": "system-id" + }, + { + "access_token_expiration": 1800.0, + "client_id": "http://localhost:8123/", + "created_at": "2018-10-03T13:43:19.774637+00:00", + "id": "hidden-because-no-jwt-id", + "last_used_at": "2018-10-03T13:43:19.774712+00:00", + "token": "some-token", + "user_id": "user-id" + }, + ] + } + } + + store = auth_store.AuthStore(hass) + groups = await store.async_get_groups() + assert len(groups) == 2 + admin_group = groups[0] + assert admin_group.name == auth_store.GROUP_NAME_ADMIN + assert admin_group.system_generated + assert admin_group.id == auth_store.GROUP_ID_ADMIN + read_group = groups[1] + assert read_group.name == auth_store.GROUP_NAME_READ_ONLY + assert read_group.system_generated + assert read_group.id == auth_store.GROUP_ID_READ_ONLY + + users = await store.async_get_users() + assert len(users) == 2 + + owner, system = users + + assert owner.system_generated is False + assert owner.groups == [admin_group] + assert len(owner.refresh_tokens) == 1 + owner_token = list(owner.refresh_tokens.values())[0] + assert owner_token.id == 'user-token-id' + + assert system.system_generated is True + assert system.groups == [] + assert len(system.refresh_tokens) == 1 + system_token = list(system.refresh_tokens.values())[0] + assert system_token.id == 'system-token-id' + + +async def test_loading_empty_data(hass, hass_storage): + """Test we correctly load with no existing data.""" + store = auth_store.AuthStore(hass) + groups = await store.async_get_groups() + assert len(groups) == 2 + admin_group = groups[0] + assert admin_group.name == auth_store.GROUP_NAME_ADMIN + assert admin_group.system_generated + assert admin_group.id == auth_store.GROUP_ID_ADMIN + read_group = groups[1] + assert read_group.name == auth_store.GROUP_NAME_READ_ONLY + assert read_group.system_generated + assert read_group.id == auth_store.GROUP_ID_READ_ONLY + + users = await store.async_get_users() + assert len(users) == 0 + + +async def test_system_groups_only_store_id(hass, hass_storage): + """Test that for system groups we only store the ID.""" + store = auth_store.AuthStore(hass) + await store._async_load() + data = store._data_to_save() + assert len(data['users']) == 0 + assert data['groups'] == [ + {'id': auth_store.GROUP_ID_ADMIN}, + {'id': auth_store.GROUP_ID_READ_ONLY}, + ] diff --git a/tests/common.py b/tests/common.py index 44f934e4cb3..b3d72cbebbf 100644 --- a/tests/common.py +++ b/tests/common.py @@ -15,6 +15,7 @@ from contextlib import contextmanager from homeassistant import auth, core as ha, config_entries from homeassistant.auth import ( models as auth_models, auth_store, providers as auth_providers) +from homeassistant.auth.permissions import system_policies from homeassistant.setup import setup_component, async_setup_component from homeassistant.config import async_process_component_config from homeassistant.helpers import ( @@ -349,7 +350,7 @@ class MockGroup(auth_models.Group): """Mock a group in Home Assistant.""" def __init__(self, id=None, name='Mock Group', - policy=auth_store.DEFAULT_POLICY): + policy=system_policies.ADMIN_POLICY): """Mock a group.""" kwargs = { 'name': name,