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

Fix Abode integration needing to reauthenticate after core update #123035

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

krx
Copy link

@krx krx commented Aug 1, 2024

Proposed change

The jaraco.abode library being used for the Abode integration uses jaraco.net.http.cookies.ShelvedCookieJar to save the session information to disk (session ID and UUID). When these are available and unexpired, subsequent logins do not need to do MFA to be able to request an oauth token.

Currently, that library saves the session information to ~/.local/share/Abode/ in the core container, which will not persist if the container is recreated, causing the Abode login to fail when launched again. This fix moves the session information to home assistant's config directory and creates a symlink to it where the library expects to find it.

Type of change

  • Dependency upgrade
  • 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

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.

To help with the load of incoming pull requests:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @krx

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

home-assistant bot commented Aug 1, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft August 1, 2024 20:25
@home-assistant
Copy link

home-assistant bot commented Aug 1, 2024

Hey there @shred86, mind taking a look at this pull request as it has been labeled with an integration (abode) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of abode can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign abode Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@joostlek
Copy link
Member

joostlek commented Aug 7, 2024

Isn't it possible to store this in .storage instead? cc @jaraco

@jaraco
Copy link
Contributor

jaraco commented Aug 7, 2024

Another possibility - jaraco.abode supports retrieving credentials from keyring, so if HA supports adding credentials to the keyring for Abode, it could allow logins to happen automatically. If HA integrations are already using keyring, I'd recommend to explore leveraging that feature.

Isn't it possible to store this in .storage instead? cc @jaraco

jaraco.abode uses the platformdirs project to determine where to store user data, which means on Linux, it honors the XDG_DATA_HOME environment variable. But if what you need is something that's cross-platform overridable, there's currently no option. You'll have to monkeypatch platformdirs to to get the behavior you desire.

jaraco.abode could potentially provide its own override. Fortunately, jaraco.abode already wraps platform dirs, so it's probably not too hard to override the behavior there.

I'm not a fan of the approach proposed, as it violates custodial norms. jaraco.abode "owns" the data at ~/.local/share/Abode (or other platform-specific locations), so to replace it just for HA could break other assumptions. If a user were, for example, to use jaraco.abode natively, then install and run HA, then delete HA and its data, the user's session from the native run will be gone (and the app might crash due to the broken symlink).

I think I agree it would be preferable for HA to somehow override the data path so jaraco.abode looks for it elsewhere.

Is it possible for the container-based deployment to configure ~/.local/share to persist across container restarts? That approach would generalize to many integrations and conventions.

@joostlek
Copy link
Member

joostlek commented Aug 7, 2024

Is it possible for the container-based deployment to configure ~/.local/share to persist across container restarts?

I am not 100% sure on this, but we always want files from integrations to exist in the config directory (And I think we prefer config/.storage even more) also because this whole folder is taken into account with backups and such

@jaraco
Copy link
Contributor

jaraco commented Aug 7, 2024

Well, these files are "data", distinct from "config". I honestly wouldn't expect them to be backed up or transferred from host to host. When I think of Mozilla Firefox, it syncs config, but it doesn't include in that config the session cookies - those are kept local to a browser instance (I think). But it sounds like HA doesn't have a distinct concept for data separate from config.

Since these files essentially contain credentials to connect to the API as that user, I'd be concerned about copying them and saving them as config.

@joostlek
Copy link
Member

joostlek commented Aug 7, 2024

I'll discuss with the core team on what is best here, so we don't break other systems. Thanks for your input :)

@joostlek
Copy link
Member

joostlek commented Sep 8, 2024

Would it be possible to add an option for jaraco.abode to set a custom path via the Python API? I think that would be the best way forward here :)

@jaraco
Copy link
Contributor

jaraco commented Sep 8, 2024

With jaraco.abode 6.2, it now supports overriding the user data path:

import jaraco.abode.config
jaraco.abode.config.paths.override(user_data=pathlib.Path('/alt/user/data/path'))

Note that the value must be a pathlib.Path.

If you want to implement that behavior early (prior to adopting jaraco.abode 6.2), use the following:

import jaraco.abode.config
# TODO: replace with `.override(user_data=)` on jaraco.abode 6.2; home-assistant/core#123035
vars(jaraco.abode.config.paths).update(user_data_path=pathlib.Path('/alt/user/data/path'))

@joostlek
Copy link
Member

joostlek commented Sep 8, 2024

Let me directly try to give a stab at upgrading the library

@joostlek
Copy link
Member

joostlek commented Sep 8, 2024

#125512

@NeckBeardPrince
Copy link

+1

@jaraco
Copy link
Contributor

jaraco commented Sep 24, 2024

With jaraco.abode 6.2 in place, the simple reconfigure operation should be used, and this PR shouldn't be merged as currently proposed. Maybe it makes sense to close this issue and start a new PR with the new approach? I don't feel strongly about how.

@krx
Copy link
Author

krx commented Sep 24, 2024

On 6.2, this doesn't seem to update correctly since user_data_path is also a property, and that won't be overridden by anything in __dict__

In [1]: from jaraco.abode import config

In [2]: from pathlib import Path

In [3]: config.paths.override(user_data=Path('/config/Abode'))

In [4]: config.paths.user_data
Out[4]: PosixPath('/home/krx/.local/share/Abode')

In [5]: config.paths.user_data_path
Out[5]: PosixPath('/home/krx/.local/share/Abode')

In [6]: vars(config.paths)['user_data_path']
Out[6]: PosixPath('/config/Abode')

If I've understood you correctly, adding this call in async_setup_entry before any usages of the Client class should be all that's needed to close this issue, does that sound right?

jaraco.abode.config.paths.override(user_data=Path(hass.config.path('Abode')))

@jaraco
Copy link
Contributor

jaraco commented Sep 25, 2024

On 6.2, this doesn't seem to update correctly since user_data_path is also a property, and that won't be overridden by anything in __dict__

I'd swear I tested that, but it seems I've somehow tricked myself into thinking I had when in fact I hadn't. I've filed jaraco/jaraco.abode#37 to track.

If I've understood you correctly, adding this call in async_setup_entry before any usages of the Client class should be all that's needed to close this issue, does that sound right?

Yes, that's the intention.

@joostlek
Copy link
Member

I have added this PR to my todo list, so when that bug gets fixed I will try to open a PR for it

@home-assistant home-assistant bot marked this pull request as draft September 26, 2024 15:32
@joostlek joostlek added this to the 2024.10.0 milestone Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abode integration needs to re-auth after every HA restart
4 participants