On Fri, 2012-03-02 at 20:01 +0100, Jan Cholasta wrote: > On 2.3.2012 19:43, Rob Crittenden wrote: > > Martin Kosek wrote: > >> On Fri, 2012-03-02 at 11:40 -0500, Rob Crittenden wrote: > >>> Jan Cholasta wrote: > >>>> On 1.3.2012 20:57, Rob Crittenden wrote: > >>>>> Rob Crittenden wrote: > >>>>>> Jan Cholasta wrote: > >>>>>>> On 17.1.2012 04:55, Rob Crittenden wrote: > >>>>>>>> Jan Cholasta wrote: > >>>>>>>>> Dne 13.1.2012 17:39, Rob Crittenden napsal(a): > >>>>>>>>>> Jan Cholasta wrote: > >>>>>>>>>>> Dne 14.12.2011 16:21, Rob Crittenden napsal(a): > >>>>>>>>>>>> Jan Cholasta wrote: > >>>>>>>>>>>>> Dne 14.12.2011 15:23, Rob Crittenden napsal(a): > >>>>>>>>>>>>>> Jan Cholasta wrote: > >>>>>>>>>>>>>>> Dne 14.12.2011 05:20, Rob Crittenden napsal(a): > >>>>>>>>>>>>>>>> The sudo schema now defines sudoOrder, sudoNotBefore and > >>>>>>>>>>>>>>>> sudoNotAfter > >>>>>>>>>>>>>>>> but these weren't available in the sudorule plugin. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> I've added support for these. sudoOrder enforces uniqueness > >>>>>>>>>>>>>>>> because > >>>>>>>>>>>>>>>> duplicates are undefined. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> I also added support for a GeneralizedTime parameter type. > >>>>>>>>>>>>>>>> This is > >>>>>>>>>>>>>>>> similar to the existing AccessTime parameter but it only > >>>>>>>>>>>>>>>> handles a > >>>>>>>>>>>>>>>> single time value. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> You should parse the date/time part of the value with > >>>>>>>>>>>>>>> time.strptime(timestr, '%Y%m%d%H%M%S') instead of doing it > >>>>>>>>>>>>>>> manually, > >>>>>>>>>>>>>>> that way you'll get most of the validation for free. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Yes but it gives a crappy error message, just saying that > >>>>>>>>>>>>>> some > >>>>>>>>>>>>>> data is > >>>>>>>>>>>>>> left over not what is wrong. > >>>>>>>>>>>>> > >>>>>>>>>>>>> IMHO having a separate error message for every field in the > >>>>>>>>>>>>> time > >>>>>>>>>>>>> string > >>>>>>>>>>>>> (like you do in the patch) is an overkill, simple "invalid > >>>>>>>>>>>>> time" > >>>>>>>>>>>>> and/or > >>>>>>>>>>>>> "unknown time format" should suffice (we don't have errors > >>>>>>>>>>>>> like > >>>>>>>>>>>>> "invalid > >>>>>>>>>>>>> 3rd octet" for IP adresses either). > >>>>>>>>>>>> > >>>>>>>>>>>> Well, the work is done, hard to go back on a better error > >>>>>>>>>>>> message. > >>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Also, it would be nice to be able to enter the value in more > >>>>>>>>>>>>>>> user-friendly format (e.g. "2011-12-14 13:01:25 +0100") and > >>>>>>>>>>>>>>> normalize > >>>>>>>>>>>>>>> that to LDAP generalized time. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> When dealing with time there are so many ways to input and > >>>>>>>>>>>>>> display > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>> same values this becomes difficult. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I'd expect that the times for these two attributes will be > >>>>>>>>>>>>>> relatively > >>>>>>>>>>>>>> simple and I somehow doubt users are going to want > >>>>>>>>>>>>>> seconds, leap > >>>>>>>>>>>>>> seconds > >>>>>>>>>>>>>> or fractions, but we'll need to consider how to do it for > >>>>>>>>>>>>>> future > >>>>>>>>>>>>>> consistency (otherwise we could have a case where time is > >>>>>>>>>>>>>> entered in > >>>>>>>>>>>>>> one > >>>>>>>>>>>>>> format for some attributes and another for others). > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> If we input in a nice way we need to output in the same way. > >>>>>>>>>>>>> > >>>>>>>>>>>>> We could make the preferred input/output time format > >>>>>>>>>>>>> user-configurable, > >>>>>>>>>>>>> defaulting to current locale time format. This format would be > >>>>>>>>>>>>> used > >>>>>>>>>>>>> for > >>>>>>>>>>>>> output. For input, we could go over a list of formats > >>>>>>>>>>>>> (first the > >>>>>>>>>>>>> user-configured format, then current locale format, then a > >>>>>>>>>>>>> handful of > >>>>>>>>>>>>> "standard" formats like YYYY-MM-DD HH:MM:SS) and use the first > >>>>>>>>>>>>> format > >>>>>>>>>>>>> that can be successfully used to parse the time string. > >>>>>>>>>>>> > >>>>>>>>>>>> See how far you get into the rabbit hole with even this simple > >>>>>>>>>>>> format? > >>>>>>>>>>> > >>>>>>>>>>> I don't mind, as long as it is the right thing to do (IMHO) :) > >>>>>>>>>>> > >>>>>>>>>>> Anyway, I think this could be done on the client side, so we > >>>>>>>>>>> might > >>>>>>>>>>> use > >>>>>>>>>>> your patch without changes. However, I would prefer if the > >>>>>>>>>>> parameter > >>>>>>>>>>> class was more generic, so we could use it (hypothetically) to > >>>>>>>>>>> store > >>>>>>>>>>> time in some other way than LDAP generalized time attribute (at > >>>>>>>>>>> least > >>>>>>>>>>> name it DateTime please). > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Ok, I'm fine with that. > >>>>>>>>> > >>>>>>>>> Thanks. > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> The LDAP GeneralizedTime needs to be either in GMT or include a > >>>>>>>>>>>> differential. This gets us into the territory where the client > >>>>>>>>>>>> could be > >>>>>>>>>>>> in a different timezone than the server which leads us to > >>>>>>>>>>>> why we > >>>>>>>>>>>> dropped > >>>>>>>>>>>> AccessTime in the first place. > >>>>>>>>>>> > >>>>>>>>>>> Speaking of time zones, the differential alone is not a > >>>>>>>>>>> sufficient > >>>>>>>>>>> time > >>>>>>>>>>> zone description, as it doesn't account for DST. Is there a > >>>>>>>>>>> way to > >>>>>>>>>>> store > >>>>>>>>>>> time in LDAP with full time zone name (just in case it's needed > >>>>>>>>>>> sometime > >>>>>>>>>>> in future)? > >>>>>>>>>> > >>>>>>>>>> There is no way to store DST in LDAP (probably for good reason). > >>>>>>>>>> Oddly > >>>>>>>>>> enough the older LDAP v3 RFC (2252) strongly recommends using > >>>>>>>>>> only > >>>>>>>>>> GMT > >>>>>>>>>> but the RFC that obsoletes it (4517) does not include this. > >>>>>>>>> > >>>>>>>>> Thanks for the info. > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>>> So I'd like the user to supply the > >>>>>>>>>>>> timezone themselves so I don't have to guess (wrongly) and let > >>>>>>>>>>>> them > >>>>>>>>>>>> worry about differing timezones. > >>>>>>>>>>> > >>>>>>>>>>> We don't have to guess, IIRC there is a way to get the local > >>>>>>>>>>> timezone > >>>>>>>>>>> differential in both Python and JavaScript, so the client could > >>>>>>>>>>> supply > >>>>>>>>>>> it automatically if necessary. > >>>>>>>>>> > >>>>>>>>>> I was thinking more about non-IPA clients (like sudo and > >>>>>>>>>> notBefore). > >>>>>>>>> > >>>>>>>>> I think this can still be done at least in CLI, but it could be > >>>>>>>>> done in > >>>>>>>>> a separate patch. > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Updated patches attached. > >>>>>>>>>> > >>>>>>>>>> rob > >>>>>>>>> > >>>>>>>>> Patch 919 doesn't cleanly apply on current master (neither does > >>>>>>>>> 916 > >>>>>>>>> BTW). > >>>>>>>>> > >>>>>>>>> Honza > >>>>>>>>> > >>>>>>>> > >>>>>>>> Rebased patch (and 916 too, separately). > >>>>>>>> > >>>>>>>> rob > >>>>>>> > >>>>>>> Patch 918: > >>>>>>> > >>>>>>> 1. LDAP generalized time allows you to omit minutes from time zone > >>>>>>> differential, your code treats such values as invalid > >>>>>>> > >>>>>>> 2. IMO a better pattern could be used, such as > >>>>>>> u'^([0-9]{2}){5,7}([.,][0-9]+)?([-+]([0-9]{2}){1,2}|Z)$' > >>>>>>> > >>>>>>> 3. 20120229000Z has malformed minutes, but the error message says > >>>>>>> "Malformed seconds" > >>>>>>> > >>>>>>> 4. 20120229000+0000 has malformed minutes, but the error message > >>>>>>> says > >>>>>>> "Missing operator for differential or malformed time string" > >>>>>>> > >>>>>>> 5. 201202290000+0000 is valid generalized time, but it causes > >>>>>>> "Missing > >>>>>>> operator for differential or malformed time string" error > >>>>>>> > >>>>>>> 6. Invalid month/day combinations (such as 201202310000Z) are > >>>>>>> treated as > >>>>>>> valid > >>>>>>> > >>>>>>> 7. When + or - is missing, the error message says "Missing operator > >>>>>>> ..." > >>>>>>> - IMO a better term than "operator" is "sign" in this case > >>>>>>> > >>>>>>> 8. The DateTime docstring includes grammar definition for MINUS, but > >>>>>>> not > >>>>>>> for PLUS, DOT or COMMA. > >>>>>>> > >>>>>>> 9. I'm not too fond of the _rule_required hack. Can't the same > >>>>>>> thing be > >>>>>>> done in _validate_scalar? > >>>>>>> > >>>>>>> 10. pattern_errmsg should say "Must be of the form > >>>>>>> YYYYMMDDHH[MM]Z or > >>>>>>> YYYYMMDDHH[MM]{+|-}HHMM" > >>>>>>> > >>>>>>> There might be more bugs in validation that I didn't discover. I > >>>>>>> would > >>>>>>> suggest you to use a more pythonic approach for the validation code > >>>>>>> (e.g. use partition() to split the time zone and fraction from > >>>>>>> the rest > >>>>>>> of the value, etc.), the current code is rather C-ish, hard to > >>>>>>> read and > >>>>>>> apparently error-prone. > >>>>>>> > >>>>>>> > >>>>>>> Patch 919: > >>>>>>> > >>>>>>> 1. sudoorder uniqueness is not properly checked in ipa sudorule-mod: > >>>>>>> > >>>>>>> $ ipa sudorule-add rule1 > >>>>>>> ----------------------- > >>>>>>> Added Sudo Rule "rule1" > >>>>>>> ----------------------- > >>>>>>> Rule name: rule1 > >>>>>>> Enabled: TRUE > >>>>>>> > >>>>>>> $ ipa sudorule-add rule2 --order=0 > >>>>>>> ----------------------- > >>>>>>> Added Sudo Rule "rule2" > >>>>>>> ----------------------- > >>>>>>> Rule name: rule2 > >>>>>>> Enabled: TRUE > >>>>>>> Sudo order: 0 > >>>>>>> > >>>>>>> $ ipa sudorule-add rule3 --order=0 > >>>>>>> ipa: ERROR: invalid 'order': order must be a unique value (0 already > >>>>>>> used by rule2) > >>>>>>> > >>>>>>> $ ipa sudorule-mod rule1 --order=0 > >>>>>>> -------------------------- > >>>>>>> Modified Sudo Rule "rule1" > >>>>>>> -------------------------- > >>>>>>> Rule name: rule1 > >>>>>>> Enabled: TRUE > >>>>>>> Sudo order: 0 > >>>>>>> > >>>>>>> (now both rule1 and rule2 have sudoorder=0) > >>>>>>> > >>>>>>> 2. Shouldn't we check that sudonotbefore<= sudonotafter? > >>>>>>> > >>>>>>> 3. sudonotbefore param doc should say "Start of time interval for > >>>>>>> which > >>>>>>> the entry is valid (YYYYMMDDHH[MM]Z)" > >>>>>>> > >>>>>>> 4. sudonotafter param doc should say "End of time interval for which > >>>>>>> the > >>>>>>> entry is valid (YYYYMMDDHH[MM]Z)" > >>>>>>> > >>>>>>> 5. In 60ipasudo.ldif, the ipaSudoRule object class is missing the > >>>>>>> sudoOrder, sudoNotBefore and sudoNotAfter attributes. Is this OK? > >>>>>>> > >>>>>>> > >>>>>>> Both patches need to be rebased. > >>>>>>> > >>>>>>> Honza > >>>>>>> > >>>>>> > >>>>>> Ok, lets take this one step at a time. > >>>>>> > >>>>>> This updated patch adds schema for all three attributes but just a > >>>>>> Param > >>>>>> for sudoOrder. I'll have to revisit DateTime for notbefore and > >>>>>> notafter. > >>>>>> > >>>>>> I addressed the issue with order and added a test for it. > >>>>>> > >>>>>> Schema is fixed on new installs. > >>>> > >>>> ipa-server-install fails with: > >>>> > >>>> [02/Mar/2012:09:13:08 +0100] dse_read_one_file - The entry cn=schema in > >>>> file /etc/dirsrv/slapd-IDM-LAB-BOS-REDHAT-COM/schema/60ipasudo.ldif > >>>> (lineno: 1) is invalid, error code 21 (Invalid syntax) - object class > >>>> ipaSudoRule: Unknown allowed attribute type "sudoNotBefore" > >>>> [02/Mar/2012:09:13:08 +0100] dse - Please edit the file to correct the > >>>> reported problems and then restart the server. > >>>> > >>>>>> > >>>>>> rob > >>>>> > >>>>> And now with updated API.txt. Forgot to squash the commit. > >>>>> > >>>>> rob > >>>>> > >>>> > >>>> Honza > >>>> > >>> > >>> And now the right patch > >> > >> The issue with clean install is that the sudo attribute types are > >> shipped in 60sudo.ldif schema, but our schema 60ipasudo.ldif which uses > >> some attributeTypes defined in 60sudo.ldif is alphabetically before > >> 60sudo.ldif and is this processed first. That's why upgrade worked and > >> clean install did not. > >> > >> ACK if you squash with the attached patch which renames 60ipasudo.ldif > >> to 65ipasudo.ldif so that our schema is processed _after_ the bundled > >> 60ipasudo.ldif. > >> > >> Martin > > > > Good idea, rolled into my patch. > > > > I hadn't fixed Honza's mod problem completely either. 0 was essentially > > a special case because of the way I was looking to see if a sudoorder > > change was being requested. I fixed that too and added a specific test > > case for it. > > > > rob > > ACK. > > Honza >
ACK from me as well. Installation is now ok, no redundant attributetypes. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel