core icon indicating copy to clipboard operation
core copied to clipboard

Bypass validation empty code for lock

Open gjohansson-ST opened this issue 2 years ago • 1 comments

Proposed change

PR #85842 implemented a validation for lock which provides a problem in two use-cases

  • Can no longer use a default code configured by the user
  • Locks where code is only required one-way (unlock but not lock) the user know need to provide a code anyway

Change so validation is only tested if code is provided, otherwise leave it to integration to do necessary checks (as mentioned above).

Type of change

  • [ ] Dependency upgrade
  • [x] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #87474 fixes #86963
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

gjohansson-ST avatar Feb 06 '23 12:02 gjohansson-ST

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (lock) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of lock can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign lock Removes the current integration label and assignees on the issue, add the integration domain after the command.

home-assistant[bot] avatar Feb 06 '23 12:02 home-assistant[bot]

Meanwhile we have discussions in the architecture repo should we merge this one as current implementation breaks functionality for several integrations? I would not like to lose the validation but reverting #85842 is also an option obviously

gjohansson-ST avatar Feb 12 '23 20:02 gjohansson-ST

@gjohansson-ST set this to draft until is clear if this is the way to go.

jbouwh avatar Feb 16 '23 11:02 jbouwh

Let us wait for the architecture discussion.

When is this planned? If it's a long time away, would you consider accepting this merge as a temporary fix until then? If you decide to revert the previous change you can always revert these commits too.

And if you do decide to go along this path then a fix is already implemented and the users hit by this in the linked issues won't be affected longer than necessary.

konstantinozh avatar Feb 19 '23 13:02 konstantinozh

Let us wait for the architecture discussion.

When is this planned? If it's a long time away, would you consider accepting this merge as a temporary fix until then? If you decide to revert the previous change you can always revert these commits too.

And if you do decide to go along this path then a fix is already implemented and the users hit by this in the linked issues won't be affected longer than necessary.

I do not believe a fix is needed.

jbouwh avatar Feb 19 '23 18:02 jbouwh

Why would i want to provide a code to lock the door which is now the case? This means i would have to provide the default code to the system. By that HA would have the "keys" to my house. I dont quite like that. Obviously opening the door is different and more critical. By this the automations that close the doors dont work. I love the HA and all the capabilities but now one of the very core functions is broken in my opinion with this.

TohtoriR avatar Feb 20 '23 22:02 TohtoriR

Still cant lock my door from HA. This should not happen and for sure it should nok take weeks to revert or fix.

crackers81 avatar Feb 21 '23 21:02 crackers81

Still cant lock my door from HA. This should not happen and for sure it should nok take weeks to revert or fix.

Please do not use this PR to log issues, open an issue instead.

jbouwh avatar Feb 22 '23 04:02 jbouwh

Still cant lock my door from HA. This should not happen and for sure it should nok take weeks to revert or fix.

Please do not use this PR to log issues, open an issue instead.

Already done that. Still No answer from anybody. https://github.com/home-assistant/core/issues/86963

crackers81 avatar Feb 22 '23 05:02 crackers81

I do not believe a fix is needed.

Well, the case right now is that users are presented with a GUI that lets them add a default code for their lock entities, and then when they try to lock/unlock these entities an exception is thrown. The same happens for integrations outside HA, for example voice assistants like Google Assistant. If you give Google Assistant access to the entity it will be fail to lock/unlock unless you explicitly set up an routine to call an automation (and this is not the default way Google Assistant handles the lock entity).

I think the code validation should be added, but as a deprecation, and if there are discussions concerning what architectural direction this should be taken (bypass code vs storing the default code in the entity itself) the breaking change should be reverted in the meantime as to not impact users while this is decided.

Anyway, I have found the discussion threads in the architecture repo so I'll follow along there. I agree this PR is not the place for this discussion.

konstantinozh avatar Feb 22 '23 14:02 konstantinozh

I don't think the suggested solution takes into consideration that there is also an issue with codes consisting of > 4 digits. My error reads: "Call-service error. Code '123456' for locking lock.myYaleDoormanLock doesn't match pattern ^\d{4}$"

Verisure forces users to use 6 digit codes for Yale Doorman locks. A six digit code will not match the pattern above and as far as I understand this use case is not covered by the proposed change in this PR.

MathiasThinsz avatar Mar 10 '23 15:03 MathiasThinsz

I don't think the suggested solution takes into consideration that there is also an issue with codes consisting of > 4 digits. My error reads: "Call-service error. Code '123456' for locking lock.myYaleDoormanLock doesn't match pattern ^\d{4}$"

Verisure forces users to use 6 digit codes for Yale Doorman locks. A six digit code will not match the pattern above and as far as I understand this use case is not covered by the proposed change in this PR.

Just purchased a very cheap Yale V2N and connected it through the Yale Smart Living integration. When I first setup the hub i made a 4-digit 1234 code for verification which worked, but then I updated it and was required to use a 6-digit code, which now puts me in the same scenario as you and I am not able to unlock through HA.

MrNews89 avatar Mar 12 '23 13:03 MrNews89

I don't think the suggested solution takes into consideration that there is also an issue with codes consisting of > 4 digits. My error reads: "Call-service error. Code '123456' for locking lock.myYaleDoormanLock doesn't match pattern ^\d{4}$" Verisure forces users to use 6 digit codes for Yale Doorman locks. A six digit code will not match the pattern above and as far as I understand this use case is not covered by the proposed change in this PR.

Just purchased a very cheap Yale V2N and connected it through the Yale Smart Living integration. When I first setup the hub i made a 4-digit 1234 code for verification which worked, but then I updated it and was required to use a 6-digit code, which now puts me in the same scenario as you and I am not able to unlock through HA.

If the code should be 6 digits and code_format is set to ^\d{4}$ this means validation is done on 4 digits. The code format can be changed to accept both formats, e.g. ^\d{4,6}$ accepts all numeric codes of 4, 5 or 6 digits. That is not a reason to bypass, Pleas keep these discussions out of the PR, and open an issue for the integration causing issues instead.

jbouwh avatar Mar 12 '23 13:03 jbouwh

Closing this as it's clear we will not add it

gjohansson-ST avatar May 07 '23 18:05 gjohansson-ST