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 captcha verify on login there may be has a bug! #2106

Closed
huacnlee opened this issue Oct 17, 2012 · 8 comments
Closed

Add captcha verify on login there may be has a bug! #2106

huacnlee opened this issue Oct 17, 2012 · 8 comments

Comments

@huacnlee
Copy link
Contributor

I want add a captcha feature on my login page. So I follow this page to do.

# coding: utf-8
class SessionsController < Devise::SessionsController
  def new
    session[:return_url] = params[:return] if !params[:return].blank?
    super
  end

  def create
    if verify_recaptcha
      super
    else
      build_resource
      # clean_up_passwords(resource)
      flash[:error] = "Captcha has wrong, try a again."
      respond_with_navigational(resource) { render :new }
    end    
  end
end

And then, I found an issue:

When I first submit with a bad captcha code, will got Captcha has wrong, try a again. message, but when I refresh that page again, the devise will auto login.

Rails logger show:

Started POST "/account/sign_in" for 127.0.0.1 at 2012-10-17 22:33:09 +0800
Creating scope :recent. Overwriting existing method User.recent.
Creating scope :exclude_ids. Overwriting existing method User.exclude_ids.
Processing by SessionsController#create as HTML
  Parameters: {"utf8"=>"", "authenticity_token"=>"zqnzxilphdasnIGkHqRH1saD4a4w/Eee/ef3OaWRyT8=", "user"=>{"email"=>"monster@gmail.com", "password"=>"[FILTERED]", "captcha"=>"WWW", "remember_me"=>"0"}, "commit"=>"登录"}
MONGODB (2ms) movieso_dev['system.namespaces'].find({})
MONGODB (0ms) movieso_dev['users'].find({:_id=>2923}).limit(-1).sort([[:_id, :asc]])
Redirected to http://127.0.0.1:3000/
Filter chain halted as :require_no_authentication rendered or redirected
Completed 302 Found in 5ms (Search: 0.0ms)

Looks like require_no_authentication has did login and redirect the page.

https://github.com/plataformatec/devise/blob/master/app/controllers/devise_controller.rb#L122


So, I add a skip_before_filter to disable that feature, and verify logic will be work fine!

class SessionsController < Devise::SessionsController
  skip_before_filter :require_no_authentication, :only => [ :new, :create]

  .....
end
@josevalim
Copy link
Contributor

Sounds good. Could you please update the wiki page? They are maintained by the community, so feel free to change it.

@rsinger104
Copy link

This change doesn't seem to work anymore.

Using omniauth (1.1.4)
Using omniauth-oauth2 (1.1.1)
Using omniauth-facebook (1.4.1)
Using recaptcha (0.3.5)
Using devise (2.2.3)

class SessionsController < Devise::SessionsController skip_before_filter :require_no_authentication, :only => [:new, :create]

def create
if verify_recaptcha
super
else
build_resource
flash[:notice] = "Incorrect Captcha entry, try again."
respond_with_navigational(resource) { render :new }
end
end
end

And logins are getting authorized even though the captcha is failing.

Any ideas? Trying to find out if devise 2.2.3 changed the before filter...

EDIT: I just realized this is probably because I'm using Omniauth and there must be an additional helper that needs to be run in order to verify the captcha before the omniauth login.

@chinacheng
Copy link

We meet this problem too. and just did that like huacnlee, but the skip_before_filter :require_no_authentication, :only => [ :new, :create] is no use for us.
we wrap the captcha valid in prepend_before_filter :captcha_valid, :only => [ :create] that will be ok.

@Feuda
Copy link

Feuda commented Jul 25, 2014

It's been changed, any ideas?

@Feuda
Copy link

Feuda commented Jul 28, 2014

@chinacheng Great! Your solution fits my issue!

@michelgrootjans
Copy link

I've encountered the exact same issue, and I can't seem to solve it. This is my implementation
using ruby 1.9.3p547 and rails 3.2.21:

class SessionsController < Devise::SessionsController
  skip_before_filter :require_no_authentication, :only => [:new, :create]

  # POST /resource/sign_in
  def create
    if verify_recaptcha
      super
    else
      render :new
    end
  end
end

I'm not quite understanding what is happening here. Devise's implementation of SessionsController has this on top of the file:
prepend_before_filter :require_no_authentication, :only => [ :new, :create ]
Which in plain English means: I don't have to be authenticated to view the sign-in form or to post a sign-in request. However, the behavior I see is the following:

  • when a user enters correct credentials without ticking the recaptcha, signed_in? is instantly true at the beginning of the create action, even before it calls super.

So to counteract this before filter, I have to write
skip_before_filter :require_no_authentication, :only => [ :new, :create ]
Why should in add only => [ :new, :create ], and not just only => [ :create ]?

Can anyone help me?

@guapolo
Copy link

guapolo commented Oct 29, 2015

Using @chinacheng's idea and this post on RoR 4 with Recaptcha gem, is as simple as:

class Users::SessionsController < Devise::SessionsController
  prepend_before_action :captcha_valid, only: [:create]
  layout "login"

  private
    def captcha_valid
      if verify_recaptcha
        true
      else
        self.resource = resource_class.new(sign_in_params)
        respond_with_navigational(resource) { render :new }
      end 
    end
end

@benaubin
Copy link

@guapolo Added that to the wiki

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

8 participants