matrix-org / sydent

Sydent: Reference Matrix Identity Server
http://matrix.org
Apache License 2.0
303 stars 84 forks source link

`POST /_matrix/identity/v2/store-invite` returns unspecced `public_key` property #561

Open richvdh opened 1 year ago

richvdh commented 1 year ago

Sydent returns:

{
    "token": "token",
    "public_key": "sydentPubKeyBase64",
    "public_keys": [
        {
            "public_key": "sydentPubKeyBase64",
            "key_validity_url":  "https://<server>/_matrix/identity/api/v1/pubkey/isvalid"
        },
        {
            "public_key": "ephemeralPublicKeyBase64",
            "key_validity_url": "https://<server>/_matrix/identity/api/v1/pubkey/ephemeral/isvalid",
        }
    ],
    "display_name": "<redacted email>"
}

the top-level public_key property is redundant. It predates public_keys (added in #10, as modified by #13), and has never been specced. It should be removed.

anoadragon453 commented 1 year ago

@richvdh This issue talks about /_matrix/identity/v2/store-invite, whereas https://github.com/matrix-org/matrix-spec/issues/495 is talking about /_matrix/identity/api/v1/store-invite (v1 vs. v2). From the description of #495, it sounds like the public_key field should not have existed in either API?

public_key is redundant. ... It should be removed.

This is currently referenced by the following projects:

/_matrix/identity/v2/store-invite

/_matrix/identity/api/v1/store-invite

Hits in for public_key field did not appear in the following projects, but were checked explicitly: matrix-rust-sdk, matrix-ios-sdk, matrix-android-sdk2, Ruma, famedlysdk (Matrix Dart SDK), ma1sd, Construct and Complement.

richvdh commented 1 year ago

@richvdh This issue talks about /_matrix/identity/v2/store-invite, whereas matrix-org/matrix-spec#495 is talking about /_matrix/identity/api/v1/store-invite (v1 vs. v2).

I don't think matrix-org/matrix-spec#495 is specific to v1 or v2: it predates the existence of v2 (which was added in https://github.com/matrix-org/matrix-spec-proposals/pull/2255), but the same error was carried over to the v2 spec.

Note also that matrix-org/matrix-spec#495 is talking about the format of public_keys rather than the top-level public_key.

From the description of #495, it sounds like the public_key field should not have existed in either API?

I agree that public_key should not have existed in either API. When I opened this issue, I only mentioned v2 because v1 shouldn't exist at all (https://github.com/matrix-org/sydent/issues/338).


public_key is redundant. ... It should be removed.

This is currently referenced by the following projects:

/_matrix/identity/v2/store-invite

It's referenced in the model, but that doesn't necessarily mean the project is actually using it.

I'm also a bit surprised that a bot SDK needs to reference this. Per the ASCII art at https://spec.matrix.org/v1.6/client-server-api/#server-behaviour-7, it should be up to the homeserver to make this request.

Yup, looks like that line needs removing too, to make sytest return spec-compliant responses.

See https://github.com/matrix-org/synapse/issues/6036. We should remove it.

/_matrix/identity/api/v1/store-invite

As above, needs a fix.

Again, hard to tell if this means it's actually used. I'd hope that fixing sytest would confirm one way or the other.

Thanks for doing all this research!