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

Storing garth session in ./garmin_session causes problems when syncing multiple users #141

Open
embear opened this issue Oct 6, 2023 · 15 comments
Labels
bug Something isn't working

Comments

@embear
Copy link
Contributor

embear commented Oct 6, 2023

Commit 64d8ec1 introduced storing the garth session in the directory ./garmin_session which in principle is a nice addition. But sadly this causes problems in my setup. I'm syncing the measurements of all family members from Withings to Garmin by repeatedly calling withings-sync with changed environment variables WITHINGS_USER, GARMIN_USERNAME and GARMIN_PASSWORD.
By introducing the session saving all Withings data is pushed to the Garmin account that had the first successful login.
It would be nice if the session information is stored in the already existing json file specified with WITHINGS_USER using the dumps and loads functions of garth. Alternatively an extra environment variable and/or an argument could be introduced to specify the location of the session data.
FYI @jrast @matin

@jrast
Copy link
Contributor

jrast commented Oct 7, 2023

Doh... Did not consider this...

@longstone longstone added the bug Something isn't working label Oct 7, 2023
@longstone
Copy link
Collaborator

Hi @embear thanks for raising this.
I also didn't consider that.
@jrast can you take this?

@jrast
Copy link
Contributor

jrast commented Oct 7, 2023

I need to take a look on how multiple users are handled for withings and come up with a nice solution.

@jrast
Copy link
Contributor

jrast commented Oct 7, 2023

Would it be acceptable to add an additional parameter / env-variable GARMIN_SESSION, which is used to set the directory in which the session data is stored?

@jrast
Copy link
Contributor

jrast commented Oct 7, 2023

So the workflow for multiple users would look something like this:

  • Initial login / sync for each account: The GARMIN_USERNAME, GARMIN_PASSWORD and GARMIN_SESSION variables / arguments must be provided
  • Later sync operations: Only the GARMIN_SESSION is required, the username and password are optional and only used if re-authentication is required.

@stynoo
Copy link
Contributor

stynoo commented Oct 7, 2023

How about storing it as ex. /root/{username}.session ?

@longstone
Copy link
Collaborator

longstone commented Oct 7, 2023

What about the withings user info? do we need to describe which user matches with which login?
From this point I would suggest using a /root/.withings-sync/{user}/... approach

Given tree users
A
B
C

They will have 3 different withings accounts and also 3 different garmin accounts

This topic was raised some time ago, see https://github.com/jaroslawhartman/withings-sync/pull/84/files

jrast added a commit to jrast/withings-sync that referenced this issue Oct 7, 2023
@jrast
Copy link
Contributor

jrast commented Oct 7, 2023

I created a quick draft for multiple garmin users, where the session storage location can be set by the GARMIN_SESSION environment variable.

Regarding multi-user support in general: In your approach the {user} in /root/.withings-sync/{user}/... seems to be handled by withings-sync. I don't know if this is the right approach. But placing all session information (withings & garmin) in a single folder sounds like a good idea. So a directory .withings-sync, defaulting to ~/.withings-sync and configurable by the environment variable WITHINGS_SYNC_SESSION (for example), would allready allow a clean multi-user experience.

@embear
Copy link
Contributor Author

embear commented Oct 7, 2023

I like the idea of having multi-user support by using different directories. Nowadays a lot of applications are using the XDG Base Directory Specification. So possibly $XDG_CONFIG_HOME/withings-sync/{user}/... (which defaults to $HOME/.config/withings-sync/{user}/...) would be a better location.
For {user} I don't understand what this would be. Is this the userid currently given in withings_user.json or will this be some additional parameter/environment variable that specifies a more useful name like "alice" or "bob"?

@jrast
Copy link
Contributor

jrast commented Oct 7, 2023

For {user} I don't understand what this would be. Is this the userid currently given in withings_user.json or will this be some additional parameter/environment variable that specifies a more useful name like "alice" or "bob"?

That's also my point with this naming schema. Is it the withings user id? Or the garmin / trainerroad id? Or is it something entierly different?

Otherwise I like the idea of the XDG directory.

@longstone
Copy link
Collaborator

TL;DR
With configurable directory, we would have an easy and simple approach to solve this issue.

Sorry for the long text, but I try to merge all the inputs. I really appreciate the efforts all you guys put in.

IMHO
We are mixing up two things, which are both achieving the same outcome (capability multi-user):

  1. Configuration of withings-sync: the support of multiuser and where to store the settings for a single user in this case
  2. Configuration of the config path: the option to configure and store the configuration other than in root

This is clearly because we can solve this issues with the one or other way.

Here is what i find looking into the /root/.withings_user.json file:

