<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> I just read the code did not testing, maybe I will read design tomorrow. I found a few minor issues 0) You use too much brackets in code, it looks more like C than python: if not(elem): --> if not elem if(len(elempair) > 2) --> if len(elempair) > 2: return(something) --> return something 1) +def _timearg_to_list(time): + ''' + Parses the argument of either of the access time language keywords + + Example: '1-5,12' into [[1,5], 12] + ''' + return (t.split('-') for t in time.split(',')) This will return [['1', '5'], ['12']] please fix example 2) in function _validate_range(keyword, lst, r1, r2, unit, unitlen): I do not like that enumeration and this condition + if not(i): + elold = elnum Can you use instead enumeration just None? elold = None if elold is None: elold = elnum 3) _validate_timeofday(t) I do not like that enumerate (see 2) ) 4) _validate_timeofday(t) There is missing check for values like '1-2-5', '1-2,', (this check is in _validate_range) 5) I do not think that unitlen arg in _validate_range is needed, IMO this is duplicated check, comparison r1, r2 is enough 6) +def validate_accesstime(ugettext, value): This function is not pythonic enough, it looks more like C I suggest something like: params = { name: {'found': False, 'method': method } for name, method in [ ('dayofweek', _validate_dayofweek), ('timeofday', _validate_timeof...), ..... ]} for i, el enumerate(ts): param_name, param_value = el.split('=', 1) try: res = params[param_name]['method'](param_value) if not params[param_name]['found'] else _("multiple blablabla of {param}".format(param=param_name)) params[param_name]['found'] = True except KeyError: res = _(Unknown.....) if res: return res 7) + res = _validate_timeofday(el[10:]) if not(found[TOD]) else \ + _("multiple appearance of timeofday") Please don't use '\', use braces instead 8) - 'externalhost', + 'memberhostgroup', 'externalhost', 'ipatimezone', + 'accesstime', 'accesstimeexclude' Are you sure that 'memberhostgroup' should be added there? I don't think so. 9) def normalize_accesstime(t): IIRC normalize is called before validate, so your precious regular expression may not work as expected, namely: + t = re.sub(r'(\d)([a-z]{2})', r'\1 \2', t) + t = re.sub(r'(\d)\s(d|w|m|y)\s', r'\1\2', t) However when normalization returns unexpected results, validation will be able catch it. IMO we should not do any magic detection, we should force user to have keyword and value pairs separated by space How about depending on '=' character in normalization? parts = re.split('[\s=]', t) eq_signs_pos = [i for i, part in enumeration(parts) if parts=="="] <here join it based on = sign, something like parts: [year, '=', 'i', 'do', 'not', 'care', 'next', '=', 'lorem', 'ipsum'] eq_signs_pos: [1, 7] keyword_pos: [0,6] Test if keyword is on position 0: else raise conversionerror normalized = " ".join(["".join([parts[0], .. parts[5]]), "".join([parts[6]..parts[9]])]) result "year=idonotcare next=loremipsum" > 10) _validate_date() <a class="moz-txt-link-freetext" href="http://stackoverflow.com/questions/16870663/how-do-i-validate-a-date-string-format-in-python">http://stackoverflow.com/questions/16870663/how-do-i-validate-a-date-string-format-in-python</a> I know the solution above is not so detailed in what is wrong, but IMO to print that specified date is not valid should be enough for user. 11) in _validate_date function I see validation for case when dayofmonth is higher than, last day of month. I do not see this check for other cases than 'repeat'. Is it safe to ignore (for example: "dayofmonth=30 monthofyear=2" this HBACrule will never be executed)? 12) + if not('1' <= t[ctlpos] <= '9'): + return _("'repeat': wrong syntax, expected a number after '+'") IMO 0 is digit/number too, and we should allow initial 0 and just ignore them during execution 13) +def _validate_repeat(t): + date1 = t[0:8] what if 't' is shorter than 8 characters? 14) + date2 = t[9:17] if t[8] == '-' else None What if t < 17 characters and t[8] == '-'? 15) + if t[ctlpos] not in ('d', 'w', 'm', 'y'): + return _("'repeat': wrong syntax, d/w/m/y expected") What if there is any continuation after? "repeat=20151003+2m-trolololo" (you can add this to tests :) ) 16) IMO _validate_repeat check looks simple enough to be checked by regexp, and then separate parts tested extra (start, end date, repeat period) 17) +def _validate_ymd_range(y1, m1, d1, y2, m2, d2): Can you instead of this black magic use python datetime object? <a class="moz-txt-link-freetext" href="https://docs.python.org/2/library/datetime.html">https://docs.python.org/2/library/datetime.html</a> And then just compare 2 objects with '>' and return error that second date is greater than first. Also datetime object creation will check if date itself. 18) + rule_time = u'timeofday=0000-2359 dayofmonth=1-15,16-31 ' \ + 'dayofyear=23-47,26-77 year=1970-2563' Please use () instead of \ 19) + # no spaces around - sign <-- ~ should be there + t = re.sub(r'\s?~\s?', r'~', t) wouldn't be better to work with ~ since first patch? I mean replace '-' -> '~' in first patch Is common to specify date ranges with '~' ? Personally I wouldn't expected that. 20) Please add design page to the ticket (if exists) <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/547">https://fedorahosted.org/freeipa/ticket/547</a> 21) PEP8 ./ipalib/plugins/hbacrule.py:190:21: E265 block comment should start with '# ' ./ipatests/test_xmlrpc/test_hbac_plugin.py:38:18: E127 continuation line over-indented for visual indent ./ipatests/test_xmlrpc/test_hbac_plugin.py:38:18: E127 continuation line over-indented for visual indent ./ipatests/test_xmlrpc/test_hbac_plugin.py:101:80: E501 line too long (84 > 79 characters) ./ipalib/plugins/hbactest.py:281:9: E124 closing bracket does not match visual indentation ./ipalib/plugins/hbacrule.py:78:80: E501 line too long (80 > 79 characters) ./ipalib/plugins/hbacrule.py:190:21: E265 block comment should start with '# ' ./ipalib/plugins/hbacrule.py:219:80: E501 line too long (84 > 79 characters) ./ipalib/plugins/hbacrule.py:592:9: E124 closing bracket does not match visual indentation ./ipalib/plugins/hbacrule.py:596:9: E124 closing bracket does not match visual indentation 22) Unwanted lines removal at the end of files/patches * Added methods for setting time-based policies: hbacrule.py * HBAC test module support for time-based policies: hbactest.py * Tests for HBAC time rules language: test_hbactest_plugin.py best regards Martin^2 <div class="moz-cite-prefix">On 07.10.2015 09:55, Stanislav Laznicka wrote: </div> <blockquote cite="mid:5614CFE1.7030705@redhat.com" type="cite">Hi, The moment's here, I'd like to share my code with you now. Let me comment on some additions from my last post here in August. The methods for testing HBAC rules in hbactest module were modified so that a time zone can now also be picked in case there are some rules with the "host" time zone in the rule time policy. I also added few tests that test setting accessTime values. The most important update of the previous month is the addition of negative values to the time rules language. Most of the keywords (all, except for timeofday and year) now accept negative values and negative value ranges. This should be useful for cases when the user should only be allowed access e.g. in the last 7 days of a month, last few weeks of a year etc. Also, it is a similar behavior to what iCalendar has. The addition of negative values also made me re-think the ways the week of a year should be calculated. There are no 0th weeks of year anymore, a week of year can hold values ranging from 1 to 53 where the 1st week of a year may appear even on a date of the previous year (if 1st January is Tue-Thu) or the 52nd or 53rd week may appear on a date of the following year (when 31st December is Thu-Sat). If my explanation seems rather rough, please see <a class="moz-txt-link-freetext" href="https://docs.oracle.com/javase/8/docs/api/java/time/temporal/WeekFields.html">https://docs.oracle.com/javase/8/docs/api/java/time/temporal/WeekFields.html</a>. The latter caused some changes to be made in my SSSD code. These changes took the most of my time last month alongside with generally polishing the code and adding comments where I thought necessary. I will push my SSSD code to the sssd-devel mailing list as a follow-up to this mail. Another thing - I updated the design page on the FreeIPA wiki, so please check it out, too (<a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/V4/Time-Based_Account_Policies">http://www.freeipa.org/page/V4/Time-Based_Account_Policies</a>). Last thing I would like to mention - there is now a copr repo with both sssd and freeipa with time-based policies (<a class="moz-txt-link-freetext" href="https://copr.fedoraproject.org/coprs/stlaz/freeipa-sssd-timerules/">https://copr.fedoraproject.org/coprs/stlaz/freeipa-sssd-timerules/</a>). This was Martin K.'s idea and I think it's pretty dandy :) As the patches I am posting only contain CLI for HBAC time policies, you might be pleased that the repo includes at least basic WebUI for this purpose (although the WebUI is for some reason not updating the page on rule addition properly, I will be hopefully looking into that shortly). You will still need mkosek/freeipa-master copr repo for some dependencies. Should it not work properly for you, please, send me an email, it's my first time taking care of a copr repo. That's it from me for now, thank you for your patience with my emails, Standa <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> </body> </html>