Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Stabilise support for MSC2918 refresh tokens as they have now been merged into the Matrix specification. #11435

Merged
merged 10 commits into from
Dec 6, 2021
Merged

Stabilise support for MSC2918 refresh tokens as they have now been merged into the Matrix specification. #11435

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dd917bd
Drop unstable prefix on `refresh_token` body parameter to login and r…
reivilibre Nov 25, 2021
8099d81
Switch to stable API endpoint for /refresh
reivilibre Nov 25, 2021
8682540
Newsfile
reivilibre Nov 26, 2021
5600fe4
Document new configuration values
reivilibre Nov 26, 2021
a8325ef
Fix tests added in concurrent branch
reivilibre Nov 26, 2021
f371ada
Update sample config
reivilibre Nov 26, 2021
07247c2
Rename msc2918_enabled to refresh_tokens_enabled
reivilibre Nov 29, 2021
0242790
Always validate refresh_token parameter and simplify logic for decidi…
reivilibre Nov 29, 2021
a8b7d3a
Do away with pointless and obscuring CLIENT_API_PREFIX.
reivilibre Nov 30, 2021
45450cf
Add notes about lack of retrospective action to the new lifetime options
reivilibre Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11435.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stabilise support for [MSC2918](https://github.com/matrix-org/matrix-doc/blob/main/proposals/2918-refreshtokens.md#msc2918-refresh-tokens) refresh tokens as they have now been merged into the Matrix specification.
29 changes: 29 additions & 0 deletions docs/sample_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,6 +1209,35 @@ oembed:
#
#session_lifetime: 24h

# Time that an access token remains valid for, if the session is
# using refresh tokens.
# For more information about refresh tokens, please see the manual.
# Note that this only applies to clients which advertise support for
# refresh tokens.
#
# By default, this is 5 minutes.
#
#refreshable_access_token_lifetime: 5m

# Time that a refresh token remains valid for (provided that it is not
# exchanged for another one first).
# This option can be used to automatically log-out inactive sessions.
# Please see the manual for more information.
#
# By default, this is infinite.
#
#refresh_token_lifetime: 24h

# Time that an access token remains valid for, if the session is NOT
# using refresh tokens.
# Please note that not all clients support refresh tokens, so setting
# this to a short value may be inconvenient for some users who will
# then be logged out frequently.
#
# By default, this is infinite.
#
#nonrefreshable_access_token_lifetime: 24h

# The user must provide all of the below types of 3PID when registering.
#
#registrations_require_3pid:
Expand Down
29 changes: 29 additions & 0 deletions synapse/config/registration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,35 @@ def generate_config_section(self, generate_secrets=False, **kwargs):
#
#session_lifetime: 24h

# Time that an access token remains valid for, if the session is
# using refresh tokens.
# For more information about refresh tokens, please see the manual.
# Note that this only applies to clients which advertise support for
# refresh tokens.
#
# By default, this is 5 minutes.
#
#refreshable_access_token_lifetime: 5m

# Time that a refresh token remains valid for (provided that it is not
# exchanged for another one first).
# This option can be used to automatically log-out inactive sessions.
# Please see the manual for more information.
#
# By default, this is infinite.
#
#refresh_token_lifetime: 24h

# Time that an access token remains valid for, if the session is NOT
# using refresh tokens.
# Please note that not all clients support refresh tokens, so setting
# this to a short value may be inconvenient for some users who will
# then be logged out frequently.
#
# By default, this is infinite.
#
#nonrefreshable_access_token_lifetime: 24h

# The user must provide all of the below types of 3PID when registering.
#
#registrations_require_3pid:
Expand Down
12 changes: 4 additions & 8 deletions synapse/rest/client/login.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class LoginRestServlet(RestServlet):
JWT_TYPE_DEPRECATED = "m.login.jwt"
APPSERVICE_TYPE = "m.login.application_service"
APPSERVICE_TYPE_UNSTABLE = "uk.half-shot.msc2778.login.application_service"
REFRESH_TOKEN_PARAM = "org.matrix.msc2918.refresh_token"
REFRESH_TOKEN_PARAM = "refresh_token"

def __init__(self, hs: "HomeServer"):
super().__init__()
Expand Down Expand Up @@ -166,12 +166,10 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]:
if self._msc2918_enabled:
reivilibre marked this conversation as resolved.
Show resolved Hide resolved
# Check if this login should also issue a refresh token, as per MSC2918
should_issue_refresh_token = login_submission.get(
"org.matrix.msc2918.refresh_token", False
LoginRestServlet.REFRESH_TOKEN_PARAM, False
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for spelling the class name explicitly here, rather than using self?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(fwiw the rest of this class does that for all the constants)

This is probably a Javaism, where accessing static fields/methods using this (self) is discouraged as it camouflages the fact that it's static.
I don't know if it's also bad practice in Python code and no linters told me off for it, but it's something I still think I'd avoid for clarity.

Maybe I'm just a fool?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't have an authoritative answer for you other than "it looks weird to me". Not worth worrying about it though.

)
if not isinstance(should_issue_refresh_token, bool):
raise SynapseError(
400, "`org.matrix.msc2918.refresh_token` should be true or false."
)
raise SynapseError(400, "`refresh_token` should be true or false.")
else:
should_issue_refresh_token = False

Expand Down Expand Up @@ -460,9 +458,7 @@ def _get_auth_flow_dict_for_idp(idp: SsoIdentityProvider) -> JsonDict:


