Fix issue #148 of kee-org/keepassrpc

[a-s-z-home]

Hi,

Entries were altered very often and without user interaction, because form initialization was not handled. In addition comparing of the entry config did not work as expected.

Arrays were not handled properly
Converting v1 to v2 always created default fields with unique uuids

See also discussion at https://forum.kee.pm/t/keepassrpc-asks-to-save-when-no-known-changes-were-made/4580/10

Kind regards,
Andre

[luckyrat]

Thanks for the PR. It has some good improvements to the accuracy of how we determine if an entry has been modified and I'd like to merge most of it into the next version of KeePassRPC.

The main issue with the changes is that we would no longer be always storing an explicit default field configuration when there are other reasons that we will save a config into the entry in V2 format. Therefore we would need to remove the changes in the "Do not create default fields, if field list is empty." commit, which I think will prevent the fix from working.

Clients (such as Kee) could infer default configurations when absent (and already do for entries with legacy V1 config) but:

1. we want to be able to rely on immutable UUIDs for every field when adding features that rely on config V2.
2. implicit configuration like this often causes unforeseen complications in future (such as migrations to future config versions, or changes to the expected default behaviour, or differences across client implementations).

Perhaps there is some way we can provide a custom equality comparator which ignores the UUID values for the two default Fields? If so, we probably shouldn't override the default equality behaviour (but your changes to make this more accurate are good and should remain).

Maybe try adding a method to EntryConfigv2 like:

// There is probably a better name for this method but hopefully this at least helps you understand my idea!
public bool SimilarEnoughToNotJustifySaving(EntryConfigv2 other)
{
    bool ignoreDefaultUuids = true;
    if (ReferenceEquals(null, other)) return false;
    if (ReferenceEquals(this, other)) return true;
    return Version == other.Version && StructuralComparisons.StructuralEqualityComparer.Equals(AltUrls, other.AltUrls) && StructuralComparisons.StructuralEqualityComparer.Equals(BlockedUrls, other.BlockedUrls) && StructuralComparisons.StructuralEqualityComparer.Equals(RegExBlockedUrls, other.RegExBlockedUrls) && StructuralComparisons.StructuralEqualityComparer.Equals(RegExUrls, other.RegExUrls) && HttpRealm == other.HttpRealm && StructuralComparisons.StructuralEqualityComparer.Equals(AuthenticationMethods, other.AuthenticationMethods) && Behaviour == other.Behaviour && StructuralComparisons.StructuralEqualityComparer.Equals(MatcherConfigs, other.MatcherConfigs) && StructuralComparisons.StructuralEqualityComparer.Equals(Fields, other.Fields, ignoreDefaultUUids);
}

[luckyrat]

Under what circumstances can this ever remain false before the EntrySaving event is fired?

[a-s-z-home]

I introduced the variable, because setting the control values in KeeEntryUserControl_Load triggered UpdateKPRPCJSON in several code paths. The changed dialog got triggered, because this method "alters" the entry without user interaction.
Only changes by the user should trigger an update of the "KPRPC JSON" field.

[a-s-z-home]

The main issue with the changes is that we would no longer be always storing an explicit default field configuration when there are other reasons that we will save a config into the entry in V2 format. Therefore we would need to remove the changes in the "Do not create default fields, if field list is empty." commit, which I think will prevent the fix from working.

OK, you want to have a "proper" default configuration. This is no problem and it it not the reason of this issue.
Please note if config V2 is mandatory, you should implement a migrate functionality that asks the user to upgrade all entries in the database at once. But this will not resolve the issue itself. In my opinion you should not ever force changes to existing data without user interaction / consent.

The problem is that you check the current config against a newly created default config to check for changes in the current config. This will lead to changes, even when there is no KPRPC config at all in the entry.

The are several options to resolve this issue:

1. Implement a modified-flag, that gets triggered only by user interaction with the ui.
2. Compare the new config json to the old config json to check for changes. It should be no problem to create and save the default config e.g. in the constructor, instead of creating it over and over again in UpdateKPRPCJSON.
3. Similar to 2., but use ConfigV2 objects instead of json strings.

All these options work even when fields have unique uuids.
I would prefer the latter ones, because entries do get altered only by "real" changes. instead of every change.

