From 570d602f2e8b44308571904c8d384b1518d266da Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Jun 2021 08:31:04 -0400 Subject: [PATCH 1/4] Only allow skipping UI auth for certain activities. --- docs/sample_config.yaml | 4 ++++ synapse/config/auth.py | 4 ++++ synapse/handlers/auth.py | 7 ++++++- synapse/rest/client/v2_alpha/devices.py | 8 ++++++++ synapse/rest/client/v2_alpha/keys.py | 3 +++ 5 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index f8925a5e2428..2ab88eb14e85 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -2318,6 +2318,10 @@ ui_auth: # the user-interactive authentication process, by allowing for multiple # (and potentially different) operations to use the same validation session. # + # This is ignored for potentially "dangerous" operations (including + # deactivating an account, modifying an account password, and + # adding a 3PID). + # # Uncomment below to allow for credential validation to last for 15 # seconds. # diff --git a/synapse/config/auth.py b/synapse/config/auth.py index e10d641a9655..53809cee2ecd 100644 --- a/synapse/config/auth.py +++ b/synapse/config/auth.py @@ -103,6 +103,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # the user-interactive authentication process, by allowing for multiple # (and potentially different) operations to use the same validation session. # + # This is ignored for potentially "dangerous" operations (including + # deactivating an account, modifying an account password, and + # adding a 3PID). + # # Uncomment below to allow for credential validation to last for 15 # seconds. # diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 8a6666a4ade6..1971e373ed03 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -302,6 +302,7 @@ async def validate_user_via_ui_auth( request: SynapseRequest, request_body: Dict[str, Any], description: str, + can_skip_ui_auth: bool = False, ) -> Tuple[dict, Optional[str]]: """ Checks that the user is who they claim to be, via a UI auth. @@ -320,6 +321,10 @@ async def validate_user_via_ui_auth( description: A human readable string to be displayed to the user that describes the operation happening on their account. + can_skip_ui_auth: True if the UI auth session timeout applies this + action. Should be set to False for any "dangerous" + actions (e.g. deactivating an account). + Returns: A tuple of (params, session_id). @@ -343,7 +348,7 @@ async def validate_user_via_ui_auth( """ if not requester.access_token_id: raise ValueError("Cannot validate a user without an access token") - if self._ui_auth_session_timeout: + if can_skip_ui_auth and self._ui_auth_session_timeout: last_validated = await self.store.get_access_token_last_validated( requester.access_token_id ) diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py index 9af05f9b11a4..977ea59405c5 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -86,6 +86,10 @@ async def on_POST(self, request): request, body, "remove device(s) from your account", + # It is frequently that users might want to call this multiple times + # in a row while cleaning up devices, so allow a single UI auth + # session to be re-used. + can_skip_ui_auth=True, ) await self.device_handler.delete_devices( @@ -135,6 +139,10 @@ async def on_DELETE(self, request, device_id): request, body, "remove a device from your account", + # It is frequently that users might want to call this multiple times + # in a row while cleaning up devices, so allow a single UI auth + # session to be re-used. + can_skip_ui_auth=True, ) await self.device_handler.delete_device(requester.user.to_string(), device_id) diff --git a/synapse/rest/client/v2_alpha/keys.py b/synapse/rest/client/v2_alpha/keys.py index 4a28f2c07243..33cf8de18633 100644 --- a/synapse/rest/client/v2_alpha/keys.py +++ b/synapse/rest/client/v2_alpha/keys.py @@ -277,6 +277,9 @@ async def on_POST(self, request): request, body, "add a device signing key to your account", + # Allow skipping of UI auth since this is frequently called directly + # after login and it is silly to ask users to re-auth immediately. + can_skip_ui_auth=True, ) result = await self.e2e_keys_handler.upload_signing_keys_for_user(user_id, body) From 8ea5848a5f87a6b53b7ec9a2b3cffc7682e5dd87 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Jun 2021 08:35:38 -0400 Subject: [PATCH 2/4] Newsfragment --- changelog.d/10184.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10184.bugfix diff --git a/changelog.d/10184.bugfix b/changelog.d/10184.bugfix new file mode 100644 index 000000000000..2f9af5c612c5 --- /dev/null +++ b/changelog.d/10184.bugfix @@ -0,0 +1 @@ +Do not apply the `ui_auth.session_timeout` to dangerous operations: deactivating an account, modifying an account password, and adding 3PIDs. From ff97bdc9126ae355d88e4efbecf2b742e6e88476 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Jun 2021 10:07:44 -0400 Subject: [PATCH 3/4] Update newsfragment. Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/10184.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10184.bugfix b/changelog.d/10184.bugfix index 2f9af5c612c5..6bf440d8f87b 100644 --- a/changelog.d/10184.bugfix +++ b/changelog.d/10184.bugfix @@ -1 +1 @@ -Do not apply the `ui_auth.session_timeout` to dangerous operations: deactivating an account, modifying an account password, and adding 3PIDs. +Always require users to re-authenticate for dangerous operations: deactivating an account, modifying an account password, and adding 3PIDs. From c97b0eb925f647873eccd3c7f4f8530e10fd4555 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Jun 2021 10:14:41 -0400 Subject: [PATCH 4/4] Fix comments. --- synapse/rest/client/v2_alpha/devices.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py index 977ea59405c5..8b9674db064a 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -86,9 +86,8 @@ async def on_POST(self, request): request, body, "remove device(s) from your account", - # It is frequently that users might want to call this multiple times - # in a row while cleaning up devices, so allow a single UI auth - # session to be re-used. + # Users might call this multiple times in a row while cleaning up + # devices, allow a single UI auth session to be re-used. can_skip_ui_auth=True, ) @@ -139,9 +138,8 @@ async def on_DELETE(self, request, device_id): request, body, "remove a device from your account", - # It is frequently that users might want to call this multiple times - # in a row while cleaning up devices, so allow a single UI auth - # session to be re-used. + # Users might call this multiple times in a row while cleaning up + # devices, allow a single UI auth session to be re-used. can_skip_ui_auth=True, )