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鈥檒l 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

Conversation

@robertomiranda
Copy link
Contributor

robertomiranda commented Dec 27, 2014

I take the audacity of include has_scure_token to Active Model since the implementation is using methods that are implemented in most of popular ORMs or ODMs in ruby, like exists?and save, is just as suggestion and for sure I'm open to move it to Active Record anyway 馃槃. Is still missing documentation but I'd like to hear first opinions about the implementation/code

Usage

class User < ActiveRecord::Base
  has_secure_token :token1, :token2, key_length: 30
end

user = User.new
user.save
user.token1 # => "973acd04bc627d6a0e31200b74e2236"
user.token2 # => "e2426a93718d1817a43abbaa8508223"
user.regenerate_token1! # => true
user.regenerate_token2! # => true

ref #16879

cc @dhh

@robertomiranda robertomiranda force-pushed the robertomiranda:has_secure_token branch Dec 27, 2014
@simi
Copy link
Contributor

simi commented Dec 27, 2014

quoting @dhh from #16879 (comment):

Not interested in a heavily configurable thing. If someone wants to go hogwilld with that, it'll work great in a plugin. But for core, I'd just like something that works without configuration.

I think this introduces a lot of magic in contrast of has_secure_password implementation.

has_secure_password is all-in-one solution with 1 option only.

I think it is good idea to follow that simplicity so usecase will be:

class User < ActiveRecord::Base
  # add token column in migration
  has_secure_token
end

user = User.new
user.save
user.token # => "973acd04bc627d6a0e31200b74e2236"
user.regenerate_token! # => true

If this this simple solution is not enough for you, you'll need to roll your own then.

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Allowing you to name the token being added or to have more than one does not qualify as "heavy configuration" in my book. I support both of those. What I wasn't interested in was configuration of the token itself. That should just work.

On Dec 27, 2014, at 03:19, Josef 艩im谩nek notifications@github.com wrote:

quoting @dhh from #16879 (comment):

Not interested in a heavily configurable thing. If someone wants to go hogwilld with that, it'll work great in a plugin. But for core, I'd just like something that works without configuration.

I think this introduces a lot of magic in contrast of has_secure_password implementation.

has_secure_password is all-in-one solution with 1 option only.

I think it is good idea to follow that simplicity so usecase will be:

class User < ActiveRecord::Base

add token column in migration

has_secure_token
end

user = User.new
user.save
user.token # => "973acd04bc627d6a0e31200b74e2236"
user.regenerate_token! # => true
If this this simple solution is not enough for you, you'll need to roll your own then.


Reply to this email directly or view it on GitHub.

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Although I do actually like the option of passing no options to has_secure_token and then having it default to a token name of "token".

On Dec 27, 2014, at 03:19, Josef 艩im谩nek notifications@github.com wrote:

quoting @dhh from #16879 (comment):

Not interested in a heavily configurable thing. If someone wants to go hogwilld with that, it'll work great in a plugin. But for core, I'd just like something that works without configuration.

I think this introduces a lot of magic in contrast of has_secure_password implementation.

has_secure_password is all-in-one solution with 1 option only.

I think it is good idea to follow that simplicity so usecase will be:

class User < ActiveRecord::Base

add token column in migration

has_secure_token
end

user = User.new
user.save
user.token # => "973acd04bc627d6a0e31200b74e2236"
user.regenerate_token! # => true
If this this simple solution is not enough for you, you'll need to roll your own then.


Reply to this email directly or view it on GitHub.

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Speaking of options. What justification are we using for key_length? Why would you want to change that number? Unless we have a very compelling reason, we should mix that.

On Dec 27, 2014, at 03:19, Josef 艩im谩nek notifications@github.com wrote:

quoting @dhh from #16879 (comment):

Not interested in a heavily configurable thing. If someone wants to go hogwilld with that, it'll work great in a plugin. But for core, I'd just like something that works without configuration.

