From ed5bfd83737d31fb23581300efe570d66a44e170 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:08:56 +0100 Subject: [PATCH 01/12] Add an option, 'only_for_reauth', to `password_config.enabled` --- synapse/config/auth.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/config/auth.py b/synapse/config/auth.py index bb417a235946..88455f23b24c 100644 --- a/synapse/config/auth.py +++ b/synapse/config/auth.py @@ -29,7 +29,15 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: if password_config is None: password_config = {} - self.password_enabled = password_config.get("enabled", True) + passwords_enabled = password_config.get("enabled", True) + self.password_enabled = passwords_enabled + # 'only_for_reauth' allows users who have previously set a password to use it, + # even though passwords would otherwise be disabled. + self.password_enabled_for_reauth = passwords_enabled == "only_for_reauth" + + if self.password_enabled_for_reauth: + self.password_enabled = False + self.password_localdb_enabled = password_config.get("localdb_enabled", True) self.password_pepper = password_config.get("pepper", "") From ca071a44562714ad2c681550ef41d07ed0377884 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:09:12 +0100 Subject: [PATCH 02/12] Allow passwords for reauth if enabled --- synapse/handlers/auth.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 1b9050ea96c8..e6066321f10f 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -211,6 +211,7 @@ def __init__(self, hs: "HomeServer"): self.hs = hs # FIXME better possibility to access registrationHandler later? self.macaroon_gen = hs.get_macaroon_generator() self._password_enabled = hs.config.auth.password_enabled + self._password_enabled_for_reauth = hs.config.auth.password_enabled_for_reauth self._password_localdb_enabled = hs.config.auth.password_localdb_enabled self._third_party_rules = hs.get_third_party_event_rules() @@ -393,7 +394,9 @@ async def _get_available_ui_auth_types(self, user: UserID) -> Iterable[str]: # if the HS supports password auth, and the user has a non-null password, we # support password auth - if self._password_localdb_enabled and self._password_enabled: + if self._password_localdb_enabled and ( + self._password_enabled or self._password_enabled_for_reauth + ): lookupres = await self._find_user_id_and_pwd_hash(user.to_string()) if lookupres: _, password_hash = lookupres @@ -1133,7 +1136,7 @@ async def validate_login( # for the auth providers password = login_submission.get("password") if login_type == LoginType.PASSWORD: - if not self._password_enabled: + if not self._password_enabled and not self._password_enabled_for_reauth: raise SynapseError(400, "Password login has been disabled.") if not isinstance(password, str): raise SynapseError(400, "Bad parameter: password", Codes.INVALID_PARAM) From 8c836bbd614818bda071dc4581674c04c0150032 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:09:36 +0100 Subject: [PATCH 03/12] Add a simple test case (I can't see a better place for this, if you find it, let me know) --- tests/rest/client/test_auth.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 9653f458379f..3182ff044e16 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -12,6 +12,7 @@ # 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 http import HTTPStatus from typing import Any, Dict, List, Optional, Tuple, Union @@ -22,6 +23,7 @@ import synapse.rest.admin from synapse.api.constants import LoginType from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker +from synapse.rest import admin from synapse.rest.client import account, auth, devices, login, logout, register from synapse.rest.synapse.client import build_synapse_client_resource_tree from synapse.server import HomeServer @@ -33,7 +35,7 @@ from tests.handlers.test_oidc import HAS_OIDC from tests.rest.client.utils import TEST_OIDC_CONFIG from tests.server import FakeChannel -from tests.unittest import override_config, skip_unless +from tests.unittest import HomeserverTestCase, override_config, skip_unless class DummyRecaptchaChecker(UserInteractiveAuthChecker): @@ -1079,3 +1081,28 @@ def _txn(txn: LoggingTransaction) -> int: # and no refresh token self.assertEqual(_table_length("access_tokens"), 0) self.assertEqual(_table_length("refresh_tokens"), 0) + + +class PasswordReauthTestCase(HomeserverTestCase): + servlets = [admin.register_servlets, login.register_servlets] + + @override_config({"password_config": {"enabled": "only_for_reauth"}}) + def test_password_reauth_succeeds_with_setting(self) -> None: + """ + A user can re-authenticate using a previously-set password if + 'only_for_reauth' is set. + """ + user_id = self.register_user("christina", "verysecret") + self.login(user_id, "verysecret") + + @override_config({"password_config": {"enabled": False}}) + def test_password_reauth_fails_if_disabled(self) -> None: + user_id = self.register_user("christina", "verysecret") + + body = {"type": "m.login.password", "user": user_id, "password": "verysecret"} + channel = self.make_request( + "POST", + "/_matrix/client/r0/login", + json.dumps(body).encode("utf8"), + ) + self.assertEqual(channel.code, 400, channel.result) From dec6316d5310fae5d74d254576b4e0cc67cae36d Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:11:27 +0100 Subject: [PATCH 04/12] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/12883.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12883.feature diff --git a/changelog.d/12883.feature b/changelog.d/12883.feature new file mode 100644 index 000000000000..d133055e4fb2 --- /dev/null +++ b/changelog.d/12883.feature @@ -0,0 +1 @@ +Add an option allowing users to use their password to log in or reauthenticate even though password authentication is disabled. \ No newline at end of file From 1634475c16a22bdc9dd6dae91f20f412ca4b7f50 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:20:44 +0100 Subject: [PATCH 05/12] Add note in the configuration manual --- docs/usage/configuration/config_documentation.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 21dad0ac41e2..1bbd055e9dd0 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2873,6 +2873,9 @@ Use this setting to enable password-based logins. This setting has the following sub-options: * `enabled`: Defaults to true. + Set to false to disable password authentication. + Set to `only_for_reauth` to allow users with existing passwords to use them + to log in and reauthenticate, whilst preventing new users from setting passwords. * `localdb_enabled`: Set to false to disable authentication against the local password database. This is ignored if `enabled` is false, and is only useful if you have other `password_providers`. Defaults to true. From 062832ec7cf1a8f9fc5d7612a569e52a52640dba Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:42:16 +0100 Subject: [PATCH 06/12] Clean up the configuration logic somewhat --- synapse/config/auth.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/config/auth.py b/synapse/config/auth.py index 88455f23b24c..b740967225f6 100644 --- a/synapse/config/auth.py +++ b/synapse/config/auth.py @@ -30,13 +30,16 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: password_config = {} passwords_enabled = password_config.get("enabled", True) - self.password_enabled = passwords_enabled # 'only_for_reauth' allows users who have previously set a password to use it, # even though passwords would otherwise be disabled. - self.password_enabled_for_reauth = passwords_enabled == "only_for_reauth" + passwords_for_reauth_only = passwords_enabled == "only_for_reauth" - if self.password_enabled_for_reauth: - self.password_enabled = False + self.password_enabled_for_login = ( + passwords_enabled and not passwords_for_reauth_only + ) + self.password_enabled_for_reauth = ( + passwords_for_reauth_only or passwords_enabled + ) self.password_localdb_enabled = password_config.get("localdb_enabled", True) self.password_pepper = password_config.get("pepper", "") From 63022f8ede8a27d5e78aae20d6e26220cd9c7851 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:46:08 +0100 Subject: [PATCH 07/12] Split the password_enabled settings depending on use case --- synapse/handlers/auth.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index e6066321f10f..7262f1520f2f 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -210,7 +210,7 @@ def __init__(self, hs: "HomeServer"): self.hs = hs # FIXME better possibility to access registrationHandler later? self.macaroon_gen = hs.get_macaroon_generator() - self._password_enabled = hs.config.auth.password_enabled + self._password_enabled_for_login = hs.config.auth.password_enabled_for_login self._password_enabled_for_reauth = hs.config.auth.password_enabled_for_reauth self._password_localdb_enabled = hs.config.auth.password_localdb_enabled self._third_party_rules = hs.get_third_party_event_rules() @@ -388,15 +388,13 @@ def get_new_session_data() -> JsonDict: return params, session_id async def _get_available_ui_auth_types(self, user: UserID) -> Iterable[str]: - """Get a list of the authentication types this user can use""" + """Get a list of the user-interactive authentication types this user can use.""" ui_auth_types = set() # if the HS supports password auth, and the user has a non-null password, we # support password auth - if self._password_localdb_enabled and ( - self._password_enabled or self._password_enabled_for_reauth - ): + if self._password_localdb_enabled and self._password_enabled_for_reauth: lookupres = await self._find_user_id_and_pwd_hash(user.to_string()) if lookupres: _, password_hash = lookupres @@ -405,7 +403,7 @@ async def _get_available_ui_auth_types(self, user: UserID) -> Iterable[str]: # also allow auth from password providers for t in self.password_auth_provider.get_supported_login_types().keys(): - if t == LoginType.PASSWORD and not self._password_enabled: + if t == LoginType.PASSWORD and not self._password_enabled_for_reauth: continue ui_auth_types.add(t) @@ -1067,7 +1065,7 @@ def can_change_password(self) -> bool: Returns: Whether users on this server are allowed to change or set a password """ - return self._password_enabled and self._password_localdb_enabled + return self._password_enabled_for_login and self._password_localdb_enabled def get_supported_login_types(self) -> Iterable[str]: """Get a the login types supported for the /login API @@ -1092,9 +1090,9 @@ def get_supported_login_types(self) -> Iterable[str]: # that comes first, where it's present. if LoginType.PASSWORD in types: types.remove(LoginType.PASSWORD) - if self._password_enabled: + if self._password_enabled_for_login: types.insert(0, LoginType.PASSWORD) - elif self._password_localdb_enabled and self._password_enabled: + elif self._password_localdb_enabled and self._password_enabled_for_login: types.insert(0, LoginType.PASSWORD) return types @@ -1136,7 +1134,7 @@ async def validate_login( # for the auth providers password = login_submission.get("password") if login_type == LoginType.PASSWORD: - if not self._password_enabled and not self._password_enabled_for_reauth: + if not self._password_enabled_for_login: raise SynapseError(400, "Password login has been disabled.") if not isinstance(password, str): raise SynapseError(400, "Bad parameter: password", Codes.INVALID_PARAM) From 9ae819efa9b2b7526aa090a3b6ca4bf1fb403052 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 13:47:00 +0100 Subject: [PATCH 08/12] Revert "Add a simple test case" This reverts commit 8c836bbd614818bda071dc4581674c04c0150032. --- tests/rest/client/test_auth.py | 29 +---------------------------- 1 file changed, 1 insertion(+), 28 deletions(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 3182ff044e16..9653f458379f 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -12,7 +12,6 @@ # 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 http import HTTPStatus from typing import Any, Dict, List, Optional, Tuple, Union @@ -23,7 +22,6 @@ import synapse.rest.admin from synapse.api.constants import LoginType from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker -from synapse.rest import admin from synapse.rest.client import account, auth, devices, login, logout, register from synapse.rest.synapse.client import build_synapse_client_resource_tree from synapse.server import HomeServer @@ -35,7 +33,7 @@ from tests.handlers.test_oidc import HAS_OIDC from tests.rest.client.utils import TEST_OIDC_CONFIG from tests.server import FakeChannel -from tests.unittest import HomeserverTestCase, override_config, skip_unless +from tests.unittest import override_config, skip_unless class DummyRecaptchaChecker(UserInteractiveAuthChecker): @@ -1081,28 +1079,3 @@ def _txn(txn: LoggingTransaction) -> int: # and no refresh token self.assertEqual(_table_length("access_tokens"), 0) self.assertEqual(_table_length("refresh_tokens"), 0) - - -class PasswordReauthTestCase(HomeserverTestCase): - servlets = [admin.register_servlets, login.register_servlets] - - @override_config({"password_config": {"enabled": "only_for_reauth"}}) - def test_password_reauth_succeeds_with_setting(self) -> None: - """ - A user can re-authenticate using a previously-set password if - 'only_for_reauth' is set. - """ - user_id = self.register_user("christina", "verysecret") - self.login(user_id, "verysecret") - - @override_config({"password_config": {"enabled": False}}) - def test_password_reauth_fails_if_disabled(self) -> None: - user_id = self.register_user("christina", "verysecret") - - body = {"type": "m.login.password", "user": user_id, "password": "verysecret"} - channel = self.make_request( - "POST", - "/_matrix/client/r0/login", - json.dumps(body).encode("utf8"), - ) - self.assertEqual(channel.code, 400, channel.result) From 9419660ec127cbb6c6d415b80db9b079a9059a97 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 14:17:03 +0100 Subject: [PATCH 09/12] Add a test for a UIAA action --- tests/rest/client/test_auth.py | 41 ++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 9653f458379f..05355c7fb6d8 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -195,8 +195,17 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.user_pass = "pass" self.user = self.register_user("test", self.user_pass) self.device_id = "dev1" + + # Force-enable password login for just long enough to log in. + auth_handler = self.hs.get_auth_handler() + allow_auth_for_login = auth_handler._password_enabled_for_login + auth_handler._password_enabled_for_login = True + self.user_tok = self.login("test", self.user_pass, self.device_id) + # Restore password login to however it was. + auth_handler._password_enabled_for_login = allow_auth_for_login + def delete_device( self, access_token: str, @@ -263,6 +272,38 @@ def test_ui_auth(self) -> None: }, ) + @override_config({"password_config": {"enabled": "only_for_reauth"}}) + def test_ui_auth_with_passwords_for_reauth_only(self) -> None: + """ + Test user interactive authentication outside of registration. + """ + + # Attempt to delete this device. + # Returns a 401 as per the spec + channel = self.delete_device( + self.user_tok, self.device_id, HTTPStatus.UNAUTHORIZED + ) + + # Grab the session + session = channel.json_body["session"] + # Ensure that flows are what is expected. + self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) + + # Make another request providing the UI auth flow. + self.delete_device( + self.user_tok, + self.device_id, + HTTPStatus.OK, + { + "auth": { + "type": "m.login.password", + "identifier": {"type": "m.id.user", "user": self.user}, + "password": self.user_pass, + "session": session, + }, + }, + ) + def test_grandfathered_identifier(self) -> None: """Check behaviour without "identifier" dict From f27e73afad139f284033036769c4f14d1010cf35 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 14:17:31 +0100 Subject: [PATCH 10/12] Reauth using passwords fall back to the login flow, so we need a new flag there --- synapse/handlers/auth.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 7262f1520f2f..fbafbbee6b0b 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -711,7 +711,7 @@ async def _check_auth_dict( return res # fall back to the v1 login flow - canonical_id, _ = await self.validate_login(authdict) + canonical_id, _ = await self.validate_login(authdict, is_reauth=True) return canonical_id def _get_params_recaptcha(self) -> dict: @@ -1101,6 +1101,7 @@ async def validate_login( self, login_submission: Dict[str, Any], ratelimit: bool = False, + is_reauth: bool = False, ) -> Tuple[str, Optional[Callable[["LoginResponse"], Awaitable[None]]]]: """Authenticates the user for the /login API @@ -1111,6 +1112,9 @@ async def validate_login( login_submission: the whole of the login submission (including 'type' and other relevant fields) ratelimit: whether to apply the failed_login_attempt ratelimiter + is_reauth: whether this is part of a User-Interactive Authorisation + flow to reauthenticate for a privileged action (rather than a + new login) Returns: A tuple of the canonical user id, and optional callback to be called once the access token and device id are issued @@ -1133,8 +1137,14 @@ async def validate_login( # special case to check for "password" for the check_password interface # for the auth providers password = login_submission.get("password") + if login_type == LoginType.PASSWORD: - if not self._password_enabled_for_login: + if is_reauth: + passwords_allowed_here = self._password_enabled_for_reauth + else: + passwords_allowed_here = self._password_enabled_for_login + + if not passwords_allowed_here: raise SynapseError(400, "Password login has been disabled.") if not isinstance(password, str): raise SynapseError(400, "Bad parameter: password", Codes.INVALID_PARAM) From 495d123af482b129e5224295f1852149fc3f3acd Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 26 May 2022 14:18:23 +0100 Subject: [PATCH 11/12] Fix Newsfile --- changelog.d/12883.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12883.feature b/changelog.d/12883.feature index d133055e4fb2..83626771c065 100644 --- a/changelog.d/12883.feature +++ b/changelog.d/12883.feature @@ -1 +1 @@ -Add an option allowing users to use their password to log in or reauthenticate even though password authentication is disabled. \ No newline at end of file +Add an option allowing users to use their password to reauthenticate for privileged actions even though password login is disabled. From 1cb24f4c2312c203d3b863d3668a82e340a76d97 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 27 May 2022 10:13:55 +0100 Subject: [PATCH 12/12] Update sample config --- docs/sample_config.yaml | 4 +++- synapse/config/auth.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index a803b8261dcd..2cb3ac7f58ac 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -2192,7 +2192,9 @@ sso: password_config: - # Uncomment to disable password login + # Uncomment to disable password login. + # Set to `only_for_reauth` to permit reauthentication for users that + # have passwords and are already logged in. # #enabled: false diff --git a/synapse/config/auth.py b/synapse/config/auth.py index b740967225f6..265a554a5da6 100644 --- a/synapse/config/auth.py +++ b/synapse/config/auth.py @@ -57,7 +57,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: def generate_config_section(self, **kwargs: Any) -> str: return """\ password_config: - # Uncomment to disable password login + # Uncomment to disable password login. + # Set to `only_for_reauth` to permit reauthentication for users that + # have passwords and are already logged in. # #enabled: false