search

matrix-org / matrix-spec

The Matrix protocol specification
Apache License 2.0
188 stars 94 forks source link

Clarify that an access token is optional on `/account/password` and `/account/deactivate` #1843

Closed zecakeh closed 3 months ago

zecakeh commented 3 months ago

The descriptions of the endpoints clearly state that the access token is not required, but it also says:

Requires Authentication: Yes

This changes it to show:

Requires Authentication: Optional

Fixes #1807.

Pull Request Checklist

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

uhoreg commented 3 months ago

Looking at the synapse code, /account/password indeed doesn't seem to require authentication, but /account/deactivate does, AFAICT. I haven't checked what other servers do yet. I think we may need to untangle whether the discrepancy in /account/deactivate is a spec bug or a synapse bug.

zecakeh commented 3 months ago

From what I can tell both Dendrite and Conduit seem to require authentication for both endpoints.

richvdh commented 3 months ago

It took me quite a while to grok what was going on here. Clearly, both endpoints require authentication of some form -- anything else would be anarchy where people go around changing other peoples' passwords -- so the phrasing of the PR is a bit confusing.

So, for the record: both endpoints require authentication via UIA. The question at stake is whether they also need an Authorization header with an access token. The spec is inconsistent. As @uhoreg says, Synapse's implementation of /account/deactivate requires an access token, but that of /account/password does not.

richvdh commented 3 months ago

Doing some archaeology:

/password was implemented in https://github.com/matrix-org/synapse/pull/126/files#diff-20445d423fa1657115c589560de173027dbba8b9a866b21fb3592cd55e30dc50R31 and specced in https://github.com/matrix-org/matrix-spec-proposals/pull/190. We can see from the start that the access token was not required if you did UIA with m.login.email.identity, which remains the case today; however the swagger implied that an access token was required. @zecakeh's change seems correct here and corrects a 9-year-old spec bug!

/deactivate was implemented in https://github.com/matrix-org/synapse/pull/921 and specced in https://github.com/matrix-org/matrix-doc/pull/361. The spec implies that an access token may not be required, but in practice, an access token was required (https://github.com/matrix-org/synapse/pull/921/files#diff-20445d423fa1657115c589560de173027dbba8b9a866b21fb3592cd55e30dc50R150).

My impression is that a server could implement /deactivate without the access token, for example by allowing UIA with m.login.token, and be spec-compliant. The fact that no implementation has yet done so isn't important.

In short: LGTM

zecakeh commented 3 months ago

It took me quite a while to grok what was going on here. Clearly, both endpoints require authentication of some form -- anything else would be anarchy where people go around changing other peoples' passwords -- so the phrasing of the PR is a bit confusing.

In that case, should we consider that the phrasing of the spec is also confusing? Do we need to change "Requires Authentication" to "Requires Access Token"?

richvdh commented 3 months ago

In that case, should we consider that the phrasing of the spec is also confusing? Do we need to change "Requires Authentication" to "Requires Access Token"?

I think something like that would be good, yes. Not sure exactly what words to use though.