Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

:allowed_clock_drift should be bidrectional #605

Merged
merged 7 commits into from
Aug 23, 2021

Conversation

johnnyshields
Copy link
Collaborator

@johnnyshields johnnyshields commented Aug 8, 2021

Fixes #599

:allowed_clock_drift should be bidrectional (allow X sec before "NotBefore" and X sec after "NotOnOrAfter"). Also improves readability of associated error messages.

…efore" and X sec after "NotOnOrAfter"). Also improves readability of associated error messages.
@pitbulk
Copy link
Collaborator

pitbulk commented Aug 13, 2021

Can you add some unittest to cover the cases?. It seems this change has not impacted the test suite and it should, so we may be missing some unitest here

@johnnyshields
Copy link
Collaborator Author

@pitbulk specs done, ready for review

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 18, 2021

@johnnyshields the specs fail

@johnnyshields
Copy link
Collaborator Author

@pitbulk this can be merged. the one failing run is not related to this PR

@johnnyshields
Copy link
Collaborator Author

@pitbulk let's merge?

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 23, 2021

  • Can you clarify why the Float::EPSILON is required?

@johnnyshields
Copy link
Collaborator Author

Yes, floats have built in inaccuracy (the uncertainty factor is "epsilon") which can cause <= to return false when it should be true.

My code extends the clock drift factor by epsilon which errs on the side of being permissive when comparing exact times.

def allowed_clock_drift
return options[:allowed_clock_drift].to_f
options[:allowed_clock_drift].to_f.abs + Float::EPSILON
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the Float::EPSILON is required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, floats have built in inaccuracy (the uncertainty factor is "epsilon") which can cause <= to return false when it should be true.

My code extends the clock drift factor by epsilon which errs on the side of being permissive when comparing exact times.

@pitbulk pitbulk merged commit dfab94b into SAML-Toolkits:master Aug 23, 2021
@johnnyshields johnnyshields deleted the fix-clock-drift branch August 23, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clock drift: sign of allowed_clock_drift and logging resolution
2 participants