From c12b9d719a3cf1eeb9c4c8d354dbaecab5e76233 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 11:56:24 +0000 Subject: [PATCH 1/9] Make registration idempotent: if you specify the same session, make it give you an access token for the user that was registered on previous uses of that session. Tweak the UI auth layer to not delete sessions when their auth has completed and hence expire themn so they don't hang around until server restart. Allow server-side data to be associated with UI auth sessions. --- synapse/handlers/auth.py | 58 +++++++++++++++++++----- synapse/rest/client/v2_alpha/register.py | 27 ++++++++++- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 5c0ea636b..5dc9d9175 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -27,6 +27,7 @@ import logging import bcrypt import pymacaroons import simplejson +import time import synapse.util.stringutils as stringutils @@ -35,6 +36,7 @@ logger = logging.getLogger(__name__) class AuthHandler(BaseHandler): + SESSION_EXPIRE_SECS = 48 * 60 * 60 def __init__(self, hs): super(AuthHandler, self).__init__(hs) @@ -66,15 +68,18 @@ class AuthHandler(BaseHandler): 'auth' key: this method prompts for auth if none is sent. clientip (str): The IP address of the client. Returns: - A tuple of (authed, dict, dict) where authed is true if the client - has successfully completed an auth flow. If it is true, the first - dict contains the authenticated credentials of each stage. + A tuple of (authed, dict, dict, session_id) where authed is true if + the client has successfully completed an auth flow. If it is true + the first dict contains the authenticated credentials of each stage. If authed is false, the first dictionary is the server response to the login request and should be passed back to the client. In either case, the second dict contains the parameters for this request (which may have been given only in a previous call). + + session_id is the ID of this session, either passed in by the client + or assigned by the call to check_auth """ authdict = None @@ -103,7 +108,10 @@ class AuthHandler(BaseHandler): if not authdict: defer.returnValue( - (False, self._auth_dict_for_flows(flows, session), clientdict) + ( + False, self._auth_dict_for_flows(flows, session), + clientdict, session['id'] + ) ) if 'creds' not in session: @@ -122,12 +130,11 @@ class AuthHandler(BaseHandler): for f in flows: if len(set(f) - set(creds.keys())) == 0: logger.info("Auth completed with creds: %r", creds) - self._remove_session(session) - defer.returnValue((True, creds, clientdict)) + defer.returnValue((True, creds, clientdict, session['id'])) ret = self._auth_dict_for_flows(flows, session) ret['completed'] = creds.keys() - defer.returnValue((False, ret, clientdict)) + defer.returnValue((False, ret, clientdict, session['id'])) @defer.inlineCallbacks def add_oob_auth(self, stagetype, authdict, clientip): @@ -154,6 +161,29 @@ class AuthHandler(BaseHandler): defer.returnValue(True) defer.returnValue(False) + def set_session_data(self, session_id, key, value): + """ + 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. + :param session_id: (string) The ID of this session as returned from check_auth + :param key: (string) The key to store the data under + :param value: (any) The data to store + """ + sess = self._get_session_info(session_id) + sess.setdefault('serverdict', {})[key] = value + self._save_session(sess) + + def get_session_data(self, session_id, key, default=None): + """ + Retrieve data stored with set_session_data + :param session_id: (string) The ID of this session as returned from check_auth + :param key: (string) The key to store the data under + :param default: (any) Value to return if the key has not been set + """ + sess = self._get_session_info(session_id) + return sess.setdefault('serverdict', {}).get(key, default) + @defer.inlineCallbacks def _check_password_auth(self, authdict, _): if "user" not in authdict or "password" not in authdict: @@ -263,7 +293,7 @@ class AuthHandler(BaseHandler): if not session_id: # create a new session while session_id is None or session_id in self.sessions: - session_id = stringutils.random_string(24) + session_id = stringutils.random_string_with_symbols(24) self.sessions[session_id] = { "id": session_id, } @@ -455,11 +485,17 @@ class AuthHandler(BaseHandler): def _save_session(self, session): # TODO: Persistent storage logger.debug("Saving session %s", session) + session["last_used"] = time.time() self.sessions[session["id"]] = session + self._prune_sessions() - def _remove_session(self, session): - logger.debug("Removing session %s", session) - del self.sessions[session["id"]] + def _prune_sessions(self): + for sid,sess in self.sessions.items(): + last_used = 0 + if 'last_used' in sess: + last_used = sess['last_used'] + if last_used < time.time() - AuthHandler.SESSION_EXPIRE_SECS: + del self.sessions[sid] def hash(self, password): """Computes a secure hash of password. diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 533ff136e..649491bdf 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -139,7 +139,7 @@ class RegisterRestServlet(RestServlet): [LoginType.EMAIL_IDENTITY] ] - authed, result, params = yield self.auth_handler.check_auth( + authed, result, params, session_id = yield self.auth_handler.check_auth( flows, body, self.hs.get_ip_from_request(request) ) @@ -147,6 +147,24 @@ class RegisterRestServlet(RestServlet): defer.returnValue((401, result)) return + # have we already registered a user for this session + registered_user_id = self.auth_handler.get_session_data( + session_id, "registered_user_id", None + ) + if registered_user_id is not None: + logger.info( + "Already registered user ID %r for this session", + registered_user_id + ) + access_token = yield self.auth_handler.issue_access_token(registered_user_id) + refresh_token = yield self.auth_handler.issue_refresh_token(registered_user_id) + defer.returnValue((200, { + "user_id": registered_user_id, + "access_token": access_token, + "home_server": self.hs.hostname, + "refresh_token": refresh_token, + })) + # NB: This may be from the auth handler and NOT from the POST if 'password' not in params: raise SynapseError(400, "Missing password.", Codes.MISSING_PARAM) @@ -161,6 +179,13 @@ class RegisterRestServlet(RestServlet): guest_access_token=guest_access_token, ) + # remember that we've now registered that user account, and with what + # user ID (since the user may not have specified) + logger.info("%r", body) + self.auth_handler.set_session_data( + session_id, "registered_user_id", user_id + ) + if result and LoginType.EMAIL_IDENTITY in result: threepid = result[LoginType.EMAIL_IDENTITY] From 99797947aa5a7cdf8fe12043b4f25a155bcf4555 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 12:51:34 +0000 Subject: [PATCH 2/9] pep8 & remove debug logging --- synapse/handlers/auth.py | 2 +- synapse/rest/client/v2_alpha/register.py | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 5dc9d9175..a9f5e3710 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -490,7 +490,7 @@ class AuthHandler(BaseHandler): self._prune_sessions() def _prune_sessions(self): - for sid,sess in self.sessions.items(): + for sid, sess in self.sessions.items(): last_used = 0 if 'last_used' in sess: last_used = sess['last_used'] diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 649491bdf..c440430e2 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -149,7 +149,7 @@ class RegisterRestServlet(RestServlet): # have we already registered a user for this session registered_user_id = self.auth_handler.get_session_data( - session_id, "registered_user_id", None + session_id, "registered_user_id", None ) if registered_user_id is not None: logger.info( @@ -157,7 +157,9 @@ class RegisterRestServlet(RestServlet): registered_user_id ) access_token = yield self.auth_handler.issue_access_token(registered_user_id) - refresh_token = yield self.auth_handler.issue_refresh_token(registered_user_id) + refresh_token = yield self.auth_handler.issue_refresh_token( + registered_user_id + ) defer.returnValue((200, { "user_id": registered_user_id, "access_token": access_token, @@ -181,9 +183,8 @@ class RegisterRestServlet(RestServlet): # remember that we've now registered that user account, and with what # user ID (since the user may not have specified) - logger.info("%r", body) self.auth_handler.set_session_data( - session_id, "registered_user_id", user_id + session_id, "registered_user_id", user_id ) if result and LoginType.EMAIL_IDENTITY in result: From ff7d3dc3a03538b08f36342b15492a348b2b0391 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 14:25:14 +0000 Subject: [PATCH 3/9] Fix tests --- tests/rest/client/v2_alpha/test_register.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py index 9a202e9dd..affd42c01 100644 --- a/tests/rest/client/v2_alpha/test_register.py +++ b/tests/rest/client/v2_alpha/test_register.py @@ -22,9 +22,10 @@ class RegisterRestServletTestCase(unittest.TestCase): side_effect=lambda x: defer.succeed(self.appservice)) ) - self.auth_result = (False, None, None) + self.auth_result = (False, None, None, None) self.auth_handler = Mock( - check_auth=Mock(side_effect=lambda x, y, z: self.auth_result) + check_auth=Mock(side_effect=lambda x, y, z: self.auth_result), + get_session_data=Mock(return_value=None) ) self.registration_handler = Mock() self.identity_handler = Mock() @@ -112,7 +113,7 @@ class RegisterRestServletTestCase(unittest.TestCase): self.auth_result = (True, None, { "username": "kermit", "password": "monkey" - }) + }, None) self.registration_handler.register = Mock(return_value=(user_id, token)) (code, result) = yield self.servlet.on_POST(self.request) @@ -135,7 +136,7 @@ class RegisterRestServletTestCase(unittest.TestCase): self.auth_result = (True, None, { "username": "kermit", "password": "monkey" - }) + }, None) self.registration_handler.register = Mock(return_value=("@user:id", "t")) d = self.servlet.on_POST(self.request) return self.assertFailure(d, SynapseError) From f5e90422f5d70afaf9bdf97cc620b563cf31a8eb Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 14:33:19 +0000 Subject: [PATCH 4/9] take extra return val from check_auth in account too --- synapse/rest/client/v2_alpha/account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index dd4ea4558..7f8a6a4cf 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -43,7 +43,7 @@ class PasswordRestServlet(RestServlet): body = parse_json_object_from_request(request) - authed, result, params = yield self.auth_handler.check_auth([ + authed, result, params, _ = yield self.auth_handler.check_auth([ [LoginType.PASSWORD], [LoginType.EMAIL_IDENTITY] ], body, self.hs.get_ip_from_request(request)) From 742b6c6d158f46a71724821ce13b8ad535df08bc Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 15:42:35 +0000 Subject: [PATCH 5/9] Use hs get_clock instead of time.time() --- synapse/handlers/auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index a9f5e3710..dba6c76df 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -36,7 +36,7 @@ logger = logging.getLogger(__name__) class AuthHandler(BaseHandler): - SESSION_EXPIRE_SECS = 48 * 60 * 60 + SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000 def __init__(self, hs): super(AuthHandler, self).__init__(hs) @@ -494,7 +494,7 @@ class AuthHandler(BaseHandler): last_used = 0 if 'last_used' in sess: last_used = sess['last_used'] - if last_used < time.time() - AuthHandler.SESSION_EXPIRE_SECS: + if last_used < self.hs.get_clock().time() - AuthHandler.SESSION_EXPIRE_MS: del self.sessions[sid] def hash(self, password): From 9671e6750c1756c7f76888b87eedfe7516ef748b Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 15:51:28 +0000 Subject: [PATCH 6/9] Replace other time.time(). --- synapse/handlers/auth.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index dba6c76df..9fa834709 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -27,7 +27,6 @@ import logging import bcrypt import pymacaroons import simplejson -import time import synapse.util.stringutils as stringutils @@ -485,7 +484,7 @@ class AuthHandler(BaseHandler): def _save_session(self, session): # TODO: Persistent storage logger.debug("Saving session %s", session) - session["last_used"] = time.time() + session["last_used"] = self.hs.get_clock().time_msec() self.sessions[session["id"]] = session self._prune_sessions() From 3176aebf9d827eeb939438deea49e12ceddc5b3e Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 15:55:49 +0000 Subject: [PATCH 7/9] string with symbols is a bit too symboly. --- synapse/handlers/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 9fa834709..85f9b8271 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -292,7 +292,7 @@ class AuthHandler(BaseHandler): if not session_id: # create a new session while session_id is None or session_id in self.sessions: - session_id = stringutils.random_string_with_symbols(24) + session_id = stringutils.random_string(24) self.sessions[session_id] = { "id": session_id, } From 3ee7d7dc7f1793beefee433f780af81a64dfa590 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 16:18:52 +0000 Subject: [PATCH 8/9] time_msec() --- synapse/handlers/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 85f9b8271..cdf9e9000 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -493,7 +493,7 @@ class AuthHandler(BaseHandler): last_used = 0 if 'last_used' in sess: last_used = sess['last_used'] - if last_used < self.hs.get_clock().time() - AuthHandler.SESSION_EXPIRE_MS: + if last_used < self.hs.get_clock().time_msec() - AuthHandler.SESSION_EXPIRE_MS: del self.sessions[sid] def hash(self, password): From b58d10a87595bd64305f46c2ac86252a67d7b0e4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 16 Mar 2016 16:22:20 +0000 Subject: [PATCH 9/9] pep8 --- synapse/handlers/auth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index cdf9e9000..d7233cd0d 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -493,7 +493,8 @@ class AuthHandler(BaseHandler): last_used = 0 if 'last_used' in sess: last_used = sess['last_used'] - if last_used < self.hs.get_clock().time_msec() - AuthHandler.SESSION_EXPIRE_MS: + now = self.hs.get_clock().time_msec() + if last_used < now - AuthHandler.SESSION_EXPIRE_MS: del self.sessions[sid] def hash(self, password):