Add Time-based Onetime Password Multi-factor Authentication Module by awarecan · Pull Request #16129 · home-assistant/core

awarecan · 2018-08-22T13:44:31Z

Description:

totp is Time-based One Time Password module which compatible with Google Authenticator and Authy.

See #15489 learn more about multi-factor authentication module

Edit: a totp MFA auth module named "Authenticator app" will be auto loaded if no auth_mfa_modules config section defined in configuration.yaml

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6089

Example entry for `configuration.yaml` (if applicable):

# configuration.yaml
homeassistant:
  # Auth Provider
  auth_providers:
    - type: homeassistant
  auth_mfa_modules:
    - type: totp

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Tests have been added to verify that the new code works.

houndci-bot · 2018-08-22T13:44:40Z

tests/auth/mfa_modules/test_totp.py

+
+    provider = hass.auth.auth_providers[0]
+
+    result = await hass.auth.login_flow.async_init((provider.type, provider.id))


line too long (80 > 79 characters)

houndci-bot · 2018-08-22T13:44:40Z

tests/auth/mfa_modules/test_totp.py

+"""Test the Time-based One Time Password (MFA) auth module."""
+from unittest.mock import patch
+
+import pytest


'pytest' imported but unused

houndci-bot · 2018-08-22T13:44:40Z

homeassistant/auth/mfa_modules/totp.py

@@ -0,0 +1,142 @@
+"""Time-based One Time Password auth module."""
+import logging
+from typing import Any, Dict, List, Optional


'typing.List' imported but unused
'typing.Optional' imported but unused

MartinHjelmare · 2018-08-22T18:40:12Z

homeassistant/auth/mfa_modules/totp.py

