Refactor datetime #518

c00kiemon5ter · 2018-07-06T18:58:16Z

This changeset introduces a new module, namely saml2.datetime. It implements the concept of datetime and duration object and isolates all operations on those, under a common namespace. The new module ensures that creation, conversion and operations on those objects are kept in one place, are correct and in accordance to the specs.
The way the new module is written is such that it is not tied to a third party. The dependency should be easily swapped by filling in the required parsers; which should be as easy as changing the following two lines: saml2/datetime/__init__.py#L48 and saml2/datetime/duration.py#L5

aniso8601 was chosen to provide the datetime and duration parsers, as:

it is focused on the standard and not generic date-time parsing
it implements the duration part of the standard, that many other libraries do not
it is concise and has a small codebase
it does not use expensive regexes; in fact it uses no regexes (of course by itself this does not mean much)
it has nice error handling; wrapping errors with custom exceptions

Things to improve:

the documentation needs more work
new tests are on their way

Motive for this was the discussion for issue #445 and the relevant thread on the mailing list. It overrides pull-request #517 .

Review is better done per commit. The reordering and cleanup of imports can be ignored.

Moreover, part of this PR is the introduction of two more modules:

compat.py: a module that gathers all compatibility code between python3 and python2
errors.py: a module that gathers all errors for pysaml2. At the moment it provide the top-level generic error Saml2Error class. All errors in the library should be a subclass of Saml2Error.

All Submissions:

Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Have you added an explanation of what problem you are trying to solve with this PR?
Have you added information on what your changes do and why you chose this as your solution?
Have you written new tests for your changes? (on their way)
Does your submission pass tests?
This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

This will be a recurring theme in the commits that follow. The check can be explained as follows: the original check is: if ... time_util.after(timestamp): raise ... which can be thought of as: is_vaild = not time_util.after(datetime) means that this is valid if the current datetime is not after the given datetime. IOW, this is valid if the current datetime is before the given datetime or, swapping the comparison elements, this is valid if the given datetime is after the current datetime. The equivalent new check is: is_valid = saml2.datetime.compare.after_now(date_time) means that this is valid if the given datetime is after the current datetime. Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

if .get_notBefore() returned None (which is allowed to do so) then the code would break. If notBefore is None then the certificate is always valid for that check. Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

codecov-io · 2018-07-06T19:11:46Z

Codecov Report

Merging #518 into master will increase coverage by 0.08%.
The diff coverage is 83.5%.

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
+ Coverage   65.17%   65.26%   +0.08%     
==========================================
  Files         100      109       +9     
  Lines       25691    25741      +50     
==========================================
+ Hits        16745    16799      +54     
+ Misses       8946     8942       -4

Impacted Files	Coverage Δ
src/saml2/mcache.py	`0% <0%> (ø)`	⬆️
src/saml2/authn.py	`0% <0%> (ø)`	⬆️
src/saml2/httputil.py	`0% <0%> (ø)`	⬆️
src/saml2/datetime/timezone.py	`100% <100%> (ø)`
src/saml2/request.py	`70.71% <100%> (ø)`	⬆️
src/saml2/client_base.py	`76.17% <100%> (+0.28%)`	⬆️
src/saml2/errors.py	`100% <100%> (ø)`
src/saml2/entity.py	`83.86% <100%> (+0.07%)`	⬆️
src/saml2/cache.py	`84.53% <100%> (+1.39%)`	⬆️
src/saml2/assertion.py	`87.5% <100%> (+0.35%)`	⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3d2f15...e6fb4a3. Read the comment docs.

johanlundberg

Overall a great addition to pysaml2 that will simplify all time manipulation and validation.

johanlundberg · 2018-07-09T11:40:45Z

src/saml2/datetime/__init__.py

+    - int: a number representing a POSIX timestamp
+    - float: a number representing a POSIX timestamp
+
+    The object returned is of type: datetime.datetime


I think your current comments about arguments and types reads better but would you (also?) consider type hints for IDEs?

Something like:

"""Return a datetime object in UTC timezone from the given data. *snip* :param data: Representation of time :type data: datetime.datetime|str|int|float :return: Datetime object :rtype: datetime.datetime """

I like Python3 type hints, as they are inline and they refer to actual types - in contrast to types written as strings (wether in docstrings or comments).

I was thinking about experimenting with function annotations which should work with Python2 and still allow us to signal actual types rather than strings. It does have the drawback that we must type the parameter names, but I think it is still better.

In the meantime, I can do the conversion to the format that the rest of the code follows.

I would also prefer the Python 3 annotations and I haven't seen the function annotations that you link to. I wasn't thrilled about the syntax of those either but if you find it workable in your experiment I would be for them.

