diff --git a/homeassistant/auth/__init__.py b/homeassistant/auth/__init__.py index 8a6daff853379a..615921b6ddd553 100644 --- a/homeassistant/auth/__init__.py +++ b/homeassistant/auth/__init__.py @@ -222,9 +222,7 @@ async def async_enable_user_mfa(self, user, mfa_module_id, data): except vol.Invalid as err: raise ValueError('Data does not match schema: {}'.format(err)) - result = await module.async_setup_user(user.id, data) - await self._store.async_enable_user_mfa(user, mfa_module_id) - return result + return await module.async_setup_user(user.id, data) async def async_disable_user_mfa(self, user, mfa_module_id): """Disable a multi-factor auth module for user.""" @@ -237,7 +235,14 @@ async def async_disable_user_mfa(self, user, mfa_module_id): module = self.get_auth_mfa_module(mfa_module_id) await module.async_depose_user(user.id) - await self._store.async_disable_user_mfa(user, mfa_module_id) + + async def async_get_enabled_mfa(self, user): + """List enabled mfa modules for user.""" + module_ids = [] + for module_id, module in self._mfa_modules.items(): + if await module.async_is_user_setup(user.id): + module_ids.append(module_id) + return module_ids async def async_create_refresh_token(self, user, client_id=None): """Create a new refresh token for a user.""" diff --git a/homeassistant/auth/auth_store.py b/homeassistant/auth/auth_store.py index 0b0173eca75d7a..98529027c1b6c0 100644 --- a/homeassistant/auth/auth_store.py +++ b/homeassistant/auth/auth_store.py @@ -107,24 +107,6 @@ async def async_remove_credentials(self, credentials): await self.async_save() - async def async_enable_user_mfa(self, user, mfa_module_id): - """Enable a mfa module for user.""" - local_user = await self.async_get_user(user.id) - if mfa_module_id in local_user.mfa_modules: - return - - local_user.mfa_modules.append(mfa_module_id) - await self.async_save() - - async def async_disable_user_mfa(self, user, mfa_module_id): - """Disable a mfa module for user.""" - local_user = await self.async_get_user(user.id) - if mfa_module_id not in local_user.mfa_modules: - return - - local_user.mfa_modules.remove(mfa_module_id) - await self.async_save() - async def async_create_refresh_token(self, user, client_id=None): """Create a new token for a user.""" refresh_token = models.RefreshToken(user=user, client_id=client_id) @@ -206,7 +188,6 @@ async def async_save(self): 'is_active': user.is_active, 'name': user.name, 'system_generated': user.system_generated, - 'mfa_modules': user.mfa_modules } for user in self._users.values() ] diff --git a/homeassistant/auth/mfa_modules/__init__.py b/homeassistant/auth/mfa_modules/__init__.py index 86d7cb7c7ec67f..62392a1c7aac4c 100644 --- a/homeassistant/auth/mfa_modules/__init__.py +++ b/homeassistant/auth/mfa_modules/__init__.py @@ -15,7 +15,7 @@ MULTI_FACTOR_AUTH_MODULE_SCHEMA = vol.Schema({ vol.Required(CONF_TYPE): str, vol.Optional(CONF_NAME): str, - # Specify ID if you have two auth module for same type. + # Specify ID if you have two mfa auth module for same type. vol.Optional(CONF_ID): str, }, extra=vol.ALLOW_EXTRA) @@ -120,7 +120,7 @@ def setup_schema(self): """ return None - async def async_setup_user(self, user_id, data): + async def async_setup_user(self, user_id, setup_data): """Setup mfa auth module for user.""" raise NotImplementedError @@ -128,6 +128,10 @@ async def async_depose_user(self, user_id): """Remove user from mfa module.""" raise NotImplementedError + async def async_is_user_setup(self, user_id): + """Return whether user is setup.""" + raise NotImplementedError + async def async_validation(self, user_id, user_input): """Return True if validation passed.""" raise NotImplementedError diff --git a/homeassistant/auth/mfa_modules/insecure_example.py b/homeassistant/auth/mfa_modules/insecure_example.py index c55297eea83659..a7d1657b2a65ec 100644 --- a/homeassistant/auth/mfa_modules/insecure_example.py +++ b/homeassistant/auth/mfa_modules/insecure_example.py @@ -7,7 +7,7 @@ MULTI_FACTOR_AUTH_MODULE_SCHEMA CONFIG_SCHEMA = MULTI_FACTOR_AUTH_MODULE_SCHEMA.extend({ - vol.Required('users'): [vol.Schema({ + vol.Required('data'): [vol.Schema({ vol.Required('user_id'): str, vol.Required('pin'): str, })] @@ -25,8 +25,7 @@ class InsecureExampleModule(MultiFactorAuthModule): def __init__(self, hass, config): """Initialize the user data store.""" super().__init__(hass, config) - self._data = None - self._users = config['users'] + self._data = config['data'] @property def input_schema(self): @@ -38,35 +37,42 @@ def setup_schema(self): """Validate async_setup_user input data.""" return vol.Schema({'pin': str}) - async def async_setup_user(self, user_id, data): + async def async_setup_user(self, user_id, setup_data): """Setup mfa module for user.""" # data shall has been validate in caller - pin = data['pin'] + pin = setup_data['pin'] - for user in self._users: - if user and user.get('user_id') == user_id: + for data in self._data: + if data['user_id'] == user_id: # already setup, override - user['pin'] = pin + data['pin'] = pin return - self._users.append({'user_id': user_id, 'pin': pin}) + self._data.append({'user_id': user_id, 'pin': pin}) async def async_depose_user(self, user_id): """Remove user from mfa module.""" found = None - for user in self._users: - if user and user.get('user_id') == user_id: - found = user + for data in self._data: + if data['user_id'] == user_id: + found = data break if found: - self._users.remove(found) + self._data.remove(found) + + async def async_is_user_setup(self, user_id): + """Return whether user is setup.""" + for data in self._data: + if data['user_id'] == user_id: + return True + return False async def async_validation(self, user_id, user_input): """Return True if validation passed.""" - for user in self._users: - if user_id == user.get('user_id'): + for data in self._data: + if data['user_id'] == user_id: # user_input has been validate in caller - if user.get('pin') == user_input['pin']: + if data['pin'] == user_input['pin']: return True return False diff --git a/homeassistant/auth/models.py b/homeassistant/auth/models.py index 25b50fd35fbb41..38e054dc7cf71a 100644 --- a/homeassistant/auth/models.py +++ b/homeassistant/auth/models.py @@ -26,9 +26,6 @@ class User: # Tokens associated with a user. refresh_tokens = attr.ib(type=dict, default=attr.Factory(dict), cmp=False) - # Enabled multi-factor auth modules of a user. - mfa_modules = attr.ib(type=list, default=attr.Factory(list), cmp=False) - @attr.s(slots=True) class RefreshToken: diff --git a/homeassistant/auth/providers/__init__.py b/homeassistant/auth/providers/__init__.py index bc8c33877fea7b..796782b1107d80 100644 --- a/homeassistant/auth/providers/__init__.py +++ b/homeassistant/auth/providers/__init__.py @@ -234,13 +234,9 @@ async def async_start_mfa(self, flow_result): self._user = await self._auth_manager.\ async_get_user_by_credentials(credentials) - # module in user.mfa_modules may not loaded - # the config may have changed after the user enabled module - # we need double check available mfa_modules for this user - if self._user and self._user.mfa_modules: - modules = [m_id for m_id in self._user.mfa_modules - if self._auth_manager. - get_auth_mfa_module(m_id)] + if self._user is not None: + modules = await self._auth_manager.async_get_enabled_mfa( + self._user) if modules: self._auth_modules = modules diff --git a/homeassistant/components/config/auth.py b/homeassistant/components/config/auth.py index ec8123e4b1417f..6f00b03dedb948 100644 --- a/homeassistant/components/config/auth.py +++ b/homeassistant/components/config/auth.py @@ -105,7 +105,6 @@ def _user_info(user): 'is_owner': user.is_owner, 'is_active': user.is_active, 'system_generated': user.system_generated, - 'enabled_multi_factor_auth': user.mfa_modules, 'credentials': [ { 'type': c.auth_provider_type, diff --git a/tests/auth/mfa_modules/test_insecure_example.py b/tests/auth/mfa_modules/test_insecure_example.py index 40070d00713b22..289db67cbc96aa 100644 --- a/tests/auth/mfa_modules/test_insecure_example.py +++ b/tests/auth/mfa_modules/test_insecure_example.py @@ -9,7 +9,7 @@ async def test_validate(hass): """Test validating pin.""" auth_module = await auth_mfa_module_from_config(hass, { 'type': 'insecure_example', - 'users': [{'user_id': 'test-user', 'pin': '123456'}] + 'data': [{'user_id': 'test-user', 'pin': '123456'}] }) result = await auth_module.async_validation( @@ -29,12 +29,12 @@ async def test_setup_user(hass): """Test setup user.""" auth_module = await auth_mfa_module_from_config(hass, { 'type': 'insecure_example', - 'users': [] + 'data': [] }) await auth_module.async_setup_user( 'test-user', {'pin': '123456'}) - assert len(auth_module._users) == 1 + assert len(auth_module._data) == 1 result = await auth_module.async_validation( 'test-user', {'pin': '123456'}) @@ -45,12 +45,22 @@ async def test_depose_user(hass): """Test despose user.""" auth_module = await auth_mfa_module_from_config(hass, { 'type': 'insecure_example', - 'users': [{'user_id': 'test-user', 'pin': '123456'}] + 'data': [{'user_id': 'test-user', 'pin': '123456'}] }) - assert len(auth_module._users) == 1 + assert len(auth_module._data) == 1 await auth_module.async_depose_user('test-user') - assert len(auth_module._users) == 0 + assert len(auth_module._data) == 0 + + +async def test_is_user_setup(hass): + """Test is user setup.""" + auth_module = await auth_mfa_module_from_config(hass, { + 'type': 'insecure_example', + 'data': [{'user_id': 'test-user', 'pin': '123456'}] + }) + assert await auth_module.async_is_user_setup('test-user') is True + assert await auth_module.async_is_user_setup('invalid-user') is False async def test_login(hass): @@ -60,14 +70,13 @@ async def test_login(hass): 'users': [{'username': 'test-user', 'password': 'test-pass'}], }], [{ 'type': 'insecure_example', - 'users': [{'user_id': 'mock-user', 'pin': '123456'}] + 'data': [{'user_id': 'mock-user', 'pin': '123456'}] }]) user = MockUser( id='mock-user', is_owner=False, is_active=False, name='Paulus', - mfa_modules=['insecure_example'] ).add_to_auth_manager(hass.auth) await hass.auth.async_link_user(user, Credentials( id='mock-id', diff --git a/tests/auth/test_init.py b/tests/auth/test_init.py index 359fe48f7e3e77..d9416ca4f87bb1 100644 --- a/tests/auth/test_init.py +++ b/tests/auth/test_init.py @@ -73,17 +73,17 @@ async def test_auth_manager_from_config_auth_modules(mock_hass): }], [{ 'name': 'Module 1', 'type': 'insecure_example', - 'users': [], + 'data': [], }, { 'name': 'Module 2', 'type': 'insecure_example', 'id': 'another', - 'users': [], + 'data': [], }, { 'name': 'Duplicate ID', 'type': 'insecure_example', 'id': 'another', - 'users': [], + 'data': [], }]) providers = [{ @@ -375,7 +375,7 @@ async def test_login_with_auth_module(mock_hass): }], }], [{ 'type': 'insecure_example', - 'users': [{ + 'data': [{ 'user_id': 'mock-user', 'pin': 'test-pin' }] @@ -389,7 +389,6 @@ async def test_login_with_auth_module(mock_hass): is_owner=False, is_active=False, name='Paulus', - mfa_modules=['insecure_example'] ).add_to_auth_manager(manager) user.credentials.append(auth_models.Credentials( id='mock-id', @@ -447,14 +446,14 @@ async def test_login_with_multi_auth_module(mock_hass): }], }], [{ 'type': 'insecure_example', - 'users': [{ + 'data': [{ 'user_id': 'mock-user', 'pin': 'test-pin' }] }, { 'type': 'insecure_example', 'id': 'module2', - 'users': [{ + 'data': [{ 'user_id': 'mock-user', 'pin': 'test-pin2' }] @@ -468,7 +467,6 @@ async def test_login_with_multi_auth_module(mock_hass): is_owner=False, is_active=False, name='Paulus', - mfa_modules=['insecure_example', 'module2'] ).add_to_auth_manager(manager) user.credentials.append(auth_models.Credentials( id='mock-id', @@ -524,7 +522,7 @@ async def test_auth_module_expired_session(mock_hass): }], }], [{ 'type': 'insecure_example', - 'users': [{ + 'data': [{ 'user_id': 'mock-user', 'pin': 'test-pin' }] @@ -538,7 +536,6 @@ async def test_auth_module_expired_session(mock_hass): is_owner=False, is_active=False, name='Paulus', - mfa_modules=['insecure_example'] ).add_to_auth_manager(manager) user.credentials.append(auth_models.Credentials( id='mock-id', @@ -588,7 +585,7 @@ async def test_enable_mfa_for_user(hass, hass_storage): }] }], [{ 'type': 'insecure_example', - 'users': [], + 'data': [], }]) step = await manager.login_flow.async_init(('insecure_example', None)) @@ -600,39 +597,49 @@ async def test_enable_mfa_for_user(hass, hass_storage): assert user is not None # new user don't have mfa enabled - assert bool(user.mfa_modules) is False + modules = await manager.async_get_enabled_mfa(user) + assert len(modules) == 0 module = manager.get_auth_mfa_module('insecure_example') # mfa module don't have data - assert bool(module._users) is False + assert bool(module._data) is False # test enable mfa for user await manager.async_enable_user_mfa(user, 'insecure_example', {'pin': 'test-pin'}) - assert len(user.mfa_modules) == 1 - assert 'insecure_example' in user.mfa_modules - assert len(module._users) == 1 - assert module._users[0] == {'user_id': user.id, 'pin': 'test-pin'} + assert len(module._data) == 1 + assert module._data[0] == {'user_id': user.id, 'pin': 'test-pin'} + + # test get enabled mfa + modules = await manager.async_get_enabled_mfa(user) + assert len(modules) == 1 + assert 'insecure_example' in modules # re-enable mfa for user will override await manager.async_enable_user_mfa(user, 'insecure_example', {'pin': 'test-pin-new'}) - assert len(user.mfa_modules) == 1 - assert 'insecure_example' in user.mfa_modules - assert len(module._users) == 1 - assert module._users[0] == {'user_id': user.id, 'pin': 'test-pin-new'} + assert len(module._data) == 1 + assert module._data[0] == {'user_id': user.id, 'pin': 'test-pin-new'} + modules = await manager.async_get_enabled_mfa(user) + assert len(modules) == 1 + assert 'insecure_example' in modules # system user cannot enable mfa system_user = await manager.async_create_system_user('system-user') with pytest.raises(ValueError): await manager.async_enable_user_mfa(system_user, 'insecure_example', {'pin': 'test-pin'}) - assert len(module._users) == 1 + assert len(module._data) == 1 + modules = await manager.async_get_enabled_mfa(system_user) + assert len(modules) == 0 # disable mfa for user await manager.async_disable_user_mfa(user, 'insecure_example') - assert bool(user.mfa_modules) is False - assert bool(module._users) is False + assert bool(module._data) is False + + # test get enabled mfa + modules = await manager.async_get_enabled_mfa(user) + assert len(modules) == 0 # disable mfa for user don't enabled just silent fail await manager.async_disable_user_mfa(user, 'insecure_example') diff --git a/tests/common.py b/tests/common.py index 1843616810f753..a5c154a51c4549 100644 --- a/tests/common.py +++ b/tests/common.py @@ -315,11 +315,11 @@ class MockUser(auth_models.User): """Mock a user in Home Assistant.""" def __init__(self, id='mock-id', is_owner=False, is_active=True, - name='Mock User', system_generated=False, mfa_modules=[]): + name='Mock User', system_generated=False): """Initialize mock user.""" super().__init__( id=id, is_owner=is_owner, is_active=is_active, name=name, - system_generated=system_generated, mfa_modules=mfa_modules) + system_generated=system_generated) def add_to_hass(self, hass): """Test helper to add entry to hass.""" diff --git a/tests/components/config/test_auth.py b/tests/components/config/test_auth.py index bdf064945cfa41..fe8f351955f8a5 100644 --- a/tests/components/config/test_auth.py +++ b/tests/components/config/test_auth.py @@ -63,18 +63,6 @@ async def test_list(hass, hass_ws_client): is_active=False, ).add_to_hass(hass) - mfa = MockUser( - id='klm', - name='MFA Enabled User', - is_active=True, - mfa_modules=['insecure_example', 'invalid'] - ).add_to_hass(hass) - mfa.credentials.append(auth_models.Credentials( - auth_provider_type='insecure_example', - auth_provider_id='insecure_example', - data={}, - )) - refresh_token = await hass.auth.async_create_refresh_token( owner, CLIENT_ID) access_token = hass.auth.async_create_access_token(refresh_token) @@ -88,14 +76,13 @@ async def test_list(hass, hass_ws_client): result = await client.receive_json() assert result['success'], result data = result['result'] - assert len(data) == 4 + assert len(data) == 3 assert data[0] == { 'id': owner.id, 'name': 'Test Owner', 'is_owner': True, 'is_active': True, 'system_generated': False, - 'enabled_multi_factor_auth': [], 'credentials': [{'type': 'homeassistant'}] } assert data[1] == { @@ -104,7 +91,6 @@ async def test_list(hass, hass_ws_client): 'is_owner': False, 'is_active': True, 'system_generated': True, - 'enabled_multi_factor_auth': [], 'credentials': [], } assert data[2] == { @@ -113,18 +99,8 @@ async def test_list(hass, hass_ws_client): 'is_owner': False, 'is_active': False, 'system_generated': False, - 'enabled_multi_factor_auth': [], 'credentials': [], } - assert data[3] == { - 'id': mfa.id, - 'name': 'MFA Enabled User', - 'is_owner': False, - 'is_active': True, - 'system_generated': False, - 'enabled_multi_factor_auth': ['insecure_example', 'invalid'], - 'credentials': [{'type': 'insecure_example'}], - } async def test_delete_requires_owner(hass, hass_ws_client, hass_access_token):