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

More options for config flow #124

Closed
leikoilja opened this issue Mar 30, 2021 · 24 comments · Fixed by #141
Closed

More options for config flow #124

leikoilja opened this issue Mar 30, 2021 · 24 comments · Fixed by #141
Assignees
Labels
feature New feature or request

Comments

@leikoilja
Copy link
Owner

Is your feature request related to a problem? Please describe.
We currently only ask for login and password on config flow. However some people might have issue with BadAuthentication cause by different custom components as #95 for example. As an alternative solution we could provide master_token input on config UI.
Also we discuss to have error catching for which we need to provide an option to 'opt out' from diagnostic data collection.

Describe the solution you'd like
More versatile config flow UI that allows to enter master token and opt in / opt out from diagnotic data collection

@leikoilja leikoilja added the feature New feature or request label Mar 30, 2021
@leikoilja leikoilja self-assigned this Mar 30, 2021
@KapJI
Copy link
Collaborator

KapJI commented Mar 30, 2021

I'd wait before introducing login with master_token. I think we can fix gpsoauth to be more reliable, its contributor seems to be open for improvements.

If even with this we will have BadAuthentication issues then we can add master_login in config flow.

@leikoilja
Copy link
Owner Author

You it not be an extra useful feature to have independent of gpsoatuth package? :D

@KapJI
Copy link
Collaborator

KapJI commented Mar 30, 2021

I think we should try to avoid to expose low level details to users.

@LEJOUI
Copy link

LEJOUI commented Mar 31, 2021

if I might do another suggestion. Not sure if it’s possible though.
It would be nice if you could select which devices from GH you want to integrate. Now it just imports all of them.

@leikoilja
Copy link
Owner Author

@LEJOUI, of course, you are welcomed for suggestions, thanks for bringing it up to a table 💥

We have spoken about it earlier, however, got discouraged to do so since there are soooooo many different devices that may support Google Assistant that it would quickly get painful to maintain that list.
However, now that I think about it... there must be some ways we can still support it.
Let me just put some thoughts down (without considering how hard/out-of-place it would be to implement them):

  • we could do an initial scan after one enters his/her google credentials to discover all devices in the local network and then show another "config UI popup" where a person can select/deselect devices/
  • could make a blacklist device selector on the config flow UI where one can select devices that he does not want to be integrated. That would enable us to maintain only "blacklist" devices, which hopefully would be a much smaller list of devices (like chromecast for example)
  • could allow an optional field on the config flow UI for manually inputting device wildcard(s) (or just a list of static IP addresses). That list would take priority over "discover all devices" and only discover and use those devices that the user has manually specified. This approach would probs be for power users alone :D

What do you guys think?

@KapJI
Copy link
Collaborator

KapJI commented Mar 31, 2021

I think the original question is not about supported devices list but about being able to select which supported devices to add.

So this can be a dialog with checkboxes during the config flow if you want to add all or only some devices. Like your first option.

But I'm not convinced if we need this because HA already provides mechanisms to disable devices on integration level or individual entities. Is any of the components in HA core provides such functionality?

Disable device

Screenshot 2021-03-31 at 09 06 03

Disable entity

Screenshot 2021-03-31 at 09 08 26

@LEJOUI
Copy link

LEJOUI commented Mar 31, 2021

Yes it's indeed that. You could disable the entity in HA.
In my case I have a Shield which isn't on all the time. So whenever it's off and also disabled in HA I still get flooded by logs:

2021-03-31 10:59:33 ERROR (MainThread) [custom_components.google_home] Request error: Cannot connect to host 192.168.XXX.XX:8443 ssl:default [None]

Logger: custom_components.google_home
Source: custom_components/google_home/api.py:162
Integration: Google Home (documentation, issues)
First occurred: 30 maart 2021 9:03:53 (4754 occurrences)
Last logged: 11:18:53

Request error: Cannot connect to host 192.168.XXX.XX:8443 ssl:default [Connect call failed ('192.168.XXX.X', 8443)]
Request error: Cannot connect to host 192.168.XXX.XX:8443 ssl:default [Connect call failed ('192.168.XXX.XX', 8443)]
Request error: [Errno 104] Connection reset by peer
Request error: Cannot connect to host 192.168.XXX.XX:8443 ssl:default [None]
Request error: Cannot connect to host 192.168.XXX.XX:8443 ssl:default [Connect call failed ('192.168.XXX.XX', 8443)]

Therefore I do think choosing what kind of devices you want to import is nice to have.

Edit: added some more logs

@KapJI
Copy link
Collaborator

KapJI commented Mar 31, 2021

