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.

Sign up for GitHub

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

Jump to bottom

MSC3550: Allow HTTP 403 as a response to profile lookups #3550

Merged
merged 9 commits into from
Dec 26, 2021
Merged

MSC3550: Allow HTTP 403 as a response to profile lookups #3550

merged 9 commits into from
Dec 26, 2021

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Dec 7, 2021 •
edited by callahad
Loading

This is a MSC proposal generated in response to the feedback on #3530.

Essentially, servers (such as Synapse) may not allow profile lookup over federation. For example, Synapse will return an HTTP 403 in response to GET /_matrix/client/v3/profile/{userId} if allow_profile_lookup_over_federation is set to False. This behavior is not currently documented in the spec. This proposal aims to add HTTP 403 to the list of responses to GET /_matrix/client/v3/profile/{userId}.

Signed off by: H.Shay hillerys@element.io

Rendered: https://github.com/matrix-org/matrix-doc/blob/3ce9c1d288aa3173caf2716b18e2a8da0bc681d7/proposals/3550-allow-403-response-profile-lookup.md

PR where this behavior was implemented: matrix-org/synapse#9203

Preview: https://pr3550--matrix-org-previews.netlify.app

@H-Shay H-Shay marked this pull request as draft December 7, 2021 21:25
@H-Shay H-Shay marked this pull request as ready for review December 7, 2021 21:50
@turt2live turt2live changed the title Allow HTTP 403 as a response to profile lookups MSC3550: Allow HTTP 403 as a response to profile lookups Dec 7, 2021
turt2live
turt2live reviewed Dec 7, 2021
View reviewed changes
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 looks fine - just a few nitpicky pieces.

If possible, it'd be good to get the original Synapse PR linked in the PR description here to count for implementation purposes. Failing that, a permalink to the relevant code.

proposals/3550-allow-403-response-profile-lookup.md Outdated Show resolved Hide resolved
proposals/3550-allow-403-response-profile-lookup.md Outdated Show resolved Hide resolved
proposals/3550-allow-403-response-profile-lookup.md Outdated Show resolved Hide resolved
@turt2live turt2live added client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal proposal-in-review labels Dec 7, 2021
@H-Shay
Copy link
Contributor Author

H-Shay commented Dec 8, 2021

Thanks for the review, @turt2live! This is my first one of these so let me know if there's anything else I am missing.

@turt2live turt2live self-requested a review December 8, 2021 21:54
turt2live
turt2live approved these changes Dec 13, 2021
View reviewed changes
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.

otherwise lgtm - just needed the added clarity that the error code is part of the spec change. Thanks!

proposals/3550-allow-403-response-profile-lookup.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

This seems relatively straightforward.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Dec 13, 2021 •
edited by ara4n
Loading

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Dec 13, 2021
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Dec 13, 2021
H-Shay and others added 2 commits December 13, 2021 19:36
@mscbot
Copy link
Collaborator

mscbot commented Dec 21, 2021

🔔 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 Dec 21, 2021
@mscbot
Copy link
Collaborator

mscbot commented Dec 26, 2021

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 Dec 26, 2021
@turt2live turt2live merged commit 3ce9c1d into main Dec 26, 2021
@turt2live turt2live deleted the msc_403 branch December 26, 2021 19:01
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review merged A proposal whose PR has merged into the spec! and removed finished-final-comment-period spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Dec 26, 2021
@turt2live
Copy link
Member

Merged 🎉

@H-Shay H-Shay mentioned this pull request Jan 13, 2022
Closed
@clokep clokep mentioned this pull request Feb 16, 2022
Open
@Johennes Johennes mentioned this pull request Jun 12, 2024
Merged
3 tasks
@richvdh
Copy link
Member

richvdh commented Jun 25, 2024

Spec PR was #3530

This was referenced Aug 19, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uhoreg uhoreg uhoreg left review comments

@KitsuneRal KitsuneRal KitsuneRal approved these changes

@turt2live turt2live turt2live approved these changes

Assignees
No one assigned
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
Spec Core Team Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@H-Shay @turt2live @mscbot @richvdh @uhoreg @KitsuneRal