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

Disable passwords for UI Auth if unable to use password login #7559

Closed
clokep opened this issue May 22, 2020 · 11 comments · Fixed by #8858
Closed

Disable passwords for UI Auth if unable to use password login #7559

clokep opened this issue May 22, 2020 · 11 comments · Fixed by #8858
Assignees
Labels
A-SSO Single Sign-On (maybe OIDC) z-bug (Deprecated Label) z-p3 (Deprecated Label)

Comments

@clokep
Copy link
Member

clokep commented May 22, 2020

Synapse providers a password option for UI Auth even if the user doesn't have a password available, this causes Riot to ask for a password during UI auth instead of asking for SSO.

A workaround for this when deploying SSO is to disable passwords in the configuration.

Originally conversation near #5667 (comment)

@clokep
Copy link
Member Author

clokep commented May 22, 2020

CC @Talanor and @ptman.

@clokep
Copy link
Member Author

clokep commented May 22, 2020

Per @erikjohnston we should be careful with this that sometimes people's passwords are removed in order to force a password reset.

@ptman
Copy link
Contributor

ptman commented May 22, 2020

element-hq/element-web#3544 - "misconfiguration"

@erikjohnston erikjohnston added improvement z-p2 (Deprecated Label) z-bug (Deprecated Label) and removed improvement z-p2 (Deprecated Label) labels Jun 4, 2020
@clokep
Copy link
Member Author

clokep commented Jun 4, 2020

We talked a bit about this today and we think this might be a client issue, it seems the flows might be prioritized differently between login and UI Auth?

@clokep
Copy link
Member Author

clokep commented Jun 4, 2020

Closing this as a client issue.

@clokep clokep closed this as completed Jun 4, 2020
@clokep
Copy link
Member Author

clokep commented Jun 4, 2020

I talked to the Riot Web team about this a bit and the situation is a bit more confusing then I realized, it seems that if SSO is enabled the only provided login methods from Synapse are m.login.sso and m.login.token, but for UI Auth we still provide the password one. So this might be an inconsistency in Synapse.

@ananace
Copy link

ananace commented Sep 9, 2020

Started running into this issue as well, though with the caveat that it also needs to support bot logins with non-SSO accounts, which means just turning off password auth entirely wasn't possible anymore.
I wasn't quite sure if the UI auth was necessary for said bots, so I'm currently just rearranging the auth types to put SSO on the top, which seems to behave for both Element and bot use-cases.

--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -102,10 +102,10 @@ class AuthHandler(BaseHandler):
         # Login types and UI Auth types have a heavy overlap, but are not
         # necessarily identical. Login types have SSO (and other login types)
         # added in the rest layer, see synapse.rest.client.v1.login.LoginRestServerlet.on_GET.
-        ui_auth_types = login_types.copy()
+        ui_auth_types = []
         if self._sso_enabled:
             ui_auth_types.append(LoginType.SSO)
-        self._supported_ui_auth_types = ui_auth_types
+        self._supported_ui_auth_types = ui_auth_types + login_types
 
         # Ratelimiter for failed auth during UIA. Uses same ratelimit config
         # as per `rc_login.failed_attempts`.

My guess is that a proper solution would be to check if the user is SSO-linked, and only offer SSO in that case, with regular password auths offered otherwise.

@clokep
Copy link
Member Author

clokep commented Sep 9, 2020

My guess is that a proper solution would be to check if the user is SSO-linked, and only offer SSO in that case, with regular password auths offered otherwise.

Unfortunately you don't know which user is trying to login until after the auth mechanisms are offered. 😄

@ananace
Copy link

ananace commented Sep 9, 2020

In our case, login is working perfectly fine, it's when an already logged-in user tries to perform a secure action (like removing a session) that things go badly.
So from my point of view, login could be kept as is, with the allowed auth types for any secure action that the logged in user then attempts being limited to only auth types that can actually be used to verify the user.

So then, logging in would offer all possible auth types, while any later authentication requirement for the logged in user would only offer the auth types that said user could possibly use.

@richvdh
Copy link
Member

richvdh commented Nov 12, 2020

Yes, what @ananace suggests sounds about right to me. I think this should be pretty easy.

@richvdh richvdh self-assigned this Nov 16, 2020
@clokep
Copy link
Member Author

clokep commented Nov 16, 2020

So then, logging in would offer all possible auth types, while any later authentication requirement for the logged in user would only offer the auth types that said user could possibly use.

This sounds reasonable, but defining what those are might be a bit tricky. I suppose we can check if the user has a password before showing that as an option. Knowing whether a user can login via SSO could be done by user_external_ids (although this wouldn't work for CAS, so we would need to do some work there).

richvdh added a commit that referenced this issue Dec 2, 2020
During user-interactive auth, do not offer password auth to users with no
password, nor SSO auth to users with no SSO.

Fixes #7559.
richvdh added a commit that referenced this issue Dec 2, 2020
During user-interactive auth, do not offer password auth to users with no
password, nor SSO auth to users with no SSO.

Fixes #7559.
richvdh added a commit that referenced this issue Dec 2, 2020
During user-interactive auth, do not offer password auth to users with no
password, nor SSO auth to users with no SSO.

Fixes #7559.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-SSO Single Sign-On (maybe OIDC) z-bug (Deprecated Label) z-p3 (Deprecated Label)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants