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

Fix definition of response of POST /_matrix/federation/v1/user/keys/claim #1559

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Jun 8, 2023

As discussed in #1351 (comment) and #1310 (comment).

The rendering of the type now looks like this (to compare with current state in the spec in the response of the endpoint):

image

Also fixes the rendering of nested OneOfs, so it has the added bonus of fixing the rendering of m.room.encrypted.

  1. It now shows the type of ciphertext:
    image
  2. And adds the definition of CiphertextInfo:
    image

cc @KitsuneRal to make sure I got it right this time.

Preview: https://pr1559--matrix-spec-previews.netlify.app

…laim

Also fixes the rendering of nested OneOf.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@zecakeh zecakeh requested a review from a team as a code owner June 8, 2023 20:05
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Comment on lines +75 to +76
- type: string
- type: object
Copy link
Member

Choose a reason for hiding this comment

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

I'm unconvinced this is correct. Is it true that a string can be returned here? The description doesn't seem to say anything about that being permissible.

@KitsuneRal you seem to have suggested this is correct at #1351 (comment), but I don't know why you said that?

Copy link
Member

Choose a reason for hiding this comment

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

Good question - might very well be a copy-pasta on my end. Can one-time keys be unsigned - guess not? If not, my suggestion is incorrect and there should be KeyObject here, without alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so it's not a copy-pasta on my end at least. The alternative between string and object has been here since the very beginning, even though undocumented. My comment in #1351 referred to the correct nesting level and I mentioned that string has to be returned only because it has been there before. If it hasn't been correct from the beginning then I'd rather we fix it in a separate PR but I'm fine either way.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I think I've figured this out!

Before we invented the signed_curve25519 key algorithm for one-time-keys, we just used curve25519 keys. These, not being signed, can be represented just by a string.

The curve25519 algorithm should no longer be used here, but removing them from the spec sounds like a different job.

So, 👍 to this fix.

Copy link
Member

@richvdh richvdh Jul 18, 2023

Choose a reason for hiding this comment

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

layouts/partials/json-schema/resolve-additional-types.html Outdated Show resolved Hide resolved
layouts/partials/json-schema/resolve-additional-types.html Outdated Show resolved Hide resolved
layouts/partials/json-schema/resolve-additional-types.html Outdated Show resolved Hide resolved
layouts/partials/json-schema/resolve-additional-types.html Outdated Show resolved Hide resolved
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Copy link
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 changes to the layouts files look good, but I remain unconvinced about the change to the endpoint definition.

@KitsuneRal KitsuneRal requested a review from richvdh July 18, 2023 09:24
@richvdh richvdh enabled auto-merge (squash) July 18, 2023 09:46
@richvdh richvdh merged commit 50fe89d into matrix-org:main Jul 18, 2023
@zecakeh zecakeh deleted the fix-user-claim branch July 18, 2023 10:59
@zecakeh zecakeh mentioned this pull request Aug 21, 2023
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants