From dd917bd4443a31babe8b1880ba284fb0d7e5a0e6 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 25 Nov 2021 16:36:46 +0000 Subject: [PATCH 01/10] Drop unstable prefix on `refresh_token` body parameter to login and register --- synapse/rest/client/login.py | 8 +++----- synapse/rest/client/register.py | 8 ++------ tests/rest/client/test_auth.py | 10 +++++----- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 09f378f91967..202cd574631c 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -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__() @@ -166,12 +166,10 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]: if self._msc2918_enabled: # 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 ) 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 diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 11fd6cd24d70..b1a5bf7dcea4 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -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 diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index d8a94f4c12b7..d7abe785c5a4 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -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) @@ -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) @@ -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", @@ -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", @@ -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", From 8099d816c916d702f0d77c26e7e0abc2d7c72aa6 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 25 Nov 2021 17:08:26 +0000 Subject: [PATCH 02/10] Switch to stable API endpoint for /refresh --- synapse/rest/client/login.py | 4 +--- tests/rest/client/test_auth.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 202cd574631c..e39c202290fb 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -458,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$")] def __init__(self, hs: "HomeServer"): self._auth_handler = hs.get_auth_handler() diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index d7abe785c5a4..557692d90820 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -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}, ) @@ -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) @@ -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) @@ -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( @@ -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( @@ -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( @@ -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( @@ -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( From 8682540e5dec6eb0a8114668685aabe25ba35fca Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 26 Nov 2021 11:24:35 +0000 Subject: [PATCH 03/10] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/11435.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11435.feature diff --git a/changelog.d/11435.feature b/changelog.d/11435.feature new file mode 100644 index 000000000000..9e127fae3cec --- /dev/null +++ b/changelog.d/11435.feature @@ -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. \ No newline at end of file From 5600fe498110453c9ba9249242166d5b9c047abc Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 26 Nov 2021 18:38:03 +0000 Subject: [PATCH 04/10] Document new configuration values --- synapse/config/registration.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 5e2154806048..2c45e4922f9e 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -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: From a8325ef6036f7f5a781981253c97567698cd322b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 26 Nov 2021 18:50:03 +0000 Subject: [PATCH 05/10] Fix tests added in concurrent branch --- tests/rest/client/test_auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 557692d90820..446dceb25385 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -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", @@ -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", From f371adab2eaa41ce22e3496684b8001338d95d3c Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 26 Nov 2021 18:53:54 +0000 Subject: [PATCH 06/10] Update sample config --- docs/sample_config.yaml | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index ae476d19ac8e..350f0966cc13 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -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: From 07247c236400b9ccba90b57105c8128538301ae0 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 29 Nov 2021 15:27:04 +0000 Subject: [PATCH 07/10] Rename msc2918_enabled to refresh_tokens_enabled --- synapse/rest/client/login.py | 4 ++-- synapse/rest/client/register.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index e39c202290fb..a1de9a57778b 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -90,7 +90,7 @@ def __init__(self, hs: "HomeServer"): self.saml2_enabled = hs.config.saml2.saml2_enabled self.cas_enabled = hs.config.cas.cas_enabled self.oidc_enabled = hs.config.oidc.oidc_enabled - self._msc2918_enabled = ( + self._refresh_tokens_enabled = ( hs.config.registration.refreshable_access_token_lifetime is not None ) @@ -163,7 +163,7 @@ def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]: login_submission = parse_json_object_from_request(request) - if self._msc2918_enabled: + if self._refresh_tokens_enabled: # Check if this login should also issue a refresh token, as per MSC2918 should_issue_refresh_token = login_submission.get( LoginRestServlet.REFRESH_TOKEN_PARAM, False diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index b1a5bf7dcea4..3bf74a8e282c 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -419,7 +419,7 @@ def __init__(self, hs: "HomeServer"): self.password_policy_handler = hs.get_password_policy_handler() self.clock = hs.get_clock() self._registration_enabled = self.hs.config.registration.enable_registration - self._msc2918_enabled = ( + self._refresh_tokens_enabled = ( hs.config.registration.refreshable_access_token_lifetime is not None ) @@ -445,7 +445,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: f"Do not understand membership kind: {kind}", ) - if self._msc2918_enabled: + if self._refresh_tokens_enabled: # Check if this registration should also issue a refresh token, as # per MSC2918 should_issue_refresh_token = body.get("refresh_token", False) From 0242790fd03dd28045a146a8ad0a0d8ca11e9dc6 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 29 Nov 2021 15:27:38 +0000 Subject: [PATCH 08/10] Always validate refresh_token parameter and simplify logic for deciding issuance --- synapse/rest/client/login.py | 19 ++++++++++--------- synapse/rest/client/register.py | 17 +++++++++-------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index a1de9a57778b..ecff6da2222d 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -163,15 +163,16 @@ def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]: login_submission = parse_json_object_from_request(request) - if self._refresh_tokens_enabled: - # Check if this login should also issue a refresh token, as per MSC2918 - should_issue_refresh_token = login_submission.get( - LoginRestServlet.REFRESH_TOKEN_PARAM, False - ) - if not isinstance(should_issue_refresh_token, bool): - raise SynapseError(400, "`refresh_token` should be true or false.") - else: - should_issue_refresh_token = False + # Check to see if the client requested a refresh token. + client_requested_refresh_token = login_submission.get( + LoginRestServlet.REFRESH_TOKEN_PARAM, False + ) + if not isinstance(client_requested_refresh_token, bool): + raise SynapseError(400, "`refresh_token` should be true or false.") + + should_issue_refresh_token = ( + self._refresh_tokens_enabled and client_requested_refresh_token + ) try: if login_submission["type"] in ( diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 3bf74a8e282c..8b56c76aed66 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -445,14 +445,15 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: f"Do not understand membership kind: {kind}", ) - if self._refresh_tokens_enabled: - # Check if this registration should also issue a refresh token, as - # per MSC2918 - should_issue_refresh_token = body.get("refresh_token", False) - if not isinstance(should_issue_refresh_token, bool): - raise SynapseError(400, "`refresh_token` should be true or false.") - else: - should_issue_refresh_token = False + # Check if the clients wishes for this registration to issue a refresh + # token. + client_requested_refresh_tokens = body.get("refresh_token", False) + if not isinstance(client_requested_refresh_tokens, bool): + raise SynapseError(400, "`refresh_token` should be true or false.") + + should_issue_refresh_token = ( + self._refresh_tokens_enabled and client_requested_refresh_tokens + ) # Pull out the provided username and do basic sanity checks early since # the auth layer will store these in sessions. From a8b7d3a236a80d4b7f277ad249c550f4a9125b9a Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 30 Nov 2021 09:45:21 +0000 Subject: [PATCH 09/10] Do away with pointless and obscuring CLIENT_API_PREFIX. --- synapse/rest/client/login.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index ecff6da2222d..ac6a94d5d54e 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -459,7 +459,7 @@ def _get_auth_flow_dict_for_idp(idp: SsoIdentityProvider) -> JsonDict: class RefreshTokenServlet(RestServlet): - PATTERNS = [re.compile("^" + CLIENT_API_PREFIX + "/v1/refresh$")] + PATTERNS = (re.compile("^/_matrix/client/v1/refresh$"),) def __init__(self, hs: "HomeServer"): self._auth_handler = hs.get_auth_handler() From 45450cf9ca1622943392307a53d975f981acd7a5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 3 Dec 2021 15:41:30 +0000 Subject: [PATCH 10/10] Add notes about lack of retrospective action to the new lifetime options --- docs/sample_config.yaml | 9 +++++++++ synapse/config/registration.py | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 350f0966cc13..6696ed5d1ef9 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1215,6 +1215,9 @@ oembed: # Note that this only applies to clients which advertise support for # refresh tokens. # +# Note also that this is calculated at login time and refresh time: +# changes are not applied to existing sessions until they are refreshed. +# # By default, this is 5 minutes. # #refreshable_access_token_lifetime: 5m @@ -1224,6 +1227,9 @@ oembed: # This option can be used to automatically log-out inactive sessions. # Please see the manual for more information. # +# Note also that this is calculated at login time and refresh time: +# changes are not applied to existing sessions until they are refreshed. +# # By default, this is infinite. # #refresh_token_lifetime: 24h @@ -1234,6 +1240,9 @@ oembed: # this to a short value may be inconvenient for some users who will # then be logged out frequently. # +# Note also that this is calculated at login time: changes are not applied +# retrospectively to existing sessions for users that have already logged in. +# # By default, this is infinite. # #nonrefreshable_access_token_lifetime: 24h diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 2c45e4922f9e..29112cdd2669 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -172,6 +172,9 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # Note that this only applies to clients which advertise support for # refresh tokens. # + # Note also that this is calculated at login time and refresh time: + # changes are not applied to existing sessions until they are refreshed. + # # By default, this is 5 minutes. # #refreshable_access_token_lifetime: 5m @@ -181,6 +184,9 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # This option can be used to automatically log-out inactive sessions. # Please see the manual for more information. # + # Note also that this is calculated at login time and refresh time: + # changes are not applied to existing sessions until they are refreshed. + # # By default, this is infinite. # #refresh_token_lifetime: 24h @@ -191,6 +197,9 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # this to a short value may be inconvenient for some users who will # then be logged out frequently. # + # Note also that this is calculated at login time: changes are not applied + # retrospectively to existing sessions for users that have already logged in. + # # By default, this is infinite. # #nonrefreshable_access_token_lifetime: 24h