Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Make federation endpoints more tolerant of trailing slashes for some endpoints#4793

Merged
anoadragon453 merged 13 commits into
developfrom
anoa/trailing_slashes
Mar 11, 2019
Merged

Make federation endpoints more tolerant of trailing slashes for some endpoints#4793
anoadragon453 merged 13 commits into
developfrom
anoa/trailing_slashes

Conversation

@anoadragon453
Copy link
Copy Markdown
Member

@anoadragon453 anoadragon453 commented Mar 4, 2019 •
edited
Loading

Receiving side of a solution towards #3622.

Client portion here: #4840

Makes Synapse accept zero or more trailing slashes on the federation endpoints mentioned in #3622.

@anoadragon453 anoadragon453 requested a review from a team March 4, 2019 14:51
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 4, 2019 •
edited
Loading

Codecov Report

Merging #4793 into develop will decrease coverage by 1%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4793      +/-   ##
===========================================
- Coverage    75.35%   74.34%   -1.01%     
===========================================
  Files          340      340              
  Lines        34889    36025    +1136     
  Branches      5708     6105     +397     
===========================================
+ Hits         26290    26784     +494     
- Misses        6988     7589     +601     
- Partials      1611     1652      +41

richvdh
richvdh previously requested changes Mar 4, 2019
View reviewed changes
Copy link
Copy Markdown
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.

this is a sensible first step modulo the nits below, but it does not, in itself, fix #3622: for that to happen we also need synapse to not include the slash on outgoing requests.

Comment thread synapse/federation/transport/server.py Outdated
Comment thread synapse/federation/transport/server.py Outdated
@richvdh richvdh mentioned this pull request Mar 7, 2019
Closed
@anoadragon453 anoadragon453 requested a review from richvdh March 8, 2019 12:06
@anoadragon453 anoadragon453 mentioned this pull request Mar 8, 2019
Closed
@anoadragon453 anoadragon453 changed the title Make federation endpoints more tolerant of trailing slashes Make federation endpoints more tolerant of trailing slashes for some endpoints Mar 8, 2019
richvdh
richvdh previously requested changes Mar 8, 2019
View reviewed changes
Copy link
Copy Markdown
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.

I'm confused about this. How is it going to work against existing servers?

I suggest you start with a PR which just makes the incoming endpoints more tolerant. Then open a different PR to sort out the outgoing requests.

@anoadragon453
Copy link
Copy Markdown
Member Author

anoadragon453 commented Mar 8, 2019

...good point.

@anoadragon453
Copy link
Copy Markdown
Member Author

anoadragon453 commented Mar 8, 2019 •
edited
Loading

This may also be relevant:

synapse/synapse/rest/client/v1/push_rule.py

Lines 33 to 36 in aba5eea

class PushRuleRestServlet(ClientV1RestServlet):
PATTERNS = client_path_patterns("/pushrules/.*$")
SLIGHTLY_PEDANTIC_TRAILING_SLASH_ERROR = (
"Unrecognised request: You probably wanted a trailing slash")

@anoadragon453 anoadragon453 mentioned this pull request Mar 8, 2019
Merged
3 tasks
@anoadragon453 anoadragon453 requested a review from richvdh March 8, 2019 16:58
@anoadragon453 anoadragon453 dismissed richvdh’s stale review March 8, 2019 16:58

Factored out client changes to #4840

richvdh
richvdh previously requested changes Mar 11, 2019
View reviewed changes
Copy link
Copy Markdown
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.

the UTs seem to be failing for some reason

Comment thread synapse/federation/transport/server.py Outdated
"""
PATH = (
"/groups/(?P<group_id>[^/]*)/categories/(?P<category_id>[^/]+)"
"/groups/(?P<group_id>[^/]*)/categories/(?P<category_id>[^/]+)/?"
Copy link
Copy Markdown
Member

@richvdh richvdh Mar 11, 2019

Choose a reason for hiding this comment

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

I'm pretty sure we decided adding an optional / here was incorrect

Copy link
Copy Markdown
Member Author

@anoadragon453 anoadragon453 Mar 11, 2019

Choose a reason for hiding this comment

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

Correct, we decided to leave out groups for now.

Comment thread synapse/federation/transport/server.py Outdated
"""
PATH = (
"/groups/(?P<group_id>[^/]*)/roles/(?P<role_id>[^/]+)"
"/groups/(?P<group_id>[^/]*)/roles/(?P<role_id>[^/]+)/?"
Copy link
Copy Markdown
Member

@richvdh richvdh Mar 11, 2019

Choose a reason for hiding this comment

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

ditto

@anoadragon453 anoadragon453 requested a review from richvdh March 11, 2019 16:51
@anoadragon453 anoadragon453 dismissed richvdh’s stale review March 11, 2019 16:52

Out of scoped groups

richvdh
richvdh approved these changes Mar 11, 2019
View reviewed changes
Copy link
Copy Markdown
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.

lgtm. Please squash-merge or I will hunt you down ;-)

@anoadragon453 anoadragon453 merged commit 290552f into develop Mar 11, 2019
@anoadragon453 anoadragon453 deleted the anoa/trailing_slashes branch March 11, 2019 17:44
erikjohnston added a commit that referenced this pull request Mar 14, 2019
@erikjohnston
Revert "Make federation endpoints more tolerant of trailing slashes f…
271cb19 
…or some endpoints (#4793)"

This reverts commit 290552f.
@erikjohnston erikjohnston mentioned this pull request Mar 14, 2019
Merged
@richvdh richvdh mentioned this pull request Mar 25, 2019
Closed
8 tasks
@anoadragon453 anoadragon453 restored the anoa/trailing_slashes branch March 25, 2019 17:04
@anoadragon453 anoadragon453 mentioned this pull request Mar 26, 2019
Merged
@anoadragon453 anoadragon453 deleted the anoa/trailing_slashes branch April 9, 2019 13:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@richvdh richvdh richvdh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@anoadragon453 @richvdh