withings-sync icon indicating copy to clipboard operation
withings-sync copied to clipboard

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

Open embear opened this issue 10 months ago • 15 comments

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

embear avatar Oct 06 '23 19:10 embear

Doh... Did not consider this...

jrast avatar Oct 07 '23 20:10 jrast

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

longstone avatar Oct 07 '23 20:10 longstone

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

jrast avatar Oct 07 '23 20:10 jrast

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 avatar Oct 07 '23 20:10 jrast

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.

jrast avatar Oct 07 '23 20:10 jrast

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

stynoo avatar Oct 07 '23 20:10 stynoo

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

longstone avatar Oct 07 '23 20:10 longstone

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.

jrast avatar Oct 07 '23 21:10 jrast

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"?

embear avatar Oct 07 '23 21:10 embear

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.

jrast avatar Oct 07 '23 22:10 jrast

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?

longstone avatar Oct 08 '23 09:10 longstone

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.

embear avatar Oct 08 '23 11:10 embear

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.

jrast avatar Oct 08 '23 19:10 jrast

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.

stynoo avatar Oct 08 '23 20:10 stynoo

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.

embear avatar Oct 19 '23 13:10 embear