Fix policy server signature merging again#19797
Conversation
| # than simply `update` the signatures on the event. | ||
| # because the event will fail authorization. This is why we add items individually | ||
| # rather than simply `update` the signatures on the event. | ||
| # |
There was a problem hiding this comment.
Is there some sort of test we could have for this kind of thing to prevent regressions?
Ideally, some sort of end-to-end Complement test. But I don't think we have any policy server tests in Complement yet. For the tests, I'm guessing it would be an engineered dummy server for the policy server like we have for engineered homeservers in Complement to federate with.
There was a problem hiding this comment.
Hmm, a unit test is probably possible, there are already other unit tests for this stuff
There was a problem hiding this comment.
Might be good enough.
Would love to see an actual end-to-end test as we could actually test whether the event is visible on other federating homeservers after it goes through the policy server. Instead of testing some arbitrary detail.
| # Remove any existing signatures to ensure we only return the new signature | ||
| # like the policy server spec says. | ||
| pdu_dict.pop("signatures", None) |
There was a problem hiding this comment.
Where in the spec is this defined? I'm not seeing it at https://spec.matrix.org/v1.18/server-server-api/#post_matrixpolicyv1sign or more generally in https://spec.matrix.org/v1.18/server-server-api/#policy-servers
If a room has enabled a Policy Server, the Policy Server’s signature appears alongside the normal event signatures, [...]
There was a problem hiding this comment.
The 200 response schema says
The Policy Server has signed the event, indicating that it recommends the event for inclusion in the room. Only the Policy Server’s signature is returned. This signature is to be added to the event before sending or processing the event further.
There was a problem hiding this comment.
Ahh, this is mocking the federation endpoint response (confusing method name).
Perhaps, this would also be more clear if we plucked the relevant single signature out to return instead.
| self._sign_with_random_key("example.org", event) | ||
| self.mock_federation_transport_client.ask_policy_server_to_sign_event.side_effect = self.policy_server_signs_event | ||
| self.get_success( | ||
| self.handler.ask_policy_server_to_sign_event(event, verify=True) | ||
| ) | ||
| self.assertEqual(len(event.signatures), 2) | ||
| self.assertEqual(len(event.signatures[self.OTHER_SERVER_NAME]), 1) |
There was a problem hiding this comment.
Probably needs some comments to describe what we're trying to do at each step and what it's trying to test
Same with the other test
There was a problem hiding this comment.
Added comments to both
| }, | ||
| }, | ||
| ) | ||
| self._sign_with_random_key("example.org", event) |
There was a problem hiding this comment.
Does this test really care about this detail? Feels like something that should be split out to another test
There was a problem hiding this comment.
Having signatures from 2 different servers (origin & policy server) is the standard case that the vast majority of events will hit, which is why I added it to the "ok" test. Having only a signature from a policy server never happens in practice. I could add another test anyway instead if that's better
There was a problem hiding this comment.
We should comment about the case. // Sign the event as the origin homeserver (normal part of the process) - Something better/more precise.
| self.get_success( | ||
| self.handler.ask_policy_server_to_sign_event(event, verify=True) | ||
| ) | ||
| self.assertEqual(len(event.signatures), 2) |
There was a problem hiding this comment.
Kinda part of the existing code but we should use a better assertion that actually compares the servers and number of keys and has a better error message of what's actually different.
| self.assertEqual(len(event.signatures), 2) | |
| self.assertEqual( | |
| {server: len(signatures) for server, signatures in event.signatures.items()}, | |
| TODO, | |
| "Expected signatures for the origin homeserver (%s) and policy server (%s)" | |
| ) |
There was a problem hiding this comment.
Just added another assert for the origin server signatures length
There was a problem hiding this comment.
The point of the assertion I'm suggesting is that it gives better info when it fails on top of asserting things more fully.
| self.assertEqual(len(event.signatures), 2) | ||
| self.assertEqual(len(event.signatures[self.OTHER_SERVER_NAME]), 1) | ||
|
|
||
| def test_ask_origin_server_to_sign_event_doesnt_replace_signatures(self) -> None: |
There was a problem hiding this comment.
This could definitely use test description to describe the point of this test. Especially when compared to test_ask_policy_server_to_sign_event_ok
There was a problem hiding this comment.
Are the comments on the asserts enough for this or should there be something else?
There was a problem hiding this comment.
Comment is decent but I think it deserves a better overview on what/why we care about.
| # Remove any existing signatures to ensure we only return the new signature | ||
| # like the policy server spec says. | ||
| pdu_dict.pop("signatures", None) |
There was a problem hiding this comment.
Ahh, this is mocking the federation endpoint response (confusing method name).
Perhaps, this would also be more clear if we plucked the relevant single signature out to return instead.
| self.get_success( | ||
| self.handler.ask_policy_server_to_sign_event(event, verify=True) | ||
| ) | ||
| self.assertEqual(len(event.signatures), 2) |
There was a problem hiding this comment.
The point of the assertion I'm suggesting is that it gives better info when it fails on top of asserting things more fully.
| }, | ||
| }, | ||
| ) | ||
| self._sign_with_random_key(self.OTHER_SERVER_NAME, event) |
There was a problem hiding this comment.
self.OTHER_SERVER_NAME is special here (and done on purpose) as it matches what the policy_server_signs_event(...) mock is doing to stress what this PR is fixing. (comment to call it out)
_sign_with_random_key(...) could use a better name as in this case, it's not that "random".
| signatures = signature.get(policy_server.server_name, {}) | ||
| for key_id, sig in signatures.items(): | ||
| event.signatures.add_signature(policy_server.server_name, key_id, sig) |
There was a problem hiding this comment.
Doing it this way is also better because we're strictly pulling out the signature for the policy_server.server_name instead of blindly trusting the entire response which can overwrite and add stray signatures for other servers.
Although we're probably in a cooperative setting anyway (we can assume you trust the policy server you have set).
It would be good to validate this earlier when we get the response. Something that can be tackled in another PR.
| self.assertEqual(len(event.signatures), 2) | ||
| self.assertEqual(len(event.signatures[self.OTHER_SERVER_NAME]), 1) | ||
|
|
||
| def test_ask_origin_server_to_sign_event_doesnt_replace_signatures(self) -> None: |
There was a problem hiding this comment.
Comment is decent but I think it deserves a better overview on what/why we care about.
| }, | ||
| }, | ||
| ) | ||
| self._sign_with_random_key("example.org", event) |
There was a problem hiding this comment.
We should comment about the case. // Sign the event as the origin homeserver (normal part of the process) - Something better/more precise.
2 participants
Fixes #19796
Haven't tested this in practice yet, but it's pretty simple
Pull Request Checklist