Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSC4133: Extending User Profile API with Key:Value Pairs #4133
base: main
Are you sure you want to change the base?
MSC4133: Extending User Profile API with Key:Value Pairs #4133
Changes from 13 commits
0e85b3b
0f373db
c5a3015
c8a5a1a
3157982
b8ed87a
a81f21f
e688eb1
39d5fa2
e160c71
a9546aa
0c43f50
613411a
4a9557f
fbb4e44
591999f
15a0bd1
26c59f7
30e82aa
2966c85
d97189e
ae19725
23a3a62
f32932c
4afb8b8
bb4fc76
09318e8
9b4741e
068d44e
fa381da
b373a55
3349123
9fa0096
da0a791
a948f5d
c82dab6
21ad83f
1726ef3
30d203e
43e1e1a
af87bbe
f686090
17cc30b
d620259
c1b419a
4b3a8ed
0f41c82
1b98d40
9b2918e
e00d2e9
a11286a
2a86235
c4fa474
7ca83db
c7bb382
3843f27
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to a thread for discussion to take place - @MTRNord said:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm failing to understand how this MSC creates problems for pronouns specifically. This MSC defines a generic key/value structure, where the value can be pretty much any type. This could be a user-supplied string, an object, or a boolean value depending on the key's requirements. If you have specific concerns with how the MSC is interfering with other MSCs, like pronouns, please leave inline comments for review.
This MSC also states it complements profiles as rooms, which is true: one of the concerns with profiles as rooms is it tries to do too much. By accepting this MSC, we're agreeing that arbitrary key/value pairs should be supported, which makes other MSCs easier to land. In essence, this is already a continuation of the work, just at an earlier stage than the current MSCs in the area. Forking the effort feels incorrect, given all the authors involved would like to work with each other towards a solution. If there's specific language which doesn't feel in line with this, please leave an inline comment for review.
As for room moderation: profile fields are deliberately kept at the user level in this MSC. A future MSC may introduce them to the room/member level as well. This makes the T&S model one where the server admin is responsible for the user's profile, not the rooms they are participating in. Room admins still have the option to eject users they don't feel fit their room's purpose, and can operate bots which scan profiles for potentially abusive content if they wish. Meanwhile, abusive users and their profiles should be reported to the homeserver admin, which this MSC calls out as the preferred mechanism for dealing with issues. A future, unrelated, MSC will make this easier. If there's specific areas which could be improved further, inline comments are appreciated for further review.
Creating an MSC for every feature may be what happens at the start of the transition towards this key/value system, but as we gain experience in reviewing these MSCs we'll be better able to calibrate the types of things which should be in the spec and which are best covered as unstable extensions. For example, we may prefer a general
m.bio
field over lots of fields for gender, websites, contact info, and interests. Or we may prefer to have a dedicated field for gender, and combine the rest underm.bio
. These types of concerns are best expressed on the individual MSCs which get opened after this MSC lands - the influx is expected, and something we'd need to manage regardless.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am currently on the move and cant draft a full reply in the meanwhile many of my concerns also have been raised by gnuxie at #4133 (comment) which likely answer some of the questions. I will write a proper reply once i am back at a computer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean #4201? Which exists just to give PaR another try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gnuxie wrote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst we do make an effort to avoid speccing stuff that doesn't have a demonstrated purpose, there is a balance to make between that, and allowing things to land in manageable pieces. Historically, some features have struggled to make it through the spec process because their scope was just too broad. (MSC1849 was an example of that.)
Reviewing large MSCs is really hard (as is reviewing a large PR of any kind), just because everybody has to keep lots of context in their head for weeks or even months, and breaking them up helps build stable foundations before moving onto the next storey.
Further: in the case of this particular MSC, I see it mostly as a generalisation of the existing API. Even if we never end up landing anything like MSC4175, it doesn't feel like this MSC is adding undue clutter to the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gnuxie wrote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I really understand the concern here. It feels like you're arguing against this MSC because you're concerned that people may, in the future, use it as a basis for other MSCs which bring problems. You've acknowledged yourself that such problems are not inherent by pointing to MSC4175, so it seems inappropriate to express your concerns on this MSC. If you have concerns with MSC4208, then they belong on MSC4208, not here.
Yeeess, but then the time to discuss them is on those MSCs, not here.
To be clear, I'm reading this as: "clients MAY provide a UI for users to view and enter their own profile fields". On that basis, it's hard to see why this, on the face of it, is any different to, say, allowing users to send freetext events.
I must admit I'm not that familiar with the T&S space, so it's entirely possible I'm missing something here, but in discussions I've had with people who are more familiar with it than me, they've not been concerned about letting users set profile data: it's when clients start showing the profile data of other users that you need to be concerned. I'd be happy to be set straight here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just reread this MSC again, and would agree that we should defer abuse concerns for specific field formats in the MSC which defines that specific field. In terms of the custom fields in this MSC (e.g. someone publishing
m.nasty.stuff: true
on their profile) then yes, some clients may expose that to other users, and then it would surely be handled like any other abusive avatar or displayname today: you'd ignore or otherwise moderate the user. So, we can use the abuse potential of today's displayname/avatars as a precedent for how you'd handle this, and don't need to block on defining new mechanisms.In other words, rather than waiting on future MSCs for new profile field types to land which flesh out antiabuse considerations which might then inform this MSC, let's keep some forwards momentum and actually land this as is - we can always have a future MSC for "antiabuse mechanisms common to different extensible profile fields" or something, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, that's roughly what I'm looking to achieve with #4208 and Jim has offered to help shape it, so I'm hoping we're off to a good start.
The hope is that we can define the dependencies for freetext content in profiles to be commonly allowed in clients, then other profile field MSCs have an easy reference to quote (or use as a dependency) to make sure they're also safe.
As always, I welcome feedback and would love to gather different perspectives so I can start translating that into firm technical requirements, then figure out whether MSCs already exist (or need to) for the dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But MSC4175 is a very simplified example, and it would be fair to suggest that MSC4133 would not be considered in the same way if its only function was to provide the timezone field or similar narrowly defined fields, such as a flag for whether a user is a bot. Therefore it does seem appropriate for me to express these concerns on this MSC.
Because there is already a significant amount of safety tooling for handling user generated content in events and all clients have been built with that in mind. With the profiles we are creating a new surface and new ways for users to interact. @turt2live has rightfully suggested in another comment #4133 (comment) that bots can scan profiles for abusive content, but this isn't something anyone has implemented at the moment. And we don't yet understand from this proposal and its implementations how profiles will be used for spam or abuse in Matrix's public rooms.
Yes this is true, but it is clear that clients will need to display profile data for this MSC to have utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gnuxie wrote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, I'm sorry if your concerns haven't been heard. I don't think I've seen the discussions you refer to, and I wasn't aware of them before you raised them here. This is one reason we ask that concerns on MSCs be raised as comments on the MSC itself: conclusions in Matrix rooms get lost. Thank you for doing so now.
Secondly: I don't really see this as "urgency". The MSC has been 9 months in the making, and it feels like a relatively incremental change to the existing API. Honestly, it felt ready to land. I'll refer you to my earlier points that it can be very helpful to land MSCs incrementally rather than having a large amount to review all at once.
As the SCT, we've also had feedback in the past that it can be unreasonably difficult to get simple changes to the spec through the MSC process. There are numerous anecdotes from people who have just found the MSC process too arduous and have walked away in frustration. We have a balance to pick between rushing things into the protocol without due diligence, and letting the protocol ossify because changing it is just to damn hard.
All that said, you're right that this isn't actively blocking anything, and it's certainly not our goal to steamroller it through against significant opposition. On that basis: thank you again for raising your concerns. This is the FCP working as it should: we've put the FCP timer at least on hold for now and we'll see what can be done to address the concerns.
Whilst we can't promise that everyone is going to be delighted by every MSC that is accepted, it is important to make sure that members of the Matrix community have the opportunity to voice their concerns and that we take the time to consider those concerns properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what I mean by urgency. It does make sense that you would want to review MSCs in smaller pieces. And it does seem like at the moment the only formal tool to review and freeze MSCs is by landing them into the specification.
Yes, this is a valid concern. Though I think there is another factor at play here, which is that clients may implement Matrix features selectively regardless of whether the features are specified in a spec release or an MSC. And at least some frustration comes from lack of implementation in a range of clients. Now I could misunderstand what people's frustrations have been, but to me that seemed to be close to the root.
The thing is this MSC does not bring any feature directly to the end user in Matrix clients. Do we believe that landing MSC4133 in specification will make a difference to the uptake of the dependent MSCs (which are still iterating) that will? It just seems unlikely to me.
On the other hand, we do need to keep MSC authors satisfied that their work is making progress, is valued. And I will concede that while contribution is scarce an optimistic merge makes sense. But there could be other ways of assuring and rewarding authors.