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

Concurrent writings to hombebridges config.json when using ioBroker #1588

Open
1 task done
muffin142 opened this issue Feb 12, 2025 · 6 comments
Open
1 task done

Concurrent writings to hombebridges config.json when using ioBroker #1588

muffin142 opened this issue Feb 12, 2025 · 6 comments
Labels
bug Something isn't working

Comments

@muffin142
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe The Bug

I use the iobroker.ham homebrige adapter for ioBroker to run homebridge-ring. On every start of that adapter it writes all its internally known config to homebridges config.json which then leads to inconsistencies regarding my refreshToken as this is updated in config.json but not written back to ioBrokers internal configuration by the iobroker.ham adapter.
The developers of that ioBroker adapter asked me to reach out to you to ask if there is a way to change the way upated refreshTokens are stored. Although I think this problem is caused by ioBroker.homebridge as its not a problem running homebridge-ring in a stanalone homebridge environment.

To Reproduce

No response

Expected behavior

refreshToken not beeing overwritten

Relevant log output

Screenshots

No response

Homebridge Ring Config

{
  "description": "Add configuration for your accessories or platforms according to the docs of the plugins.",
  "accessories": [],
  "platforms": [
    {
      "alarmOnEntryDelay": true,
      "refreshToken": "<myRefreshToken>",
      "hideCameraLight": false,
      "hideCameraMotionSensor": false,
      "hideCameraSirenSwitch": false,
      "hideInHomeDoorbellSwitch": false,
      "hideAlarmSirenSwitch": false,
      "showPanicButtons": true,
      "platform": "Ring"
    }
  ]
}

Additional context

No response

OS

Linux

Node.js Version

20.18.2

NPM Version

10.8.1

Homebridge/HOOBs Version

unknown as integrated in adaper

Homebridge Ring Plugin Version

13.2.1

Operating System

Debian

@muffin142 muffin142 added the bug Something isn't working label Feb 12, 2025
@tsightler
Copy link
Collaborator

tsightler commented Feb 12, 2025

What does "change the way it is stored" mean, the plugin simply stores the token using native Homebridge capabilities, which just writes it to to the Homebridge config file, they can read it from that file as well as anyone, just like all other settings. If they are not picking up or are overwritting such settings then, IMO, they are not properly supporting Homebridge plugins. I'm not sure why the plugin code should be changed to handle this.

Isn't there a native ioBroker adapter for Ring anyway?

@muffin142
Copy link
Author

Hm, the config.json file is written in https://github.com/dgreif/ring/blob/main/packages/homebridge-ring/config.ts by fs.writeFileSync(), IMHO thats not a native Homebridge capability. Although I already tried to find other homebridge plugins doing the same to proof that this is at least "best practice" but found none so far. Of course in general its quite usual that a software writes to its own config file.

The only native ring Adapter for ioBroker does only support ring cameras but not ring alarm - which I need to connect to.

@tsightler
Copy link
Collaborator

tsightler commented Feb 13, 2025

Fair enough, I just meant that it updates the existing property in the default homebridge configuration file, and I'm not sure where else would be appropriate, since the token is, as of today, part of the plugin configuration.

I had a similar issue in ring-mqtt, in early versions I kept the token in the config file, but I also have various per-device settings like snapshot intervals, sensor bypass modes, etc, that I needed to keep for each device, and it didn't seem to make sense to keep that in the config file, plus I didn't want the token displayed in the config UI, so I moved all of that to a ring-state.json file. I suppose a similar concept could work here.

However, I would have to defer this to @dgreif as it's been this way from the start, appears to work for 1000's of users for years now, and I'm not sure if it's worth the effort and risk just because it doesn't work with some 3rd party tool that appears to use Homebridge in an unexpected way.

@tsightler
Copy link
Collaborator

tsightler commented Feb 13, 2025

Just for a bit of research, the only specific recommendation seems to be:

  • If the plugin needs to write files to disk (cache, keys, etc.), it must store them inside the Homebridge storage directory.

Based on this, it could be argued that the token should be stored in a separate file in the storagePath(), however, this is a bit complicated by some other requirements such as:

  • The plugin must successfully install and not start unless it is configured.

Today, I'm pretty sure we don't start if there's no token, as that's the only required configuration. For most users, the initial token is generated via web UI, which does use standard Homebridge UI API to set the refreshToken property. So we'd need to use the token initially, and then probably put a placeholder there so the code would use a new token, but we also allow users to generate a new token so we'd need to use he token from the config if the placeholder wasn't there.

@muffin142
Copy link
Author

I don't think that will solve my problem, it would just be a hassle for you with no benefit. What if the user wants to set a new token themselves?
In Homebridge, the user would replace the placeholder (via WebGui) with a token that would then be written back into the config.json and would have to be transferred to the separate configuration.
When using Homebridge in ioBroker, however, ioBroker would write an outdated token into the config.json - which brings us back to the beginning.
The longer we look at this topic, the clearer it becomes that if ioBroker overwrites the config.json every time it starts, it must also transfer changes to it back into its own database.

@tsightler
Copy link
Collaborator

Yes, that would certainly be the best option.

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

2 participants