Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Do not call save on the model when only a few attributes are being changed. #279

Closed
wants to merge 22 commits into from

6 participants

@pupeno

Hello,

In our user model we have a couple of serialized attributes and Rails apparently always saves serialized attributes when calling save because it cannot keep track of whether they are dirty or not (I think). This meant that in many requests that used Sorcery, we end up doing a rather big update to a database. Also, we have some after filters that were unexpectedly run due to Sorcery calling save on the model.

We noticed this didn't always happen because some modules of Sorcery use update_single_attribute. We wrote a similar but more generic update_many_attributes and made sure it is used instead of save everywhere where it makes sense (it doesn't when changing the password or creating a new user).

We did this change on top of the last stable version (v0.7.11) but the difference to the current master is small and documented in the commit.

We ran all the tests before and after this commit and we got the same amount of failures (everything passed but three oauth related tests). We also made sure we ran all the touched lines by manual testing from our application that is using Sorcery, except the mongo implementation of update_many_attributes, that is untested.

We did this change pair-programming, @dmagliola and I.

and others added some commits
@NoamB adding mongo_mapper to Gemfile caa3622
@xpepermint xpepermint Adding the delayed_job integration instructions. 29e7e70
@NoamB Merge pull request #269 from xpepermint/master
Adding the delayed_job integration instructions
0ecc67f
@banyan banyan fix to work redirect_back_or_to when with externals 1a6691f
@banyan banyan Adding specs for #270 38d285f
@NoamB Merge pull request #270 from banyan/redirect_back_or_to-work-with-ext…
…ernal

fix `redirect_back_or_to` when we login with externals
949e710
@banyan banyan bundler will use if parent bundler exists already, so empty it. 71fe3f4
@banyan banyan pwd doesn't need it d9beed9
@banyan banyan Override env as less as possible bb37450
@NoamB Merge pull request #271 from banyan/work-all-sorcery-specs
bundler won't bundle if parent bundler exists already, so empty it.
7603676
@sanemat sanemat Set valid travis-ci configuration. a097a2a
@NoamB Merge pull request #273 from sanemat/travis-lint
Set valid travis-ci configuration.
47793d6
@stepantubanov stepantubanov Fix unlock token for Mongoid and MongoMapper
Also changed logic a bit. It will send out an email (if option is enabled) once
when locking instead of sending it out on every save.
56b2058
@NoamB Merge pull request #275 from stephan778/master
Brute force protection: Fix unlock token for Mongoid and MongoMapper
facc486
@banyan banyan Fix bcrypt-ruby dependency 1ee42cc
@NoamB Merge pull request #276 from banyan/bcrypt-dependency
Fix bcrypt-ruby dependency
769b679
@NoamB regenerated gemspec 9409172
@pupeno pupeno Do not call save on the model when only a few attributes are being ch…
…anged.
63b0a24
@NoamB
Owner

mongo_mapper is totally broken after this merge.

@pupeno

Yeah, I couldn't test the mongo implementation. If nobody else that's familiar with mongo can, I might give it a try.

@pupeno

I updated the pull request to work on top of the 0.7.12 branch (there was one conflict, which is now resolved).

@pupeno

Superseded by #305.

@pupeno pupeno closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 18, 2012
  1. adding mongo_mapper to Gemfile

    authored
Commits on Apr 19, 2012
  1. @xpepermint
Commits on Apr 21, 2012
  1. Merge pull request #269 from xpepermint/master

    authored
    Adding the delayed_job integration instructions
  2. @banyan
Commits on Apr 22, 2012
  1. @banyan

    Adding specs for #270

    banyan authored
  2. Merge pull request #270 from banyan/redirect_back_or_to-work-with-ext…

    authored
    …ernal
    
    fix `redirect_back_or_to` when we login with externals
Commits on Apr 23, 2012
  1. @banyan
  2. @banyan

    pwd doesn't need it

    banyan authored
  3. @banyan
  4. Merge pull request #271 from banyan/work-all-sorcery-specs

    authored
    bundler won't bundle if parent bundler exists already, so empty it.
  5. @sanemat