I think this introduces a lot of magic in contrast of has_secure_password implementation.

has_secure_password is all-in-one solution with 1 option only.

I think it is good idea to follow that simplicity so usecase will be:

class User < ActiveRecord::Base

add token column in migration

has_secure_token
end

user = User.new
user.save
user.token # => "973acd04bc627d6a0e31200b74e2236"
user.regenerate_token! # => true
If this this simple solution is not enough for you, you'll need to roll your own then.


Reply to this email directly or view it on GitHub.

@matthewd
Copy link
Member

matthewd commented Dec 27, 2014

I don't think multiple tokens is such a common use case that we need to do a *names thing -- one per call seems fine, and gives us a simpler API to maintain compatibility with in future.

And as it's apparently necessary, I'll join the existing chorus: this needs to be part of ActiveRecord, not ActiveModel.

@dhh
Copy link
Member

dhh commented Dec 27, 2014

I鈥檇 be happy with it being one call per token. Don鈥檛 have a strong opinion on AR vs AM, but if others do, that鈥檚 fine.

On Dec 27, 2014, at 8:29 AM, Matthew Draper notifications@github.com wrote:

I don't think multiple tokens is such a common use case that we need to do a *names thing -- one per call seems fine, and gives us a simpler API to maintain compatibility with in future.

And as it's apparently necessary, I'll join the #16879 (comment) existing #16879 (comment) chorus #16879 (comment): this needs to be part of ActiveRecord, not ActiveModel.


Reply to this email directly or view it on GitHub #18217 (comment).

@simi
Copy link
Contributor

simi commented Dec 27, 2014

I think this should be part of ActiveModel and just that part checking existing token should be part of ActiveRecord. It will be easy to integrate this into another ActiveModel based libraries like Mongoid.

@dhh
dhh reviewed Dec 27, 2014
View changes
activemodel/lib/active_model/secure_token.rb Outdated
# Load securerandom only when has_secure_key is used.
require 'securerandom'
include InstanceMethodsOnActivation
cattr_accessor :token_columns, :options

This comment has been minimized.

Copy link
@dhh

dhh Dec 27, 2014

Member

What's the token_columns variable for?

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Actually, I'd say that as it was mentioned elsewhere, that this SecureRandom implementation is unlikely enough to generate a duplicate, then we don't need the #exists? check. Someone could just implement that as a column constraint or similar. Then this thing becomes even simpler and doesn't require any dependency on AR at all.

@robertomiranda
Copy link
Contributor Author

robertomiranda commented Dec 27, 2014

@dhh 馃憤

@dhh
Copy link
Member

dhh commented Dec 27, 2014

I boiled this down to its essence:

module ActiveModel
  module SecureToken
    extend ActiveSupport::Concern

    module ClassMethods
      # Example using Active Record (which automatically includes ActiveModel::SecurePassword):
      #
      #   # Schema: User(auth_token:string, invitation_token:string)
      #   class User < ActiveRecord::Base
      #     has_secure_token :auth_token
      #     has_secure_token :invitation_token
      #   end
      #
      #   user = User.create
      #   user.auth_token # => "44539a6a59835a4ee9d7b112"
      #   user.invitation_token # => "226dd46af6be78953bde1648"
      #   user.regenerate_auth_token # => true
      #   user.regenerate_invitation_token # => true
      #
      # SecureRandom is used to generate the 24-character unique token, so collisions are highly unlikely.
      # But you're still encouraged to enforce uniqueness, if that's required, by something like a unique index
      # in the database or through a validation check.
      def has_secure_token(attribute)
        require 'securerandom'
        define_method("regenerate_#{attribute}") { update! attribute => SecureRandom.hex(12) }
        before_create { self[attribute] = SecureRandom.hex(12) }
      end
    end
  end
end
@dhh
Copy link
Member

dhh commented Dec 27, 2014

Sorry, that should be has_secure_token(attribute = 'token') to enable the default case as well.

