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

Authrequest date fix #109

Closed
wants to merge 2 commits into from
Closed

Authrequest date fix #109

wants to merge 2 commits into from

Conversation

wesselej
Copy link

Ran into some problems with Microsoft ADFS server drifting out of sync with time server. Led me to find this issue with the time format. This change fixes the broken times generated as "2014-02-23T15:30:00Z" so that it says instead "2014-02-23T15:30:00GMT" (or whatever actual time zone the server is configured to).

I would have written a test to demonstrate this, but the code was buried in a way that did not lend it self to a direct test, and I didn't know if I should refactor the code just to allow for a test.

@Lordnibbler
Copy link
Contributor

@wesselej can you please add specs if the refactor is straightforward?

@wesselej
Copy link
Author

The refactor would have to set the time stamp (as a string) to a property of the Authrequest object, which could then be validated via a regular expression in the test. The variable would then be used to build the value of the XML request document instead of the method variable it's in now.

Alternatively, the test could just generate an XML request, parse the XML out with REXML or Nokogiri, find the value off the IssueInstant attribute and then validate it using a regex from there. Only after I sent the pull request did I think if this, but it still seems a like a lot of heavy lifting to do in the test, and I still think the refactor would be the better route to go.

@Lordnibbler
Copy link
Contributor

Seems like a duplicate of #101

@wesselej Can you please write a test for this? 101 has gone stale and unanswered.

@luisvm @pitbulk @inakidelamadrid can you guys review?

@Lordnibbler
Copy link
Contributor

handled via #138

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.

None yet

2 participants