From 76f9c701c3920d83c0fe8f08b9197e2e92e12dad Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Jun 2021 11:07:28 -0400 Subject: [PATCH] Always require users to re-authenticate for dangerous operations. (#10184) Dangerous actions means deactivating an account, modifying an account password, or adding a 3PID. Other actions (deleting devices, uploading keys) can re-use the same UI auth session if ui_auth.session_timeout is configured. --- changelog.d/10184.bugfix | 1 + docs/sample_config.yaml | 4 ++++ synapse/config/auth.py | 4 ++++ synapse/handlers/auth.py | 7 ++++++- synapse/rest/client/v2_alpha/devices.py | 6 ++++++ synapse/rest/client/v2_alpha/keys.py | 3 +++ 6 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 changelog.d/10184.bugfix diff --git a/changelog.d/10184.bugfix b/changelog.d/10184.bugfix new file mode 100644 index 000000000..6bf440d8f --- /dev/null +++ b/changelog.d/10184.bugfix @@ -0,0 +1 @@ +Always require users to re-authenticate for dangerous operations: deactivating an account, modifying an account password, and adding 3PIDs. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index f8925a5e2..2ab88eb14 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 e10d641a9..53809cee2 100644 --- a/synapse/config/auth.py +++ b/synapse/config/auth.py @@ -103,6 +103,10 @@ class AuthConfig(Config): # 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 8a6666a4a..1971e373e 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -302,6 +302,7 @@ class AuthHandler(BaseHandler): 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 @@ class AuthHandler(BaseHandler): 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 @@ class AuthHandler(BaseHandler): """ 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 9af05f9b1..8b9674db0 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -86,6 +86,9 @@ class DeleteDevicesRestServlet(RestServlet): request, body, "remove device(s) from your account", + # 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, ) await self.device_handler.delete_devices( @@ -135,6 +138,9 @@ class DeviceRestServlet(RestServlet): request, body, "remove a device from your account", + # 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, ) 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 4a28f2c07..33cf8de18 100644 --- a/synapse/rest/client/v2_alpha/keys.py +++ b/synapse/rest/client/v2_alpha/keys.py @@ -277,6 +277,9 @@ class SigningKeyUploadServlet(RestServlet): 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)