johanlundberg · 2018-07-09T11:51:29Z

src/saml2/datetime/compare.py

+    """Return if dt is within period amount before and after the current datetime.
+
+    Return whether the datetime object `dt` is equal or later than the current
+    UTC datetime object decreased by the given perid object and earlier than


perid -> period

johanlundberg · 2018-07-09T12:02:46Z

src/saml2/datetime/compare.py

+    return dt1 <= dt < dt2
+
+
+def within_now(period, dt):


I would assume that "within now" would have now as the lower boundary for the calculation. This is why I shouldn't assume :). Maybe within_period would be a better name? But that also raises the question if the period is a full period behind now to a full period of after now or half period behind and half after.

I am skeptical about all functions with the _now suffix in their name. within_period sounds much better, but what you mention is also true; is it a half or full period before/after now?. ATM, I cannot think of anything better. I have looked at other APIs (specifically Clojure's clj-time and Java's joda-time) and I haven't seen anything much better. People will have to look at the documentation to make sure.

The other thing I was thinking is if the functions that return a bool should start with is_.

Yeah, I don't have a better suggestion either. So no (more) complaining from me :)

I don't think these names are ambiguous enough to warrant the is_ prefix. They also live in a module called compare. But if you decide the is_ convention is something you want I support that too.

Maybe it becomes clear if you change the name of "dt" to "start"?

A start and a period should be pretty unambiguous?

johanlundberg · 2018-07-09T12:35:01Z

src/saml2/authn.py

@@ -170,7 +172,7 @@ def verify(self, request, **kwargs):
        # verify username and password
        try:
            self._verify(_dict["password"][0], _dict["login"][0])
-            timestamp = str(int(time.mktime(time.gmtime())))
+            timestamp = saml2.datetime.utils.instant()


These kind of changes makes me happy. 👍

johanlundberg · 2018-07-09T12:58:35Z

src/saml2/mdstore.py

-                                self.entities_descr.valid_until,))
-                except AttributeError:
-                    pass
+                if self.entities_descr.valid_until is None:


Could this use the same flow as above?

if self.entities_descr.valid_until: valid_until_date_time = saml2.datetime.parse(self.entities_descr.valid_until) is_valid = saml2.datetime.compare.after_now(valid_until_date_time) else: is_valid = self.entities_descr.valid_until is None

yes, it should ;)

johanlundberg · 2018-07-09T13:05:49Z

src/saml2/validate.py

-            raise ToEarly("Can't use response yet: (now=%s + slack=%d) "
-                          "<= notbefore=%s" % (now_str, slack, not_before))
+    """
+    It is valid if the current time is not earlier (before) than the nbefore


nbefore -> not_before

johanlundberg · 2018-07-09T13:45:34Z

tests/test_13_validate.py

@@ -33,7 +33,7 @@ def test_duration():
    assert valid_duration("-P1347M")
    assert valid_duration("P1Y2MT2.5H")

-    raises(NotValid, 'valid_duration("P-1347M")')
+    # XXX raises(NotValid, 'valid_duration("P-1347M")')


I think it is ok to not support the "negative per element" duration as the XML xs:duration spec says it is invalid.
https://www.w3.org/TR/xmlschema-2/#duration

That is right; however, aniso8601 does support it and thus we do too. This will change soon, as aniso8601 wants to strictly conform to ISO 8601 and negative durations will be dropped. You can read about this in issue#20. Until then, I am planning to change this comment to an expected failure.

Cool, sounds like good decisions all around.

pjsg · 2018-10-19T15:54:51Z

What is the status of this PR? I have another (small) changeset that adds support for explicit timezones in the date string parsing. However, this PR being merged would obviate the need for that work.

My actual change is at https://github.com/IdentityPython/pysaml2/compare/master...CBitLabs:fix/explicit_offset?expand=1

pjsg · 2018-12-19T15:54:24Z

Bump.... I'd really like to see this merged (once the conflicts are fixed).

peppelinux · 2019-02-26T22:19:11Z

This Is a very important code refactor 🖖

…ts](../../pulls) for the same update/change? * [x] Have you added an explanation of what problem you are trying to solve with this PR? It is needed for returning attributes like ```` <saml:Attribute Name="dateOfBirth"> <saml:AttributeValue xsi:type="xs:date">2015-12-06</saml:AttributeValue> </saml:Attribute> ```` Otherwise I get an ValueError Exception like ```` Type and value do not match: date:<class 'str'>:2015-12-06 ```` because `type(value) is not valid_type == True` I didn't write any unit test, just tested in my context. Two important things: 1. This PR is on top of IdentityPython#597 but it doesn't have any dependencies on it. The only file I changed is saml.py, let me know if this PR will be merged in next releases and if you need separate branch for every features. In the future I will this way. 2. I think that the following code https://github.com/IdentityPython/pysaml2/pull/602/files#diff-6c156669cad61eda35e679329251dce9R197 could be also improved according to IdentityPython#518

pjsg · 2019-06-18T20:19:58Z

Please can we get this merged...... (after fixing the conflicts)

ioparaskev · 2019-07-25T15:57:41Z

my 2 cents here since I haven't caught up with the discussion and the PR:
I tried testing this locally and I noticed the following:

When this PR was opened, latest version of package aniso8601 was 3.0.2

Package aniso8601 is now in version 7.0.0 (in version 6.0.0 it is mentioned that version 7.0.0 will drop py2 support but pypi stills shows compatibility). In version 4.0.0 the UTCOffset class (which is used in pysaml2 compat.py (from aniso8601.timezone import UTCOffset as _Timezone) is moved to a new file called builder.py and from 5.0.0 this class is moved to a new file called utcoffset.py.
So I suggest we pin the aniso8601 to at least version 5.0.0 and make the change to compat.py

```` <saml:Attribute Name="dateOfBirth"> <saml:AttributeValue xsi:type="xs:date">2015-12-06</saml:AttributeValue> </saml:Attribute> ```` Otherwise I get an ValueError Exception like ```` Type and value do not match: date:<class 'str'>:2015-12-06 ```` because `type(value) is not valid_type == True` I didn't write any unit test, just tested in my context. I also think that the following code https://github.com/IdentityPython/pysaml2/pull/602/files#diff-6c156669cad61eda35e679329251dce9R197 could be also improved according to IdentityPython#518 if you agree.

c00kiemon5ter added 23 commits July 6, 2018 21:12

Add saml2.datetime module

d3346ed

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update assertion to use the datetime module

b84fc0f

Update authn to use the datetime module

eb35f6b

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update cert to use the datetime module

c22811a

if .get_notBefore() returned None (which is allowed to do so) then the code would break. If notBefore is None then the certificate is always valid for that check. Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update client to use the datetime module

4a69ad8

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update client_base to use the datetime module

44e9912

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update entity to use the datetime module

45c5141

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update httputil and httpbase to use the datetime module

8477c4c

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update mcache to use the datetime module

9dd3200

Update mdbcache to use the datetime module

c903afc

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update mdstore to use the datetime module

b3f2f6d

Update metadata to use the datetime module

e113510

Update population to use the datetime module

6159d9a

Update validate to use the datetime module

9e5f374

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Make issue_instant_ok part of validate module

79321d0

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update request to use the datetime module

7f9eea6

Update response to use the datetime module

e46ad9f

Update s_utils to use the datetime module

922d21f

Update sigver to use the datetime module

4a0cee1

Update tests

ce66189

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Remove time_util module

9b9240e

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

Update documentation

e6fb4a3

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

c00kiemon5ter requested a review from johanlundberg July 7, 2018 22:00

johanlundberg approved these changes Jul 9, 2018

View reviewed changes

c00kiemon5ter mentioned this pull request Jul 18, 2018

Fix typo in error message #521

Closed

c00kiemon5ter self-assigned this Aug 3, 2018

c00kiemon5ter mentioned this pull request Apr 2, 2019

add session_not_on_or_after IdentityPython/SATOSA#206

Open

6 tasks

peppelinux mentioned this pull request Apr 8, 2019

Add xsd date type #602

Closed

spaceone mentioned this pull request Nov 11, 2020

Performance measurement #747

Open

c00kiemon5ter mentioned this pull request Jan 6, 2021

test_filter_ava_registration_authority_1 fails on i586 #759

Closed

Refactor datetime #518

Are you sure you want to change the base?

Refactor datetime #518

Uh oh!

Conversation

c00kiemon5ter commented Jul 6, 2018

All Submissions:

Uh oh!

codecov-io commented Jul 6, 2018

Codecov Report

Uh oh!

johanlundberg left a comment

Choose a reason for hiding this comment

Uh oh!

johanlundberg Jul 9, 2018 • edited by c00kiemon5ter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjsg commented Oct 19, 2018

Uh oh!

pjsg commented Dec 19, 2018

Uh oh!

peppelinux commented Feb 26, 2019

Uh oh!

pjsg commented Jun 18, 2019

Uh oh!

ioparaskev commented Jul 25, 2019

Uh oh!

Uh oh!

johanlundberg Jul 9, 2018 •

edited by c00kiemon5ter

Loading