class RefreshTokenServlet(RestServlet):
PATTERNS = client_patterns(
"/org.matrix.msc2918.refresh_token/refresh$", releases=(), unstable=True
)
PATTERNS = [re.compile("^" + CLIENT_API_PREFIX + "/v1/refresh$")]
clokep marked this conversation as resolved.
Show resolved Hide resolved

def __init__(self, hs: "HomeServer"):
self._auth_handler = hs.get_auth_handler()
Expand Down
8 changes: 2 additions & 6 deletions synapse/rest/client/register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,13 +448,9 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
if self._msc2918_enabled:
# Check if this registration should also issue a refresh token, as
# per MSC2918
should_issue_refresh_token = body.get(
"org.matrix.msc2918.refresh_token", False
)
should_issue_refresh_token = body.get("refresh_token", False)
if not isinstance(should_issue_refresh_token, bool):
raise SynapseError(
400, "`org.matrix.msc2918.refresh_token` should be true or false."
)
raise SynapseError(400, "`refresh_token` should be true or false.")
else:
should_issue_refresh_token = False

Expand Down
30 changes: 15 additions & 15 deletions tests/rest/client/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ def use_refresh_token(self, refresh_token: str) -> FakeChannel:
"""
return self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": refresh_token},
)

Expand All @@ -544,7 +544,7 @@ def test_login_issue_refresh_token(self):
login_with_refresh = self.make_request(
"POST",
"/_matrix/client/r0/login",
{"org.matrix.msc2918.refresh_token": True, **body},
{"refresh_token": True, **body},
)
self.assertEqual(login_with_refresh.code, 200, login_with_refresh.result)
self.assertIn("refresh_token", login_with_refresh.json_body)
Expand Down Expand Up @@ -575,7 +575,7 @@ def test_register_issue_refresh_token(self):
"username": "test3",
"password": self.user_pass,
"auth": {"type": LoginType.DUMMY},
"org.matrix.msc2918.refresh_token": True,
"refresh_token": True,
},
)
self.assertEqual(register_with_refresh.code, 200, register_with_refresh.result)
Expand All @@ -590,7 +590,7 @@ def test_token_refresh(self):
"type": "m.login.password",
"user": "test",
"password": self.user_pass,
"org.matrix.msc2918.refresh_token": True,
"refresh_token": True,
}
login_response = self.make_request(
"POST",
Expand All @@ -601,7 +601,7 @@ def test_token_refresh(self):

refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": login_response.json_body["refresh_token"]},
)
self.assertEqual(refresh_response.code, 200, refresh_response.result)
Expand All @@ -628,7 +628,7 @@ def test_refreshable_access_token_expiration(self):
"type": "m.login.password",
"user": "test",
"password": self.user_pass,
"org.matrix.msc2918.refresh_token": True,
"refresh_token": True,
}
login_response = self.make_request(
"POST",
Expand All @@ -642,7 +642,7 @@ def test_refreshable_access_token_expiration(self):

refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": login_response.json_body["refresh_token"]},
)
self.assertEqual(refresh_response.code, 200, refresh_response.result)
Expand Down Expand Up @@ -685,7 +685,7 @@ def test_refresh_token_expiry(self):
"type": "m.login.password",
"user": "test",
"password": self.user_pass,
"org.matrix.msc2918.refresh_token": True,
"refresh_token": True,
}
login_response = self.make_request(
"POST",
Expand Down Expand Up @@ -735,7 +735,7 @@ def test_ultimate_session_expiry(self):
"type": "m.login.password",
"user": "test",
"password": self.user_pass,
"org.matrix.msc2918.refresh_token": True,
"refresh_token": True,
}
login_response = self.make_request(
"POST",
Expand Down Expand Up @@ -792,7 +792,7 @@ def test_refresh_token_invalidation(self):
"type": "m.login.password",
"user": "test",
"password": self.user_pass,
"org.matrix.msc2918.refresh_token": True,
"refresh_token": True,
}
login_response = self.make_request(
"POST",
Expand All @@ -804,7 +804,7 @@ def test_refresh_token_invalidation(self):
# This first refresh should work properly
first_refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": login_response.json_body["refresh_token"]},
)
self.assertEqual(
Expand All @@ -814,7 +814,7 @@ def test_refresh_token_invalidation(self):
# This one as well, since the token in the first one was never used
second_refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": login_response.json_body["refresh_token"]},
)
self.assertEqual(
Expand All @@ -824,7 +824,7 @@ def test_refresh_token_invalidation(self):
# This one should not, since the token from the first refresh is not valid anymore
third_refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": first_refresh_response.json_body["refresh_token"]},
)
self.assertEqual(
Expand Down Expand Up @@ -852,7 +852,7 @@ def test_refresh_token_invalidation(self):
# Now that the access token from the last valid refresh was used once, refreshing with the N-1 token should fail
fourth_refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": login_response.json_body["refresh_token"]},
)
self.assertEqual(
Expand All @@ -862,7 +862,7 @@ def test_refresh_token_invalidation(self):
# But refreshing from the last valid refresh token still works
fifth_refresh_response = self.make_request(
"POST",
"/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh",
"/_matrix/client/v1/refresh",
{"refresh_token": second_refresh_response.json_body["refresh_token"]},
)
self.assertEqual(
Expand Down