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

Handle HttpResponseException more safely for federated groups #3969

Merged
merged 8 commits into from Oct 23, 2018

Conversation

Projects
None yet
2 participants
@turt2live
Member

turt2live commented Sep 26, 2018

This doesn't feel right, but I think it's in the right direction? This fixes vector-im/riot-web#7204 although it does so by trapping all 4xx errors and re-throwing them for synapse to handle differently, specifically on the group API.

Riot already has the required bits for handling the 403, but synapse currently returns a 500 instead. See vector-im/riot-web#7204 for details.

Sytest: matrix-org/sytest#512

@turt2live turt2live requested a review from matrix-org/synapse-core Sep 26, 2018

@richvdh

could we have a sytest please?

e = failure.value
if e.code >= 400 and e.code < 500:
raise SynapseError(e.code, e.msg)
failure.raiseException()

This comment has been minimized.

@richvdh

richvdh Sep 27, 2018

Member

I think you can just return failure here.

failure.trap(HttpResponseException)
e = failure.value
if e.code >= 400 and e.code < 500:
raise SynapseError(e.code, e.msg)

This comment has been minimized.

@richvdh

richvdh Sep 27, 2018

Member

note that HttpResponseException has a to_synapse_error method which you probably want here.

def h(failure):
failure.trap(HttpResponseException)
e = failure.value
if e.code >= 400 and e.code < 500:

This comment has been minimized.

@richvdh

richvdh Sep 27, 2018

Member

I think I'd like us to be conservative about which error codes we pass through here; can you start with if e.code == 403 and we'll work up from there?

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 27, 2018

(otherwise it looks sensible to me)

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 27, 2018

(CI is failing due to pep8/import sorting)

@turt2live

This comment has been minimized.

Member

turt2live commented Oct 12, 2018

I'll work on sytests next week alongside the other PRs I have.

turt2live added a commit to matrix-org/sytest that referenced this pull request Oct 17, 2018

@turt2live turt2live requested a review from richvdh Oct 17, 2018

@turt2live turt2live removed their assignment Oct 17, 2018

@turt2live

This comment has been minimized.

Member

turt2live commented Oct 17, 2018

Have made a sytest: matrix-org/sytest#512

@richvdh

lgtm otherwise

Show resolved Hide resolved synapse/handlers/groups_local.py

@turt2live turt2live merged commit 43c3f0b into matrix-org:develop Oct 23, 2018

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@turt2live turt2live deleted the turt2live:travis/fix-federated-group-requests branch Oct 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment