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

Add has_secure_token to Active Record #18217

Merged
merged 1 commit into from Jan 4, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
* Added ActiveRecord::SecureToken in order to encapsulate generation of
unique tokens for attributes in a model using SecureRandom

*Roberto Miranda*

* Change the behavior of boolean columns to be closer to Ruby's semantics.

Before this change we had a small set of "truthy", and all others are "falsy".
Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record.rb
Expand Up @@ -67,6 +67,7 @@ module ActiveRecord
autoload :Transactions
autoload :Translation
autoload :Validations
autoload :SecureToken

eager_autoload do
autoload :ActiveRecordError, 'active_record/errors'
Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record/base.rb
Expand Up @@ -312,6 +312,7 @@ class Base
include Reflection
include Serialization
include Store
include SecureToken
end

ActiveSupport.run_load_hooks(:active_record, Base)
Expand Down
49 changes: 49 additions & 0 deletions activerecord/lib/active_record/secure_token.rb
@@ -0,0 +1,49 @@
module ActiveRecord
module SecureToken
extend ActiveSupport::Concern

module ClassMethods
# Example using has_secure_token
#
# # Schema: User(toke:string, auth_token:string)

Choose a reason for hiding this comment

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

missed 'n'

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed via #18537.

# class User < ActiveRecord::Base
# has_secure_token
# has_secure_token :auth_token
# end
#
# user = User.new
# user.save
# user.token # => "44539a6a59835a4ee9d7b112"
# user.auth_token # => "e2426a93718d1817a43abbaa"
# user.regenerate_token # => true
# user.regenerate_auth_token # => true
#
# SecureRandom is used to generate the 24-character unique token, so collisions are highly unlikely.
# We'll check to see if the generated token has been used already using #exists?, and retry up to 10
# times to find another unused token. After that a RuntimeError is raised if the problem persists.
#
# Note that it's still possible to generate a race condition in the database in the same way that
# validates_presence_of can. You're encouraged to add a unique index in the database to deal with
# this even more unlikely scenario.
def has_secure_token(attribute = :token)
# Load securerandom only when has_secure_key is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/key/token/?

require 'securerandom'
define_method("regenerate_#{attribute}") { update! attribute => self.class.generate_unique_secure_token(attribute) }
before_create { self.send("#{attribute}=", self.class.generate_unique_secure_token(attribute)) }
end

def generate_unique_secure_token(attribute)
10.times do |i|
SecureRandom.hex(12).tap do |token|
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use hex? How about uuid? It would work nicely with PG.

Copy link
Member

Choose a reason for hiding this comment

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

uuid is 36 characters and uses dashes. We were looking for a format that was alphanumeric only and shorter.

Copy link
Member

Choose a reason for hiding this comment

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

I thought we'd settled on base62, in order to achieve a sufficient keyspace while keeping the string optimally short.

Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember something about that. Can you recap the conversation? What was the benefit of base62 over hex(12)?

Copy link
Member

Choose a reason for hiding this comment

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

For a given length of 24:

16 ** 24  # => 79228162514264337593543950336
62 ** 24  # => 10408797222153426578715765348940396820955136

and apart from needing slightly more code to generate it, it's basically free.

The discussion went a bit into the woods because base62-the-algorithm is complicated, but we don't care: we just want to generate a series of characters using base62-the-charset.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. So even less chance of a collision. Gotcha. Sounds good to me 👍. Could happen in a separate PR though.

On Dec 29, 2014, at 15:13, Matthew Draper notifications@github.com wrote:

In activerecord/lib/active_record/secure_token.rb:

  •  #   end
    
  •  #
    
  •  #   user = User.new
    
  •  #   user.save
    
  •  #   user.token # => "44539a6a59835a4ee9d7b112"
    
  •  #   user.regenerate_token # => true
    
  •  def has_secure_token(attribute = :token)
    
  •    # Load securerandom only when has_secure_key is used.
    
  •    require 'securerandom'
    
  •    define_method("regenerate_#{attribute}") { update! attribute => self.class.generate_unique_secure_token(attribute) }
    
  •    before_create { self.send("#{attribute}=", self.class.generate_unique_secure_token(attribute)) }
    
  •  end
    
  •  def generate_unique_secure_token(attribute)
    
  •    10.times do |i|
    
  •      SecureRandom.hex(12).tap do |token|
    

