Skip to content
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

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

tcpipuk
Copy link

@tcpipuk tcpipuk commented Apr 19, 2024

@tcpipuk tcpipuk changed the title MSC0000: Extending User Profile API with Key:Value Pairs MSC4133: Extending User Profile API with Key:Value Pairs Apr 19, 2024
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Apr 19, 2024
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

@tcpipuk when you get a chance, please sign off on your changes to allow the MSC to eventually be eligible for acceptance.

@tcpipuk

This comment was marked as resolved.

@turt2live
Copy link
Member

Looks great, thanks!

@tcpipuk tcpipuk marked this pull request as ready for review April 19, 2024 19:46
@andrewzhurov
Copy link

Seems we discuss here a key-value CRDT, as keys would need to be updated over time. Personally, I'm all in favor.
With no regard be those events a part of a (messaging) Room or be in its own Profile Room (#1769) (which would be neat),
we need a mechanism to get the latest key-value pairs.

Matrix Event Graph is isomorphic to Merkle-CRDT that powers OrbitDB (key-value DB is one of the supported CRDT).
Taking an inspiration from how key-value CRDT works may be of help to spec it for those Servers wishing to opt-in.

proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

turt2live commented Jan 4, 2025

FCP checklist:

  • Are appropriate implementation(s)
    specified in the MSC’s PR description?
  • Are all MSCs that this MSC depends on already accepted?
  • For each new endpoint that is introduced:
    • Have authentication requirements been specified?
    • Have rate-limiting requirements been specified?
    • Have guest access requirements been specified?
    • Are error responses specified?
      • Does each error case have a specified errcode (e.g. M_FORBIDDEN) and HTTP status code?
        • If a new errcode is introduced, is it clear that it is new?
  • Will the MSC require a new room version, and if so, has that been made clear?
    • Is the reason for a new room version clearly stated? For example,
      modifying the set of redacted fields changes how event IDs are calculated,
      thus requiring a new room version.
  • Are backwards-compatibility concerns appropriately addressed?
  • Are the endpoint conventions honoured?
    • Do HTTP endpoints use_underscores_like_this?
    • Will the endpoint return unbounded data? If so, has pagination been considered?
    • If the endpoint utilises pagination, is it consistent with
      the appendices?
  • An introduction exists and clearly outlines the problem being solved.
    Ideally, the first paragraph should be understandable by a non-technical audience.
  • All outstanding threads are resolved
    • All feedback is incorporated into the proposal text itself, either as a fix or noted as an alternative
  • While the exact sections do not need to be present,
    the details implied by the proposal template are covered. Namely:
    • Introduction
    • Proposal text
    • Potential issues
    • Alternatives
    • Dependencies
  • Stable identifiers are used throughout the proposal, except for the unstable prefix section
    • Unstable prefixes consider the awkward accepted-but-not-merged state
    • Chosen unstable prefixes do not pollute any global namespace (use “org.matrix.mscXXXX”, not “org.matrix”).
  • Changes have applicable Sign Off from all authors/editors/contributors
  • There is a dedicated "Security Considerations" section which detail
    any possible attacks/vulnerabilities this proposal may introduce, even if this is "None.".
    See RFC3552 for things to think about,
    but in particular pay attention to the OWASP Top Ten.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall the MSC looks good - thanks for taking the time to update it! A few low-medium concerns for your consideration.

proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

@mscbot concern [High] Checklist is incomplete
@mscbot concern [Medium] Clarity on how servers can deny a value being set, regardless of capability

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Jan 4, 2025
@tcpipuk
Copy link
Author

tcpipuk commented Jan 5, 2025

This is an attempt to answer the authentication/rate-limiting/guest access requirements in the FCP checklist: f686090

Please post a review if any further clarification is required, thanks!

@tcpipuk tcpipuk requested a review from turt2live January 5, 2025 18:54
@turt2live
Copy link
Member

@mscbot resolve [Medium] Clarity on how servers can deny a value being set, regardless of capability
@mscbot resolve [High] Checklist is incomplete

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label Jan 7, 2025
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor comments, but overall, this looks great. I particularly like that you're keeping the initial proposal small and encouraging future extensions via future proposals.

I know a lot of time and effort has gone into putting this proposal together, and it shows. Thank you.

proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
[Canonical JSON](https://spec.matrix.org/v1.13/appendices/#canonical-json), including the
`avatar_url` and `displayname` fields.

Homeservers SHOULD NOT enforce namespaces, as future expansions may be unknown to the server, but
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Homeservers SHOULD NOT enforce namespaces

I feel like homeservers SHOULD enforce at least some of the requirements of the CNIG (key length, character set, starts with [a-z])?

I notice that an error is defined for M_KEY_TOO_LARGE which implies that there is some expectation here.

Copy link
Author

@tcpipuk tcpipuk Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, the wording of this should be clearer... homeservers should not be checking whether the key uses a known namespace, but they must still enforce the grammar. I'll try to make this a bit more explicit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a go at this - what do you think? 9b2918e

Copy link
Author

@tcpipuk tcpipuk Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I screwed up the line wrapping, so this should fix that: e00d2e9

proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
proposals/4133-extended-profiles.md Outdated Show resolved Hide resolved
tcpipuk and others added 6 commits January 10, 2025 08:17
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear and easy to read proposal. Well done!

Only two minor comments below - the rest looks good to me!

unstable features. Proposal [MSC4175](https://github.com/matrix-org/matrix-spec-proposals/pull/4175)
demonstrates the process of defining new fields in the `m.*` namespace.

## Error Handling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a case for M_MISSING_PARAM for if the client tries to set the profile field "foobar", yet neglects to include a "foobar" field in the PUT request body JSON?

I noticed this case was handled in the Synapse implementation, but clients will not be expecting it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied that from the avatar URL profile endpoint, so probably is a spec bug that it was never documented?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to take this opportunity to document it now? 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Was mostly implying it housing be controversial!

unstable features. Proposal [MSC4175](https://github.com/matrix-org/matrix-spec-proposals/pull/4175)
demonstrates the process of defining new fields in the `m.*` namespace.

## Error Handling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it comes time to write the spec PR, the author will be trying to match up error codes to each individual endpoint.

Is it expected that all status code/error code combinations here can be used for all Client-Server and Server-Server endpoints introduced in this MSC? If so, could you explicitly say so at the beginning of this section?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that the Server-Server endpoints would have identical errors to the current ones, as federation access is still read-only in this MSC.

I'm happy to try to make the Client-Server endpoints a bit clearer on which errors apply to each of them though!

I might not have time to work on this over the weekend as my schedule is pretty full, but I'll try to get time ASAP 🙂

@richvdh richvdh self-requested a review January 10, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API disposition-merge kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period.
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.