Use correct /v3 prefix for /refresh

[davidisaaclee]

Checklist

• Tests written for new code (and old code if feasible) – I only added tests for the default behavior, not for all configurations of the options I added. Will wait on review to add any more.
• Linter and other CI checks pass
• Sign-off given on the changes (see CONTRIBUTING.md)

Synapse v1.71 and below incorrectly exposed /refresh under the /v1 endpoint. This was fixed in matrix-org/synapse#14364, which was released with v1.72.0. This fix causes matrix-js-sdk's MatrixClient#refreshToken to fail when targeting newer Synapse instances (or any server which correctly implements the spec). matrix-js-sdk currently fails refreshToken when targeting the matrix.org homeserver.

I've supplied some alternative levels of fixing in this PR, since I don't know how committed you are to preserving backwards compatibility / matching the spec; my goal was that we could drop or squash commits as maintainers desire to match the expectations of this repo.

• a8df2a9 simply swaps out the /v1 prefix to instead use /v3. This is correct, but breaks clients targeting older Synapses.
• 2c85e75 gives an option to refreshToken for library consumers to explicitly ask for /v1; will break things by default, but consumers can individually manage calling the incorrect /v1 prefix.
• c8aa908 adds an option to first try the correct /v3 prefix, then retry with /v1 on M_UNRECOGNIZED, defaulting to false. Default behavior is correct to spec without extra weight, but breaks users on old Synapses; but users can opt in to an automatic fix by adding the option.
• 39edcd5 enables the retry option by default, which should work for everyone, but adds complexity by default (e.g. this could be confusing if /refresh raises an M_UNRECOGNIZED which is not relevant to this issue)

I noted that getRoomHierarchy has a similar approach, but for switching between unstable and stable endpoints:

matrix-js-sdk/src/client.ts

Lines 9266 to 9279 in e6bf5eb

	return this.http
	.authedRequest<IRoomHierarchy>(Method.Get, path, queryParams, undefined, {
	prefix: ClientPrefix.V1,
	})
	.catch((e) => {
	if (e.errcode === "M_UNRECOGNIZED") {
	// fall back to the prefixed hierarchy API.
	return this.http.authedRequest<IRoomHierarchy>(Method.Get, path, queryParams, undefined, {
	prefix: "/_matrix/client/unstable/org.matrix.msc2946",
	});
	}
	throw e;
	});

Signed-off-by: David Lee david.isaac.lee@gmail.com

---

Here's what your changelog entry will look like:

✨ Features

• Use correct /v3 prefix for /refresh (#3016). Contributed by @davidisaaclee.

[richvdh]

@davidisaaclee thanks very much for picking this up, and sorry that it appears to have languished rather than getting a prompt review.

In general, I'd say that exposing both forcePrefixV1 and retryWithPrefixV1OnUnrecognizedRequest to the application are unnecessary flexibility. The application should expect the SDK to take care of picking the right endpoint to use.

I think your default behaviour of retrying would be fine.

However, in this particular case, I think there's a strong argument that we shouldn't support /v1/refresh at all. It was never written down in the spec, and I would expect any users trying to use refresh tokens to be doing it with recent versions of Synapse. So personally I would drop all the fancy logic and just use /v3/refresh. YMMV though.

[davidisaaclee]

@richvdh Thank you for reviewing. It makes sense that the library should just work, so I'll remove these options.

I think there's a strong argument that we shouldn't support /v1/refresh at all [...] I would expect any users trying to use refresh tokens to be doing it with recent versions of Synapse

My motivation for this PR is that I'm trying to use refresh tokens with an older version of Synapse as well as with the matrix.org server, so the retry logic is directly helping my use case and likely would help others like me (probably what you meant by "YMMV") — so I would selfishly prefer to keep the default behavior of "try /v3/refresh first, then /v1/refresh on M_UNRECOGNIZED." Without that behavior, I would need to manually catch the error and do a raw HTTP request from my application – yuck!

I'll plan to make these code changes soon (requires more than reverts to add retry without exposing configuration param).

[davidisaaclee]

I rewrote git history on this branch since the changes were significant. To recap:

• Request /refresh with v3 prefix, and quietly fall back to v1 is the simpler approach of "try /v3 first; if that fails with M_UNRECOGNIZED, fall back to /v1" – no user configuration
• Add tests to ensure /v3/refresh is called + automatic /v1 retry checks the fallback behavior, and Add tests checking re-raising errors checks that errors raised in each prefixed endpoint are raised appropriately (which seemed like a potential bug given my implementation).

[richvdh]

other than my comments on comments, this looks great.

Please could you sign off your contribution by adding a comment to the PR with "Signed-off-by: ...", as per https://github.com/vector-im/element-web/blob/develop/CONTRIBUTING.md#sign-off.

[davidisaaclee]

@richvdh added sign-off to PR description

thanks again for review. I think everything is addressed now, lmk what's next!

[richvdh]

looks great, thank you!

[davidisaaclee]

Hey – is there anything I can do to help merge this?

[richvdh]

Thank you for the reminder; I don't know why github hasn't merged it. Hopefully it will work this time; if it hasn't auto-merged in a couple of hours feel free to ping me again