jaroslawhartman / withings-sync

Synchronisation of Withings weight
MIT License
457 stars 89 forks source link

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

Open embear opened 1 year ago

embear commented 1 year ago

Commit 64d8ec1b31 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 commented 1 year ago

Doh... Did not consider this...

longstone commented 1 year ago

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

jrast commented 1 year ago

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

jrast commented 1 year ago

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 commented 1 year ago

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

stynoo commented 1 year ago

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

longstone commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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
}

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

embear commented 1 year ago

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.