+                user[STORAGE_OTA_SECRET] = ota_secret
+                return ota_secret
+
+        self._users.append({  # type: ignore


Why should users be a list of dicts? It looks like we're assuming that user ids are unique so then a dict (of dicts) makes more sense to me.

Copied code from auth provider, should change to dict

MartinHjelmare · 2018-08-22T18:44:19Z

homeassistant/auth/mfa_modules/totp.py

+
+    def _validate_2fa(self, user_id: str, code: str) -> bool:
+        """Validate two factor authentication code."""
+        if user_id is None or code is None:


When can this happen? Code input is validated by the input schema.

MartinHjelmare · 2018-08-22T18:47:49Z

homeassistant/auth/mfa_modules/totp.py

+        if ota_secret is None:
+            # even we cannot find user, we still do verify
+            # to make timing the same as if user was found.
+            pyotp.TOTP(pyotp.random_base32()).verify(code)


Is the time for generating the ota_secret via random_base32 insignificant compared to the rest of the validation?

Change to a fixed dummy code

balloob · 2018-08-23T10:40:52Z

homeassistant/auth/__init__.py

+    async def async_get_enabled_mfa(self, user: models.User) -> Dict[str, str]:
        """List enabled mfa modules for user."""
-        module_ids = []
+        modules = {}


OrderedDict

balloob

I don't see anything related to setting up TOTP for a user in this PR besides a call to setup. Where is the schema?

See my PR on how this should look: https://github.com/home-assistant/home-assistant/pull/16141/files#r212261624

awarecan · 2018-08-23T13:55:42Z

PR order problem, the setup flow part need wait #16141 merged first.

awarecan · 2018-08-24T09:31:11Z

Rebase on #16141, add setup flow, add QR code generation

Need first merge #16141, then merge home-assistant/frontend#1590, then merge this PR, finally need add/fix translate to display QR code

MartinHjelmare · 2018-08-24T09:56:47Z

homeassistant/auth/mfa_modules/totp.py

+                 user: User) -> None:
+        """Initialize the setup flow."""
+        super().__init__(auth_module, user.id)
+        self._auth_module = auth_module  # type: TotpAuthModule


This is already set in the parent class.

For correct the stupid tying

I think it looks like we don't need the parent class. Do we really think the default init step will not be overridden in other mfa flows?

I think the inheritance is not needed as we're overriding everything.

MartinHjelmare · 2018-08-24T09:59:36Z

homeassistant/auth/mfa_modules/totp.py

+
+        if user_input:
+            verified = await self.hass.async_add_executor_job(  # type: ignore
+                pyotp.TOTP(self._ota_secret).verify, user_input['code'])


Shouldn't self._auth_module.async_validation be used here?

No, this is validate against a temp secret has not been saved yet.

balloob · 2018-08-24T12:51:33Z

homeassistant/auth/mfa_modules/totp.py

+    import pyqrcode
+
+    qr_code = pyqrcode.create(data)
+    return str(qr_code.png_as_base64_str(scale=4))


Use the svg method and we don't need to install pypng as dependency.

balloob · 2018-08-24T12:54:16Z

homeassistant/auth/mfa_modules/totp.py

+    return str(qr_code.png_as_base64_str(scale=4))
+
+
+@bind_hass


No need to add @bind_hass for internal methods.

balloob · 2018-08-24T12:54:35Z

homeassistant/auth/mfa_modules/totp.py

+        image = _generate_qr_code(url)
+        return ota_secret, url, image
+
+    return await hass.async_add_executor_job(


If the whole method is about running a sync method, just make the method sync on the global scope and call with async_add_executor_job directly.

balloob · 2018-08-24T12:59:46Z

homeassistant/components/auth/strings.json

+      "step": {
+        "init": {
+          "title": "Scan this QR code with your app",
+          "description": "Scan the QR code with your authentication app, such as **Google Authenticator** or **Authy**. \nOr enter {code} instead. \n\n[QR Code]{url}",


Markdown image syntax is ![alt text](url). I don't see that in the description?

Add TOTP setup flow, generate QR code

awarecan · 2018-08-24T18:18:56Z

The only thing left is translation string. I am not familiar with the process, @balloob can you take care of it?

balloob · 2018-08-25T09:05:19Z

I will but won't have time today. Will do it tomorrow.

Here is an example of using placeholders btw

https://github.com/home-assistant/home-assistant/blob/456aa5a2b227ee4553b6d302a8a194fccff1c5f1/homeassistant/components/nest/config_flow.py#L127-L136

https://github.com/home-assistant/home-assistant/blob/26a485d43c8ba2bafa2e989ab65524fc427416ad/homeassistant/components/nest/strings.json#L14

awarecan · 2018-08-25T19:24:30Z

Finally get this work. There is a change in front end need to be done. We are using data:image to embed image in markdown, however it will be filter out by filterXSS. Filter out data:image is sort of core function provided by js-xss lib, there is no easy way to workaround it.

So, I have to disable filterXSS to make my QR code displayed. I think the properly way to handle it (see front end PR: home-assistant/frontend#1599), maybe we should make filterXSS as an option in ha-markdown component, and disable it in config flow, and this mfa setup flow, since we can tightly control the markdown string be parsed in those flow, we don't have XSS concern for it. But it may help on other use cases.

My quick fix in front end to get following screenshot.

diff --git a/src/components/ha-markdown.js b/src/components/ha-markdown.js
index 355507ec..e13f1b4b 100644
--- a/src/components/ha-markdown.js
+++ b/src/components/ha-markdown.js
@@ -46,11 +46,11 @@ class HaMarkdown extends EventsMixin(PolymerElement) {
       this._renderScheduled = false;
 
       if (this._scriptLoaded === 1) {
-        this.innerHTML = this.filterXSS(this.marked(this.content, {
+        this.innerHTML = this.marked(this.content, {
           gfm: true,
           tables: true,
           breaks: true
-        }));
+        });
         this._resize();
 
         const walker = document.createTreeWalker(this, 1 /* SHOW_ELEMENT */, null, false);

houndci-bot · 2018-08-26T14:43:54Z

homeassistant/auth/mfa_modules/totp.py

@@ -0,0 +1,213 @@
+"""Time-based One Time Password auth module."""
+import logging
+from base64 import b64encode


'base64.b64encode' imported but unused

…16129) * Add Time-based Onetime Password Multi-factor Auth Add TOTP setup flow, generate QR code * Resolve rebase issue * Use svg instead png for QR code * Lint and typing * Fix translation * Load totp auth module by default * use <svg> tag instead markdown image * Update strings * Cleanup

…ome-assistant#16129) * Add Time-based Onetime Password Multi-factor Auth Add TOTP setup flow, generate QR code * Resolve rebase issue * Use svg instead png for QR code * Lint and typing * Fix translation * Load totp auth module by default * use <svg> tag instead markdown image * Update strings * Cleanup

homeassistant added cla-signed core labels Aug 22, 2018

ghost assigned awarecan Aug 22, 2018

ghost added the in progress label Aug 22, 2018

houndci-bot reviewed Aug 22, 2018

View reviewed changes

awarecan force-pushed the mfa-totp branch from 2396eb7 to 047d9b1 Compare August 22, 2018 13:47

homeassistant added the cla-signed label Aug 22, 2018

awarecan force-pushed the mfa-totp branch from 047d9b1 to 5fb3034 Compare August 22, 2018 15:02

homeassistant added the cla-signed label Aug 22, 2018

MartinHjelmare reviewed Aug 22, 2018

View reviewed changes

balloob reviewed Aug 23, 2018

View reviewed changes

balloob requested changes Aug 23, 2018

View reviewed changes

awarecan force-pushed the mfa-totp branch from 0094b7a to 1a2a2a8 Compare August 24, 2018 09:26

homeassistant added the cla-signed label Aug 24, 2018

MartinHjelmare reviewed Aug 24, 2018

View reviewed changes

balloob reviewed Aug 24, 2018

View reviewed changes

awarecan added 2 commits August 24, 2018 10:35

Add Time-based Onetime Password Multi-factor Auth

37842c2

Add TOTP setup flow, generate QR code

Resolve rebase issue

1911abb

awarecan force-pushed the mfa-totp branch from 9fe4d2a to 1911abb Compare August 24, 2018 17:44

homeassistant added the cla-signed label Aug 24, 2018

awarecan added 2 commits August 24, 2018 11:08

Use svg instead png for QR code

145aefe

Lint and typing

a2cfebc

awarecan added this to the 0.77 milestone Aug 24, 2018

Fix translation

06e015f

homeassistant added the cla-signed label Aug 25, 2018

Load totp auth module by default

b98f65d

awarecan requested a review from a team as a code owner August 25, 2018 22:52

homeassistant added the cla-signed label Aug 25, 2018

awarecan mentioned this pull request Aug 26, 2018

Disable XSS filter in flow step description markdown home-assistant/frontend#1599

Merged

houndci-bot reviewed Aug 26, 2018

View reviewed changes

use <svg> tag instead markdown image

535abf0

awarecan force-pushed the mfa-totp branch from c3f699d to 535abf0 Compare August 26, 2018 16:42

homeassistant added the cla-signed label Aug 26, 2018

Update strings

a8b4ef5

ghost assigned balloob Aug 26, 2018

Cleanup

dabafbe

homeassistant added the cla-signed label Aug 26, 2018

balloob approved these changes Aug 26, 2018

View reviewed changes

balloob merged commit 47755fb into home-assistant:dev Aug 26, 2018

ghost removed the in progress label Aug 26, 2018

balloob added the cherry-picked label Aug 26, 2018

awarecan mentioned this pull request Aug 26, 2018

Add TOTP MFA docs home-assistant/home-assistant.io#6089

Merged

2 tasks

balloob mentioned this pull request Aug 29, 2018

0.77 #16256

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018


		provider = hass.auth.auth_providers[0]

		result = await hass.auth.login_flow.async_init((provider.type, provider.id))

Uh oh!

Conversation

awarecan commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awarecan commented Aug 23, 2018

Uh oh!

awarecan commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awarecan commented Aug 24, 2018

Uh oh!

balloob commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awarecan commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

awarecan commented Aug 22, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

balloob left a comment •

edited

Loading

balloob commented Aug 25, 2018 •

edited

Loading

awarecan commented Aug 25, 2018 •

edited

Loading