mozilla / application-services

Firefox Application Services
https://mozilla.github.io/application-services/
Other
608 stars 224 forks source link

logins: Store `origin` and `formActionOrigin` in Punycode and migrate existing data #2153

Closed mnoorenberghe closed 4 years ago

mnoorenberghe commented 4 years ago

This will help prevent duplicates and help to properly handle updates vs. saves.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1407146 for toolkit context.

Some test URLs: https://blogs.msdn.microsoft.com/shawnste/2006/09/14/idn-test-urls/

┆Issue is synchronized with this Jira Story ┆Epic: important not urgent

rfk commented 4 years ago

Add a new 'displayOrigin` getter on each login to expose the Unicode/non-Punycode values for display.

I'm a little unclear about the division of responsibilities between the storage layer and callers here. Should we expect callers to provide origin and formActionOrigin as unicode strings that we translate to punycode internally for storage? Should we return the punycode value as the canonical value of this field when reading items from storage, with displayOrigin being calculated on-demand if the caller asks for it?

mnoorenberghe commented 4 years ago

Add a new 'displayOrigin` getter on each login to expose the Unicode/non-Punycode values for display.

I'm a little unclear about the division of responsibilities between the storage layer and callers here. Should we expect callers to provide origin and formActionOrigin as unicode strings that we translate to punycode internally for storage?

Ideally these two properties could accept unicode or punycode values but would be normalized to punycode for storage and duplicate checking.

Should we return the punycode value as the canonical value of this field when reading items from storage, with displayOrigin being calculated on-demand if the caller asks for it?

Right, displayOrigin should be a read-only (lazy) getter. Setting the origin should always be done via origin.

rfk commented 4 years ago

I'm hoping that Login::validate_and_fixup gives us a bit of space to do this effectively, and we can treat the conversion to punycode as just another a "fixup" case. If we do this at the same time as renaming the fields to origin and formActionOrigin then I'm also hoping that we can do it without confusing the consuming apps:

displayOrigin should be a read-only (lazy) getter.

This, I'm not sure on the right implementation strategy for. It seems like a waste for the app to have to call into rust code over our FFI just to decode something from punycode. I wonder if we're better off implementing this independently in the Kotlin and Swift wrappers rather than in the shared rust code.