owntracks / android

OwnTracks Android App
http://owntracks.org
Eclipse Public License 1.0
1.34k stars 474 forks source link

OTRC import doesn't clear keys set in existing app but unset in config file #872

Closed jpmens closed 3 years ago

jpmens commented 3 years ago

2.2.1 (22111)

9

Samsung Galaxy S8

issue

A running OwnTracks has a configuration with tlsCaCert defined and set (seen in Configuration Management). If I import an .otrc file which does not have this key, the import succeeds, but tlsCaCrt remains set.

This is quite confusing and I assume (but haven't tested) that it would occur with all other configuration keys as well.

growse commented 3 years ago

Hmmm, this should work. I was twiddling with a test on this only last week.

Let me try and reproduce, see what's happening.

growse commented 3 years ago

Managed to reproduce.

Main question to clarify is that this behaviour should be the same on config import and also when receiving a remote config? Ie, if importing a config with only a single key should reset/clear the others, should that also happen if a partial configuration message is received over MQTT?

jpmens commented 3 years ago

ooh, that's a good question.

I think the behavior when getting configuration over MQTT should be different: remote config could update just the single value, and it's my responsability to update the values I want set.

It occurs to me that if the OTRC export would produce all keys (also those with empty values), this wouldn't have manifested itself. Would that be an easier solution?

growse commented 3 years ago

Hmm, so I worry about if there's going to be two "modes" of import (replace and merge) whether that might be confusing for users.

If import always did a "merge" (don't touch keys which aren't mentioned), and export included all keys, then that might be a more consistent experience.

Does export drop keys currently?

jpmens commented 3 years ago

Does export drop keys currently?

Yes:

$ jq . < /tmp/config.otrc | grep -i tls
  "tls": true,
growse commented 3 years ago

It feels like fixing this is the right thing to do.

jpmens commented 3 years ago

I'm sorry, @growse, I cannot reproduce the missing key/values in the current version. A fresh export:

$ jq . < keys-config.otrc | grep -i tls
  "tls": true,
  "tlsCaCrt": "",
  "tlsClientCrt": "",
  "tlsClientCrtPassword": "",
growse commented 3 years ago

Ok - I'm going to create a test for this though so we don't get regressions, but it seems like the behaviour of master is in line with what we actually want.

growse commented 3 years ago

Got some tests now - going to close.