@simi
Copy link
Contributor

simi commented Dec 27, 2014

And I think it is better to call that regenerate method reset_token! .

@dhh 馃憤

@dhh
Copy link
Member

dhh commented Dec 27, 2014

We already use the word reset to signify returning an attribute to its original state after modification, so that won't work well.

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Still on the fence about whether leaning exclusively on a unique index or the like to ensure no collisions is the right move here. The whole point is to make this as simple as possible. If it needs a db unique index to work, it loses a fair amount of that simplicity (even if the implantation obviously becomes much simpler).

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Here's another stab at a version that includes collision checking/mitigation:

      def has_secure_token(attribute)
        require 'securerandom'
        define_method("regenerate_#{attribute}") { update! attribute => self.class.generate_unique_secure_token(attribute) }
        before_create { self[attribute] = self.class.generate_unique_secure_token(attribute) }
      end

      def generate_unique_secure_token(attribute)
        (1..10).detect { SecureRandom.hex(12) unless exists?(attribute => random_token) } ||
          raise "Couldn't generate a unique token in 10 attempts!"
      end

(All untested code that I just mocked out).

@simi
Copy link
Contributor

simi commented Dec 27, 2014

I did a little experiment with SecureRandom.hex collision using https://gist.github.com/simi/d041f95d2e6a6b8ea2f7.

On Windows there wasn't collision in 100_000_000 iterations with 24 key length.
On Linux there wasn't collision in 10_000_000 iterations with 24 key length.
I reduced iterations on Linux since I owe a crappy linux machine.

But it is still random and collision can happen anytime. 馃槥

@dhh
Copy link
Member

dhh commented Dec 27, 2014

Yeah, I think we鈥檒l just deal with it as proposed in the second code example I shared. This should be plug鈥檔鈥檉orget.

On Dec 27, 2014, at 10:17 AM, Josef 艩im谩nek notifications@github.com wrote:

I did a little experiment with SecureRandom.hex collision using https://gist.github.com/simi/d041f95d2e6a6b8ea2f7 https://gist.github.com/simi/d041f95d2e6a6b8ea2f7.

On Windows there wasn't collision in 100_000_000 iterations with 24 key length.
On Linux there wasn't collision in 10_000_000 iterations with 24 key length.
I reduced iterations on Linux since I owe a crappy linux machine.

But it is still random and collision can happen anytime.


Reply to this email directly or view it on GitHub #18217 (comment).

@robertomiranda robertomiranda force-pushed the robertomiranda:has_secure_token branch 2 times, most recently Dec 27, 2014
@robertomiranda robertomiranda changed the title Add has_secure_token to Active Model Add has_secure_token to Active Record Dec 27, 2014
@robertomiranda
Copy link
Contributor Author

robertomiranda commented Dec 27, 2014

Guys I moved this to AR and keeping guideline of the second @dhh's code example

@robertomiranda
robertomiranda reviewed Dec 27, 2014
View changes
activerecord/lib/active_record/secure_token.rb Outdated
random_token = SecureRandom.hex(12)
return random_token unless exists?(attribute => random_token)
end
raise "Couldn't generate a unique token in 10 attempts!"

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Dec 27, 2014

Author Contributor

not sure if we should have a custom exception here wdyt?

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Dec 27, 2014

Author Contributor

something like raise CollisionLimitReached, "Couldn't generate a unique token in 10 attempts!"?

This comment has been minimized.

Copy link
@dhh

dhh Dec 27, 2014

Member

Don't think we need a custom exception. This is extremely unlikely to happen, so not something anyone would custom catch anyway.

This comment has been minimized.

Copy link
@dhh

dhh Dec 27, 2014

Member

I don't like this return / raise flow, though. What was wrong with the detect approach?

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Dec 28, 2014

Author Contributor

The issue with detect is that this methods finds the element on an enumerable, so basically in most of the cases will returns 1 or 2 instead of the token needed

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Dec 28, 2014

