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

MSC4170: 403 error responses for profile APIs #4170

Merged
merged 20 commits into from
Oct 7, 2024

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Jul 8, 2024

Rendered

Relates to matrix-org/matrix-spec#1867


In line with matrix-org/matrix-spec#1700, the following disclosure applies:

I am a Systems Architect at gematik, Software Engineer at Unomed, Matrix community member and former Element employee. This proposal was written and published with my gematik hat on.


FCP tickyboxes

@Johennes Johennes force-pushed the johannes/profile-403 branch 2 times, most recently from f68e635 to 6ec8f9d Compare July 8, 2024 13:52
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes Johennes changed the title MSCXXXX: 403 error responses for profile APIs MSC4170: 403 error responses for profile APIs Jul 8, 2024
@Johennes Johennes marked this pull request as ready for review July 8, 2024 13:56
@clokep clokep added proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec labels Jul 8, 2024
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jul 8, 2024
@turt2live turt2live self-requested a review July 23, 2024 19:27
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Show resolved Hide resolved
Johennes and others added 5 commits August 5, 2024 14:36
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>
@richvdh richvdh self-requested a review August 8, 2024 10:25
@richvdh richvdh removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Aug 14, 2024
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Show resolved Hide resolved
Johennes and others added 4 commits August 14, 2024 19:32
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>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
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 concept looks great - just a few small details to clarify before this is set for FCP imo.

proposals/4170-profile-403.md Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
proposals/4170-profile-403.md Outdated Show resolved Hide resolved
@Johennes
Copy link
Contributor Author

I think this would fix matrix-org/matrix-spec#168? @Johennes: might be good to mention that in the proposal text.

@richvdh I think this issue has actually already been addressed through MSC3550. Have commented on the issue.

@thoraj
Copy link

thoraj commented Sep 10, 2024

We're building a multitenant solution based on matrix (homeserver is Synapse).

An important part of this is delegating user_directory/search to our own backend to enforce user discoverability.

If a user is discoverable (according to rules enforced in our custom user_directory) it must also be possible to invite the user to a room.

Currently Element require a profile lookup to be able to do so. The solution is to allow profile lookups WITHOUT requiring users to share a room (using the synapse setting mentioned above).

AFAIK nothing in the spec mandates such a flag, but IMO the spec shoul make a mention that HS implementation MAY (or SHOULD) allow opting into profile lookup (non 403/404 responses) even if a user is not in a public or shared room.

Unrelated to this specific MSC, but the spec should perhaps also spell out the requirements to be allowed to invite someone into a room?

@Johennes
Copy link
Contributor Author

Thanks for the feedback from an actual use case! 🙏

Currently Element require a profile lookup to be able to do so. The solution is to allow profile lookups WITHOUT requiring users to share a room (using the synapse setting mentioned above).

AFAIK nothing in the spec mandates such a flag, but IMO the spec shoul make a mention that HS implementation MAY (or SHOULD) allow opting into profile lookup (non 403/404 responses) even if a user is not in a public or shared room.

Leaving servers the freedom to allow profile look-ups in more, possibly even all cases was definitely the intention of this proposal. It's implicitly captured in this paragraph. The room membership conditions are only a "minimum" requirement and servers "MAY" (not MUST) deny profile queries if these conditions are unmet.

Whether it's worth spelling this out further is probably a discussion for the matrix-spec pull request1 that follows if and when this proposal is accepted.

Unrelated to this specific MSC, but the spec should perhaps also spell out the requirements to be allowed to invite someone into a room?

Interesting question. This is probably best covered in an issue on https://github.com/matrix-org/matrix-spec.

Footnotes

  1. I'll probably just revive https://github.com/matrix-org/matrix-spec/pull/1867

@mscbot
Copy link
Collaborator

mscbot commented Oct 1, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Oct 1, 2024
@mscbot
Copy link
Collaborator

mscbot commented Oct 6, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Oct 6, 2024
@Johennes
Copy link
Contributor Author

Johennes commented Oct 7, 2024

Spec PR: matrix-org/matrix-spec#1867

@richvdh richvdh merged commit 6b10266 into matrix-org:main Oct 7, 2024
1 check passed
@richvdh
Copy link
Member

richvdh commented Oct 7, 2024

Merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
Status: Done to some definition
Development

Successfully merging this pull request may close these issues.

7 participants