Commits on Apr 24, 2012
  1. Merge pull request #273 from sanemat/travis-lint

    authored
    Set valid travis-ci configuration.
  2. @stepantubanov

    Fix unlock token for Mongoid and MongoMapper

    stepantubanov authored
    Also changed logic a bit. It will send out an email (if option is enabled) once
    when locking instead of sending it out on every save.
  3. Merge pull request #275 from stephan778/master

    authored
    Brute force protection: Fix unlock token for Mongoid and MongoMapper
Commits on Apr 25, 2012
  1. @banyan

    Fix bcrypt-ruby dependency

    banyan authored
  2. Merge pull request #276 from banyan/bcrypt-dependency

    authored
    Fix bcrypt-ruby dependency
  3. regenerated gemspec

    authored
Commits on May 2, 2012
  1. @pupeno
Commits on May 6, 2012
  1. updated README

    authored
  2. Version bump to 0.7.12

    authored
Commits on Jun 6, 2012
  1. @pupeno
This page is out of date. Refresh to see the latest.
View
1  .travis.yml
@@ -1,2 +1,3 @@
+language: ruby
rvm:
- 1.9.2
View
5 Gemfile
@@ -4,6 +4,7 @@ source :rubygems
# gem "activesupport", ">= 2.3.5"
gem 'oauth', "~> 0.4.4"
gem 'oauth2', "~> 0.5.1"
+gem 'bcrypt-ruby', "~> 3.0.0"
# Add dependencies to develop your gem here.
# Include everything needed to run rake, tests, features, etc.
@@ -20,6 +21,6 @@ group :development do
gem 'simplecov', '>= 0.3.8', :require => false # Will install simplecov-html as a dependency
gem 'timecop'
gem 'capybara'
- gem 'bcrypt-ruby', "~> 3.0.0"
+ gem 'mongo_mapper'
gem 'mongoid', "~> 2.4.4"
-end
+end
View
7 Gemfile.lock
@@ -70,6 +70,10 @@ GEM
mime-types (1.18)
mongo (1.6.1)
bson (~> 1.6.1)
+ mongo_mapper (0.11.1)
+ activemodel (~> 3.0)
+ activesupport (~> 3.0)
+ plucky (~> 0.4.0)
mongoid (2.4.7)
activemodel (~> 3.1)
mongo (~> 1.3)
@@ -81,6 +85,8 @@ GEM
oauth2 (0.5.2)
faraday (~> 0.7)
multi_json (~> 1.0)
+ plucky (0.4.4)
+ mongo (~> 1.5)
polyglot (0.3.3)
rack (1.4.1)
rack-cache (1.2)
@@ -167,6 +173,7 @@ DEPENDENCIES
capybara
jeweler (~> 1.8.3)
json (>= 1.5.1)
+ mongo_mapper
mongoid (~> 2.4.4)
oauth (~> 0.4.4)
oauth2 (~> 0.5.1)
View
22 README.rdoc
@@ -29,7 +29,7 @@ Railscast: http://railscasts.com/episodes/283-authentication-with-sorcery
Example Rails 3 app using sorcery: https://github.com/NoamB/sorcery-example-app
-Documentation: http://rubydoc.info/gems/sorcery/0.7.10/frames
+Documentation: http://rubydoc.info/gems/sorcery/0.7.12/frames
Check out the tutorials in the github wiki!
@@ -118,6 +118,26 @@ add them to the initializer file.
Inside the initializer, the comments will tell you what each setting does.
+== DelayedJob Integration
+
+By default emails are sent synchronously. You can send them asynchronously by using the
+[delayed_job gem](https://github.com/collectiveidea/delayed_job).
+
+After implementing the `delayed_job` into your project add the code below at the end of
+the `config/initializers/sorcery.rb` file. After that all emails will be sent asynchronously.
+
+ module Sorcery
+ module Model
+ module InstanceMethods
+ def generic_send_email(method, mailer)
+ config = sorcery_config
+ mail = config.send(mailer).delay.send(config.send(method), self)
+ end
+ end
+ end
+ end
+
+
== Full Features List by module:
View
11 Rakefile
@@ -43,13 +43,14 @@ task :default => :all_sorcery_specs
desc "Run all sorcery specs"
task :all_sorcery_specs do
+ # we need to be empty, otherwise bundler will use parent bundler.
+ env = {
+ 'BUNDLE_GEMFILE' => nil,
+ 'GEM_HOME' => nil
+ }
Dir['spec/**/Rakefile'].each do |rakefile|
directory_name = File.dirname(rakefile)
- sh <<-CMD
- cd #{directory_name}
- bundle
- bundle exec rake
- CMD
+ system(env, "cd #{directory_name} && bundle && bundle exec rake")
end
end
View
2  VERSION
@@ -1 +1 @@
-0.7.11
+0.7.12
View
3  lib/sorcery/controller/submodules/brute_force_protection.rb
@@ -29,8 +29,7 @@ def update_failed_logins_count!(credentials)
# Resets the failed logins counter.
# Runs as a hook after a successful login.
def reset_failed_logins_count!(user, credentials)
- user.send(:"#{user_class.sorcery_config.failed_logins_count_attribute_name}=", 0)
- user.save!(:validate => false)
+ user.update_many_attributes(user_class.sorcery_config.failed_logins_count_attribute_name => 0)
end
end
end
View
2  lib/sorcery/controller/submodules/external.rb
@@ -46,7 +46,9 @@ def login_from(provider)
@provider.process_callback(params,session)
@user_hash = @provider.get_user_hash
if user = user_class.load_from_provider(provider,@user_hash[:uid].to_s)
+ return_to_url = session[:return_to_url]
reset_session
+ session[:return_to_url] = return_to_url
auto_login(user)
user
end
View
13 lib/sorcery/model/adapters/active_record.rb
@@ -8,11 +8,16 @@ def self.included(klass)
end
module InstanceMethods
- def update_single_attribute(name, value)
- self.send(:"#{name}=", value)
-
+ def update_many_attributes(attrs)
+ attrs.each do |name, value|
+ self.send(:"#{name}=", value)
+ end
primary_key = self.class.primary_key
- self.class.where(:"#{primary_key}" => self.send(:"#{primary_key}")).update_all(name => value)
+ self.class.where(:"#{primary_key}" => self.send(:"#{primary_key}")).update_all(attrs)
+ end
+
+ def update_single_attribute(name, value)
+ update_many_attributes(name => value)
end
end
View
14 lib/sorcery/model/adapters/mongoid.rb
@@ -11,11 +11,17 @@ module InstanceMethods
def increment(attr)
self.inc(attr,1)
end
-
+
+ def update_many_attributes(attrs)
+ attrs.each do |name, value|
+ attrs[name] = value.utc if value.is_a?(ActiveSupport::TimeWithZone)
+ self.send(:"#{name}=", value)
+ end
+ self.class.where(:_id => self.id).update_all(attrs)
+ end
+
def update_single_attribute(name, value)
- value = value.utc if value.is_a?(ActiveSupport::TimeWithZone)
- self.send(:"#{name}=", value)
- self.class.where(:_id => self.id).update_all(name => value)
+ update_many_attributes(name => value)
end
end
View
25 lib/sorcery/model/submodules/brute_force_protection.rb
@@ -41,10 +41,6 @@ def self.included(base)
end
base.extend(ClassMethods)
base.send(:include, InstanceMethods)
-
- base.class_eval do
- after_update :send_unlock_token_email!, :if => Proc.new { |user| user.send(sorcery_config.unlock_token_attribute_name).present? }
- end
end
module ClassMethods
@@ -59,11 +55,13 @@ def load_from_unlock_token(token)
def define_brute_force_protection_mongoid_fields
field sorcery_config.failed_logins_count_attribute_name, :type => Integer, :default => 0
field sorcery_config.lock_expires_at_attribute_name, :type => Time
+ field sorcery_config.unlock_token_attribute_name, :type => String
end
def define_brute_force_protection_mongo_mapper_fields
key sorcery_config.failed_logins_count_attribute_name, Integer, :default => 0
key sorcery_config.lock_expires_at_attribute_name, Time
+ key sorcery_config.unlock_token_attribute_name, String
end
end
@@ -74,7 +72,7 @@ def register_failed_login!
config = sorcery_config
return if !unlocked?
self.increment(config.failed_logins_count_attribute_name)
- self.save!(:validate => false)
+ self.update_many_attributes(config.failed_logins_count_attribute_name => self.send(config.failed_logins_count_attribute_name))
self.lock! if self.send(config.failed_logins_count_attribute_name) >= config.consecutive_login_retries_amount_limit
end
@@ -83,19 +81,22 @@ def register_failed_login!
# /!\
def unlock!
config = sorcery_config
- self.send(:"#{config.lock_expires_at_attribute_name}=", nil)
- self.send(:"#{config.failed_logins_count_attribute_name}=", 0)
- self.send(:"#{config.unlock_token_attribute_name}=", nil) unless config.unlock_token_mailer_disabled or config.unlock_token_mailer.nil?
- self.save!(:validate => false)
+ attributes = {config.lock_expires_at_attribute_name => nil,
+ config.failed_logins_count_attribute_name => 0}
+ attributes[config.unlock_token_attribute_name] = nil unless config.unlock_token_mailer_disabled or config.unlock_token_mailer.nil?
+ self.update_many_attributes(attributes)
end
protected
def lock!
config = sorcery_config
- self.send(:"#{config.lock_expires_at_attribute_name}=", Time.now.in_time_zone + config.login_lock_time_period)
- self.send(:"#{config.unlock_token_attribute_name}=", TemporaryToken.generate_random_token) unless config.unlock_token_mailer_disabled or config.unlock_token_mailer.nil?
- self.save!(:validate => false)
+ attributes = {config.lock_expires_at_attribute_name => Time.now.in_time_zone + config.login_lock_time_period}
+ unless config.unlock_token_mailer_disabled || config.unlock_token_mailer.nil?
+ attributes[config.unlock_token_attribute_name] = TemporaryToken.generate_random_token
+ send_unlock_token_email!
+ end
+ self.update_many_attributes(attributes)
end
def unlocked?
View
10 lib/sorcery/model/submodules/remember_me.rb
@@ -49,17 +49,15 @@ module InstanceMethods
# You shouldn't really use this one yourself - it's called by the controller's 'remember_me!' method.
def remember_me!
config = sorcery_config
- self.send(:"#{config.remember_me_token_attribute_name}=", TemporaryToken.generate_random_token)
- self.send(:"#{config.remember_me_token_expires_at_attribute_name}=", Time.now.in_time_zone + config.remember_me_for)
- self.save!(:validate => false)
+ self.update_many_attributes(config.remember_me_token_attribute_name => TemporaryToken.generate_random_token,
+ config.remember_me_token_expires_at_attribute_name => Time.now.in_time_zone + config.remember_me_for)
end
# You shouldn't really use this one yourself - it's called by the controller's 'forget_me!' method.
def forget_me!
config = sorcery_config
- self.send(:"#{config.remember_me_token_attribute_name}=", nil)
- self.send(:"#{config.remember_me_token_expires_at_attribute_name}=", nil)
- self.save!(:validate => false)
+ self.update_many_attributes(config.remember_me_token_attribute_name => nil,
+ config.remember_me_token_expires_at_attribute_name => nil)
end
end
end
View
8 lib/sorcery/model/submodules/reset_password.rb
@@ -96,11 +96,11 @@ def deliver_reset_password_instructions!
config = sorcery_config
# hammering protection
return false if config.reset_password_time_between_emails && self.send(config.reset_password_email_sent_at_attribute_name) && self.send(config.reset_password_email_sent_at_attribute_name) > config.reset_password_time_between_emails.ago.utc
- self.send(:"#{config.reset_password_token_attribute_name}=", TemporaryToken.generate_random_token)
- self.send(:"#{config.reset_password_token_expires_at_attribute_name}=", Time.now.in_time_zone + config.reset_password_expiration_period) if config.reset_password_expiration_period
- self.send(:"#{config.reset_password_email_sent_at_attribute_name}=", Time.now.in_time_zone)
+ attributes = {config.reset_password_token_attribute_name => TemporaryToken.generate_random_token,
+ config.reset_password_email_sent_at_attribute_name => Time.now.in_time_zone}
+ attributes[config.reset_password_token_expires_at_attribute_name] = Time.now.in_time_zone + config.reset_password_expiration_period if config.reset_password_expiration_period
self.class.transaction do
- self.save!(:validate => false)
+ self.update_many_attributes(attributes)
generic_send_email(:reset_password_email_method_name, :reset_password_mailer) unless config.reset_password_mailer_disabled
end
end
View
15 sorcery.gemspec
@@ -5,11 +5,11 @@
Gem::Specification.new do |s|
s.name = "sorcery"
- s.version = "0.7.11"
+ s.version = "0.7.12"
s.required_rubygems_version = Gem::Requirement.new(">= 0") if s.respond_to? :required_rubygems_version=
s.authors = ["Noam Ben Ari"]
- s.date = "2012-04-18"
+ s.date = "2012-05-06"
s.description = "Provides common authentication needs such as signing in/out, activating by email and resetting password."
s.email = "nbenari@gmail.com"
s.extra_rdoc_files = [
@@ -302,7 +302,7 @@ Gem::Specification.new do |s|
s.homepage = "http://github.com/NoamB/sorcery"
s.licenses = ["MIT"]
s.require_paths = ["lib"]
- s.rubygems_version = "1.8.21"
+ s.rubygems_version = "1.8.10"
s.summary = "Magical authentication for Rails 3 applications"
if s.respond_to? :specification_version then
@@ -311,6 +311,7 @@ Gem::Specification.new do |s|
if Gem::Version.new(Gem::VERSION) >= Gem::Version.new('1.2.0') then
s.add_runtime_dependency(%q<oauth>, ["~> 0.4.4"])
s.add_runtime_dependency(%q<oauth2>, ["~> 0.5.1"])
+ s.add_runtime_dependency(%q<bcrypt-ruby>, ["~> 3.0.0"])
s.add_development_dependency(%q<rails>, [">= 3.0.0"])
s.add_development_dependency(%q<json>, [">= 1.5.1"])
s.add_development_dependency(%q<rspec>, ["~> 2.5.0"])
@@ -323,11 +324,12 @@ Gem::Specification.new do |s|
s.add_development_dependency(%q<simplecov>, [">= 0.3.8"])
s.add_development_dependency(%q<timecop>, [">= 0"])
s.add_development_dependency(%q<capybara>, [">= 0"])
- s.add_development_dependency(%q<bcrypt-ruby>, ["~> 3.0.0"])
+ s.add_development_dependency(%q<mongo_mapper>, [">= 0"])
s.add_development_dependency(%q<mongoid>, ["~> 2.4.4"])
else
s.add_dependency(%q<oauth>, ["~> 0.4.4"])
s.add_dependency(%q<oauth2>, ["~> 0.5.1"])
+ s.add_dependency(%q<bcrypt-ruby>, ["~> 3.0.0"])
s.add_dependency(%q<rails>, [">= 3.0.0"])
s.add_dependency(%q<json>, [">= 1.5.1"])
s.add_dependency(%q<rspec>, ["~> 2.5.0"])
@@ -340,12 +342,13 @@ Gem::Specification.new do |s|
s.add_dependency(%q<simplecov>, [">= 0.3.8"])
s.add_dependency(%q<timecop>, [">= 0"])
s.add_dependency(%q<capybara>, [">= 0"])
- s.add_dependency(%q<bcrypt-ruby>, ["~> 3.0.0"])
+ s.add_dependency(%q<mongo_mapper>, [">= 0"])
s.add_dependency(%q<mongoid>, ["~> 2.4.4"])
end
else
s.add_dependency(%q<oauth>, ["~> 0.4.4"])
s.add_dependency(%q<oauth2>, ["~> 0.5.1"])
+ s.add_dependency(%q<bcrypt-ruby>, ["~> 3.0.0"])
s.add_dependency(%q<rails>, [">= 3.0.0"])
s.add_dependency(%q<json>, [">= 1.5.1"])
s.add_dependency(%q<rspec>, ["~> 2.5.0"])
@@ -358,7 +361,7 @@ Gem::Specification.new do |s|
s.add_dependency(%q<simplecov>, [">= 0.3.8"])
s.add_dependency(%q<timecop>, [">= 0"])
s.add_dependency(%q<capybara>, [">= 0"])
- s.add_dependency(%q<bcrypt-ruby>, ["~> 3.0.0"])
+ s.add_dependency(%q<mongo_mapper>, [">= 0"])
s.add_dependency(%q<mongoid>, ["~> 2.4.4"])
end
end
View
40 spec/rails3/app/controllers/application_controller.rb
@@ -137,6 +137,46 @@ def test_login_from5
end
end
+ def test_return_to_with_external
+ if @user = login_from(:twitter)
+ redirect_back_or_to "bla", :notice => "Success!"
+ else
+ redirect_to "blu", :alert => "Failed!"
+ end
+ end
+
+ def test_return_to_with_external2
+ if @user = login_from(:facebook)
+ redirect_back_or_to "bla", :notice => "Success!"
+ else
+ redirect_to "blu", :alert => "Failed!"
+ end
+ end
+
+ def test_return_to_with_external3
+ if @user = login_from(:github)
+ redirect_back_or_to "bla", :notice => "Success!"
+ else
+ redirect_to "blu", :alert => "Failed!"
+ end
+ end
+
+ def test_return_to_with_external4
+ if @user = login_from(:google)
+ redirect_back_or_to "bla", :notice => "Success!"
+ else
+ redirect_to "blu", :alert => "Failed!"
+ end
+ end
+
+ def test_return_to_with_external5
+ if @user = login_from(:liveid)
+ redirect_back_or_to "bla", :notice => "Success!"
+ else
+ redirect_to "blu", :alert => "Failed!"
+ end
+ end
+
def test_create_from_provider
provider = params[:provider]
login_from(provider)
View
32 spec/rails3/spec/controller_oauth2_spec.rb
@@ -81,6 +81,14 @@ def stub_all_oauth2_requests!
flash[:alert].should == "Failed!"
end
+ it "on successful login_from the user should be redirected to the url he originally wanted" do
+ sorcery_model_property_set(:authentications_class, Authentication)
+ create_new_external_user(:facebook)
+ get :test_return_to_with_external2, {}, :return_to_url => "fuu"
+ response.should redirect_to("fuu")
+ flash[:notice].should == "Success!"
+ end
+
# provider: github
it "login_at redirects correctly (github)" do
create_new_user
@@ -103,6 +111,14 @@ def stub_all_oauth2_requests!
flash[:alert].should == "Failed!"
end
+ it "on successful login_from the user should be redirected to the url he originally wanted (github)" do
+ sorcery_model_property_set(:authentications_class, Authentication)
+ create_new_external_user(:github)
+ get :test_return_to_with_external3, {}, :return_to_url => "fuu"
+ response.should redirect_to("fuu")
+ flash[:notice].should == "Success!"
+ end
+
# provider: google
it "login_at redirects correctly (google)" do
create_new_user
@@ -125,6 +141,14 @@ def stub_all_oauth2_requests!
flash[:alert].should == "Failed!"
end
+ it "on successful login_from the user should be redirected to the url he originally wanted (google)" do
+ sorcery_model_property_set(:authentications_class, Authentication)
+ create_new_external_user(:google)
+ get :test_return_to_with_external4, {}, :return_to_url => "fuu"
+ response.should redirect_to("fuu")
+ flash[:notice].should == "Success!"
+ end
+
# provider: liveid
it "login_at redirects correctly (liveid)" do
create_new_user
@@ -147,6 +171,14 @@ def stub_all_oauth2_requests!
flash[:alert].should == "Failed!"
end
+ it "on successful login_from the user should be redirected to the url he originally wanted (liveid)" do
+ sorcery_model_property_set(:authentications_class, Authentication)
+ create_new_external_user(:liveid)
+ get :test_return_to_with_external5, {}, :return_to_url => "fuu"
+ response.should redirect_to("fuu")
+ flash[:notice].should == "Success!"
+ end
+
end
View
9 spec/rails3/spec/controller_oauth_spec.rb
@@ -63,6 +63,15 @@ def stub_all_oauth_requests!
get :test_login_from, :oauth_verifier => "blablaRERASDFcxvSDFA"
flash[:alert].should == "Failed!"
end
+
+ it "on successful 'login_from' the user should be redirected to the url he originally wanted" do
+ sorcery_model_property_set(:authentications_class, Authentication)
+ create_new_external_user(:twitter)
+ get :test_return_to_with_external, {}, :return_to_url => "fuu"
+ response.should redirect_to("fuu")
+ flash[:notice].should == "Success!"
+ end
+
end
describe ApplicationController do
Something went wrong with that request. Please try again.