Return m.change_password.enabled=false if local database is disabled

[dklimpel]

Return m.change_password enabled:false if local database not used for authentication
Fixes: #9456

I had to remove a part of the test. Because I do not know how to create an access_token for an user who uses SSO.

What is the best config switch for this situation? password_config.enabled or password_config.localdb_enabled?

synapse/synapse/handlers/set_password.py

Lines 37 to 45 in 1baab20

	async def set_password(
	self,
	user_id: str,
	password_hash: str,
	logout_devices: bool,
	requester: Optional[Requester] = None,
	) -> None:
	if not self.hs.config.password_localdb_enabled:
	raise SynapseError(403, "Password change disabled", errcode=Codes.FORBIDDEN)

set_password uses password_config.localdb_enabled

synapse/docs/sample_config.yaml

Lines 2211 to 2220 in a7a3790

	password_config:
	# Uncomment to disable password login
	#
	#enabled: false
	# Uncomment 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.
	#
	#localdb_enabled: false

The documentation says that localdb_enabled is ignored if password_config.enabled is false. But I can not find the code where localdb_enabled becomes unset or set to false if localdb_enabled is false.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[clokep]

I wonder if we should also disable the server endpoint to change the password if no hash is currently set? This would avoid people logging in via SSO adding a password.

Again it is unclear to me whether we want to support that or not though. 😢

[dklimpel]

I wonder if we should also disable the server endpoint to change the password if no hash is currently set? This would avoid people logging in via SSO adding a password.

Again it is unclear to me whether we want to support that or not though. 😢

This exactly what _get_available_ui_auth_types does:

synapse/synapse/handlers/auth.py

Lines 399 to 411 in a7a3790

	async def _get_available_ui_auth_types(self, user: UserID) -> Iterable[str]:
	"""Get a list of the 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:
	lookupres = await self._find_user_id_and_pwd_hash(user.to_string())
	if lookupres:
	_, password_hash = lookupres
	if password_hash:
	ui_auth_types.add(LoginType.PASSWORD)

This is the state of this PR at the moment.

But than it is not possible to switch from SSO to password.
Only from passwort to SSO.
But it is more secure. To add a password is a little bit like creating a backdoor if your SSO is compromized.

@richvdh wrote in #synapse-dev:matrix.org

the answer to this is that, in the matrix.org usecase at least, we do want to allow people to add a password to their account even if they created that account with SSO

[clokep]

I don't think we need the _change_password property here, we can return self._password_enabled and self._password_localdb_enabled.

[clokep]

Looks good! Thanks for fixing this!

[xrayn]

IMO it would have been better to introduce a new configuration option for a possible password change and not make this solely dependent from the setting localdb_enabled. Ideal would be an option where one can disable password changes, but can still use the local db for authentication.

Because now, it is no longer possible to mix local users and a SSO (or LDAP, ...).

This might not be a concern for many people, but actually one can no longer add local users for testing or local users for bots an alike. For small deployments this is no concern at all, but for bigger deployments, adding users to the identity management system of a company or institution might be complicated (or nearly impossible) in terms of operational procedures.

[clokep]

@xrayn This code is pretty nuanced and I believe there could be bugs when having both local and SSO passwords, if you're seeing one please file a separate issue with details of your setup.

I don't see how this change would preclude having both local users and SSO (or LDAP) users.

[xrayn]

I don't see how this change would preclude having both local users and SSO (or LDAP) users.

Perhaps this is a misunderstanding, but I assume that when setting localdb_enabled to false then a user can no longer login with credentials from the localdb right?

[clokep]

I don't see how this change would preclude having both local users and SSO (or LDAP) users.

Perhaps this is a misunderstanding, but I assume that when setting localdb_enabled to false then a user can no longer login with credentials from the localdb right?

Yes, that's the point of that setting -- it disables the local database. If you don't have a local database then there's no way for a user to change their password, which this PR fixes.