Add multi-factor auth module setup flow#16141
Conversation
ghost
assigned
awarecan
force-pushed
the
profile-mfa-v2
branch
from
da54c6f to
92e6abc
Compare
| """Get current user.""" | ||
| enabled_modules = await hass.auth.async_get_enabled_mfa(user) | ||
|
|
||
| connection.to_write.put_nowait( |
There was a problem hiding this comment.
This should be connection.send_message_outside(
| async def async_setup_flow(msg): | ||
| """Helper to return a setup flow for mfa auth module.""" | ||
| flow_manager = hass.data.get(DATA_SETUP_FLOW_MGR) | ||
| if flow_manager is None: |
There was a problem hiding this comment.
Changed init order, not possible now
| if flow_id is not None: | ||
| result = await flow_manager.async_configure( | ||
| flow_id, msg.get('user_input')) | ||
|
|
There was a problem hiding this comment.
Inside this if, add the write and return. That way you can drop the else
| mfa_module_id = msg.get('mfa_module_id') | ||
| mfa_module = hass.auth.get_auth_mfa_module(mfa_module_id) | ||
| if mfa_module is None: | ||
| connection.to_write.put_nowait(websocket_api.error_message( |
There was a problem hiding this comment.
All these writes need to be connection.send_message_outside(
| websocket_api.result_message( | ||
| msg['id'], _prepare_result_json(result))) | ||
|
|
||
| hass.async_add_job(async_setup_flow(msg)) |
| connection.to_write.put_nowait(websocket_api.error_message( | ||
| msg['id'], 'no_user', 'Not authenticated as a user')) | ||
| return | ||
| if user.system_generated: |
There was a problem hiding this comment.
Wonder if we should add this in a decorator.
| ' {}: {}'.format(mfa_module_id, err))) | ||
| return | ||
|
|
||
| connection.to_write.put_nowait( |
|
|
||
| return self.async_show_form( | ||
| step_id='init', | ||
| data_schema=self._auth_module.setup_schema, |
There was a problem hiding this comment.
I think that all MFA modules need to have their own flows instead of a generic flow wrapped around a setup_schema property. Because in the case of TOTP, the flow will need to be:
- Start login flow
- Login flow generates a secret and stores it on instance of login flow
- Returns show_form with a QR code in the description (injected as base64 via
description_placeholder) - User scans code and enters a code to verify it scanned it correctly
- TOTP module is enabled for user
This can only be done if each MFA module has their own setup flow.
Example of how GitHub does this:
There was a problem hiding this comment.
Generic SetupFlow is default implement, other module can implement its own if need.
There was a problem hiding this comment.
Okay, in that case we should not have it be the default method, because now we are requiring an extra property setup_schema to be optionally defined.
Instead, if an MFA module wants to use the default config flow, they can just do:
setup_schema = vol.Schema({})
return SetupFlow(self.name, setup_schema, user_id)That way we keep the API for MFA modules simpler while providing clean API that is also similar to Auth Providers.
awarecan
force-pushed
the
profile-mfa-v2
branch
from
39df36f to
409a76d
Compare
| """Return a data entry flow handler for setup module. | ||
|
|
||
| Optional | ||
| Mfa module should extend SetupFlow |
There was a problem hiding this comment.
Edit: it is required. MFA module has to be set up to enable.
| """Handle the first step of setup flow. | ||
|
|
||
| Return self.async_show_form(step_id='init') if user_input == None. | ||
| Return await self.async_finish(flow_result) if finish. |
|
|
||
| DOMAIN = 'auth' | ||
| DEPENDENCIES = ['http'] | ||
| DEPENDENCIES = ['http', 'websocket_api'] |
There was a problem hiding this comment.
Websocket API commands can be registered without loading the websocket API. It is not a dependency.
If the user wants the websocket API, the commands will be available.
| from homeassistant.core import HomeAssistant | ||
|
|
||
|
|
||
| def validate_current_user( |
There was a problem hiding this comment.
This should have ws in the name. What about ws_require_user ?
There was a problem hiding this comment.
I think that this method should be part of the websocket component.
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
Few minor comments. Ok to merge when addressed.
awarecan
force-pushed
the
profile-mfa-v2
branch
from
3261666 to
58e6b4a
Compare
* Add mfa setup flow * Lint * Address code review comment * Fix unit test * Add assertion for WS response ordering * Missed a return * Remove setup_schema from MFA base class * Move auth.util.validate_current_user -> webscoket_api.ws_require_user
4 participants
Description:
Add websocket command to setup or remove current user for mfa module
Frontend: home-assistant/frontend#1590
Related issue (if applicable): fixes #
Pull request in developers.home-assistant with documentation (if applicable): home-assistant/developers.home-assistant#79
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices: