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
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