For a given length of 24:

16 ** 24 # => 79228162514264337593543950336
62 ** 24 # => 10408797222153426578715765348940396820955136

and apart from needing slightly more code to generate it, it's basically free.

The discussion went a bit into the woods because base62-the-algorithm is complicated, but we don't care: we just want to generate a series of characters using base62-the-charset.


Reply to this email directly or view it on GitHub.

if exists?(attribute => token)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we bothering with exists?? There is still a race condition. We can't guarantee the uniqueness until it actually inserts in to the database.

Copy link
Member

Choose a reason for hiding this comment

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

Decrease the number of times that uniqueness exception will be thrown. Similar to the validates_presence_of case. There's still a race condition, but we work to minimize the likelihood that an exception needs to bubble up to the user.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just wasteful most of the time though?

Copy link
Member

Choose a reason for hiding this comment

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

Also, shouldn't we go to 11? Wouldn't that decrease the chances even further?

Copy link
Member

Choose a reason for hiding this comment

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

When there's not a collision (most of the time), it'll return on the first loop. So only 1 exists? call for the majority case. That seems legit enough.

Heh. I don't think the iteration count really matters. I'd be fine to TURN IT UP TO 11 just for the fun of it ;)

Copy link
Member

Choose a reason for hiding this comment

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

cc @dhh, what do you think about adding this to the migration generator?

Copy link
Member

Choose a reason for hiding this comment

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

Me likey 👍 – and sounds good to drop the retry with base62.

Copy link
Member

Choose a reason for hiding this comment

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

Great 😁 @robertomiranda are you still interested in pursuing these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chancancode yes, I am. I think that the path to take here is:

  1. Add Base62 monkey patch to ActiveSupport
  2. Switch to Base62 for generates secure tokens and remove the retry
  3. Add the secure tokens migration helper and generator
  4. Move SecureToken to ActiveModel, in favor of "plug and play"

Choose a reason for hiding this comment

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

By "it adds a unique constraint" is this an index with a unique constraint on that column?

raise "Couldn't generate a unique token in 10 attempts!" if i == 9
else
return token
end
end
end
end
end
end
end

39 changes: 39 additions & 0 deletions activerecord/test/cases/secure_token_test.rb
@@ -0,0 +1,39 @@
require 'cases/helper'
require 'models/user'

class SecureTokenTest < ActiveRecord::TestCase
setup do
@user = User.new
end

test "assing unique token values" do
@user.save
assert_not_nil @user.token
assert_not_nil @user.auth_token
end

test "regenerate the secure key for the attribute" do
@user.save
old_token = @user.token
old_auth_token = @user.auth_token
@user.regenerate_token
@user.regenerate_auth_token

assert_not_equal @user.token, old_token
assert_not_equal @user.auth_token, old_auth_token
end

test "raise and exception when with 10 attemps is reached" do
User.stubs(:exists?).returns(*Array.new(10, true))
assert_raises(RuntimeError) do
@user.save
end
end

test "assing unique token after 9 attemps reached" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhh: We still need another test case for "9 collisions, then a success = win" and "10 collisions exactly = fail" to test that the main loop is functioning as it should.

done!

User.stubs(:exists?).returns(*Array.new(10){ |i| i == 9 ? false : true})
@user.save
assert_not_nil @user.token
assert_not_nil @user.auth_token
end
end
4 changes: 4 additions & 0 deletions activerecord/test/models/user.rb
@@ -0,0 +1,4 @@
class User < ActiveRecord::Base
has_secure_token
has_secure_token :auth_token
end
5 changes: 5 additions & 0 deletions activerecord/test/schema/schema.rb
Expand Up @@ -892,6 +892,11 @@ def except(adapter_names_to_exclude)
t.string :overloaded_string_with_limit, limit: 255
t.string :string_with_default, default: 'the original default'
end

create_table :users, force: true do |t|
t.string :token
t.string :auth_token
end
end

Course.connection.create_table :courses, force: true do |t|
Expand Down