Well, I think we need to fix this and don't show error messages for devices which are just turned off 🙂

@leikoilja
Copy link
Owner Author

It's most likely because we "cache" the devices and try fetching their related data even though the device is off, so as @KapJI has said it's probably better we fix the log error message and using in-build mechanisms for entity disabling (@KapJI, i m on the go, could you please create a separate issue for that?)

@KapJI
Copy link
Collaborator

KapJI commented Mar 31, 2021

#128

@coleya
Copy link

coleya commented Sep 17, 2021

I think it would be a useful option to allow the user to specify a master_token instead of the account username and app password as is suggested as best security practice in the glocaltokens readme. Having to store my Google account credentials on the home assistant server is the main thing keeping me from using this extension so any way around that is appreciated, this was just what I came up with to maybe achieve that.

@DurgNomis-drol
Copy link
Collaborator

@coleya Storing the master_tokenin plain text, as it is done in HA, is even more dangerous. The master_token have the ability to gain access to feature you have not even enabled on your account og you have disabled.

@leikoilja
Copy link
Owner Author

leikoilja commented Sep 17, 2021

@coleya this is a good suggestion.
@DurgNomis-drol you are right. But also is it not how HA is treating all other logic around it? I would expect these all credentials being encrypted or smth 🤔

The glocaltokens client already mostly supports authentication simply with master_token, so the only thing left is to expand the functionality of ha-google-home config_flow to support master token authentication without the need to login and password. I ll create corresponding issues so we can discuss it in both repos so we can easily track and pick it up.

leikoilja/glocaltokens#168
#339

@DurgNomis-drol
Copy link
Collaborator

HA does NOT encrypt anything as far as I know. 😬😬

I am neither for or against @coleya suggestion, i just wanted to point out that it won't necessarily make this more secure. 😊

@coleya
Copy link

coleya commented Sep 17, 2021

I am unsure about the authorization granted by the master_token but the app specific passwords can be used to send email which is the main point of concern for me.

@DurgNomis-drol
Copy link
Collaborator

@coleya ``master_token` can do literally anything to your account. It is a bad design, but that is how Google Designed it unfortunately. I would be nice if Google could make it possible to limit what the app password can do.

@coleya
Copy link

coleya commented Sep 17, 2021

So since the app specific passwords are revokable this would actually be worse so I guess I won't be using this if you implement it.

@DurgNomis-drol
Copy link
Collaborator

DurgNomis-drol commented Sep 17, 2021

The integration stores it anyway right now, so i won't change anything. The only reason i made the post before (And I should probably have made that more clear) was so that you didn't get lulled into a false sense of security.

@coleya
Copy link

coleya commented Sep 17, 2021

I'm glad you did, I probably won't use the integration unless I can figure out a clean way of determining who set a particular timer (this would make it worth the risk) because of the inherent security risk. I wrongly assumed that the google account security model would be more than this one token. Hopefully one day google will provide a proper api for this data and we can use tokens like for everything else.

@DurgNomis-drol
Copy link
Collaborator

DurgNomis-drol commented Sep 17, 2021

Clarification:

You still access the API through the access_token, but you use the master_token to fetch this. access_token expires after an hour. To access each device, you use the access_token, to fetch the cast-local-authorization-token which lives about a day. The last one is then used to connect to the device. Also the master_token changes if you change the password.

@coleya
Copy link

coleya commented Sep 17, 2021

Since the token changes when you change your password I assume it's tied to the app specific password it was generated with (which would have become apparent to me when I revoked it immediately after generating it which was my plan). If that's true it's even less important of a difference between the token and the password than I realized. If the token provides full control of the account then the only security using it instead would bring would be through obscurity (kinda ruined that by posting on github).

@DurgNomis-drol
Copy link
Collaborator

The authentication flow for Google is well known secret. What makes it secure is simply having a good password. There are also some more details that I haven't listed in my previous post surrounding how Google authenticates and so forth.

@leikoilja
Copy link
Owner Author

leikoilja commented Sep 17, 2021

One of the cool ideas I heard from @KapJI the other day is to de-couple your google speakers / automations from your google account. Create a new secure google account for home automation things and simply add it as a "user" to your google home. That will allow your 2nd account full access to google devices without compromising the security of your main google account :)

@KapJI
Copy link
Collaborator

KapJI commented Sep 17, 2021

This is how I use it since day 1. The 2nd account doesn't have access to anything other than Google Home, so even if it's hacked, it's not a huge concern for me.

@coleya Of course it's up to you if you want to use this integration. But as others said master token has the same power as username and password, so authenticating with the token doesn't really change anything but will make auth process more complex. And yes, it invalidates when you change the password.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants