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

Cross-signing [3/4] -- uploading signatures edition #5726

Merged
merged 21 commits into from Oct 22, 2019

Conversation

@uhoreg
Copy link
Member

uhoreg commented Jul 19, 2019

DO NOT MERGE until uhoreg/e2e_cross-signing2 is merged, and the base of this PR changes to develop

Allows users to upload signatures using the new /keys/signatures/upload endpoint.

The upload_signatures_for_device_keys is really long. I'm not sure what's the best way to make it more manageable.

@uhoreg uhoreg changed the title Cross-signing -- uploading signatures edition Cross-signing [2/3] -- uploading signatures edition Jul 19, 2019
@uhoreg uhoreg referenced this pull request Jul 19, 2019
@uhoreg uhoreg force-pushed the uhoreg/e2e_cross-signing2-part2 branch from 9306355 to 9d38b5e Jul 22, 2019
@uhoreg uhoreg requested a review from matrix-org/synapse-core Jul 22, 2019
Copy link
Member

richvdh left a comment

I haven't done a full review here, just some ideas for upload_signatures_for_device_keys. I'm going to wait for #4970 to mature a bit more before proceeding.

synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
device_id
] = _exception_to_failure(e)
except SynapseError as e:
failures[user_id] = {

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 23, 2019

Member

kinda wondering under what circumstances we are likely to hit this case, and whether it is worth special handling rather than just failing the whole request.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Sep 6, 2019

Author Member

I'm not sure. It's probably a spec discussion about whether the server can fail the whole request, and under what conditions.

@uhoreg uhoreg force-pushed the uhoreg/e2e_cross-signing2-part2 branch from 47c5925 to 415d0a0 Sep 6, 2019
@uhoreg uhoreg changed the base branch from uhoreg/e2e_cross-signing2 to uhoreg/e2e_cross-signing_merged Sep 6, 2019
@uhoreg uhoreg changed the title Cross-signing [2/3] -- uploading signatures edition Cross-signing [3/4] -- uploading signatures edition Sep 6, 2019
@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Sep 7, 2019

There seems to be an intermittent sytest is failure. b6e3dec failed with

not ok 268 Real non-joined user doesn't get events before room made world_readable
# Started: 2019-09-07 17:27:56.835
# Ended: 2019-09-07 17:28:06.949
# Timed out waiting for test at ./run-tests.pl line 727.
# 0.091850: Registered new user @anon-20190907_172406-230:localhost:8828
# 0.108166: Registered new user @anon-20190907_172406-231:localhost:8828
# 0.781076: room_id=!dtQHGsYefGEhtkVyUv:localhost:8828

but d3f2fbc (which only adds documentation) passed. I suspect that it is unrelated to my changes.

I think this is ready for review again.

@uhoreg uhoreg requested a review from matrix-org/synapse-core Sep 7, 2019
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Sep 9, 2019

There seems to be an intermittent sytest is failure. b6e3dec failed with

Indeed. I've raised matrix-org/sytest#696.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Sep 9, 2019

please could you link to the MSC or spec this is implementing?

Also: can this target develop rather than an intermediate branch?

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Sep 9, 2019

Also: can this target develop rather than an intermediate branch?

I've realised that the earlier PRs have landed on said intermediate branch. I'm not a fan of long-lived feature branches: can we get it merged to develop?

@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Sep 9, 2019

please could you link to the MSC or spec this is implementing?

matrix-org/matrix-doc#1756

I've realised that the earlier PRs have landed on said intermediate branch. I'm not a fan of long-lived feature branches: can we get it merged to develop?

@erikjohnston had suggested that I merge to a separate branch rather than develop. I can do either way, so the two of you can fight it out discuss it, and let me know what I should do.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Sep 10, 2019

please could you link to the MSC or spec this is implementing?

matrix-org/matrix-doc#1756

thanks.

I've realised that the earlier PRs have landed on said intermediate branch. I'm not a fan of long-lived feature branches: can we get it merged to develop?

@erikjohnston had suggested that I merge to a separate branch rather than develop. I can do either way, so the two of you can fight it out discuss it, and let me know what I should do.

I've discussed this with erik a bit, and I think our position goes like this: we prefer to avoid feature branches (especially long-lived ones) because:

  • if things are in develop, it saves having to repeatedly merge develop into the branch (or run the risk of it bit-rotting)
  • having a feature branch makes it easy to walk into a trap where PRs can be of poor quality, or introduce bugs, etc, because it's "only a feature branch", which means you rapidly lose track of what is left to be done to clean it up. (Not an issue in this case, I hasten to point out: just a natural consequence.)
  • in general, it's preferable to think about how to break a large change up so that it can safely land incrementally, rather than (say) splitting it up by file where none of it can land until all of it does - which again makes it easy to lose track of what's left to be done (and in many ways isn't much of an improvement over a single huge PR, since you still end up with the cognitive load of having to hold the whole change in your head at once). Again a feature branch makes it easier to fall into this trap.

On the other hand:

  • if it's really impossible to land a large change incrementally, we certainly prefer a feature branch with a few PRs over a 1000-line diff (I'm unconvinced that this is a real thing, though).
  • If you're making DB changes, and there's likely to be more change down the line, that's an argument for keeping things on a feature branch, since if you discover that the original DB schema was a bit wrong, you have to write a migration if the first PR has landed on develop.

TL;DR: If there are no particular reasons why the changes so far can't land on develop, I'd like to see them merged. If there are such reasons, could you identify them?

Copy link
Member

richvdh left a comment

There's a lot of stuff here, and I'm struggling to hold it all in my head, so I'll take your word for it that it's doing about the right thing. A few requests though.

As a general principle: I'd like to see more validation of input (eg checking that things that should be dicts are in fact dicts), so that we return a 400 rather than a 500 in the case of malformed requests.

synapse/handlers/e2e_keys.py Show resolved Hide resolved
synapse/handlers/e2e_keys.py Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
uhoreg added 4 commits Sep 24, 2019
@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Sep 25, 2019

Sytest failures seem to be new 3pid-related stuff, which will hopefully get fixed when I merge develop in (again). The synapse unit test failure seems to be my old friend test_replace_master_key which is the unit test failure from #5769 that I had thought that I had fixed by using stream ID generators.

Aside from that, though, I think this is ready for review again now. I can fix #5726 (comment) if what I've written there makes sense.

@uhoreg uhoreg requested a review from matrix-org/synapse-core Sep 25, 2019
synapse/handlers/e2e_keys.py Show resolved Hide resolved
synapse/handlers/e2e_keys.py Show resolved Hide resolved
synapse/handlers/e2e_keys.py Show resolved Hide resolved
if (
signing_device_id not in devices
or signing_key_id
not in devices[signing_device_id]["keys"]["keys"]

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 3, 2019

Member

that sounds sensible, I think. I'd love it if it could be a preliminary PR rather than rolled into this one, though.

Copy link
Member

richvdh left a comment

Once again I'm afraid I've totally paged out most of the context on this PR, but I think it's probably sensible. As well as the nits above, and fixing whatever's wrong with the CI: can we target develop as per #5726 (comment)?

@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Oct 3, 2019

As well as the nits above, and fixing whatever's wrong with the CI: can we target develop as per #5726 (comment)?

Yes, I'll do that, although I want to try to fix the CI before I merge the previous PRs into develop, so that we don't have a broken CI in develop.

@uhoreg uhoreg changed the base branch from uhoreg/e2e_cross-signing_merged to develop Oct 18, 2019
... again?  How did you make it disappear, git?
@uhoreg uhoreg requested a review from richvdh Oct 18, 2019
@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Oct 18, 2019

I think I've addressed all the issues, and I've re-merged develop so the tests pass. I've also changed this PR so that it targets develop, now that the previous PRs have been merged in.

Copy link
Member

richvdh left a comment

Looks plausible!

@uhoreg uhoreg merged commit 2761731 into develop Oct 22, 2019
18 checks passed
18 checks passed
buildkite/synapse Build #4991 passed (22 minutes, 39 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 50 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 37 seconds)
Details
buildkite/synapse/isort Passed (18 seconds)
Details
buildkite/synapse/mypy Passed (51 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (16 seconds)
Details
buildkite/synapse/packaging Passed (44 seconds)
Details
buildkite/synapse/pipeline Passed (2 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (16 minutes, 49 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (7 minutes, 17 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (9 minutes, 26 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (6 minutes, 49 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (16 minutes, 44 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 18 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 50 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (13 minutes, 51 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (16 minutes, 7 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (15 minutes, 25 seconds)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.