Require refresh_token on refresh token endpoint

[BramvdnHeuvel]

At API endpoint POST /_matrix/client/v3/refresh, the refresh_token field is not marked as required. This implies that the client is allowed to refresh an access token without specifying a refresh token - which doesn't really make any sense.

Implications

It is not specified how the server should behave if the client sends an empty object as the request body, and the idea of sending an empty object is quite nonsensical for this endpoint. To solve this issue, this pull request specifies that the refresh_token is required on the given API endpoint.

Implementations

• Synapse already requires the refresh_token field.
• I haven't found a Dendrite implementation of this API endpoint.
• Conduit doesn't support refresh tokens.

Potential issues

Since the field has been specified as optional for a while, it is possible that a client has implemented a script that refreshes a long list of access tokens and then sends an empty object if no refresh token is available or supported. With the implementation of this bug fix, the expected response from a homeserver might change.

A given implementation might suggest smelly code if they intentionally send nonsensical requests to a homeserver, however, and I do not think that that is an issue worth considering.

Sign off

Signed-off-by: Bram van den Heuvel matrix-spec@noordstar.me

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

[BramvdnHeuvel]

This message adds context to the pull request. It is not required knowledge to understand the pull request.

Personal stake

I am writing a client library in Elm, a functional language that requires you to specify whether fields are required or optional when decoding JSON. The task that fetches the API endpoint is currently dealing with the following three scenarios:

1. The user provides a refresh token. The server returns a 200 response.
2. The user provides a refresh token. The server returns an unexpected response.
3. The user doesn't provide a refresh token. The server returns a response.

The spec doesn't define the server's response for case 3, and it might return a 200 response with an access token but without a refresh token. In that case, according to spec, Elm should take the PREVIOUS refresh token, which isn't given as an input in case 3. This increases the function's complexity significantly without adding any real value.

Temporary workaround

My current workaround is simply to not support not providing a refresh_token field (i.e. require a refresh token).

[dbkr]

Seems entirely sensible, although will let someone with more expertise here give their opinion before merging.

[richvdh]

for reference: this API was added in MSC2918, which does mention that absence of a refresh_token should result in a 401.