Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add an option allowing users to use their password to reauthenticate even though password authentication is disabled. #12883

Merged
merged 12 commits into from
May 27, 2022
1 change: 1 addition & 0 deletions changelog.d/12883.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add an option allowing users to use their password to reauthenticate for privileged actions even though password login is disabled.
3 changes: 3 additions & 0 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also want to update the documentation in the sample config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought it got removed but no you're right

* `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.
Expand Down
13 changes: 12 additions & 1 deletion synapse/config/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,18 @@ 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)
# 'only_for_reauth' allows users who have previously set a password to use it,
# even though passwords would otherwise be disabled.
passwords_for_reauth_only = passwords_enabled == "only_for_reauth"

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", "")

Expand Down
29 changes: 20 additions & 9 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ 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()

Expand Down Expand Up @@ -387,13 +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:
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
Expand All @@ -402,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)

Expand Down Expand Up @@ -710,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:
Expand Down Expand Up @@ -1064,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
Expand All @@ -1089,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
Expand All @@ -1100,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
Expand All @@ -1110,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
Expand All @@ -1132,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:
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)
Expand Down
41 changes: 41 additions & 0 deletions tests/rest/client/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down