Perhaps there is some way we can provide a custom equality comparator which ignores the UUID values for the two default Fields? If so, we probably shouldn't override the default equality behaviour (but your changes to make this more accurate are good and should remain).

When using one the options above, there will not be different uuids for an entry :)

Different uuids for different field "types" like name, password may make sense, but I still do not get the reason for unique field uuids for every field instance. Can you explain this more detailed?

Maybe try adding a method to EntryConfigv2 like:

// There is probably a better name for this method but hopefully this at least helps you understand my idea!
public bool SimilarEnoughToNotJustifySaving(EntryConfigv2 other)
{

I think it would be the better to implement special Comparer classes for EntryV2, Field, ... :)

Kind regards,
Andre

[luckyrat]

Config V2 is not mandatory. However, if V2 is used, it has to be consistent (we don't want to find some "V2" configs with missing information in future).

The intention is that all V1 configs will be migrated to V2 eventually. At the moment, the transition is in a phase where we are waiting for all old clients that don't understand the V2 format to be upgraded. Kee still does not understand the new format (I started work on a branch to enable this support but have not yet finished it, let alone rolled out a new version to all users).

While the entry persistence format has been upgraded, the main client of the plugin still communicates only with DTOs that represent the V1 format.

In any case, I wanted a long transitional period before offering or requiring a full upgrade of every entry. I figured at least a year from now but I may consider bringing that forward if we find that there isn't a quick way forward to resolve these bugs with the "just in time" migration.

The idea of silently upgrading the config format only when an entry was already being touched is exactly what I thought had been released last year and it does appear to work in a lot of cases, just not all. If it's just going to be too much work to fix this in all cases then perhaps the better alternative is an optional mass-migration feature which users impacted by the current bugs might want to invoke manually to update all their entries in one go. We would need to write that code before the full transition to the V2 format is complete anyway, and probably require the user to invoke it before continuing to use the plugin (details and timing TBD but I doubt that would be within the next couple of years).

Until an entry has been saved with the migrated V2 state, clients requesting a V2 version will be given one that was migrated on the fly so the Field UUIDs will vary. I've not decided exactly how to handle that yet because it is still a problem of some significant time in the future but one option includes just telling the user that they need to perform a "full V2 migration" if they want all of the new client features that rely on Field UUIDs to work correctly.

The default config would indeed be better initialised in the constructor to improve performance. Well spotted.

The entire config editing UI was designed in a time where there was not even a KeePass "EntrySaved" event so we had to perform all changes to the persisted config with every user interaction. Maybe there is a simpler approach now.

I'd vote for option 3 over 2 just in case the JSON persistence format changes in a subtle way in future.

The point of comparing to the default config was that if we found the current configuration to be equal to the default configuration, we could just avoid storing anything and regenerate the default config when required. However, I think this might be the crux of the problem - while this approach worked for V1, it may no longer make any sense when we have a unique field ID.

I'll need more time to think about whether this is still a valuable optimisation but I suspect that if we were to just store a config in every entry, much of the complications around equality comparisons would vanish. My main area of concern is whether clients can just happily handle these default configurations while the UUIDs change all the time - perhaps that's not a problem as long as KeePassRPC does persist them at the first point that the client really needs them to become immutable.

Different uuids for different field "types" like name, password may make sense, but I still do not get the reason for unique field uuids for every field instance. Can you explain this more detailed?

When each field instance can be uniquely identified, clients such as Kee will be able to make many more decisions about how to use different entries and fields for themselves, as well as clearly communicate new information back to the KeePassRPC server.

I've not got a complete list of ideas that can come from this improvement yet but some include:

1. Automated or manual feedback from the browser to improve match accuracy as sites change layout and multi-page sign ins over time.
2. Additional configuration options for advanced filling situations (e.g. "pick field X rather than Y if the entry has been matched on a page that matches a certain RegEx or has a particular DOM structure").
3. Improved multi-page sign-in form support - creating, updating and filling operations can all be made more accurate by having a guarantee that each field can be uniquely identified.

I think it would be the better to implement special Comparer classes for EntryV2, Field, ... :)

Cool. As long as the default equality comparisons are accurate (in case we ever need to know of any difference, including a default field's UUID) I'm not fixed on exactly what the best approach for this would be.

[schmurtzm]

Following.
Looking forward a migration tool or just a temporary fix would be welcome because I admit that it is annoying 😉