[Freeipa-devel] [PATCHES 0002-0008] [RFE] Implement iCal based time managment in HBAC

[Martin Basti]

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()
http://stackoverflow.com/questions/16870663/how-do-i-validate-a-date-string-format-in-python

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? 
https://docs.python.org/2/library/datetime.html
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) 
https://fedorahosted.org/freeipa/ticket/547

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


On 07.10.2015 09:55, Stanislav Laznicka wrote:
> 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 
> https://docs.oracle.com/javase/8/docs/api/java/time/temporal/WeekFields.html.
>
> 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 
> (http://www.freeipa.org/page/V4/Time-Based_Account_Policies).
>
> Last thing I would like to mention - there is now a copr repo with 
> both sssd and freeipa with time-based policies 
> (https://copr.fedoraproject.org/coprs/stlaz/freeipa-sssd-timerules/). 
> 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
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151014/80651298/attachment.htm>