This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement MSC3984 to proxy /keys/query requests to appservices. #15321
Implement MSC3984 to proxy /keys/query requests to appservices. #15321
Changes from 1 commit
5690b28
943291a
4a40ffe
2e37b01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really OK to just eat up errors and return
{}
? It's not clear how come this is safe — would benefit from an explanation here/in docstring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MSC says:
I can expand the docstring though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. The MSC should probably explain why this is safe, then — as it is I'm not convinced? Is this really so unimportant that doing nothing is fine if there's an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave a comment on the MSC!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-link matrix-org/matrix-spec-proposals#3984 (comment)
Are you OK with closing this for now then? We can update the implementation based on changes to the MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to get something into the code because realistically as soon as this thread gets closed it will be forgotten about for the rest of time and we'll be stuck with it.
Unfortunately I think the answer is slowly revealing itself as 'this isn't safe to do but we do it anyway'. This feels like a design problem in an already warty system — I'm not really happy with it and it feels like we should consider whether there are other options here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we have the thread in the MSC for tracking. That's usually what we've done for this sort of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty big TODO compared to what the MSC says, but it isn't abundantly clear how to merge the cross-signing info (
master_keys
/self_signing_keys
/user_signing_keys
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also -- that will likely require some refactoring since
get_cross_signing_keys_from_cache
gets called after this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MSC now kind of shrugs at this 🤷 , see matrix-org/matrix-spec-proposals#3984 (comment)
I think it is probably OK to gloss over this for now.