{
    "access_token": string,
    "authentification_code": string,
    "last_sync": int,
    "refresh_token": string,
    "userid": int
}
  • This userid is in my opinion the userid of withings.
  • Withings is the source of information and garmin and trainerroad are targets
  • We have this session created at the first run and then can ignore it when invoking withings-sync with garmin credentials.
  • Therefore, I see the only valid user the withings user, as it is the identifier of the source.

I like the approach of having everything configurable and flexibel.


@embear

I like the idea of having multi-user support by using different directories. Nowadays a lot of applications are using the XDG Base Directory Specification. So possibly $XDG_CONFIG_HOME/withings-sync/{user}/... (which defaults to $HOME/.config/withings-sync/{user}/...) would be a better location. For {user} I don't understand what this would be. Is this the userid currently given in withings_user.json or will this be some additional parameter/environment variable that specifies a more useful name like "alice" or "bob"?


@jrast

For {user} I don't understand what this would be. Is this the userid currently given in withings_user.json or will this be some additional parameter/environment variable that specifies a more useful name like "alice" or "bob"?

That's also my point with this naming schema. Is it the withings user id? Or the garmin / trainerroad id? Or is it something entierly different?

Otherwise I like the idea of the XDG directory.

  1. As mentioned before, in my opinion the user should be the withings user. BUT if I want to sync for example only on user, it would mean we need the opportunity to have the knowledge wich user id we have to call.
  2. Would $XDG_STATE_HOME be a better match, because we save state information (sessions).

My conclusion:

  1. feature: Support multi user by using the XDG approach: introducing a configurable directory holding configuration
    1.1 Backward compatibility: default will still be /root/
    1.2 decide: $XDG_CONFIG_HOME vs $XDG_STATE_HOME (I found this)
    1.3 decide: if $XDG_STATE_HOME is used, should there be a $XDG_CONFIG_HOME to put the .env
  2. how to determine how to define a {user} if needed
    2.1 proposal: if a user identifier is needed we use the withings identifier

@embear @jrast @stynoo what do you think?

@embear
Copy link
Contributor Author

embear commented Oct 8, 2023

My conclusion:

  1. feature: Support multi user by using the XDG approach: introducing a configurable directory holding configuration
    1.1 Backward compatibility: default will still be /root/
    1.2 decide: $XDG_CONFIG_HOME vs $XDG_STATE_HOME (I found this)
    1.3 decide: if $XDG_STATE_HOME is used, should there be a $XDG_CONFIG_HOME to put the .env
  2. how to determine how to define a {user} if needed
    2.1 proposal: if a user identifier is needed we use the withings identifier

@embear @jrast @stynoo what do you think?

I missed $XDG_STATE_HOME and I think this is a better match than $XDG_CONFIG_HOME. Also using /root/ as default for backward compatibility is good. It is not necessary to break backwards compatibility here.

For the definition of {user} have a different opinion. While it is true that the Withings userid might be a unique anchor point I'm not totally sure about this. There might be users, that would like to sync one Withings user to multiple Garmin or trainer road accounts. Not my use case but possibly useful for someone else.

Furthermore, although the userid is unique, it is difficult to identify which user is really behind it. I would prefer to use a descriptive {user}. As mentioned in the first description I currently use different values to WITHINGS_USER that point for example to withings_alice.json or withings_bob.json. This way it is easy to identify the configuration for a specific user, modify/delete the configuration and do an re-authentication on the Withings connection if something goes wrong.

@jrast
Copy link
Contributor

jrast commented Oct 8, 2023

I don't understand why you are all talking about keeping /root as a default. I think the only case where /root is actually used is in Docker Files where the USER seems to be root (which is bad practice by the way). Currently the location is always derived from the HOME variable.

A clean (and proven) way to handle configuration / data directories in python is by using platformdirs. Of course only if you are OK with an additional dependency.

@stynoo
Copy link
Contributor

stynoo commented Oct 8, 2023

How about storing it as ex. /root/{username}.session ?

It was an example, as this is (currently) the way the docker image is set up. Imho the default should still be ~
As we support pip installs or direct git clones cross os path compatibility is an issue. Platformdirs or python's own pathlib should idd solve this. Additional dependencies should not be an issue.

longstone added a commit that referenced this issue Oct 10, 2023
Fix for #141: Support for multiple garmin users
@embear
Copy link
Contributor Author

embear commented Oct 19, 2023

From my point of view this issue can be closed. The introduced GARMIN_SESSION environment variable enables me to switch the session for the individual family members. Works perfect since last week.

BTW: I'm so glad that the session is reused. I was forced to enabled 2FA in my Garmin account because of ECG. With 2FA each sync would require to provide a security code as part of the log in procedure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants