-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(2fa): Handle twofactor_enforced configuration parameter as boolean #44090
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jarkko Lehtoranta <jarkko@bytecap.fi>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me, but didn't take the time to see if that behaves well on updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution.
However, this will break existing installations that are using a string, e.g. "false"
will evaluate to true when cast to a boolean. I get where you are coming from but let's not refactor what is not broken. I don't know if it is a good idea to migrate config.php
files.
Code example: https://3v4l.org/HMTpE
I always thought |
The current implementation is a bit naive: https://github.com/nextcloud/server/blob/master/lib%2Fprivate%2FAllConfig.php#L111 |
Summary
The
twofactor_enforced
configuration parameter should be handled as a boolean value like all the other boolean configuration parameters. This reduces configuration errors at least for those, who might edit config.php directly.TODO
getSystemValueBool
return the correct value regardless if it's e.g.'true'
ortrue
?Checklist