Author Contributor

maybe another approach more clear would be

10.times do
  if random_token = SecureRandom.hex(12) && exists?(attribute => random_token)
    raise "Couldn't generate a unique token in 10 attempts!"
  else
    return random_token      
  end
end

This comment has been minimized.

Copy link
@dhh

dhh Dec 28, 2014

Member

Ah, right, here's a variation that might work:

10.times do |i|
  SecureRandom.hex(12).tap do |token|
    if exists?(attribute => token) && i == 9
      raise "Couldn't generate a unique token in 10 attempts!"
    else
      return token
    end
  end
end

Don't love that the exceptional state is the first conditional, but ok.

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Dec 28, 2014

Author Contributor

Done 馃憤

This comment has been minimized.

Copy link
@dhh

dhh Dec 28, 2014

Member

This isn't right. This will raise an exception on the first collision. It's supposed to not raise an exception until the 10th time it still produces a collision.

On Dec 28, 2014, at 09:47, Roberto Miranda notifications@github.com wrote:

In activerecord/lib/active_record/secure_token.rb:

  •  #   user.save
    
  •  #   user.token # => "44539a6a59835a4ee9d7b112"
    
  •  #   user.regenerate_token # => true
    
  •  def has_secure_token(attribute = :token)
    
  •    # Load securerandom only when has_secure_key is used.
    
  •    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)
    
  •    require 'securerandom'
    
  •    10.times do
    
  •      random_token = SecureRandom.hex(12)
    
  •      return random_token unless exists?(attribute => random_token)
    
  •    end
    
  •    raise "Couldn't generate a unique token in 10 attempts!"
    
    Done


Reply to this email directly or view it on GitHub.

@robertomiranda robertomiranda force-pushed the robertomiranda:has_secure_token branch 2 times, most recently Dec 27, 2014
@simi
simi reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/secure_token.rb Outdated
10.times do |i|
SecureRandom.hex(12).tap do |token|
if exists?(attribute => token)
raise "Couldn't generate a unique token in 10 attempts!"

This comment has been minimized.

Copy link
@simi

simi Dec 28, 2014

Contributor

This can raise on every iteration.

What about?

raise "Couldn't generate a unique token in 10 attempts!" if i == 9
@robertomiranda robertomiranda force-pushed the robertomiranda:has_secure_token branch Dec 28, 2014
@robertomiranda
robertomiranda reviewed Dec 28, 2014
View changes
activerecord/test/cases/secure_token_test.rb Outdated

def test_raise_and_exception_when_there_is_a_collision
User.stubs(:exists?).returns(true)
assert_raises(RuntimeError) do

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Dec 28, 2014

Author Contributor

fixed the issue of each iterations that raise an exception, but not sure with change this test is failing, seems to be everything covered. I was debugging into the test case code and 10.times only makes to iterations and is always returning 2 through the variable i, really weird

This comment has been minimized.

Copy link
@dhh

dhh Dec 28, 2014

Member

Not sure I understand? We should have a test case that covers exists? returning true, say, three times, and then returns false to cover the retry action.

I think you can use User.expects(:exists?).times(3).returns(true); User.expects(:exists?).returns(false) or something like that?

This comment has been minimized.

Copy link
@simi
@dhh
dhh reviewed Dec 28, 2014
View changes
activemodel/test/models/user.rb Outdated
@@ -1,7 +1,7 @@
class User
extend ActiveModel::Callbacks
include ActiveModel::SecurePassword

This comment has been minimized.

Copy link
@dhh

dhh Dec 28, 2014

Member

Let's remove this unrelated change from the PR.

Update SecureToken Docs

Add Changelog entry for has_secure_token [ci skip]
@robertomiranda robertomiranda force-pushed the robertomiranda:has_secure_token branch to 5a58ba3 Jan 4, 2015
end
end

