From b030fdbce52d9ef111ca7f58511d0df2347b0558 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 10 Apr 2020 10:18:05 -0400 Subject: [PATCH 01/36] Persist the storage of UI Auth sessions into the database. --- synapse/handlers/auth.py | 124 ++---------- synapse/handlers/cas_handler.py | 2 +- synapse/handlers/saml_handler.py | 2 +- synapse/rest/client/v2_alpha/register.py | 4 +- synapse/storage/data_stores/main/__init__.py | 2 + .../main/schema/delta/58/persist_ui_auth.sql | 34 ++++ synapse/storage/data_stores/main/ui_auth.py | 179 ++++++++++++++++++ 7 files changed, 235 insertions(+), 112 deletions(-) create mode 100644 synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql create mode 100644 synapse/storage/data_stores/main/ui_auth.py diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index dbe165ce1ed5..b06dc43e0224 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -44,7 +44,6 @@ from synapse.module_api import ModuleApi from synapse.push.mailer import load_jinja2_templates from synapse.types import Requester, UserID -from synapse.util.caches.expiringcache import ExpiringCache from ._base import BaseHandler @@ -52,8 +51,6 @@ class AuthHandler(BaseHandler): - SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000 - def __init__(self, hs): """ Args: @@ -69,15 +66,6 @@ def __init__(self, hs): self.bcrypt_rounds = hs.config.bcrypt_rounds - # This is not a cache per se, but a store of all current sessions that - # expire after N hours - self.sessions = ExpiringCache( - cache_name="register_sessions", - clock=hs.get_clock(), - expiry_ms=self.SESSION_EXPIRE_MS, - reset_expiry_on_get=True, - ) - account_handler = ModuleApi(hs, self) self.password_providers = [ module(config=config, account_handler=account_handler) @@ -303,13 +291,12 @@ async def check_auth( # If there's no session ID, create a new session. if not sid: - session = self._create_session( - clientdict, (request.uri, request.method, clientdict), description + session_id = await self.store.create_session( + clientdict, request.uri, request.method, description ) - session_id = session["id"] else: - session = self._get_session_info(sid) + session = await self.store.get_session(sid) session_id = sid if not clientdict: @@ -330,7 +317,7 @@ async def check_auth( # and storing it during the initial query. Subsequent queries ensure # that this comparator has not changed. comparator = (request.uri, request.method, clientdict) - if session["ui_auth"] != comparator: + if (session["uri"], session["method"], session["clientdict"]) != comparator: raise SynapseError( 403, "Requested operation has changed during the UI authentication session.", @@ -341,8 +328,6 @@ async def check_auth( self._auth_dict_for_flows(flows, session_id) ) - creds = session["creds"] - # check auth type currently being presented errordict = {} # type: Dict[str, Any] if "type" in authdict: @@ -350,8 +335,7 @@ async def check_auth( try: result = await self._check_auth_dict(authdict, clientip) if result: - creds[login_type] = result - self._save_session(session) + await self.store.mark_stage_complete(session_id, login_type, result) except LoginError as e: if login_type == LoginType.EMAIL_IDENTITY: # riot used to have a bug where it would request a new @@ -367,6 +351,7 @@ async def check_auth( # so that the client can have another go. errordict = e.error_dict() + creds = await self.store.get_completed_stages(session_id) for f in flows: if len(set(f) - set(creds)) == 0: # it's very useful to know what args are stored, but this can @@ -399,13 +384,9 @@ async def add_oob_auth( if "session" not in authdict: raise LoginError(400, "", Codes.MISSING_PARAM) - sess = self._get_session_info(authdict["session"]) - creds = sess["creds"] - result = await self.checkers[stagetype].check_auth(authdict, clientip) if result: - creds[stagetype] = result - self._save_session(sess) + await self.store.mark_stage_complete(authdict["session"], stagetype, result) return True return False @@ -427,7 +408,7 @@ def get_session_id(self, clientdict: Dict[str, Any]) -> Optional[str]: sid = authdict["session"] return sid - def set_session_data(self, session_id: str, key: str, value: Any) -> None: + async def set_session_data(self, session_id: str, key: str, value: Any) -> None: """ Store a key-value pair into the sessions data associated with this request. This data is stored server-side and cannot be modified by @@ -438,11 +419,9 @@ def set_session_data(self, session_id: str, key: str, value: Any) -> None: key: The key to store the data under value: The data to store """ - sess = self._get_session_info(session_id) - sess["serverdict"][key] = value - self._save_session(sess) + await self.store.set_session_data(session_id, key, value) - def get_session_data( + async def get_session_data( self, session_id: str, key: str, default: Optional[Any] = None ) -> Any: """ @@ -453,8 +432,7 @@ def get_session_data( key: The key to store the data under default: Value to return if the key has not been set """ - sess = self._get_session_info(session_id) - return sess["serverdict"].get(key, default) + return await self.store.get_session_data(session_id, key, default) async def _check_auth_dict( self, authdict: Dict[str, Any], clientip: str @@ -534,67 +512,6 @@ def _auth_dict_for_flows( "params": params, } - def _create_session( - self, - clientdict: Dict[str, Any], - ui_auth: Tuple[bytes, bytes, Dict[str, Any]], - description: str, - ) -> dict: - """ - Creates a new user interactive authentication session. - - The session can be used to track data across multiple requests, e.g. for - interactive authentication. - - Each session has the following keys: - - id: - A unique identifier for this session. Passed back to the client - and returned for each stage. - clientdict: - The dictionary from the client root level, not the 'auth' key. - ui_auth: - A tuple which is checked at each stage of the authentication to - ensure that the asked for operation has not changed. - creds: - A map, which maps each auth-type (str) to the relevant identity - authenticated by that auth-type (mostly str, but for captcha, bool). - serverdict: - A map of data that is stored server-side and cannot be modified - by the client. - description: - A string description of the operation that the current - authentication is authorising. - Returns: - The newly created session. - """ - session_id = None - while session_id is None or session_id in self.sessions: - session_id = stringutils.random_string(24) - - self.sessions[session_id] = { - "id": session_id, - "clientdict": clientdict, - "ui_auth": ui_auth, - "creds": {}, - "serverdict": {}, - "description": description, - } - - return self.sessions[session_id] - - def _get_session_info(self, session_id: str) -> dict: - """ - Gets a session given a session ID. - - The session can be used to track data across multiple requests, e.g. for - interactive authentication. - """ - try: - return self.sessions[session_id] - except KeyError: - raise SynapseError(400, "Unknown session ID: %s" % (session_id,)) - async def get_access_token_for_user_id( self, user_id: str, device_id: Optional[str], valid_until_ms: Optional[int] ): @@ -994,13 +911,6 @@ async def delete_threepid( await self.store.user_delete_threepid(user_id, medium, address) return result - def _save_session(self, session: Dict[str, Any]) -> None: - """Update the last used time on the session to now and add it back to the session store.""" - # TODO: Persistent storage - logger.debug("Saving session %s", session) - session["last_used"] = self.hs.get_clock().time_msec() - self.sessions[session["id"]] = session - async def hash(self, password: str) -> str: """Computes a secure hash of password. @@ -1063,12 +973,12 @@ def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: Returns: The HTML to render. """ - session = self._get_session_info(session_id) + session = self.store.get_session(session_id) return self._sso_auth_confirm_template.render( description=session["description"], redirect_url=redirect_url, ) - def complete_sso_ui_auth( + async def complete_sso_ui_auth( self, registered_user_id: str, session_id: str, request: SynapseRequest, ): """Having figured out a mxid for this user, complete the HTTP request @@ -1080,13 +990,11 @@ def complete_sso_ui_auth( process. """ # Mark the stage of the authentication as successful. - sess = self._get_session_info(session_id) - creds = sess["creds"] - # Save the user who authenticated with SSO, this will be used to ensure # that the account be modified is also the person who logged in. - creds[LoginType.SSO] = registered_user_id - self._save_session(sess) + await self.store.mark_stage_complete( + session_id, LoginType.SSO, registered_user_id + ) # Render the HTML and return. html_bytes = self._sso_auth_success_template.encode("utf-8") diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index 5cb3f9d13317..64aaa1335c57 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -206,7 +206,7 @@ async def handle_ticket( registered_user_id = await self._auth_handler.check_user_exists(user_id) if session: - self._auth_handler.complete_sso_ui_auth( + await self._auth_handler.complete_sso_ui_auth( registered_user_id, session, request, ) diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 7c9454b50460..96f2dd36ad20 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -149,7 +149,7 @@ async def handle_saml_response(self, request): # Complete the interactive auth session or the login. if current_session and current_session.ui_auth_session_id: - self._auth_handler.complete_sso_ui_auth( + await self._auth_handler.complete_sso_ui_auth( user_id, current_session.ui_auth_session_id, request ) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 431ecf4f84e9..8af3ab4d6911 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -489,7 +489,7 @@ async def on_POST(self, request): # registered a user for this session, so we could just return the # user here. We carry on and go through the auth checks though, # for paranoia. - registered_user_id = self.auth_handler.get_session_data( + registered_user_id = await self.auth_handler.get_session_data( session_id, "registered_user_id", None ) @@ -588,7 +588,7 @@ async def on_POST(self, request): # remember that we've now registered that user account, and with # what user ID (since the user may not have specified) - self.auth_handler.set_session_data( + await self.auth_handler.set_session_data( session_id, "registered_user_id", registered_user_id ) diff --git a/synapse/storage/data_stores/main/__init__.py b/synapse/storage/data_stores/main/__init__.py index 649e835303bd..22ae44864fd9 100644 --- a/synapse/storage/data_stores/main/__init__.py +++ b/synapse/storage/data_stores/main/__init__.py @@ -66,6 +66,7 @@ from .stream import StreamStore from .tags import TagsStore from .transactions import TransactionStore +from .ui_auth import UIAuthStore from .user_directory import UserDirectoryStore from .user_erasure_store import UserErasureStore @@ -112,6 +113,7 @@ class DataStore( StatsStore, RelationsStore, CacheInvalidationStore, + UIAuthStore, ): def __init__(self, database: Database, db_conn, hs): self.hs = hs diff --git a/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql new file mode 100644 index 000000000000..f9384096b643 --- /dev/null +++ b/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql @@ -0,0 +1,34 @@ +/* Copyright 2020 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +CREATE TABLE IF NOT EXISTS ui_auth_sessions( + session_id TEXT NOT NULL, -- The session ID passed to the client. + last_used BIGINT UNSIGNED NOT NULL, -- The last time this session was seen. + serverdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data added by Synapse. + clientdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data from the client. + uri TEXT NOT NULL, -- The URI the UI authentication session is using. + method TEXT NOT NULL, -- The HTTP method the UI authentication session is using. + -- The clientdict, uri, and method make up an tuple that must be immutable + -- throughout the lifetime of the UI Auth session. + description TEXT NOT NULL, -- A human readable description of the operation which caused the UI Auth flow to occur. + UNIQUE (session_id) +); + +CREATE TABLE IF NOT EXISTS ui_auth_sessions_credentials( + session_id TEXT NOT NULL, -- The corresponding UI Auth session. + stage_type TEXT NOT NULL, -- The stage type. + identity TEXT NOT NULL, -- The identity authenticated by the stage, stored as JSON. + UNIQUE (session_id, stage_type) +); diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py new file mode 100644 index 000000000000..0d179f375807 --- /dev/null +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -0,0 +1,179 @@ +# -*- coding: utf-8 -*- +# Copyright 2020 Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import json +from typing import Any, Dict, Optional, Union + +import synapse.util.stringutils as stringutils +from synapse.api.errors import StoreError, SynapseError +from synapse.storage._base import SQLBaseStore + + +class UIAuthStore(SQLBaseStore): + """ + Manage user interactive authentication sessions. + """ + + # TODO Expire old entries. + SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000 + + async def create_session( + self, clientdict: Dict[str, Any], uri: str, method: str, description: str, + ) -> str: + """ + Creates a new user interactive authentication session. + + The session can be used to track data across multiple requests, e.g. for + interactive authentication. + """ + # TODO How to generate the session ID. + session_id = stringutils.random_string(24) + + await self.db.simple_insert( + table="ui_auth_sessions", + values={ + "session_id": session_id, + "clientdict": json.dumps(clientdict), + "uri": uri, + "method": method, + "description": description, + "serverdict": "{}", + # TODO Keep this up-to-date. + "last_used": self.hs.get_clock().time_msec(), + }, + ) + + return session_id + + async def get_session(self, session_id: str): + """Retrieve a UI auth session. + + Args: + session_id: The ID of the session. + Returns: + defer.Deferred for a dict containing the device information. + Raises: + SynapseError: if the session is not found. + """ + try: + result = await self.db.simple_select_one( + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + retcols=("clientdict", "uri", "method", "description"), + desc="get_session", + ) + except StoreError: + raise SynapseError(400, "Unknown session ID: %s" % session_id) + + result["clientdict"] = json.loads(result["clientdict"]) + + return result + + async def mark_stage_complete( + self, + session_id: str, + stage_type: str, + identity: Union[str, bool, Dict[str, any]], + ): + """ + Mark a session stage as completed. + + Args: + session_id: The ID of the corresponding session. + stage_type: The completed stage type. + identity: The identity authenticated by the stage. + """ + await self.db.simple_insert( + table="ui_auth_sessions_credentials", + values={ + "session_id": session_id, + "stage_type": stage_type, + "identity": json.dumps(identity), + }, + desc="mark_stage_complete", + ) + + async def get_completed_stages( + self, session_id: str + ) -> Dict[str, Union[str, bool, Dict[str, Any]]]: + """ + Retrieve the completed stages of a UI authentication session. + + Args: + session_id: The ID of the session. + Returns: + The completed stages mapped to the user which completed that stage. + Raises: + StoreError: if the session is not found. + """ + results = {} + for row in await self.db.simple_select_list( + table="ui_auth_sessions_credentials", + keyvalues={"session_id": session_id}, + retcols=("stage_type", "identity"), + desc="get_completed_stages", + ): + results[row["stage_type"]] = json.loads(row["identity"]) + + return results + + async def set_session_data(self, session_id: str, key: str, value: Any): + """ + Store a key-value pair into the sessions data associated with this + request. This data is stored server-side and cannot be modified by + the client. + + Args: + session_id: The ID of this session as returned from check_auth + key: The key to store the data under + value: The data to store + """ + result = await self.db.simple_select_one( + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + retcols=("serverdict",), + desc="set_server_data_select", + ) + + serverdict = json.loads(result["serverdict"]) + serverdict[key] = value + + await self.db.simple_update_one( + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + updatevalues={"serverdict": json.dumps(serverdict)}, + desc="set_server_data_update", + ) + + async def get_session_data( + self, session_id: str, key: str, default: Optional[Any] = None + ): + """ + Retrieve data stored with set_session_data + + Args: + session_id: The ID of this session as returned from check_auth + key: The key to store the data under + default: Value to return if the key has not been set + """ + result = await self.db.simple_select_one( + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + retcols=("serverdict",), + desc="get_server_data", + ) + + serverdict = json.loads(result["serverdict"]) + + return serverdict.get(key, default) From 4a83da9a227fff1c276b0a542cd519c46cafabbd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 15 Apr 2020 15:07:13 -0400 Subject: [PATCH 02/36] Ensure that UI auth stages are idempotent. --- synapse/storage/data_stores/main/ui_auth.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 0d179f375807..9245e9ce2e2c 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -94,13 +94,10 @@ async def mark_stage_complete( stage_type: The completed stage type. identity: The identity authenticated by the stage. """ - await self.db.simple_insert( + await self.db.simple_upsert( table="ui_auth_sessions_credentials", - values={ - "session_id": session_id, - "stage_type": stage_type, - "identity": json.dumps(identity), - }, + keyvalues={"session_id": session_id, "stage_type": stage_type}, + values={"identity": json.dumps(identity)}, desc="mark_stage_complete", ) From e7a2db6fcb2af26d0671ed3ffa35ce96e2f126f0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 17 Apr 2020 14:53:19 -0400 Subject: [PATCH 03/36] Fix postgresql issues. --- synapse/handlers/auth.py | 8 ++++++-- .../data_stores/main/schema/delta/58/persist_ui_auth.sql | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index b06dc43e0224..f591a7ebb4f4 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -289,10 +289,14 @@ async def check_auth( if "session" in authdict: sid = authdict["session"] + # Convert the URI and method to strings. + uri = request.uri.decode("utf-8") + method = request.uri.decode("utf-8") + # If there's no session ID, create a new session. if not sid: session_id = await self.store.create_session( - clientdict, request.uri, request.method, description + clientdict, uri, method, description ) else: @@ -316,7 +320,7 @@ async def check_auth( # comparator based on the URI, method, and body (minus the auth dict) # and storing it during the initial query. Subsequent queries ensure # that this comparator has not changed. - comparator = (request.uri, request.method, clientdict) + comparator = (uri, method, clientdict) if (session["uri"], session["method"], session["clientdict"]) != comparator: raise SynapseError( 403, diff --git a/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql index f9384096b643..d4cf015107f0 100644 --- a/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql @@ -15,7 +15,7 @@ CREATE TABLE IF NOT EXISTS ui_auth_sessions( session_id TEXT NOT NULL, -- The session ID passed to the client. - last_used BIGINT UNSIGNED NOT NULL, -- The last time this session was seen. + last_used BIGINT NOT NULL, -- The last time this session was seen. serverdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data added by Synapse. clientdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data from the client. uri TEXT NOT NULL, -- The URI the UI authentication session is using. From b9dd110c02fd786e8b0112b842f37263c9217acf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 09:26:01 -0400 Subject: [PATCH 04/36] Add a changelog file. --- changelog.d/7302.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7302.bugfix diff --git a/changelog.d/7302.bugfix b/changelog.d/7302.bugfix new file mode 100644 index 000000000000..820646d1f90d --- /dev/null +++ b/changelog.d/7302.bugfix @@ -0,0 +1 @@ +Persist user interactive authentication sessions across workers and Synapse restarts. From 831f8a285392d3db363ff0be745e4ac8f19802d7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 10:01:45 -0400 Subject: [PATCH 05/36] Expire old sessions. --- synapse/handlers/auth.py | 15 +++++++ .../main/schema/delta/58/persist_ui_auth.sql | 2 +- synapse/storage/data_stores/main/ui_auth.py | 40 +++++++++++++++++-- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index f591a7ebb4f4..35c296065b2b 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -51,6 +51,8 @@ class AuthHandler(BaseHandler): + SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000 + def __init__(self, hs): """ Args: @@ -107,6 +109,9 @@ def __init__(self, hs): self._clock = self.hs.get_clock() + # Expire old UI auth sessions after a period of time. + self._clock.looping_call(self.expire_old_sessions, 5 * 60 * 1000) + # Load the SSO HTML templates. # The following template is shown to the user during a client login via SSO, @@ -438,6 +443,16 @@ async def get_session_data( """ return await self.store.get_session_data(session_id, key, default) + def expire_old_sessions(self): + """ + Invalidate any user interactive authentication sessions that have expired. + """ + now = self._clock.time_msec() + + expiration_time = now - self.SESSION_EXPIRE_MS + + self.store.delete_old_sessions(expiration_time) + async def _check_auth_dict( self, authdict: Dict[str, Any], clientip: str ) -> Union[Dict[str, Any], str]: diff --git a/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql index d4cf015107f0..93d6f3ee00e4 100644 --- a/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql @@ -15,7 +15,7 @@ CREATE TABLE IF NOT EXISTS ui_auth_sessions( session_id TEXT NOT NULL, -- The session ID passed to the client. - last_used BIGINT NOT NULL, -- The last time this session was seen. + last_used BIGINT NOT NULL, -- The last time this session was seen (epoch time in milliseconds). serverdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data added by Synapse. clientdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data from the client. uri TEXT NOT NULL, -- The URI the UI authentication session is using. diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 9245e9ce2e2c..f5696e110b3e 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -25,9 +25,6 @@ class UIAuthStore(SQLBaseStore): Manage user interactive authentication sessions. """ - # TODO Expire old entries. - SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000 - async def create_session( self, clientdict: Dict[str, Any], uri: str, method: str, description: str, ) -> str: @@ -80,6 +77,43 @@ async def get_session(self, session_id: str): return result + def delete_old_sessions(self, expiration_time: int): + """ + Remove sessions which were last used earlier than the expiration time. + + Args: + expiration_time: The latest time that is still considered valid. + This is an epoch time in milliseconds. + + """ + return self.db.runInteraction( + "delete_sessions", self._delete_old_sessions, expiration_time + ) + + def _delete_old_sessions(self, txn, expiration_time: int): + # Get the expired sessions. + sql = "SELECT session_id FROM ui_auth_sessions WHERE last_used <= ?" + txn.execute(sql, [expiration_time]) + session_ids = [r[0] for r in txn.fetchall()] + + # Delete the corresponding completed credentials. + self.db.simple_delete_many_txn( + txn, + table="ui_auth_sessions_credentials", + column="session_id", + iterable=session_ids, + keyvalues={}, + ) + + # Finally, delete the sessions. + self.db.simple_delete_many_txn( + txn, + table="ui_auth_sessions", + column="session_id", + iterable=session_ids, + keyvalues={}, + ) + async def mark_stage_complete( self, session_id: str, From 04d3d8b8b01ccfd66c75e930ae79dfdae8458129 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 10:12:02 -0400 Subject: [PATCH 06/36] Keep the last_used time up-to-date. --- synapse/storage/data_stores/main/ui_auth.py | 51 ++++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index f5696e110b3e..2d30de4889ed 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -46,7 +46,6 @@ async def create_session( "method": method, "description": description, "serverdict": "{}", - # TODO Keep this up-to-date. "last_used": self.hs.get_clock().time_msec(), }, ) @@ -128,11 +127,34 @@ async def mark_stage_complete( stage_type: The completed stage type. identity: The identity authenticated by the stage. """ - await self.db.simple_upsert( + self.db.runInteraction( + "mark_stage_complete", + self._mark_stage_completed, + session_id, + stage_type, + identity, + ) + + def _mark_stage_completed( + self, + txn, + session_id: str, + stage_type: str, + identity: Union[str, bool, Dict[str, any]], + ): + # Add (or update) the results of the current stage to the database. + self.db.simple_upsert_txn( + txn, table="ui_auth_sessions_credentials", keyvalues={"session_id": session_id, "stage_type": stage_type}, values={"identity": json.dumps(identity)}, - desc="mark_stage_complete", + ) + # Mark the session as still in use. + self.db.simple_update_one_txn( + txn, + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + updatevalues={"last_used": self.hs.get_clock().time_msec()}, ) async def get_completed_stages( @@ -170,21 +192,36 @@ async def set_session_data(self, session_id: str, key: str, value: Any): key: The key to store the data under value: The data to store """ - result = await self.db.simple_select_one( + return self.db.runInteraction( + "set_session_data", self._set_session_data, session_id, key, value + ) + + def _set_session_data(self, txn, session_id: str, key: str, value: Any): + # Get the current value. + result = self.db.simple_select_one_txn( + txn, table="ui_auth_sessions", keyvalues={"session_id": session_id}, retcols=("serverdict",), - desc="set_server_data_select", ) + # Update it and add it back to the database. serverdict = json.loads(result["serverdict"]) serverdict[key] = value - await self.db.simple_update_one( + self.db.simple_update_one_txn( + txn, table="ui_auth_sessions", keyvalues={"session_id": session_id}, updatevalues={"serverdict": json.dumps(serverdict)}, - desc="set_server_data_update", + ) + + # Mark the session as still in use. + self.db.simple_update_one_txn( + txn, + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + updatevalues={"last_used": self.hs.get_clock().time_msec()}, ) async def get_session_data( From 0fdb22cfe381f21ab5aa87316885852c3dc29083 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 10:33:50 -0400 Subject: [PATCH 07/36] Properly run the looping call in the background. --- synapse/handlers/auth.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 35c296065b2b..b2572446575f 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -41,6 +41,7 @@ from synapse.http.server import finish_request from synapse.http.site import SynapseRequest from synapse.logging.context import defer_to_thread +from synapse.metrics.background_process_metrics import run_as_background_process from synapse.module_api import ModuleApi from synapse.push.mailer import load_jinja2_templates from synapse.types import Requester, UserID @@ -110,7 +111,12 @@ def __init__(self, hs): self._clock = self.hs.get_clock() # Expire old UI auth sessions after a period of time. - self._clock.looping_call(self.expire_old_sessions, 5 * 60 * 1000) + self._clock.looping_call( + lambda: run_as_background_process( + "expire_old_sessions", self.expire_old_sessions + ), + 5 * 60 * 1000, + ) # Load the SSO HTML templates. @@ -448,9 +454,7 @@ def expire_old_sessions(self): Invalidate any user interactive authentication sessions that have expired. """ now = self._clock.time_msec() - expiration_time = now - self.SESSION_EXPIRE_MS - self.store.delete_old_sessions(expiration_time) async def _check_auth_dict( From 3d9b3a819da5721707778bd81c47b710d001d493 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 11:03:11 -0400 Subject: [PATCH 08/36] Properly call get_session as async. --- synapse/handlers/auth.py | 4 ++-- synapse/rest/client/v2_alpha/auth.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index b2572446575f..155c0a30df63 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -985,7 +985,7 @@ def _do_validate_hash(): else: return False - def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: + async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: """ Get the HTML for the SSO redirect confirmation page. @@ -996,7 +996,7 @@ def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: Returns: The HTML to render. """ - session = self.store.get_session(session_id) + session = await self.store.get_session(session_id) return self._sso_auth_confirm_template.render( description=session["description"], redirect_url=redirect_url, ) diff --git a/synapse/rest/client/v2_alpha/auth.py b/synapse/rest/client/v2_alpha/auth.py index 11599f50054a..24dd3d3e96d1 100644 --- a/synapse/rest/client/v2_alpha/auth.py +++ b/synapse/rest/client/v2_alpha/auth.py @@ -140,7 +140,7 @@ def __init__(self, hs): self._cas_server_url = hs.config.cas_server_url self._cas_service_url = hs.config.cas_service_url - def on_GET(self, request, stagetype): + async def on_GET(self, request, stagetype): session = parse_string(request, "session") if not session: raise SynapseError(400, "No session supplied") @@ -180,7 +180,7 @@ def on_GET(self, request, stagetype): else: raise SynapseError(400, "Homeserver not configured for SSO.") - html = self.auth_handler.start_sso_ui_auth(sso_redirect_url, session) + html = await self.auth_handler.start_sso_ui_auth(sso_redirect_url, session) else: raise SynapseError(404, "Unknown auth stage type") From b5fc1b9d11614a0c3f74cb066eac4534fd6a9c74 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 11:35:54 -0400 Subject: [PATCH 09/36] Attempt to avoid clashes in session IDs. --- synapse/storage/data_stores/main/ui_auth.py | 40 ++++++++++++--------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 2d30de4889ed..afc7f7d6030c 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -34,23 +34,29 @@ async def create_session( The session can be used to track data across multiple requests, e.g. for interactive authentication. """ - # TODO How to generate the session ID. - session_id = stringutils.random_string(24) - - await self.db.simple_insert( - table="ui_auth_sessions", - values={ - "session_id": session_id, - "clientdict": json.dumps(clientdict), - "uri": uri, - "method": method, - "description": description, - "serverdict": "{}", - "last_used": self.hs.get_clock().time_msec(), - }, - ) - - return session_id + # autogen a session ID and try to create it. We may clash, so just + # try a few times till one goes through, giving up eventually. + attempts = 0 + while attempts < 5: + session_id = stringutils.random_string(24) + + try: + await self.db.simple_insert( + table="ui_auth_sessions", + values={ + "session_id": session_id, + "clientdict": json.dumps(clientdict), + "uri": uri, + "method": method, + "description": description, + "serverdict": "{}", + "last_used": self.hs.get_clock().time_msec(), + }, + ) + return session_id + except self.db.engine.module.IntegrityError: + attempts += 1 + raise StoreError(500, "Couldn't generate a session ID.") async def get_session(self, session_id: str): """Retrieve a UI auth session. From d9157c4e7f69285582bba8b8edf750d6afcd7ac6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 20 Apr 2020 14:21:50 -0400 Subject: [PATCH 10/36] Properly await runInteraction calls. --- synapse/handlers/auth.py | 4 ++-- synapse/storage/data_stores/main/ui_auth.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 155c0a30df63..5652fcfec159 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -449,13 +449,13 @@ async def get_session_data( """ return await self.store.get_session_data(session_id, key, default) - def expire_old_sessions(self): + async def expire_old_sessions(self): """ Invalidate any user interactive authentication sessions that have expired. """ now = self._clock.time_msec() expiration_time = now - self.SESSION_EXPIRE_MS - self.store.delete_old_sessions(expiration_time) + await self.store.delete_old_sessions(expiration_time) async def _check_auth_dict( self, authdict: Dict[str, Any], clientip: str diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index afc7f7d6030c..624de7e89723 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -133,7 +133,7 @@ async def mark_stage_complete( stage_type: The completed stage type. identity: The identity authenticated by the stage. """ - self.db.runInteraction( + await self.db.runInteraction( "mark_stage_complete", self._mark_stage_completed, session_id, @@ -198,7 +198,7 @@ async def set_session_data(self, session_id: str, key: str, value: Any): key: The key to store the data under value: The data to store """ - return self.db.runInteraction( + await self.db.runInteraction( "set_session_data", self._set_session_data, session_id, key, value ) From ae45238b2d9e624398482e39770bc1ee00838d5e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Apr 2020 08:37:54 -0400 Subject: [PATCH 11/36] Add the UIAuthStore to workers. --- synapse/app/generic_worker.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index 37afd2f81034..a9fab272971a 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -124,6 +124,7 @@ MonthlyActiveUsersWorkerStore, ) from synapse.storage.data_stores.main.presence import UserPresenceState +from synapse.storage.data_stores.main.ui_auth import UIAuthStore from synapse.storage.data_stores.main.user_directory import UserDirectoryStore from synapse.types import ReadReceipt from synapse.util.async_helpers import Linearizer @@ -425,6 +426,7 @@ class GenericWorkerSlavedStore( # FIXME(#3714): We need to add UserDirectoryStore as we write directly # rather than going via the correct worker. UserDirectoryStore, + UIAuthStore, SlavedDeviceInboxStore, SlavedDeviceStore, SlavedReceiptsStore, From 9ac9c54592bcefeca4c0724adcd281f39427321f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Apr 2020 08:41:18 -0400 Subject: [PATCH 12/36] Remove unnecessary lambda Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/auth.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 5652fcfec159..13a9bd0cb67d 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -112,10 +112,10 @@ def __init__(self, hs): # Expire old UI auth sessions after a period of time. self._clock.looping_call( - lambda: run_as_background_process( - "expire_old_sessions", self.expire_old_sessions - ), + run_as_background_process, 5 * 60 * 1000, + "expire_old_sessions", + self.expire_old_sessions, ) # Load the SSO HTML templates. From 1c861b86ab2256e9a393ef3c03b778bd5009ecef Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Apr 2020 08:43:26 -0400 Subject: [PATCH 13/36] Only expire old sessions on the master. --- synapse/handlers/auth.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 13a9bd0cb67d..fd57a4bb8873 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -111,12 +111,13 @@ def __init__(self, hs): self._clock = self.hs.get_clock() # Expire old UI auth sessions after a period of time. - self._clock.looping_call( - run_as_background_process, - 5 * 60 * 1000, - "expire_old_sessions", - self.expire_old_sessions, - ) + if hs.config.worker_name is None: + self._clock.looping_call( + run_as_background_process, + 5 * 60 * 1000, + "expire_old_sessions", + self.expire_old_sessions, + ) # Load the SSO HTML templates. From 08959716e00a98021de78ac3c5eb1f3f5b77389d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Apr 2020 09:18:17 -0400 Subject: [PATCH 14/36] Match the looping_call signature in unit tests. --- tests/utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index 2079e0143d69..e7f86fa1d5a7 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -509,8 +509,8 @@ def wrapped_callback(): return t - def looping_call(self, function, interval): - self.loopers.append([function, interval / 1000.0, self.now]) + def looping_call(self, function, interval, *args, **kwargs): + self.loopers.append([function, interval / 1000.0, self.now, args, kwargs]) def cancel_call_later(self, timer, ignore_errs=False): if timer[2]: @@ -540,9 +540,9 @@ def advance_time(self, secs): self.timers.append(t) for looped in self.loopers: - func, interval, last = looped + func, interval, last, args, kwargs = looped if last + interval < self.now: - func() + func(*args, **kwargs) looped[2] = self.now def advance_time_msec(self, ms): From 42c4bca5109adf129deff30b2f9a6133a79dd0da Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Apr 2020 09:49:56 -0400 Subject: [PATCH 15/36] Fix mypy typing and run mypy on the new file. --- synapse/storage/data_stores/main/ui_auth.py | 4 ++-- tox.ini | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 624de7e89723..5ef0efdb37cc 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -123,7 +123,7 @@ async def mark_stage_complete( self, session_id: str, stage_type: str, - identity: Union[str, bool, Dict[str, any]], + identity: Union[str, bool, Dict[str, Any]], ): """ Mark a session stage as completed. @@ -146,7 +146,7 @@ def _mark_stage_completed( txn, session_id: str, stage_type: str, - identity: Union[str, bool, Dict[str, any]], + identity: Union[str, bool, Dict[str, Any]], ): # Add (or update) the results of the current stage to the database. self.db.simple_upsert_txn( diff --git a/tox.ini b/tox.ini index 31011d7436f5..75d58ce02cce 100644 --- a/tox.ini +++ b/tox.ini @@ -200,8 +200,9 @@ commands = mypy \ synapse/replication \ synapse/rest \ synapse/spam_checker_api \ - synapse/storage/engines \ + synapse/storage/data_stores/main/ui_auth.py \ synapse/storage/database.py \ + synapse/storage/engines \ synapse/streams \ synapse/util/caches/stream_change_cache.py \ tests/util/test_stream_change_cache.py From 2d1bcad60626fa8c3994cc4b8c03e5dce6cfbcd4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Apr 2020 10:02:32 -0400 Subject: [PATCH 16/36] Add a few return types. --- synapse/storage/data_stores/main/ui_auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 5ef0efdb37cc..a738ecef6184 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -58,7 +58,7 @@ async def create_session( attempts += 1 raise StoreError(500, "Couldn't generate a session ID.") - async def get_session(self, session_id: str): + async def get_session(self, session_id: str) -> Dict[str, Any]: """Retrieve a UI auth session. Args: @@ -232,7 +232,7 @@ def _set_session_data(self, txn, session_id: str, key: str, value: Any): async def get_session_data( self, session_id: str, key: str, default: Optional[Any] = None - ): + ) -> Any: """ Retrieve data stored with set_session_data From f2e5151130159ee2a5eaf39c7b272121650f2838 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 09:40:47 -0400 Subject: [PATCH 17/36] Prefix a number to the delta file. --- .../delta/58/{persist_ui_auth.sql => 03persist_ui_auth.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename synapse/storage/data_stores/main/schema/delta/58/{persist_ui_auth.sql => 03persist_ui_auth.sql} (100%) diff --git a/synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql similarity index 100% rename from synapse/storage/data_stores/main/schema/delta/58/persist_ui_auth.sql rename to synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql From 7091341589a00c3deb9aaa747e7fe3875bdea85a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 09:42:47 -0400 Subject: [PATCH 18/36] Clarify comments. --- synapse/storage/data_stores/main/ui_auth.py | 28 ++++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index a738ecef6184..82e70bced61d 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -31,8 +31,27 @@ async def create_session( """ Creates a new user interactive authentication session. - The session can be used to track data across multiple requests, e.g. for - interactive authentication. + The session can be used to track the stages necessary to authenticate a + user across multiple HTTP requests. + + Args: + clientdict: + The dictionary from the client root level, not the 'auth' key. + uri: + The URI this session was initiated from, this is checked at each + stage of the authentication to ensure that the asked for + operation has not changed. + method: + The method this session was initiated with, this is checked at each + stage of the authentication to ensure that the asked for + operation has not changed. + description: + A string description of the operation that the current + authentication is authorising. + + Returns: + The created session ID. + """ # autogen a session ID and try to create it. We may clash, so just # try a few times till one goes through, giving up eventually. @@ -64,7 +83,7 @@ async def get_session(self, session_id: str) -> Dict[str, Any]: Args: session_id: The ID of the session. Returns: - defer.Deferred for a dict containing the device information. + A dict containing the device information. Raises: SynapseError: if the session is not found. """ @@ -172,7 +191,8 @@ async def get_completed_stages( Args: session_id: The ID of the session. Returns: - The completed stages mapped to the user which completed that stage. + The completed stages mapped to the relevant identity authenticated + by that auth-type (mostly str, but for captcha, bool). Raises: StoreError: if the session is not found. """ From ff14b666c8a52d40291a07c3c86075535586c98f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 10:05:53 -0400 Subject: [PATCH 19/36] Rename methods based on feedback. --- synapse/handlers/auth.py | 28 ++++++++------ synapse/storage/data_stores/main/ui_auth.py | 41 ++++++++++++--------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index fd57a4bb8873..06a5059c7bf3 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -116,7 +116,7 @@ def __init__(self, hs): run_as_background_process, 5 * 60 * 1000, "expire_old_sessions", - self.expire_old_sessions, + self._expire_old_sessions, ) # Load the SSO HTML templates. @@ -307,12 +307,12 @@ async def check_auth( # If there's no session ID, create a new session. if not sid: - session_id = await self.store.create_session( + session_id = await self.store.create_ui_auth_session( clientdict, uri, method, description ) else: - session = await self.store.get_session(sid) + session = await self.store.get_ui_auth_session(sid) session_id = sid if not clientdict: @@ -351,7 +351,9 @@ async def check_auth( try: result = await self._check_auth_dict(authdict, clientip) if result: - await self.store.mark_stage_complete(session_id, login_type, result) + await self.store.mark_ui_auth_stage_complete( + session_id, login_type, result + ) except LoginError as e: if login_type == LoginType.EMAIL_IDENTITY: # riot used to have a bug where it would request a new @@ -367,7 +369,7 @@ async def check_auth( # so that the client can have another go. errordict = e.error_dict() - creds = await self.store.get_completed_stages(session_id) + creds = await self.store.get_completed_ui_auth_stages(session_id) for f in flows: if len(set(f) - set(creds)) == 0: # it's very useful to know what args are stored, but this can @@ -402,7 +404,9 @@ async def add_oob_auth( result = await self.checkers[stagetype].check_auth(authdict, clientip) if result: - await self.store.mark_stage_complete(authdict["session"], stagetype, result) + await self.store.mark_ui_auth_stage_complete( + authdict["session"], stagetype, result + ) return True return False @@ -435,7 +439,7 @@ async def set_session_data(self, session_id: str, key: str, value: Any) -> None: key: The key to store the data under value: The data to store """ - await self.store.set_session_data(session_id, key, value) + await self.store.set_ui_auth_session_data(session_id, key, value) async def get_session_data( self, session_id: str, key: str, default: Optional[Any] = None @@ -448,15 +452,15 @@ async def get_session_data( key: The key to store the data under default: Value to return if the key has not been set """ - return await self.store.get_session_data(session_id, key, default) + return await self.store.get_ui_auth_session_data(session_id, key, default) - async def expire_old_sessions(self): + async def _expire_old_sessions(self): """ Invalidate any user interactive authentication sessions that have expired. """ now = self._clock.time_msec() expiration_time = now - self.SESSION_EXPIRE_MS - await self.store.delete_old_sessions(expiration_time) + await self.store.delete_old_ui_auth_sessions(expiration_time) async def _check_auth_dict( self, authdict: Dict[str, Any], clientip: str @@ -997,7 +1001,7 @@ async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: Returns: The HTML to render. """ - session = await self.store.get_session(session_id) + session = await self.store.get_ui_auth_session(session_id) return self._sso_auth_confirm_template.render( description=session["description"], redirect_url=redirect_url, ) @@ -1016,7 +1020,7 @@ async def complete_sso_ui_auth( # Mark the stage of the authentication as successful. # Save the user who authenticated with SSO, this will be used to ensure # that the account be modified is also the person who logged in. - await self.store.mark_stage_complete( + await self.store.mark_ui_auth_stage_complete( session_id, LoginType.SSO, registered_user_id ) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 82e70bced61d..cfc51eb0fb08 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -25,7 +25,7 @@ class UIAuthStore(SQLBaseStore): Manage user interactive authentication sessions. """ - async def create_session( + async def create_ui_auth_session( self, clientdict: Dict[str, Any], uri: str, method: str, description: str, ) -> str: """ @@ -71,13 +71,14 @@ async def create_session( "serverdict": "{}", "last_used": self.hs.get_clock().time_msec(), }, + desc="create_ui_auth_session", ) return session_id except self.db.engine.module.IntegrityError: attempts += 1 raise StoreError(500, "Couldn't generate a session ID.") - async def get_session(self, session_id: str) -> Dict[str, Any]: + async def get_ui_auth_session(self, session_id: str) -> Dict[str, Any]: """Retrieve a UI auth session. Args: @@ -92,7 +93,7 @@ async def get_session(self, session_id: str) -> Dict[str, Any]: table="ui_auth_sessions", keyvalues={"session_id": session_id}, retcols=("clientdict", "uri", "method", "description"), - desc="get_session", + desc="get_ui_auth_session", ) except StoreError: raise SynapseError(400, "Unknown session ID: %s" % session_id) @@ -101,7 +102,7 @@ async def get_session(self, session_id: str) -> Dict[str, Any]: return result - def delete_old_sessions(self, expiration_time: int): + def delete_old_ui_auth_sessions(self, expiration_time: int): """ Remove sessions which were last used earlier than the expiration time. @@ -111,10 +112,12 @@ def delete_old_sessions(self, expiration_time: int): """ return self.db.runInteraction( - "delete_sessions", self._delete_old_sessions, expiration_time + "delete_old_ui_auth_sessions", + self._delete_old_ui_auth_sessions, + expiration_time, ) - def _delete_old_sessions(self, txn, expiration_time: int): + def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): # Get the expired sessions. sql = "SELECT session_id FROM ui_auth_sessions WHERE last_used <= ?" txn.execute(sql, [expiration_time]) @@ -138,7 +141,7 @@ def _delete_old_sessions(self, txn, expiration_time: int): keyvalues={}, ) - async def mark_stage_complete( + async def mark_ui_auth_stage_complete( self, session_id: str, stage_type: str, @@ -153,14 +156,14 @@ async def mark_stage_complete( identity: The identity authenticated by the stage. """ await self.db.runInteraction( - "mark_stage_complete", - self._mark_stage_completed, + "mark_ui_auth_stage_complete", + self._mark_ui_auth_stage_complete, session_id, stage_type, identity, ) - def _mark_stage_completed( + def _mark_ui_auth_stage_complete( self, txn, session_id: str, @@ -182,7 +185,7 @@ def _mark_stage_completed( updatevalues={"last_used": self.hs.get_clock().time_msec()}, ) - async def get_completed_stages( + async def get_completed_ui_auth_stages( self, session_id: str ) -> Dict[str, Union[str, bool, Dict[str, Any]]]: """ @@ -201,13 +204,13 @@ async def get_completed_stages( table="ui_auth_sessions_credentials", keyvalues={"session_id": session_id}, retcols=("stage_type", "identity"), - desc="get_completed_stages", + desc="get_completed_ui_auth_stages", ): results[row["stage_type"]] = json.loads(row["identity"]) return results - async def set_session_data(self, session_id: str, key: str, value: Any): + async def set_ui_auth_session_data(self, session_id: str, key: str, value: Any): """ Store a key-value pair into the sessions data associated with this request. This data is stored server-side and cannot be modified by @@ -219,10 +222,14 @@ async def set_session_data(self, session_id: str, key: str, value: Any): value: The data to store """ await self.db.runInteraction( - "set_session_data", self._set_session_data, session_id, key, value + "set_ui_auth_session_data", + self._set_ui_auth_session_data, + session_id, + key, + value, ) - def _set_session_data(self, txn, session_id: str, key: str, value: Any): + def _set_ui_auth_session_data(self, txn, session_id: str, key: str, value: Any): # Get the current value. result = self.db.simple_select_one_txn( txn, @@ -250,7 +257,7 @@ def _set_session_data(self, txn, session_id: str, key: str, value: Any): updatevalues={"last_used": self.hs.get_clock().time_msec()}, ) - async def get_session_data( + async def get_ui_auth_session_data( self, session_id: str, key: str, default: Optional[Any] = None ) -> Any: """ @@ -265,7 +272,7 @@ async def get_session_data( table="ui_auth_sessions", keyvalues={"session_id": session_id}, retcols=("serverdict",), - desc="get_server_data", + desc="get_ui_auth_session_data", ) serverdict = json.loads(result["serverdict"]) From 2a4a910e278772a35d74bb5c3196272bf4b516ef Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 10:07:30 -0400 Subject: [PATCH 20/36] Avoid re-doing work. --- synapse/storage/data_stores/main/ui_auth.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index cfc51eb0fb08..5889ecd17e9d 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -53,6 +53,9 @@ async def create_ui_auth_session( The created session ID. """ + # The clientdict gets stored as JSON. + clientdict_json = json.dumps(clientdict) + # autogen a session ID and try to create it. We may clash, so just # try a few times till one goes through, giving up eventually. attempts = 0 @@ -64,7 +67,7 @@ async def create_ui_auth_session( table="ui_auth_sessions", values={ "session_id": session_id, - "clientdict": json.dumps(clientdict), + "clientdict": clientdict_json, "uri": uri, "method": method, "description": description, From 5a60f2d6baa8c45d594202aec29a8843f3f2eba6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 10:23:24 -0400 Subject: [PATCH 21/36] Use JsonDict in some places. --- synapse/storage/data_stores/main/ui_auth.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 5889ecd17e9d..9b748a6738cf 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -18,6 +18,7 @@ import synapse.util.stringutils as stringutils from synapse.api.errors import StoreError, SynapseError from synapse.storage._base import SQLBaseStore +from synapse.types import JsonDict class UIAuthStore(SQLBaseStore): @@ -26,7 +27,7 @@ class UIAuthStore(SQLBaseStore): """ async def create_ui_auth_session( - self, clientdict: Dict[str, Any], uri: str, method: str, description: str, + self, clientdict: JsonDict, uri: str, method: str, description: str, ) -> str: """ Creates a new user interactive authentication session. @@ -145,10 +146,7 @@ def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): ) async def mark_ui_auth_stage_complete( - self, - session_id: str, - stage_type: str, - identity: Union[str, bool, Dict[str, Any]], + self, session_id: str, stage_type: str, identity: Union[str, bool, JsonDict], ): """ Mark a session stage as completed. @@ -171,7 +169,7 @@ def _mark_ui_auth_stage_complete( txn, session_id: str, stage_type: str, - identity: Union[str, bool, Dict[str, Any]], + identity: Union[str, bool, JsonDict], ): # Add (or update) the results of the current stage to the database. self.db.simple_upsert_txn( @@ -190,7 +188,7 @@ def _mark_ui_auth_stage_complete( async def get_completed_ui_auth_stages( self, session_id: str - ) -> Dict[str, Union[str, bool, Dict[str, Any]]]: + ) -> Dict[str, Union[str, bool, JsonDict]]: """ Retrieve the completed stages of a UI authentication session. From 6b4a6dfbb3ad3bbf298a23878239eb93ba6af1df Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 10:36:25 -0400 Subject: [PATCH 22/36] Create a return type for UI auth session data. --- synapse/handlers/auth.py | 19 ++++++------- synapse/storage/data_stores/main/ui_auth.py | 31 +++++++++++++++++---- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 06a5059c7bf3..9c8267473d53 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -307,13 +307,12 @@ async def check_auth( # If there's no session ID, create a new session. if not sid: - session_id = await self.store.create_ui_auth_session( + session = await self.store.create_ui_auth_session( clientdict, uri, method, description ) else: session = await self.store.get_ui_auth_session(sid) - session_id = sid if not clientdict: # This was designed to allow the client to omit the parameters @@ -325,7 +324,7 @@ async def check_auth( # on a homeserver. # Revisit: Assuming the REST APIs do sensible validation, the data # isn't arbitrary. - clientdict = session["clientdict"] + clientdict = session.clientdict # Ensure that the queried operation does not vary between stages of # the UI authentication session. This is done by generating a stable @@ -333,7 +332,7 @@ async def check_auth( # and storing it during the initial query. Subsequent queries ensure # that this comparator has not changed. comparator = (uri, method, clientdict) - if (session["uri"], session["method"], session["clientdict"]) != comparator: + if (session.uri, session.method, session.clientdict) != comparator: raise SynapseError( 403, "Requested operation has changed during the UI authentication session.", @@ -341,7 +340,7 @@ async def check_auth( if not authdict: raise InteractiveAuthIncompleteError( - self._auth_dict_for_flows(flows, session_id) + self._auth_dict_for_flows(flows, session.session_id) ) # check auth type currently being presented @@ -352,7 +351,7 @@ async def check_auth( result = await self._check_auth_dict(authdict, clientip) if result: await self.store.mark_ui_auth_stage_complete( - session_id, login_type, result + session.session_id, login_type, result ) except LoginError as e: if login_type == LoginType.EMAIL_IDENTITY: @@ -369,7 +368,7 @@ async def check_auth( # so that the client can have another go. errordict = e.error_dict() - creds = await self.store.get_completed_ui_auth_stages(session_id) + creds = await self.store.get_completed_ui_auth_stages(session.session_id) for f in flows: if len(set(f) - set(creds)) == 0: # it's very useful to know what args are stored, but this can @@ -383,9 +382,9 @@ async def check_auth( list(clientdict), ) - return creds, clientdict, session_id + return creds, clientdict, session.session_id - ret = self._auth_dict_for_flows(flows, session_id) + ret = self._auth_dict_for_flows(flows, session.session_id) ret["completed"] = list(creds) ret.update(errordict) raise InteractiveAuthIncompleteError(ret) @@ -1003,7 +1002,7 @@ async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: """ session = await self.store.get_ui_auth_session(session_id) return self._sso_auth_confirm_template.render( - description=session["description"], redirect_url=redirect_url, + description=session.description, redirect_url=redirect_url, ) async def complete_sso_ui_auth( diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 9b748a6738cf..17fc9caab67e 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -15,12 +15,29 @@ import json from typing import Any, Dict, Optional, Union +import attr + import synapse.util.stringutils as stringutils from synapse.api.errors import StoreError, SynapseError from synapse.storage._base import SQLBaseStore from synapse.types import JsonDict +@attr.s +class UIAuthSessionData: + session_id = attr.ib(type=str) + # The dictionary from the client root level, not the 'auth' key. + clientdict = attr.ib(type=JsonDict) + # The URI and method the session was intiatied with, these are checked at + # each stage of the authentication to ensure that the asked for operation + # has not changed. + uri = attr.ib(type=str) + method = attr.ib(type=str) + # A string description of the operation that the current authentication is + # authorising. + description = attr.ib(type=str) + + class UIAuthStore(SQLBaseStore): """ Manage user interactive authentication sessions. @@ -28,7 +45,7 @@ class UIAuthStore(SQLBaseStore): async def create_ui_auth_session( self, clientdict: JsonDict, uri: str, method: str, description: str, - ) -> str: + ) -> UIAuthSessionData: """ Creates a new user interactive authentication session. @@ -39,7 +56,7 @@ async def create_ui_auth_session( clientdict: The dictionary from the client root level, not the 'auth' key. uri: - The URI this session was initiated from, this is checked at each + The URI this session was initiated with, this is checked at each stage of the authentication to ensure that the asked for operation has not changed. method: @@ -51,7 +68,7 @@ async def create_ui_auth_session( authentication is authorising. Returns: - The created session ID. + The newly created session. """ # The clientdict gets stored as JSON. @@ -77,12 +94,14 @@ async def create_ui_auth_session( }, desc="create_ui_auth_session", ) - return session_id + return UIAuthSessionData( + session_id, clientdict, uri, method, description + ) except self.db.engine.module.IntegrityError: attempts += 1 raise StoreError(500, "Couldn't generate a session ID.") - async def get_ui_auth_session(self, session_id: str) -> Dict[str, Any]: + async def get_ui_auth_session(self, session_id: str) -> UIAuthSessionData: """Retrieve a UI auth session. Args: @@ -104,7 +123,7 @@ async def get_ui_auth_session(self, session_id: str) -> Dict[str, Any]: result["clientdict"] = json.loads(result["clientdict"]) - return result + return UIAuthSessionData(session_id, **result) def delete_old_ui_auth_sessions(self, expiration_time: int): """ From 1a5101b67c92799bf25167ae086f0c048ea5a8b6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 11:13:37 -0400 Subject: [PATCH 23/36] Ensure the session exists before marking a stage complete. --- synapse/storage/data_stores/main/ui_auth.py | 11 ++++++ tests/rest/client/v2_alpha/test_auth.py | 39 +++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 17fc9caab67e..ebdda2d7cdd6 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -190,6 +190,17 @@ def _mark_ui_auth_stage_complete( stage_type: str, identity: Union[str, bool, JsonDict], ): + try: + # Get the session to ensure it exists. + self.db.simple_select_one_txn( + txn, + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + retcols=("session_id",), + ) + except StoreError: + raise SynapseError(400, "Unknown session ID: %s" % session_id) + # Add (or update) the results of the current stage to the database. self.db.simple_upsert_txn( txn, diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 624bf5ada23f..a08ffdda3754 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -181,3 +181,42 @@ def test_cannot_change_operation(self): ) self.render(request) self.assertEqual(channel.code, 403) + + def test_complete_operation_unknown_session(self): + """ + Attempting to mark an invalid session as complete should error. + """ + + # Make the initial request to register. (Later on a different password + # will be used.) + request, channel = self.make_request( + "POST", + "register", + {"username": "user", "type": "m.login.password", "password": "bar"}, + ) + self.render(request) + + # Returns a 401 as per the spec + self.assertEqual(request.code, 401) + # Grab the session + session = channel.json_body["session"] + # Assert our configured public key is being given + self.assertEqual( + channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake" + ) + + request, channel = self.make_request( + "GET", "auth/m.login.recaptcha/fallback/web?session=" + session + ) + self.render(request) + self.assertEqual(request.code, 200) + + invalid_session = session + "invalid" + request, channel = self.make_request( + "POST", + "auth/m.login.recaptcha/fallback/web?session=" + + invalid_session + + "&g-recaptcha-response=a", + ) + self.render(request) + self.assertEqual(request.code, 400) From 264ef03986f9c7d9c9e84f2c52078b7934e62d54 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 11:27:14 -0400 Subject: [PATCH 24/36] Use creation time instead of last updated time. --- .../schema/delta/58/03persist_ui_auth.sql | 2 +- synapse/storage/data_stores/main/ui_auth.py | 19 ++----------------- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql index 93d6f3ee00e4..9cc3813bddbf 100644 --- a/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql @@ -15,7 +15,7 @@ CREATE TABLE IF NOT EXISTS ui_auth_sessions( session_id TEXT NOT NULL, -- The session ID passed to the client. - last_used BIGINT NOT NULL, -- The last time this session was seen (epoch time in milliseconds). + creation_time BIGINT NOT NULL, -- The time this session was created (epoch time in milliseconds). serverdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data added by Synapse. clientdict TEXT NOT NULL, -- A JSON dictionary of arbitrary data from the client. uri TEXT NOT NULL, -- The URI the UI authentication session is using. diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index ebdda2d7cdd6..54d40c04ddf1 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -90,7 +90,7 @@ async def create_ui_auth_session( "method": method, "description": description, "serverdict": "{}", - "last_used": self.hs.get_clock().time_msec(), + "creation_time": self.hs.get_clock().time_msec(), }, desc="create_ui_auth_session", ) @@ -142,7 +142,7 @@ def delete_old_ui_auth_sessions(self, expiration_time: int): def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): # Get the expired sessions. - sql = "SELECT session_id FROM ui_auth_sessions WHERE last_used <= ?" + sql = "SELECT session_id FROM ui_auth_sessions WHERE creation_time <= ?" txn.execute(sql, [expiration_time]) session_ids = [r[0] for r in txn.fetchall()] @@ -208,13 +208,6 @@ def _mark_ui_auth_stage_complete( keyvalues={"session_id": session_id, "stage_type": stage_type}, values={"identity": json.dumps(identity)}, ) - # Mark the session as still in use. - self.db.simple_update_one_txn( - txn, - table="ui_auth_sessions", - keyvalues={"session_id": session_id}, - updatevalues={"last_used": self.hs.get_clock().time_msec()}, - ) async def get_completed_ui_auth_stages( self, session_id: str @@ -280,14 +273,6 @@ def _set_ui_auth_session_data(self, txn, session_id: str, key: str, value: Any): updatevalues={"serverdict": json.dumps(serverdict)}, ) - # Mark the session as still in use. - self.db.simple_update_one_txn( - txn, - table="ui_auth_sessions", - keyvalues={"session_id": session_id}, - updatevalues={"last_used": self.hs.get_clock().time_msec()}, - ) - async def get_ui_auth_session_data( self, session_id: str, key: str, default: Optional[Any] = None ) -> Any: From 8b5ef4a00b92296cf6bada840b739372ac66e576 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 11:50:14 -0400 Subject: [PATCH 25/36] Rename the identity parameter to result. --- .../schema/delta/58/03persist_ui_auth.sql | 2 +- synapse/storage/data_stores/main/ui_auth.py | 22 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql index 9cc3813bddbf..49093cf63ae9 100644 --- a/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql @@ -29,6 +29,6 @@ CREATE TABLE IF NOT EXISTS ui_auth_sessions( CREATE TABLE IF NOT EXISTS ui_auth_sessions_credentials( session_id TEXT NOT NULL, -- The corresponding UI Auth session. stage_type TEXT NOT NULL, -- The stage type. - identity TEXT NOT NULL, -- The identity authenticated by the stage, stored as JSON. + result TEXT NOT NULL, -- The result of the stage verification, stored as JSON. UNIQUE (session_id, stage_type) ); diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 54d40c04ddf1..567f5b8bf9f2 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -165,7 +165,7 @@ def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): ) async def mark_ui_auth_stage_complete( - self, session_id: str, stage_type: str, identity: Union[str, bool, JsonDict], + self, session_id: str, stage_type: str, result: Union[str, bool, JsonDict], ): """ Mark a session stage as completed. @@ -173,22 +173,18 @@ async def mark_ui_auth_stage_complete( Args: session_id: The ID of the corresponding session. stage_type: The completed stage type. - identity: The identity authenticated by the stage. + result: The result of the stage verification. """ await self.db.runInteraction( "mark_ui_auth_stage_complete", self._mark_ui_auth_stage_complete, session_id, stage_type, - identity, + result, ) def _mark_ui_auth_stage_complete( - self, - txn, - session_id: str, - stage_type: str, - identity: Union[str, bool, JsonDict], + self, txn, session_id: str, stage_type: str, result: Union[str, bool, JsonDict], ): try: # Get the session to ensure it exists. @@ -206,7 +202,7 @@ def _mark_ui_auth_stage_complete( txn, table="ui_auth_sessions_credentials", keyvalues={"session_id": session_id, "stage_type": stage_type}, - values={"identity": json.dumps(identity)}, + values={"result": json.dumps(result)}, ) async def get_completed_ui_auth_stages( @@ -218,8 +214,8 @@ async def get_completed_ui_auth_stages( Args: session_id: The ID of the session. Returns: - The completed stages mapped to the relevant identity authenticated - by that auth-type (mostly str, but for captcha, bool). + The completed stages mapped to the result of the verification of + that auth-type. Raises: StoreError: if the session is not found. """ @@ -227,10 +223,10 @@ async def get_completed_ui_auth_stages( for row in await self.db.simple_select_list( table="ui_auth_sessions_credentials", keyvalues={"session_id": session_id}, - retcols=("stage_type", "identity"), + retcols=("stage_type", "result"), desc="get_completed_ui_auth_stages", ): - results[row["stage_type"]] = json.loads(row["identity"]) + results[row["stage_type"]] = json.loads(row["result"]) return results From f179c2175b35cbda69c09f9f832e3b6dbe80727e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Apr 2020 14:46:29 -0400 Subject: [PATCH 26/36] Separate the unsafe worker methods to a separate object. --- synapse/app/generic_worker.py | 4 +- synapse/storage/data_stores/main/ui_auth.py | 82 +++++++++++---------- 2 files changed, 44 insertions(+), 42 deletions(-) diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index a9fab272971a..5d21ea478be7 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -124,7 +124,7 @@ MonthlyActiveUsersWorkerStore, ) from synapse.storage.data_stores.main.presence import UserPresenceState -from synapse.storage.data_stores.main.ui_auth import UIAuthStore +from synapse.storage.data_stores.main.ui_auth import UIAuthWorkerStore from synapse.storage.data_stores.main.user_directory import UserDirectoryStore from synapse.types import ReadReceipt from synapse.util.async_helpers import Linearizer @@ -426,7 +426,7 @@ class GenericWorkerSlavedStore( # FIXME(#3714): We need to add UserDirectoryStore as we write directly # rather than going via the correct worker. UserDirectoryStore, - UIAuthStore, + UIAuthWorkerStore, SlavedDeviceInboxStore, SlavedDeviceStore, SlavedReceiptsStore, diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 567f5b8bf9f2..bf82e1b853bf 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -38,7 +38,7 @@ class UIAuthSessionData: description = attr.ib(type=str) -class UIAuthStore(SQLBaseStore): +class UIAuthWorkerStore(SQLBaseStore): """ Manage user interactive authentication sessions. """ @@ -125,45 +125,6 @@ async def get_ui_auth_session(self, session_id: str) -> UIAuthSessionData: return UIAuthSessionData(session_id, **result) - def delete_old_ui_auth_sessions(self, expiration_time: int): - """ - Remove sessions which were last used earlier than the expiration time. - - Args: - expiration_time: The latest time that is still considered valid. - This is an epoch time in milliseconds. - - """ - return self.db.runInteraction( - "delete_old_ui_auth_sessions", - self._delete_old_ui_auth_sessions, - expiration_time, - ) - - def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): - # Get the expired sessions. - sql = "SELECT session_id FROM ui_auth_sessions WHERE creation_time <= ?" - txn.execute(sql, [expiration_time]) - session_ids = [r[0] for r in txn.fetchall()] - - # Delete the corresponding completed credentials. - self.db.simple_delete_many_txn( - txn, - table="ui_auth_sessions_credentials", - column="session_id", - iterable=session_ids, - keyvalues={}, - ) - - # Finally, delete the sessions. - self.db.simple_delete_many_txn( - txn, - table="ui_auth_sessions", - column="session_id", - iterable=session_ids, - keyvalues={}, - ) - async def mark_ui_auth_stage_complete( self, session_id: str, stage_type: str, result: Union[str, bool, JsonDict], ): @@ -290,3 +251,44 @@ async def get_ui_auth_session_data( serverdict = json.loads(result["serverdict"]) return serverdict.get(key, default) + + +class UIAuthStore(UIAuthWorkerStore): + def delete_old_ui_auth_sessions(self, expiration_time: int): + """ + Remove sessions which were last used earlier than the expiration time. + + Args: + expiration_time: The latest time that is still considered valid. + This is an epoch time in milliseconds. + + """ + return self.db.runInteraction( + "delete_old_ui_auth_sessions", + self._delete_old_ui_auth_sessions, + expiration_time, + ) + + def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): + # Get the expired sessions. + sql = "SELECT session_id FROM ui_auth_sessions WHERE creation_time <= ?" + txn.execute(sql, [expiration_time]) + session_ids = [r[0] for r in txn.fetchall()] + + # Delete the corresponding completed credentials. + self.db.simple_delete_many_txn( + txn, + table="ui_auth_sessions_credentials", + column="session_id", + iterable=session_ids, + keyvalues={}, + ) + + # Finally, delete the sessions. + self.db.simple_delete_many_txn( + txn, + table="ui_auth_sessions", + column="session_id", + iterable=session_ids, + keyvalues={}, + ) From 64c709b18d6fec205a5b476247818684250a0f56 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 09:43:51 -0400 Subject: [PATCH 27/36] Use _txn in method names. --- synapse/storage/data_stores/main/ui_auth.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index bf82e1b853bf..bd348b5e3121 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -138,13 +138,13 @@ async def mark_ui_auth_stage_complete( """ await self.db.runInteraction( "mark_ui_auth_stage_complete", - self._mark_ui_auth_stage_complete, + self._mark_ui_auth_stage_complete_txn, session_id, stage_type, result, ) - def _mark_ui_auth_stage_complete( + def _mark_ui_auth_stage_complete_txn( self, txn, session_id: str, stage_type: str, result: Union[str, bool, JsonDict], ): try: @@ -204,13 +204,13 @@ async def set_ui_auth_session_data(self, session_id: str, key: str, value: Any): """ await self.db.runInteraction( "set_ui_auth_session_data", - self._set_ui_auth_session_data, + self._set_ui_auth_session_data_txn, session_id, key, value, ) - def _set_ui_auth_session_data(self, txn, session_id: str, key: str, value: Any): + def _set_ui_auth_session_data_txn(self, txn, session_id: str, key: str, value: Any): # Get the current value. result = self.db.simple_select_one_txn( txn, @@ -265,11 +265,11 @@ def delete_old_ui_auth_sessions(self, expiration_time: int): """ return self.db.runInteraction( "delete_old_ui_auth_sessions", - self._delete_old_ui_auth_sessions, + self._delete_old_ui_auth_sessions_txn, expiration_time, ) - def _delete_old_ui_auth_sessions(self, txn, expiration_time: int): + def _delete_old_ui_auth_sessions_txn(self, txn, expiration_time: int): # Get the expired sessions. sql = "SELECT session_id FROM ui_auth_sessions WHERE creation_time <= ?" txn.execute(sql, [expiration_time]) From 568a77891c39cdeac7094f53318109cd4b00657e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 09:48:47 -0400 Subject: [PATCH 28/36] Document possible error states better. --- synapse/storage/data_stores/main/ui_auth.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index bd348b5e3121..e38a515e03cd 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -135,6 +135,8 @@ async def mark_ui_auth_stage_complete( session_id: The ID of the corresponding session. stage_type: The completed stage type. result: The result of the stage verification. + Raises: + SynapseError if the session cannot be found. """ await self.db.runInteraction( "mark_ui_auth_stage_complete", @@ -177,8 +179,6 @@ async def get_completed_ui_auth_stages( Returns: The completed stages mapped to the result of the verification of that auth-type. - Raises: - StoreError: if the session is not found. """ results = {} for row in await self.db.simple_select_list( @@ -201,6 +201,8 @@ async def set_ui_auth_session_data(self, session_id: str, key: str, value: Any): session_id: The ID of this session as returned from check_auth key: The key to store the data under value: The data to store + Raises: + StoreError if the session cannot be found. """ await self.db.runInteraction( "set_ui_auth_session_data", @@ -240,6 +242,8 @@ async def get_ui_auth_session_data( session_id: The ID of this session as returned from check_auth key: The key to store the data under default: Value to return if the key has not been set + Raises: + StoreError if the session cannot be found. """ result = await self.db.simple_select_one( table="ui_auth_sessions", From 79c5be5d8ecd45265a4140830b967b0d31add446 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 09:51:49 -0400 Subject: [PATCH 29/36] Review feedback. --- synapse/handlers/auth.py | 2 +- synapse/storage/data_stores/main/ui_auth.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 9c8267473d53..b59ce1d3baf2 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -111,7 +111,7 @@ def __init__(self, hs): self._clock = self.hs.get_clock() # Expire old UI auth sessions after a period of time. - if hs.config.worker_name is None: + if hs.config.worker_app is None: self._clock.looping_call( run_as_background_process, 5 * 60 * 1000, diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index e38a515e03cd..b8069d6e31d9 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -28,7 +28,7 @@ class UIAuthSessionData: session_id = attr.ib(type=str) # The dictionary from the client root level, not the 'auth' key. clientdict = attr.ib(type=JsonDict) - # The URI and method the session was intiatied with, these are checked at + # The URI and method the session was intiatied with. These are checked at # each stage of the authentication to ensure that the asked for operation # has not changed. uri = attr.ib(type=str) From 18b349461c1ec8b431af39918dcd6acfdc302ca4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 12:20:55 -0400 Subject: [PATCH 30/36] Do not directly raise SynapseError. --- synapse/storage/data_stores/main/ui_auth.py | 42 +++++++++------------ tests/rest/client/v2_alpha/test_auth.py | 2 +- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index b8069d6e31d9..363e65c19932 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -18,7 +18,7 @@ import attr import synapse.util.stringutils as stringutils -from synapse.api.errors import StoreError, SynapseError +from synapse.api.errors import StoreError from synapse.storage._base import SQLBaseStore from synapse.types import JsonDict @@ -66,10 +66,10 @@ async def create_ui_auth_session( description: A string description of the operation that the current authentication is authorising. - Returns: The newly created session. - + Raise: + StoreError if a unique session ID cannot be generated. """ # The clientdict gets stored as JSON. clientdict_json = json.dumps(clientdict) @@ -109,17 +109,14 @@ async def get_ui_auth_session(self, session_id: str) -> UIAuthSessionData: Returns: A dict containing the device information. Raises: - SynapseError: if the session is not found. + StoreError if the session is not found. """ - try: - result = await self.db.simple_select_one( - table="ui_auth_sessions", - keyvalues={"session_id": session_id}, - retcols=("clientdict", "uri", "method", "description"), - desc="get_ui_auth_session", - ) - except StoreError: - raise SynapseError(400, "Unknown session ID: %s" % session_id) + result = await self.db.simple_select_one( + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + retcols=("clientdict", "uri", "method", "description"), + desc="get_ui_auth_session", + ) result["clientdict"] = json.loads(result["clientdict"]) @@ -136,7 +133,7 @@ async def mark_ui_auth_stage_complete( stage_type: The completed stage type. result: The result of the stage verification. Raises: - SynapseError if the session cannot be found. + StoreError if the session cannot be found. """ await self.db.runInteraction( "mark_ui_auth_stage_complete", @@ -149,16 +146,13 @@ async def mark_ui_auth_stage_complete( def _mark_ui_auth_stage_complete_txn( self, txn, session_id: str, stage_type: str, result: Union[str, bool, JsonDict], ): - try: - # Get the session to ensure it exists. - self.db.simple_select_one_txn( - txn, - table="ui_auth_sessions", - keyvalues={"session_id": session_id}, - retcols=("session_id",), - ) - except StoreError: - raise SynapseError(400, "Unknown session ID: %s" % session_id) + # Get the session to ensure it exists. + self.db.simple_select_one_txn( + txn, + table="ui_auth_sessions", + keyvalues={"session_id": session_id}, + retcols=("session_id",), + ) # Add (or update) the results of the current stage to the database. self.db.simple_upsert_txn( diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index a08ffdda3754..2c2e6b87c9e0 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -219,4 +219,4 @@ def test_complete_operation_unknown_session(self): + "&g-recaptcha-response=a", ) self.render(request) - self.assertEqual(request.code, 400) + self.assertEqual(request.code, 404) From 534066247841bdac1df320d4429391e81e26377d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 14:08:31 -0400 Subject: [PATCH 31/36] Use foreign keys to simplify logic. --- .../schema/delta/58/03persist_ui_auth.sql | 4 +- synapse/storage/data_stores/main/ui_auth.py | 37 ++++++------------- synapse/storage/engines/sqlite.py | 1 + tests/rest/client/v2_alpha/test_auth.py | 5 ++- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql index 49093cf63ae9..dcb593fc2deb 100644 --- a/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/03persist_ui_auth.sql @@ -30,5 +30,7 @@ CREATE TABLE IF NOT EXISTS ui_auth_sessions_credentials( session_id TEXT NOT NULL, -- The corresponding UI Auth session. stage_type TEXT NOT NULL, -- The stage type. result TEXT NOT NULL, -- The result of the stage verification, stored as JSON. - UNIQUE (session_id, stage_type) + UNIQUE (session_id, stage_type), + FOREIGN KEY (session_id) + REFERENCES ui_auth_sessions (session_id) ); diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 363e65c19932..78639d834639 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -135,32 +135,19 @@ async def mark_ui_auth_stage_complete( Raises: StoreError if the session cannot be found. """ - await self.db.runInteraction( - "mark_ui_auth_stage_complete", - self._mark_ui_auth_stage_complete_txn, - session_id, - stage_type, - result, - ) - - def _mark_ui_auth_stage_complete_txn( - self, txn, session_id: str, stage_type: str, result: Union[str, bool, JsonDict], - ): - # Get the session to ensure it exists. - self.db.simple_select_one_txn( - txn, - table="ui_auth_sessions", - keyvalues={"session_id": session_id}, - retcols=("session_id",), - ) - # Add (or update) the results of the current stage to the database. - self.db.simple_upsert_txn( - txn, - table="ui_auth_sessions_credentials", - keyvalues={"session_id": session_id, "stage_type": stage_type}, - values={"result": json.dumps(result)}, - ) + try: + await self.db.simple_insert( + table="ui_auth_sessions_credentials", + values={ + "session_id": session_id, + "stage_type": stage_type, + "result": json.dumps(result), + }, + desc="mark_ui_auth_stage_complete", + ) + except self.db.engine.module.IntegrityError: + raise StoreError(404, "Unknown session ID: %s" % (session_id,)) async def get_completed_ui_auth_stages( self, session_id: str diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py index 3bc2e8b9863f..215a94944287 100644 --- a/synapse/storage/engines/sqlite.py +++ b/synapse/storage/engines/sqlite.py @@ -85,6 +85,7 @@ def on_new_connection(self, db_conn): prepare_database(db_conn, self, config=None) db_conn.create_function("rank", 1, _rank) + db_conn.execute("PRAGMA foreign_keys = ON;") def is_deadlock(self, error): return False diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 2c2e6b87c9e0..941cc9648e39 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -211,11 +211,12 @@ def test_complete_operation_unknown_session(self): self.render(request) self.assertEqual(request.code, 200) - invalid_session = session + "invalid" + # Attempt to complete an unknown session, which should return an error. + unknown_session = session + "unknown" request, channel = self.make_request( "POST", "auth/m.login.recaptcha/fallback/web?session=" - + invalid_session + + unknown_session + "&g-recaptcha-response=a", ) self.render(request) From 5f0bf195ee8d44c185f8e2ffa913063b1bc04cb2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 16:03:38 -0400 Subject: [PATCH 32/36] Again fix idempotency of the registration API. --- synapse/storage/data_stores/main/ui_auth.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 78639d834639..671b34211053 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -136,14 +136,14 @@ async def mark_ui_auth_stage_complete( StoreError if the session cannot be found. """ # Add (or update) the results of the current stage to the database. + # + # Note that we need to allow for the same stage to complete multiple + # times here so that registration is idempotent. try: - await self.db.simple_insert( + await self.db.simple_upsert( table="ui_auth_sessions_credentials", - values={ - "session_id": session_id, - "stage_type": stage_type, - "result": json.dumps(result), - }, + keyvalues={"session_id": session_id, "stage_type": stage_type}, + values={"result": json.dumps(result),}, desc="mark_ui_auth_stage_complete", ) except self.db.engine.module.IntegrityError: From eccb67040de1517ec232875e64474849030998ee Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Apr 2020 16:53:30 -0400 Subject: [PATCH 33/36] Fix lint. --- synapse/storage/data_stores/main/ui_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 671b34211053..da687e042193 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -143,7 +143,7 @@ async def mark_ui_auth_stage_complete( await self.db.simple_upsert( table="ui_auth_sessions_credentials", keyvalues={"session_id": session_id, "stage_type": stage_type}, - values={"result": json.dumps(result),}, + values={"result": json.dumps(result)}, desc="mark_ui_auth_stage_complete", ) except self.db.engine.module.IntegrityError: From 106dca9536867ad8ef88ed0ee9e6cfb37944ee2d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Apr 2020 07:19:54 -0400 Subject: [PATCH 34/36] Fix typo in docstring. Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/storage/data_stores/main/ui_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index da687e042193..5bc34db2110b 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -68,7 +68,7 @@ async def create_ui_auth_session( authentication is authorising. Returns: The newly created session. - Raise: + Raises: StoreError if a unique session ID cannot be generated. """ # The clientdict gets stored as JSON. From 64852bf58df9a69e82bb7d7f14d110730942af38 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Apr 2020 11:12:15 -0400 Subject: [PATCH 35/36] Raise a 400 error, not 404. --- synapse/storage/data_stores/main/ui_auth.py | 2 +- tests/rest/client/v2_alpha/test_auth.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/data_stores/main/ui_auth.py b/synapse/storage/data_stores/main/ui_auth.py index 5bc34db2110b..c8eebc937843 100644 --- a/synapse/storage/data_stores/main/ui_auth.py +++ b/synapse/storage/data_stores/main/ui_auth.py @@ -147,7 +147,7 @@ async def mark_ui_auth_stage_complete( desc="mark_ui_auth_stage_complete", ) except self.db.engine.module.IntegrityError: - raise StoreError(404, "Unknown session ID: %s" % (session_id,)) + raise StoreError(400, "Unknown session ID: %s" % (session_id,)) async def get_completed_ui_auth_stages( self, session_id: str diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 941cc9648e39..587be7b2e714 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -220,4 +220,4 @@ def test_complete_operation_unknown_session(self): + "&g-recaptcha-response=a", ) self.render(request) - self.assertEqual(request.code, 404) + self.assertEqual(request.code, 400) From ae27afdaa9842d103e64d6d746f7a06a5885bc2c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Apr 2020 12:45:23 -0400 Subject: [PATCH 36/36] Convert StoreErrors to SynapseErrors. --- synapse/handlers/auth.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index b59ce1d3baf2..7613e5b6ab3b 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -312,7 +312,10 @@ async def check_auth( ) else: - session = await self.store.get_ui_auth_session(sid) + try: + session = await self.store.get_ui_auth_session(sid) + except StoreError: + raise SynapseError(400, "Unknown session ID: %s" % (sid,)) if not clientdict: # This was designed to allow the client to omit the parameters @@ -438,7 +441,10 @@ async def set_session_data(self, session_id: str, key: str, value: Any) -> None: key: The key to store the data under value: The data to store """ - await self.store.set_ui_auth_session_data(session_id, key, value) + try: + await self.store.set_ui_auth_session_data(session_id, key, value) + except StoreError: + raise SynapseError(400, "Unknown session ID: %s" % (session_id,)) async def get_session_data( self, session_id: str, key: str, default: Optional[Any] = None @@ -451,7 +457,10 @@ async def get_session_data( key: The key to store the data under default: Value to return if the key has not been set """ - return await self.store.get_ui_auth_session_data(session_id, key, default) + try: + return await self.store.get_ui_auth_session_data(session_id, key, default) + except StoreError: + raise SynapseError(400, "Unknown session ID: %s" % (session_id,)) async def _expire_old_sessions(self): """ @@ -1000,7 +1009,10 @@ async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: Returns: The HTML to render. """ - session = await self.store.get_ui_auth_session(session_id) + try: + session = await self.store.get_ui_auth_session(session_id) + except StoreError: + raise SynapseError(400, "Unknown session ID: %s" % (session_id,)) return self._sso_auth_confirm_template.render( description=session.description, redirect_url=redirect_url, )