Prefer forcing timezones #730

lcpriest · 2015-10-22T15:22:46Z

I believe this repositories hound settings encourage not using Time.now without specifying something like timezone or UTC. This should fix those hound errors.

tute · 2015-10-22T22:02:58Z

@elle should doorkeeper force UTC times? It's used in Rails and Grape frameworks. Thank you!

elle · 2015-10-22T22:06:03Z

@tute We should never use Time.now but rather Time.current
Doing so solves most issues with querying the database.

tute · 2015-10-22T22:09:15Z

Thank you Elle! And how would doorkeeper-for-non-rails frameworks define current?

elle · 2015-10-22T22:17:31Z

Good point. My apologies. So, yes, we would like to use .utc.

Would be good to freeze time in the specs and check these methods in various time zones.

lcpriest · 2015-10-23T00:09:41Z

@elle is there a standard way to freeze time without timecop?

elle · 2015-10-23T01:36:43Z

@lcpriest looking at the gemspec file, timecop is already a development dependency [https://github.com/doorkeeper-gem/doorkeeper/blob/master/doorkeeper.gemspec#L25]

Alternatively, we can use http://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html

Looking at http://api.rubyonrails.org/classes/ActiveRecord/Timestamp.html, timestamps are saved in UTC by default. So it's a good thing that the methods compare both times in utc. Possible problem would be if someone ever set: config.active_record.default_timezone = :local

lcpriest · 2015-10-23T01:38:51Z

Given that timecop isn't used anywhere, could we drop the dependency and use the TimeHelpers instead?

elle · 2015-10-23T01:40:32Z

Would be good to give it a go

lcpriest · 2015-10-23T01:42:33Z

I'm at work now, so will be unable to have a go at that - would you be okay to have a look?

tute · 2015-10-23T02:54:36Z

Thank you Lachlan and Elle for your work on this. I'm totally up for dropping Timecop; if it's not used anywhere It shouldn't break tests if you remove it as part of this changeset.

tute · 2015-10-23T03:03:44Z

@lcpriest: is this potentially backwards incompatible? In what cases?

lcpriest · 2015-10-23T03:07:19Z

Any incompatibilities would be if your server was in non UTC time, this could expire a token too soon (or give it an extra day or so)

tute · 2015-10-23T03:40:52Z

And so I understand the implications of this change: what are we missing while we don't have doorkeeper do UTC? What issue are we solving?

Thanks, again!

lcpriest · 2015-10-23T03:48:34Z

It's really just a best practice:
http://rails-bestpractices.com/posts/2014/10/22/use-time-zone-now-instead-of-time-now/
http://danilenko.org/2012/7/6/rails_timezones/

tute · 2015-10-23T17:55:50Z

Whoops, sorry. :) Will check those links out to learn the why of this best practice. Elle also wrote about this:

Won't be able to do this homework today, hopefully next Friday! Thanks.

tute · 2015-10-29T04:11:50Z

@lcpriest, the two links you show are Rails specific. Questions still stand:

What bugs can arise because doorkeeper is not calling .utc?
Would this changeset introduce potentially backwards incompatible changes? What would be the upgrade process?

If it was a new library I would definitly follow your suggestion and code, but there are thousands of existing installations, not only in Rails.

elle · 2015-10-29T15:46:41Z

@tute this might be an issue with comparing two times that are not equal.

Time.now reads the local time on the machine, which can be anywhere, but let's assume for the example that it is in NY, so UTC -0400. Then it is compared to revoked_at which I am not sure how it is saved (i.e. in what time zone), and to created_at that is by default saved as utc (unless someone changed the defaults in their app).

Comparing two times in two different time zones is like comparing apples and oranges. To be useful, both times should be in the same time zone. And the least painful way to solve this is to have both compared times in utc.

I think we should use TimeCop and stub the times and time zones in the specs to make sure the time comparison works as intended. I would also probably call .utc on created_at and revoked_at to be safe.

