[Samba] samba-tool max-pwd-age error

[Tim Beale]

> Sorry Tim, but I do not agree ;-)
>
> If you do not supply the minimum password age, then the 'def' sets
> 'min_pwd_age' to 'None', the code then goes to this:
>
>         if min_pwd_age is not None:
>             if min_pwd_age == "default":
>                 min_pwd_age = 1
>             else:
>                 min_pwd_age = int(min_pwd_age)
>
> It checks if 'min_pwd_age' is supplied (not None) and does something
> if it is, 

That's correct.

> though now I look at that code, it depends on the user
> supplying either 'default' or a number, there is nothing to check that
> what is supplied is valid.
> The user could supply anything, what does 'int(anything)' give you ?
A python exception. Most other samba-tool commands handle this sort of
thing more nicely, because they only deal with integers. The problem
here is these parameters are trying to support both string (i.e.
'default') and integer. The attached diff would be one way to fix this
up - if you felt like making some code changes, you could apply it to
the other parameters. (The extra .lower() bit means the command will
correctly accept 'DEFAULT' or 'Default' as options).

> If 'min_pwd_age' is 'None', the code above will NOT be run.
That's correct.

> I initially supplied what I thought was the fix, but I got that wrong,
> it should be:
>
>         if min_pwd_age is None:
>             min_pwd_age = 1
>         else:
>              # put code here to check for valid input
>              min_pwd_age = int(min_pwd_age)
This approach means that if you change one setting, it will reset all
other settings (that weren't specified) back to the default values. This
makes the tool kind of awkward to use, e.g. if you have already
configured some non-default settings in the past, and then a week or
year later you want to change a different/unrelated setting - unless you
specify all previous modifications again, you'll end up back with the
defaults. It's also likely to confuse any users who are already familiar
with using the command.

> As far as I can see, the problem stems from when 'samba-tool domain
> passwordsettings' was split up, before this happening, if you didn't
> supply something, it was obtained from AD, this meant that
> 'min_pwd_age' was NEVER 'None'
You mean commit 7255e0ced33826d1e5 'netcmd: Split 'domain
passwordsettings' into a super-command', right?

You're correct that prior to this, the 'set' command was retrieving the
current settings from the DB. However, it wasn't actually using these
settings - they only got used for the 'show' command. There were 2
different variables involved: cur_min_pwd_age (retrieved from the DB)
and min_pwd_age (the command-line parameter).

So as far as I can tell, the min_pwd_age has always been None when it's
not specified. The difference in behaviour is that Python 2 will allow
you to compare an integer to None (although the code won't really work
as intended), whereas with Python 3 on v4.10 this now triggers an
exception.


-------------- next part --------------
diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index 8ebaefa..e86a1da 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -1274,6 +1274,17 @@ def timestamp_to_days(timestamp_str):
     return timestamp_to_mins(timestamp_str) / (60 * 24)
 
 
+# helper function to ensure bad integer values get reported as errors nicely
+def convert_to_int(option_name, value):
+    try:
+        int_value = int(value)
+    except ValueError as e:
+        errmsg = "option {0}: invalid integer value: '{1}'".format(option_name,
+                                                                   value)
+        raise CommandError(errmsg)
+    return int_value
+
+
 class cmd_domain_passwordsettings_show(Command):
     """Display current password settings for the domain."""
 
@@ -1448,10 +1459,10 @@ class cmd_domain_passwordsettings_set(Command):
             msgs.append("Minimum password length changed!")
 
         if min_pwd_age is not None:
-            if min_pwd_age == "default":
+            if min_pwd_age.lower() == "default":
                 min_pwd_age = 1
             else:
-                min_pwd_age = int(min_pwd_age)
+                min_pwd_age = convert_to_int('--min-pwd-age', min_pwd_age)
 
             if min_pwd_age < 0 or min_pwd_age > 998:
                 raise CommandError("Minimum password age must be in the range of 0 to 998!")