Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activating a user without setting a password is a valid user case #39

Merged
merged 1 commit into from
Nov 7, 2021

Conversation

andir
Copy link
Contributor

@andir andir commented Jul 4, 2021

Activating users in an SSO environment is a valid operation even when not providing a password. We shouldn't require a password to be set in those cases.

Activating users in an SSO environment is a valid operation even when not providing a password. We shouldn't require a password to be set in those cases.
@JOJ0
Copy link
Owner

JOJ0 commented Jul 9, 2021

Hi, thanks a lot for the pull.
I just had a look and realized that I probably misread the last sentence here: https://matrix-org.github.io/synapse/develop/admin_api/user_admin_api.html

In order to re-activate an account deactivated must be set to false. If users do not login via single-sign-on, a new password must be provided.

So what you are suggesting is actually well documented here. My bad. Thanks a lot for the hint!
What I am thinking now is: How could we make sure that a user that is not in an SSO environment is reactivated but does not have a password set. E.g the admin forgets to set it one on reactivation.

I did not test it yet and also not investigate thoroughly but maybe you know already: On reactivation, does the password that was set prior to deactivation still exist and is valid after the activation? Or would we now have a user that has no password set, and is also allowed to login without one? The latter would be a case I'd like to prevent/warn about.

@andir
Copy link
Contributor Author

andir commented Jul 9, 2021

A new password is required as the password hash is wiped on deactivation: https://github.com/matrix-org/synapse/blob/c1ddbbde4fb948cf740d4c59869157943d3711c6/synapse/handlers/deactivate_account.py#L122

@JOJ0
Copy link
Owner

JOJ0 commented Jul 11, 2021

Please be so kind and spit out a warning to the user when no password is set on re-activation. State that this usually is ok in SSO environments. Or something like that.
The warning should only appear in non-batch mode. See here on how to ask if we are running in batch or not:
https://github.com/JOJ0/synadm/blob/master/synadm/cli/user.py#L116
https://github.com/JOJ0/synadm/blob/master/synadm/cli/user.py#L456

@JOJ0
Copy link
Owner

JOJ0 commented Jul 25, 2021

hi @andir maybe you didn't get notified because of github notification setting, thus direct mention.
HTH

@andir
Copy link
Contributor Author

andir commented Jul 25, 2021 via email

@JOJ0
Copy link
Owner

JOJ0 commented Sep 14, 2021

Hi @andir, can I assist with anything to get this patch merged?

@JOJ0
Copy link
Owner

JOJ0 commented Oct 5, 2021

Hi @andir I hope you don't mind if I merge this and add a warning as suggested? Thanks for the pull.

@andir
Copy link
Contributor Author

andir commented Oct 5, 2021 via email

JOJ0 added a commit that referenced this pull request Nov 7, 2021
- Log a warning when 'user modify --activate' is issued without setting
  a password (but don't quit synadm).
- This fixes allowing silently activating passwordless users,
  instroduced in PR #39.
- In batch mode though, the warning is not logged and the command still
  is silently accepted!
@JOJ0 JOJ0 merged commit 6c9723f into JOJ0:master Nov 7, 2021
@JOJ0
Copy link
Owner

JOJ0 commented Nov 28, 2021

@andir Never got back to this. Wanted to note: I noticed that on my prod Synapse (latest stable) the API itself spits out an error/a note that a user can't be activate when no password is passed. Thus I didn't merge commit 333166d because it's redundant.

On your systems, where SSO is enabled, what is the behaviour there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants