tobischo / gokeepasslib

A library to read and write keepass 2 files written in go
MIT License
251 stars 30 forks source link

Override time format according to KDBX version #59

Closed korovan-software closed 4 years ago

korovan-software commented 4 years ago

Currently in TimeWrapper implementation time format depends on if it was successfully parsed from raw string or not.

I offer to override format depending on database version but not on current value format. It will help to correct and not to keep wrong time format, which could already exists in database by some mistake.

korovan-software commented 4 years ago

Also for me is not very clear following code.

        // Kdbx v4 - Count since year 1
        total := float64(0)
        zero := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC)

        temp := t
        for {
            diff := temp.Sub(zero)
            if diff.Seconds() == 0 {
                break
            }

            total = total + diff.Seconds()
            temp = temp.Add(-diff)
        }

Can we just replace it with next?

        zero := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC)
        total := t.Sub(zero).Seconds()

At least inside the for loop it's need to do if diff.Seconds() <= 0 to check negative values too.

tobischo commented 4 years ago

The wrong format would be a plausible explanation. Using it from the file version would be the correct way to go. However unless we define something that is at least package local to track the state and make it everywhere available, it would definitely mean that we couldn't just use golangs existing xml marshalling as is.

As for the simplification: You can test that out and see if it works. The time packing for KDBX format v4 was a bit non-obvious in the original code.

korovan-software commented 4 years ago

Ok, understood.

My simplification doesn't work. Let's leave all as it is.

tobischo commented 4 years ago

The marshalling https://github.com/tobischo/gokeepasslib/blob/33ee01c62f906287ea17f854796a6adfbb7d8f9b/wrappers/time.go#L107 uses the const intLimit https://github.com/tobischo/gokeepasslib/blob/33ee01c62f906287ea17f854796a6adfbb7d8f9b/wrappers/time.go#L19 which is set to 9_000_000_000, which is approximately 265 years

The largest representable duration is approximately 290 years.

A Duration represents the elapsed time between two instants as an int64 nanosecond count. The representation limits the largest representable duration to approximately 290 years. source: https://golang.org/pkg/time/#Duration

So for the marshalling it was necessary to go that way.

On the other side for the unmarshalling, it is also necessary, but currently not very well documented. diff := temp.Sub(zero) would result in a duration, which again, can only cover up to approximately 290 years. So we have to loop through it.

tobischo commented 4 years ago

Oh, I think it is worth exploring how we could best use the xml marshal/unmarshal behaviour while passing through our own information.

However, an option that should be relatively easy, while maybe not as elegant, would be to loop over the entire database when executing the Encode method https://github.com/tobischo/gokeepasslib/blob/33ee01c62f906287ea17f854796a6adfbb7d8f9b/encoder.go#L25 That would allow to just overwrite the Formatted field, however it would be prone to being forgotten when adding or removing new fields. I'd say that that would be ok though, considering that the format is not changing that often.