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.

6 files changed, 24 insertions, 1 deletions

diff --git a/changelog.d/10184.bugfix b/changelog.d/10184.bugfix
new file mode 100644
index 0000000000..6bf440d8f8
--- /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 f8925a5e24..2ab88eb14e 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 e10d641a96..53809cee2e 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 8a6666a4ad..1971e373ed 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 9af05f9b11..8b9674db06 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 4a28f2c072..33cf8de186 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)