Accept any string as a key for m.direct account data

[MatMaul]

This is because a lot of m.direct in the wild have non valid data, and an email or phone number can end up as a key here when doing 3pid invites.

Current behavior is that m.direct parsing fully failed in this case, so there is no DMs anymore.

You can trigger that easily by trying to invite an email with no MXID bound to it with Element Web.

[giomfo]

Putting the email in the m.direct was not clearly defined in the spec

But the email address is the only user ID, the client has when a new DM is created by inviting someone by email. So email address is currently supported in m.direct event in the existing implementation in the Legacy Mobile clients and in the Element Web.

See for example in Element-Web: https://github.com/element-hq/matrix-react-sdk/blob/develop/src/createRoom.ts#L325
where opts.dmUserId may be an email address

[giomfo]

Note: This PR is aligned with the following Element-Web implementation: https://github.com/element-hq/element-web/blob/2f8e98242c6de16cbfb6ebb6bc29cfe404b343cb/src/utils/dm/filterValidMDirect.ts#L35 (where the m.direct content is filtered)

[zecakeh]

So what you are saying is that the 3pid addresses in m.direct have a valid use-case? Because an alternative to this change would be to ignore non-user ID strings during deserialization, which would avoid the breaking change while remaining strictly aligned with the spec. But that means that when the account data is updated and re-serialized, the 3PID adresses would be removed.

[jplatte]

I don't like the idea of throwing away entries during deserialization. I think we should have an opaque struct that can represent any string, but allows easy access of it as a &UserId (and maybe OwnedUserId) if it's that. We'd also need a constructor from a user ID, but not necessarily from an arbitrary string (or if we do, make it a named c'tor). Also for access, probably have a &str / String accessor.

This would mean a breaking change of course (just like this PR). It's pretty unfortunate that this is coming up (again) just after we did a release, but such is life I guess 🤷🏼

[MatMaul]

Following the discussion here, I've rework the approach to use existing UserIdentifier.

It looks a lot bigger but a lot of the changes are moving UserIdentifier to a common place.

I still have to write some logic to properly validate email and phone number (cf TODOs), but I am interested if you prefer this approach or the one you suggested, that looks fine for me too.

[jplatte]

As you can see in the (de)serialization tests for UserIdentifier, that types is not just a string on the wire. I don't think it's useful here.

W.r.t. which solution to go with, that's @zecakeh's call.

[MatMaul]

As you can see in the (de)serialization tests for UserIdentifier, that types is not just a string on the wire. I don't think it's useful here.

Yes I've seen that, but we basically want to represent the same thing (either a MXID or a 3PID), so I thought it would be nice to use the structure and customize the parsing.

[zecakeh]

From the Ruma room, to keep a trace of it here:

Because I like Jonas's suggestion, it seems to me that the simplest solution is to create a new type with our IdZst macro without validating the input (like DeviceId). It will generate all the code necessary for conversion to/from string types, we will only need to add methods/implementation to convert to/from (Owned)UserId.

[jplatte]

I know this has already gone through a lot of review rounds but.. A changelog entry would be nice 😅

[zecakeh]

Thanks a lot for this!