test "assing unique token after 9 attemps reached" do

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Jan 4, 2015

Author Contributor

@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!

@dhh
Copy link
Member

dhh commented Jan 4, 2015

This is looking good to me now 馃憤. I say we still want to do the base62 upgrade, but we can do that after merging.

@robertomiranda
Copy link
Contributor Author

robertomiranda commented Jan 4, 2015

@dhh great, once this PR be already merged I'll open another one with base62 upgrade 馃憤

dhh added a commit that referenced this pull request Jan 4, 2015
Add has_secure_token to Active Record
@dhh dhh merged commit 33a13c9 into rails:master Jan 4, 2015
@dhh
Copy link
Member

dhh commented Jan 4, 2015

There you go :)

@simi
Copy link
Contributor

simi commented Jan 4, 2015

馃憦

@jonatack
Copy link
Contributor

jonatack commented Jan 5, 2015

馃帀

# 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.

This comment has been minimized.

Copy link
@aripollak

aripollak Jan 10, 2015

Contributor

s/key/token/?

@aripollak
Copy link
Contributor

aripollak commented Jan 10, 2015

It seems like @hundredwatt's comment wasn't addressed here. It seems weird to me to call something a secure token if it's stored in the database as plaintext, when the equivalent for passwords, has_secure_password, stores its value encrypted.

@dhh
Copy link
Member

dhh commented Jan 10, 2015

The security is in terms of its random generation. See SecureRandom.

On Jan 9, 2015, at 9:45 PM, Ari Pollak notifications@github.com wrote:

It seems like @hundredwatt https://github.com/hundredwatt's comment wasn't addressed here. It seems weird to me to call something a secure token if it's stored in the database as plaintext, when the equivalent for passwords, has_secure_password, stores its value encrypted.


Reply to this email directly or view it on GitHub #18217 (comment).

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

This comment has been minimized.

Copy link
@abscondite

This comment has been minimized.

Copy link
@maclover7

maclover7 Feb 9, 2016

Member

Fixed via #18537.

shaunxp20 added a commit to Iridescent-CM/technovation-app that referenced this pull request Mar 19, 2020
We don't need this gem anymore because the functionality has been added
to Rails (by the author of the gem).

rails/rails#18217
https://api.rubyonrails.org/classes/ActiveRecord/SecureToken/ClassMethods.html
https://blog.bigbinary.com/2016/03/23/has-secure-token-to-generate-unique-random-token-in-rails-5.html

Removing this gem will take care of the warning:
already initialized constant SecureRandom::BASE58_ALPHABET

Refs: #2430
shaunxp20 added a commit to Iridescent-CM/technovation-app that referenced this pull request Mar 19, 2020
We don't need this gem anymore because the functionality has been added
to Rails (by the author of the gem).

rails/rails#18217
https://api.rubyonrails.org/classes/ActiveRecord/SecureToken/ClassMethods.html
https://blog.bigbinary.com/2016/03/23/has-secure-token-to-generate-unique-random-token-in-rails-5.html

Removing this gem will take care of the warning:
already initialized constant SecureRandom::BASE58_ALPHABET

Refs: #2430
shaunxp20 added a commit to Iridescent-CM/technovation-app that referenced this pull request Mar 19, 2020
* Remove has_secure_token gem

We don't need this gem anymore because the functionality has been added
to Rails (by the author of the gem).

rails/rails#18217
https://api.rubyonrails.org/classes/ActiveRecord/SecureToken/ClassMethods.html
https://blog.bigbinary.com/2016/03/23/has-secure-token-to-generate-unique-random-token-in-rails-5.html

Removing this gem will take care of the warning:
already initialized constant SecureRandom::BASE58_ALPHABET

Refs: #2430

* Use BigDecimal instead of BigDecimal.new

This will take care of the warning:
BigDecimal.new is deprecated; use BigDecimal() method instead

Refs: #2430
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can鈥檛 perform that action at this time.