Rewrite base45 encoding/decoding, delete vaccination certificates with malformed base45 (Community) EXPOSUREAPP-10556 #4356
Conversation
… to infinite recursion
mtwalli
changed the title
…xceptions This should allow easier tracking, where handling of decoding errors is needed, because exceptions are unchecked and might not be catched everywhere.
…lure value instead of thrown exceptions) The main part of the change is, that in the VaccinationContainer the contents of the certificate are optional (filled only when decoding was successful). Only when the container is converted `toVaccinationCertificate()` in the `VaccinationCertificate` those fields are always present. This change makes it so that vaccination certificates with invalid Base45 are silently removed, if they were imported in an earlier version of the app.
|
The PR is now updated to no longer have merge conflicts. Also it now also addresses #4368 in a slightly different way. The decoder now does not throw exceptions, but returns an object of type This also made more changes necessary, especially to For now vaccination certificates with malformed Base45 that were imported in a previous version are silently deleted and no longer displayed. I'd prefer if they were just marked as invalid, but for now it was easier to just simply drop them. |
floscher
changed the title
|
@floscher Thanks. Internal Tracking ID: EXPOSUREAPP-10556 |
dsarkar
changed the title
|
Note: I noticed that these edge cases are currently not addressed in this PR: Will add a commit fixing them later, when I have time again. |
|
Ok, now the decoder should recognize these edge cases too, where three characters in base45 are too big to fit into two bytes, or when two characters in base45 are too big to fit into one byte. And I also added the suggestion from #4341 (comment) |
As suggested here: corona-warn-app#4341 (comment) I also split the certificate into the first part that should decode fine and the next part that was appended, hoping the decoder would discard that second part.
…ig to fit into two bytes or one byte respectively
|
Thanks a lot for submitting this, and apologies for the delayed response! We'll be looking at it as soon as we can. |
|
@floscher Thank you very much for your contribution. We appreciate the time effort you have put into this. |
|
That's sad, indeed. @chiljamgossow & @mtwalli But could you consider to replace the certificate for the unit test like suggested in #4341 (comment) , or is the cert/unit test also part of the EU reference implementation? |
Description
A while ago I wrote a Kotlin multiplatform (Java/JS) base45 encoder, I adapted it a bit for this project and this PR proposes to use that for base45 encoding/decoding, in case you would like to use that. It was built with the goal to produce the same results as the encoder you currently use, but developed from scratch independently.
It's more compact and in my opinion more readable and as far as I can see it also takes edge cases into account that were fixed e.g. in #4341 for the currently used encoder.
Internal Tracking ID: EXPOSUREAPP-10556