MSC4133: Extending User Profile API with Key:Value Pairs #4133
Conversation
tcpipuk
changed the title
turt2live
added
proposal
This comment was marked as resolved.
This comment was marked as resolved.
|
Looks great, thanks! |
|
Seems we discuss here a key-value CRDT, as keys would need to be updated over time. Personally, I'm all in favor. Matrix Event Graph is isomorphic to Merkle-CRDT that powers OrbitDB (key-value DB is one of the supported CRDT). |
|
I believe I've covered all issues so far, please let me know if there are any further concerns! |
proposals/4133-extended-profiles.md
Outdated
| `PUT` methods allowed when the unstable capability (detailed below) is advertised by the server. | ||
|
|
||
| The existing `GET` method would act as normal and remain on `/_matrix/client/v3/profile/{userId}` | ||
| without need for an unstable endpoint. |
There was a problem hiding this comment.
I'm slightly worried about this. I know that the spec allows custom fields, but I've never once seen an implementation take advantage of it. I guess is there a concern here that extra fields appearing on an existing endpoint might cause crashes in the cases of sensitive parsers (looking at you serde), or data previously thought to be small and cachable suddenly filling peoples local storages.
maybe the SCT can share their thoughts
There was a problem hiding this comment.
This is partly why the upper limit on profile size is quite strict. We don't want people to suddenly be able to have 1MiB of JSON on an endpoint that was likely under 1KiB before.
Currently I believe implementations expect the profile to be valid JSON, but read these specific 2 keys out, and ignore everything else. Servers are allowed to include extra fields in the profile, but there's no agreed standard on what they might contain, so clients and servers typically ignore any extra data and just individually handle the two fields they know about.
My assumption is that most implementations will separately store custom fields to avatar_url and displayname (because it's a simpler database change) but this MSC just provides APIs for this data. I'd personally prefer implementations store a block of JSON and just ensure the writes are atomic (as hungryserv has done) as it'd allow a lot more flexibility on types and sizes, as well as making it easier to calculate the full profile size.
There was a problem hiding this comment.
I've never once seen an implementation take advantage of it
Beeper has been using custom profile fields for the past year
There was a problem hiding this comment.
@tulir does Beeper publish anything over federation at all? Can you show an example?
There was a problem hiding this comment.
The custom profiles are only for bridged users which are unfederated
|
I think I've covered all the previous threads now - any more reviews (or tickyboxes) for me please? 🙂 |
proposals/4133-extended-profiles.md
Outdated
|
|
||
| As this data is stored only at the global level, it won't allow users to modify fields per-room, | ||
| or track historical changes in profile fields. However, this is for performance and moderation | ||
| reasons, as many users will struggle to maintain many fields of personal data between different | ||
| rooms, and publishing state events for every field change could become an additional burden on | ||
| servers and moderators. |
There was a problem hiding this comment.
Even if profiles are global, it would be nice to have some kind of option that signals to clients to disable the profile of some or all room members from being viewed from any part of the room's interface.
Related to #4133 (comment), because this endpoint isn't linked to matrix rooms in any way, it cuts room moderators out entirely. Room moderators have no control over which profiles can be viewed/discovered in clients from clicking on members of the room. As @bkil mentions #4133 (comment) this is more problematic if a member draws attention to their profile in their displayname or messages.
There was a problem hiding this comment.
I agree that further features will likely be desired to make tracking profile changes more convenient in the future, but I don't think anyone is eager to see this MSC dig any deeper into any more fundamental parts of Matrix.
It would definitely be interesting to see an MSC that prevents all/some profiles from being viewed in a room, but that I'm sure that would make a better MSC of its own, especially as currently a user can easily be banned if their profiles are deemed unacceptable.
Do you foresee it being a common occurrence that someone's profile is found to be unacceptable but room admins choose for them to stay in the room with their profile hidden? That sounds like quite an edge case to me.
There was a problem hiding this comment.
I meant it in the sense that profiles will be hidden by default and later enabled selectively. Generally this kind of opening up of features as a user participates more in a community is the path that Draupnir is going down.
There was a problem hiding this comment.
Ah, like a power level requirement for profiles to be visible?
I'm certain that shouldn't be touched by this MSC, as we only touch the global profile data and not anything inside a room, but it sounds like a sensible (and relatively simple/short) MSC to raise to add that feature.
This MSC is intended to inspire people to explore and add more features, and provide a framework to help that to happen, so I'm excited to hear it's doing that in multiple ways than just specific types of field to add!
There was a problem hiding this comment.
So, because it seems like there is some disagreement about why this is relevant from a moderation perspective, I want to add my own view on things:
We currently often have people put abusive content into their displayname. This can be removed by redacting the membership event. This is important even when banning a user, because it affects how the historical timeline is rendered.
For a very similar reason status messages (part of presence) aren't that widely supported. They are hard to moderate, because showing them in a visible place means room admins have no control over it.
I think this MSC is in a similar boat. By not having the extensible fields be part of the membership event, they can't be redacted. As such showing these fields is an abuse concern. And for that reason I think this MSC should address that in some way. One option is to put those fields into member events (we already limit them in size, so I assumed that should have been the case). Another option is to explicitly mention that such fields should not be shown for banned members or similar. A third option could be a different mechanism, but I think there should be some mechanism to allow room moderators to remove extensible profile content from being shown for abusive users.
There was a problem hiding this comment.
The implementation requirement for this MSC has been met
How is that possible when custom fields unimplemented? The implementation in element web does not show them or allow you to edit them?
There was a problem hiding this comment.
To my knowledge, a partial implementation is required. I'd love more implementations on this MSC, but IIRC previous MSCs have cited a demonstration implementation in a client or server that didn't actually exist.
I very much welcome more implementations though. I'm currently working with a couple of server contributors to get more server implementations, but I'd be overjoyed if you happen to know any client contributors that would consider adding more 💜
There was a problem hiding this comment.
Please do not add custom fields to m.room.member events. Those events form part of the core auth rules for the room, and are expensive enough to maintain without bloating the size of them.
Why can't we fix the moderation issues with just mentioning in the MSC:
When showing profile information in the context of a room, if the
m.room.memberevent is redacted (missing displayname/avatar_url) then clients MUST NOT show any other custom profile fields in the context of that room.
This would address @deepbluev7 's concern about:
Let's say for example that someone builds a pronouns field based on this MSC. Innocent clients might assume, that it is fine to display that next to the displayname in the timeline. And that will work fine for most users and be very useful, you will then see "Displayname [pronouns]" for every user. But nothing prevents a user from changing their pronouns to "Displayname [buy sketchy here]" or worse.
Because a redacted displayname (the current flow in today's Matrix) would also force redaction of all custom fields.
There was a problem hiding this comment.
I've adjusted the existing implementation advice to clients to expand on this, does this sound about right? tcpipuk@f32932c
| "capabilities": { | ||
| "m.profile_fields": { | ||
| "enabled": true, | ||
| "disallowed": ["org.example.job_title"] |
There was a problem hiding this comment.
Given the flexibility of user-defined fields, this looks unwieldy. Have you considered an allow-list approach, instead of a deny-list?
There was a problem hiding this comment.
Or in addition to the deny-list approach!
There was a problem hiding this comment.
I like that idea! I'll add both as optional keys so that we're not including empty lists unnecessarily, but gives homeservers plenty of scope to tell clients what they can do!
There was a problem hiding this comment.
I've updated the capability and added this now, which hopefully covers this: tcpipuk@d97189e#diff-e408d6c936a5ae3f7dd4386ed643830fb2191b459b35cbec73b2987d73cae439R162-R167
There was a problem hiding this comment.
Resurrecting this because I think, if we're adding both, we need to specify how they interact. I think it's probably, "in the obvious way" (ie. if there's an allow list, only those things, the disallow list is irrelevant) but it needs to be there.
There was a problem hiding this comment.
Totally agree, thanks for calling that out! I've clarified that here: tcpipuk@4afb8b8#diff-e408d6c936a5ae3f7dd4386ed643830fb2191b459b35cbec73b2987d73cae439R163-R169
| @@ -0,0 +1,437 @@ | |||
| # MSC4133: Extending User Profile API with Custom Key:Value Pairs | |||
There was a problem hiding this comment.
Overall, I like it.
It was always the intention to allow more keys than just display name / avatar URL, and this proposal considers the impacts of opening up the k:v store more generally: in particular, strong size limits and the ability to PATCH.
There was a problem hiding this comment.
Thank you, positive feedback means a lot!
I hoped PATCH wasn't assuming too much since MSC4138 was merged - I'm excited to see what bots (especially AppServices) will be able to do with profiles in the future 🙂
| [MSC4201](https://github.com/matrix-org/matrix-spec-proposals/pull/4201)) is under development for | ||
| richer and more granular content and privacy controls, which this proposal does not intend to | ||
| replace - this proposal focuses on basic global profile data without the complexity of per-room | ||
| profile management. |
There was a problem hiding this comment.
this proposal focuses on basic global profile data without the complexity of per-room
profile management.
Imho this proposal shouldnt be merged without the T&S considerations. If this is out of scope of this MSC this should be not merged before this is solved. T&S imho should be a MAJOR concern especially considering the recent spam waves we have seen...
There was a problem hiding this comment.
I don't see how privacy controls and being able to use different profiles in different rooms would be relevant to spam waves? Those are privacy features, not anti-abuse features.
|
@mscbot concern no client implementation for user-defined fields |
There was a problem hiding this comment.
I've got a few more concerns off the back of the latest round of changes. I'd like to give personal thanks to @tcpipuk at this stage for preserving on one of the more difficult MSCs. You're doing a great job of wrangling this.
On the MSC, I am slightly worried it is starting to include an absolute behemoth of implementation details. Some of which feel more like they are rephrasing other parts of the MSC in different language.
On the need for an implementation for user defined fields, I agree. Albeit, I am starting to feel inclined to suggest reserving the namespace in this MSC and creating a successor MSC that deals with the implications (that means leaving u*. effectively ignored until clients choose to support that specific MSC). That would leave clients able to use custom profiles, for defined fields they choose to support.
Anyway, that's my 2c. I suspect the SCT will have their own beliefs on what's right here. Again, good job!
proposals/4133-extended-profiles.md
Outdated
| - Servers are *allowed* to suppress some or all local/remote profile fields they do not wish to | ||
| share with their local users (e.g. if there are moderation concerns during a go-live phase); | ||
| however, server admins *must* disclose to users if they are publishing profile fields on behalf | ||
| of a user over federation that they cannot see, as this could be considered a breach of trust. | ||
| This feature could cause confusion if some users can see fields that other users cannot, so this | ||
| should be used sparingly. |
There was a problem hiding this comment.
In what situation would this occur? This seems extremely unusual. I'd almost say this seems like a bad practice thing to do, and contorts how profiles work.
There was a problem hiding this comment.
I believe Jim raised this as an option for large servers to roll out profile fields, i.e. they'd update to a server that supports them but want to block fields being seen/updated until they were ready.
I agree it feels like a bit of a hack, I'm just not sure whether there's a better way to soft-enable/disable profile fields, or if we should even do that at all?
There was a problem hiding this comment.
Hmm, I'd like to understand what @jimmackenzie had in mind here. This feels like it raises the complexity of profiles AND reduces trust in profiles being accurate to me.
There was a problem hiding this comment.
In the meantime, I've rewritten this to be a lot less "servers can suppress whatever they like, whenever they like" and more "this entire feature can be temporarily disabled, but only sparingly": tcpipuk@4afb8b8#diff-e408d6c936a5ae3f7dd4386ed643830fb2191b459b35cbec73b2987d73cae439R294-R296
proposals/4133-extended-profiles.md
Outdated
| Currently, the Matrix protocol supports limited user profile fields: `avatar_url` and | ||
| `displayname`. This proposal extends the API to include custom fields, enabling users to add | ||
| free-text key:value pairs to their global profiles. This extension provides a flexible framework | ||
| for users to share additional public information. |
There was a problem hiding this comment.
To me, there are two different things being proposed here:
- key-value fields for profiles
- User-specifiable profile info
The first feels fairly straightforward. The latter is a different question altogether and I would have a lot more reservations about. This MSC without the part requiring clients to display u. fields like room tags would be a much easier sell to me, personally.
There was a problem hiding this comment.
Thanks for the review!
One or two people have mentioned I could've split this into two MSCs, but I was wary of presenting an MSC that didn't technically have a problem to solve (i.e. adding endpoints to set extra keys without having keys to set).
In the short term, following Half-Shot's advice, I've downgraded the UI to a COULD so clients aren't compelled to implement it: tcpipuk@4afb8b8#diff-e408d6c936a5ae3f7dd4386ed643830fb2191b459b35cbec73b2987d73cae439R278
I could go further and remove some further client references so the u.* namespace is reserved but there's less "this is what your client needs to do" but I'd appreciate your perspective on this?
There was a problem hiding this comment.
My worry with a 'COULD' is that essentially it's saying that some clients will display the fields and others won't, which means more fragmentation. Thinking on it a bit more, user defined tags feel kind of okay for room tags where only the user themself will ever see them. It feels a lot more of a challenge for something that other user will see. I'd support splitting user defined tags out into a separate MSC, maybe even as something where we can try the regular MSC process for new profile fields and then come back to if we still feel that it's necessary.
There was a problem hiding this comment.
From discussions with the SCT there seemed to be general consensus that this proposal would benefit from splitting out the user-defined fields, as that seems to be the main point of contention.
Although it is a bit more abstract, I think it is still obvious to see the value in allowing for additional (and custom) profile fields that other MSCs can define. Eg as MSC4175 already does. Alternately a single concrete "custom field" could be added to this proposal, but that's more area to bike shed then.
As a process point, if this is done we'll cancel proposed FCP and restart it later due to major changes made to the MSC.
(I had also previously suggested splitting out some of the endpoint changes to a separate MSC but the SCT generally thought that was unnecessary.)
There was a problem hiding this comment.
I should have another MSC raised for custom fields this evening, I'm just reviewing what actually wants removing from this one. I think it's actually a (fractionally) small change to remove the custom fields as all of the endpoints (and size/key requirements) stay the same.
Happy to cancel proposed FCP if you'd prefer, though also happy if you want to wait and see what the changes look like later today?
There was a problem hiding this comment.
I believe this should cover the needed changes - what do you think? bb4fc76
(apologies there are a few extra changes because I tidied up some MAY/COULD/SHOULD/MUST along the way)
| "capabilities": { | ||
| "m.profile_fields": { | ||
| "enabled": true, | ||
| "disallowed": ["org.example.job_title"] |
There was a problem hiding this comment.
Resurrecting this because I think, if we're adding both, we need to specify how they interact. I think it's probably, "in the obvious way" (ie. if there's an allow list, only those things, the disallow list is irrelevant) but it needs to be there.
Rendered
Signed-off-by: Tom Foster tom@tcpip.uk
Known Implementations:
FCP tickyboxes