tute · 2015-11-01T02:38:53Z

Thank you, Elle! Copying bug report #738 that will be fixed when we merge this PR.

I found this issue when following a tutorial, as mentioned in my other issue.

In file lib/doorkeeper/models/concerns/expirable.rb, method expired? executes:

expires_in && Time.now > expired_time

In my dev setup, I had to change the line to:

expires_in && Time.now.utc > expired_time

so it would correctly validate.

This was preventing me from getting to the URI validation and any other validation happening after this.

This was an issue because grant dates in the DB were in UTC and my Mac is running in timezone -06.00 (Mexico's CST). So when comparing dates, it wasn't comparing them correctly. I thought they would since they are supposed to be Date/Time objects and comparison would be done internally, accounting for time zones, but it seems it is not like that.

This answers my first question (what bugs are right now present), but I still didn't answer my second quesition:

Would this changeset introduce potentially backwards incompatible changes? If so, what would be the upgrade process?

lcpriest · 2015-11-02T00:57:13Z

Any code outside of the gem written to compare times would not have taken this change into account, but I think if this is adequately changelogged, that isn't necessarily an issue for the gem.

The other is that we need to convert both sides of the equivalences to use utc, in cases of servers with non-utc timezones set.

lcpriest · 2015-11-02T01:01:03Z

lib/doorkeeper/models/concerns/expirable.rb

      end

      def expires_in_seconds
        return nil if expires_in.nil?
-        expires = (created_at + expires_in.seconds) - Time.now
+        expires = (created_at + expires_in.seconds) - Time.now.utc


How does the .seconds method work outside of rails? It's added to the Numeric module:
http://api.rubyonrails.org/classes/Numeric.html

How does the .seconds method work outside of rails? It's added to the Numeric module:

Pretty sure it does not, I'm surprised Grape users didn't submit related issues.

tute · 2015-11-02T02:05:41Z

The other is that we need to convert both sides of the equivalences to use utc, in cases of servers with non-utc timezones set.

Right, and potentially convert doorkeeper's legacy data in different time zones.

lcpriest · 2015-11-11T10:44:52Z

I've not got time right now to work on this, should I close this or is there someone else who can work on it?

tute · 2015-11-12T04:53:08Z

If no one has found issues yet, I'm inclined to close it, as it might introduce new bugs for existing apps.

That said, I will have time starting in December to investigate these issues, and I'm ok having this open until then, as it is a potentially useful fix.

Your call. Thanks!

tute · 2015-12-23T19:08:20Z

master is now targetting the next major release, with Rails 5 support, and dropping support for rails < 5. I think we should merge this in with the upcoming release. Is there any work left in this branch, or is it ready to be rebased and merged? Thank you!

lcpriest · 2015-12-24T10:04:38Z

There are no timezone specific tests on this branch.

johnnyshields · 2016-03-12T20:48:04Z

@tute your issue here sounds like a problem in your database serialization layer/ORM. You should be persisting dates in UTC in the DB, and setting TZ when you load them into Ruby. If you are storing dates in local TZ, then you should to ensure the TZ is set correctly when they are converted from DB to Ruby objects. (In other words, if you are serializing/deserializing from DB with correct TZ handling, then .utc is needed.)

tute · 2016-04-19T03:42:58Z

@lcpriest would you update the NEWS file, and rebase this branch on top of latest master? I'd like to get this merged in.

lcpriest · 2016-04-19T03:46:48Z

@tute Done :)

tute · 2016-04-19T03:51:08Z

Nice! Can you add more context from this PR's discussion into your commit message, and explicitly state this as a potentially backwards incompatible change in the NEWS file? Then we merge. Thanks!

lcpriest · 2016-04-19T03:56:12Z

rc3?

tute · 2016-04-19T03:57:30Z

rc3?

Yep!

