Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ask users for a pin when interacting with locks/garage doors #23223

Merged
merged 2 commits into from Apr 19, 2019

Conversation

Projects
None yet
7 participants
@balloob
Copy link
Member

commented Apr 19, 2019

Breaking Change:

The allow_unlock option has been removed for both Cloud and manual Google Assistant installations. Instead, you need to define a pin. Google Cloud users can do this in the cloud preferences UI. Manual installation will have to add secure_devices_pin to their config.

Description:

This replaces the allow_unlock toggle with the official approach to interacting with secure devices via Google Assistant: 2FA using a pin challenge.

This change has been requested by Google hence tagged for 0.92.

Related issue (if applicable): fixes #23219

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@codeowners-notifier

This comment has been minimized.

Copy link

commented Apr 19, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (cloud) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@balloob balloob added this to the 0.92.0 milestone Apr 19, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 19, 2019

Codecov Report

Merging #23223 into dev will decrease coverage by <.01%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #23223      +/-   ##
==========================================
- Coverage   94.17%   94.17%   -0.01%     
==========================================
  Files         457      457              
  Lines       37095    37126      +31     
==========================================
+ Hits        34935    34962      +27     
- Misses       2160     2164       +4
Impacted Files Coverage Δ
homeassistant/components/cloud/client.py 87.25% <ø> (ø) ⬆️
...meassistant/components/google_assistant/helpers.py 96.15% <100%> (+0.11%) ⬆️
homeassistant/components/cloud/http_api.py 99% <100%> (-0.01%) ⬇️
homeassistant/components/cloud/prefs.py 98.11% <100%> (ø) ⬆️
homeassistant/components/google_assistant/http.py 94.28% <100%> (+0.16%) ⬆️
homeassistant/components/cloud/const.py 100% <100%> (ø) ⬆️
...ssistant/components/google_assistant/smart_home.py 91.2% <100%> (ø) ⬆️
homeassistant/components/google_assistant/const.py 100% <100%> (ø) ⬆️
homeassistant/components/google_assistant/error.py 92.3% <88.88%> (-7.7%) ⬇️
homeassistant/components/google_assistant/trait.py 96.03% <89.28%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b4e24...ac42076. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

commented Apr 19, 2019

Codecov Report

Merging #23223 into dev will decrease coverage by <.01%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #23223      +/-   ##
==========================================
- Coverage   94.17%   94.17%   -0.01%     
==========================================
  Files         457      457              
  Lines       37095    37126      +31     
==========================================
+ Hits        34935    34962      +27     
- Misses       2160     2164       +4
Impacted Files Coverage Δ
homeassistant/components/cloud/client.py 87.25% <ø> (ø) ⬆️
...meassistant/components/google_assistant/helpers.py 96.15% <100%> (+0.11%) ⬆️
homeassistant/components/cloud/http_api.py 99% <100%> (-0.01%) ⬇️
homeassistant/components/cloud/prefs.py 98.11% <100%> (ø) ⬆️
homeassistant/components/google_assistant/http.py 94.28% <100%> (+0.16%) ⬆️
homeassistant/components/cloud/const.py 100% <100%> (ø) ⬆️
...ssistant/components/google_assistant/smart_home.py 91.2% <100%> (ø) ⬆️
homeassistant/components/google_assistant/const.py 100% <100%> (ø) ⬆️
homeassistant/components/google_assistant/error.py 92.3% <88.88%> (-7.7%) ⬇️
homeassistant/components/google_assistant/trait.py 96.03% <89.28%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b4e24...ac42076. Read the comment docs.

@pvizeli
Copy link
Member

left a comment

google_secure_devices_pin is very long, maybe google_devices_pin would be enough to hold the code short

@balloob balloob merged commit 0533f56 into dev Apr 19, 2019

12 of 13 checks passed

codecov/patch 93.54% of diff hit (target 94.17%)
Details
build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/project 94.17% (target 90%)
Details

@delete-merged-branch delete-merged-branch bot deleted the google-ass-challenge branch Apr 19, 2019

balloob added a commit that referenced this pull request Apr 20, 2019

Ask users for a pin when interacting with locks/garage doors (#23223)
* Ask users for a pin when interacting with locks/garage doors

* Deprecate allow_unlock option

mxworm added a commit to mxworm/home-assistant that referenced this pull request Apr 21, 2019

Merge branch 'dev' into current
* dev:
  Upgrade pyotp to 2.2.7 (home-assistant#23274)
  Upgrade xmltodict to 0.12.0 (home-assistant#23277)
  Add ctags file to .gitignore (home-assistant#23279)
  Bump zigpy and zigpy-xbee (home-assistant#23275)
  Bump zigpy-deconz (home-assistant#23270)
  Update pyheos and log service errors in HEOS integration (home-assistant#23222)
  Updated frontend to 20190419.0
  Return 0 instead of None (home-assistant#23261)
  Drop unnecessary block_till_done (home-assistant#23256)
  Drop unnecessary block_till_done, improve tests for MQTT Cover tests (home-assistant#23255)
  Drop unnecessary block_till_done for MQTT tests (home-assistant#23254)
  Drop unnecessary block_till_done for MQTT fan tests (home-assistant#23253)
  Added component named switcher_kis switcher water heater integration. (home-assistant#22325)
  Add missing services.yaml file for hue (home-assistant#23217)
  Drop unnecessary block_till_done, improve tests (home-assistant#23252)
  Drop unnecessary block_till_done (home-assistant#23251)
  Ask users for a pin when interacting with locks/garage doors (home-assistant#23223)
  Drop unnecessary block_till_done (home-assistant#23250)
  Drop unnecessary block_till_done, improve tests (home-assistant#23249)
  Drop unnecessary block_till_done, improve tests (home-assistant#23248)
@FutureTense

This comment has been minimized.

Copy link

commented Apr 29, 2019

This change has been requested by Google hence tagged for 0.92.

@balloob is google requiring this change? While the feature is nice to have, I’m not a fan of being forced to use a pin. Is there a way to make the pin optional?

Regardless, how is one supposed to use the pin? What is the syntax to unlock “Front Door” with pin 1234?

OK Google, unlock the front door 1234

@brianjamescarter

This comment has been minimized.

Copy link

commented May 5, 2019

has anyone got this to work? my google just asks for the code then says, "I looked but this can't be played at this time"

this is beyond frustrating thanks google

@brmo

This comment has been minimized.

Copy link

commented May 6, 2019

I got it to work just by adding my cover.garage_door entity to the include_entities section. However even after setting a pin, it never asks me for it, but rather carries on doing the action. Security flaw there.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators May 6, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.