-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Use cookie for authentication #167
Conversation
@Cruguah one of the main reasons why this PR didn't get merged yet (even as pre release / test) is because this will break the authentication for current users that are still using the auth token method. I am looking if we can combine this and only update the Config Flow for new users. |
CI/CD failing on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some comments to get the review started. I will try to make some time to test this PR myself as well asap, so we can start merging this in.
.vscode/settings.json
Outdated
@@ -1,9 +1,30 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert the changes here? They don't seem necessary for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks a lot and good to see all checks are green. I will see if we can merge this in a beta branch, to keep this code in this repository as well. In order to be merged to main, the following issues would need to be fixed first:
beta (cookie-method)
|
@iMicknl, I've implemented a reconfiguration option to renew the issue_token and cookie. Would you be so kind to update you HA instance with this version to see if that works? You can reinstall the nest integration using HACS by reinstalling the current integration, Open the integration and click on: Then you can find the (re)configure option in the integration: Hope this helps! |
Would be nice if this could finally be merged so that cookie auth can be utilised. |
Thanks @Cruguah. Must admit I'm confused by the versioning/forks of this PR/integration (and confused about correct terminology!), but for the record I'm currently using the fork at https://github.com/nicoinch/ha-nest-protect (re-installed it 1-2 weeks ago). For the benefit of anyone else reading this now/in the future: the above fork continued to work (with cookie auth) after upgrading past 2023.02 (to 2023.04.05). The warning "Detected integration that called async_setup_platforms instead of awaiting async_forward_entry_setups; this will fail in version 2023.3" persists in the logs, but it doesn't appear to have actually failed / broken anything obvious (confirmed at least the occupancy sensors from my Nest Protects continue to update properly in HA). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't edit your branch unfortunately, so trying to fix your linting issues via interface..
@@ -1,26 +1,30 @@ | |||
"""Adds config flow for Nest Protect.""" | |||
from __future__ import annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from __future__ import annotations | |
from __future__ import annotations | |
@@ -1,26 +1,30 @@ | |||
"""Adds config flow for Nest Protect.""" | |||
from __future__ import annotations | |||
|
|||
from typing import Any, cast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from typing import Any, cast | |
from typing import Any, cast | |
from .pynest.client import NestClient | ||
from .pynest.const import NEST_ENVIRONMENTS | ||
from .pynest.exceptions import BadCredentialsException | ||
import voluptuous as vol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import voluptuous as vol |
.github/workflows/pre-commit.yml
Outdated
@@ -2,7 +2,7 @@ name: Linters (flake8, black, isort) | |||
|
|||
on: | |||
pull_request: | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cruguah could you unprotect your main branch? You did your PR from the main branch, and thus I cannot help you and make changes. My proposal would be to fix this PR, bring it to beta branch and release this one as prerelease, so that HACS users can easily install and we can do version management. |
I just did it! |
@Cruguah not yet "You can’t commit to main because it is a protected branch". You can override this as admin, but you would need to remove the protection (or do the PR from another branch), for me to be able to help. |
Ok, this looks better! |
@Cruguah see |
Yep, and installed this beta version. |
I tagged all issues with https://github.com/iMicknl/ha-nest-protect/issues?q=is%3Aissue+is%3Aopen+label%3A%22beta+%28cookie-method%29%22, so that we can easily track which issues we need to resolve to merge to main. |
My Nest Protect integration (2) were broken since 2023.6.x but now HA is on 2023.7.1. Is the use of the BETA 0.4.0b1 still valid for my version of HA? |
@m4v3r1ckNl you are using the fork, not my version if it was broken. Remove the custom repository from HACS and install beta 0.4.0b2. |
Thank you @iMicknl for your reply, but I'm not sure which version (forked) I installed. I used your cookie auth method on my instance of Nest Protect. Please see my screencap. All was functioning 100%. |
@m4v3r1ckNl please create a new issue if you face issues. However, if you follow my instructions (remove the custom repository), it should be fixed. |
Thank you @iMicknl I'll give that a try, the current integration is broken anyway. Cheers |
Hi @iMicknl Up-and-running again. Deleted the nest_protect under custom components and reinstalled the beta 0.4.0b2 today. Thank you for your help! |
This pull request:
adds issue_token / cookies authentication + tests
fixes the is_online key