This was initially a change for the sake of the HoundCI, but it is also just A Good Idea™. Forcing UTC should not cause any issues as the way two times are compared takes the timezone into account anyway. Here are a few articles regarding the best practice of Time in Ruby. https://robots.thoughtbot.com/its-about-time-zones https://robots.thoughtbot.com/a-case-study-in-multiple-time-zones http://rails-bestpractices.com/posts/2014/10/22/use-time-zone-now-instead-of-time-now/ http://danilenko.org/2012/7/6/rails_timezones/

houndci-bot · 2016-04-19T04:02:44Z

lib/doorkeeper/version.rb

@@ -1,3 +1,3 @@
 module Doorkeeper
-  VERSION = "4.0.0.rc2"
+  VERSION = "4.0.0.rc3"


Freeze mutable objects assigned to constants.

If this shouldn't be another commit, I think github now allows PRs to be squashed on merge.
https://github.com/blog/2141-squash-your-commits

tute · 2016-04-19T04:08:13Z

Thanks so much! 🎉

lcpriest · 2016-04-19T04:16:19Z

No, Thank you! Doorkeeper is such a great project and everyone I have spoken to so far has been really nice.

tute closed this Oct 23, 2015

tute reopened this Oct 23, 2015

tute mentioned this pull request Nov 1, 2015

Issue when checking expiration date of grants #738

Closed

lcpriest force-pushed the chore/its-time branch from 6cf549c to da8cb1f Compare November 2, 2015 00:53

lcpriest reviewed Nov 2, 2015
View reviewed changes

lcpriest mentioned this pull request Nov 11, 2015

Revoke Refresh Token on access token use #691

Closed

tute mentioned this pull request Nov 22, 2015

Add created_at to token generation options #745

Merged

tute force-pushed the master branch from 8e193e8 to 588445c Compare March 3, 2016 01:44

lcpriest force-pushed the chore/its-time branch from da8cb1f to 02755b1 Compare April 19, 2016 03:46

lcpriest force-pushed the chore/its-time branch from 02755b1 to f981001 Compare April 19, 2016 04:02

Version bump

a510e50

houndci-bot reviewed Apr 19, 2016
View reviewed changes

tute merged commit 4d7c82b into doorkeeper-gem:master Apr 19, 2016

lcpriest deleted the chore/its-time branch April 19, 2016 04:16

Prefer forcing timezones #730

Prefer forcing timezones #730

Conversation

lcpriest commented Oct 22, 2015

tute commented Oct 22, 2015

elle commented Oct 22, 2015

tute commented Oct 22, 2015

elle commented Oct 22, 2015

lcpriest commented Oct 23, 2015

elle commented Oct 23, 2015

lcpriest commented Oct 23, 2015

elle commented Oct 23, 2015

lcpriest commented Oct 23, 2015

tute commented Oct 23, 2015

tute commented Oct 23, 2015

lcpriest commented Oct 23, 2015

tute commented Oct 23, 2015

lcpriest commented Oct 23, 2015

tute commented Oct 23, 2015

tute commented Oct 29, 2015

elle commented Oct 29, 2015

tute commented Nov 1, 2015

lcpriest commented Nov 2, 2015

lcpriest Nov 2, 2015

Choose a reason for hiding this comment

tute Nov 2, 2015

Choose a reason for hiding this comment

tute commented Nov 2, 2015

lcpriest commented Nov 11, 2015

tute commented Nov 12, 2015

tute commented Dec 23, 2015

lcpriest commented Dec 24, 2015

johnnyshields commented Mar 12, 2016

tute commented Apr 19, 2016

lcpriest commented Apr 19, 2016

tute commented Apr 19, 2016

lcpriest commented Apr 19, 2016

tute commented Apr 19, 2016

houndci-bot Apr 19, 2016

Choose a reason for hiding this comment

lcpriest Apr 19, 2016

Choose a reason for hiding this comment

tute commented Apr 19, 2016

lcpriest commented Apr 19, 2016