Fix handling of config tri-state bool values (like acl_public)#940
Fix handling of config tri-state bool values (like acl_public)#940fviard merged 1 commit intos3tools:masterfrom
Conversation
|
This should also solve #793 but in a more generic way. |
|
Thank you for having catch that. But I still see a little drawback, if you can add a fix for that in your PR, that would be awesome:
Possible fix: |
Commit bc38c2a added the ability to set acl_public from the config file -- but it did not take into account that it is a 'tri-state' value so the string is what is written, causing it to ALWAYS be true. This results in unexpected and insecure behavior if 'acl_public = False' is used in the config file. This patch modifies the Config.update_option to check the option type AND the value if the original is None. If an option's default is None and the value is a bool (true, false, yes, no, 0, 1) then it will set it to a bool value.
ea12ce0 to
618d357
Compare
|
I've updated the PR with a small change, dropping use of str() and renaming the functions. From what I can see they will always be called with unicode so it can just use .lower() on that. And if it ever gets called with a str, .lower() still works. I don't think the corner case of using a bool string for one of the other entries is going to be a problem, but if you want to deal with that separately I'll leave that up to you. |
|
Finally merged, thank you. Sorry for the long delay. |
…lt value is now "" and not anymore None
Commit bc38c2a added the ability to set
acl_public from the config file -- but it did not take into account that
it is a 'tri-state' value so the string is what is written, causing it
to ALWAYS be true.
This results in unexpected and insecure behavior if 'acl_public = False'
is used in the config file.
This patch modifies the Config.update_option to check the option type
AND the value if the original is None. If an option's default is None
and the value is a bool (true, false, yes, no, 0, 1) then it will set it
to a bool value.