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

devise_token_auth assumes you want the :confirmable functionality #40

Closed
jasonswett opened this issue Oct 2, 2014 · 8 comments
Closed

Comments

@jasonswett
Copy link
Contributor

I have an app that doesn't use :confirmable, and I had to do a couple somewhat hacky things in order for user registration to function without errors with devise_token_auth.

In the Devise gem itself, :confirmable is of course off by default. Is there any reason why it wouldn't be a good idea to make devise_token_auth match?

I would be willing to do the actual implementation work if we decide it would be a good idea.

@booleanbetrayal
Copy link
Collaborator

I'd personally like to see this. Can help test and such.

@lynndylanhurley
Copy link
Owner

I had to do a couple somewhat hacky things in order for user registration to function without errors with devise_token_auth.

What do you mean by "hacky things"? You should only need to add before_create :skip_confirmation! to your user model. Please post a bug if that doesn't work as stated in the docs.

In the Devise gem itself, :confirmable is of course off by default. Is there any reason why it wouldn't be a good idea to make devise_token_auth match?

I prefer defaulting to :confirmable for the following reasons:

  • It inhibits spam bots. It doesn't solve the problem completely, but it makes it more difficult for a bot to create an account.
  • It helps to ensure user accountability. For example, if a user is banned from your site, the user will need to create a new email account to re-register. Again, this doesn't solve the issue completely, but it does mitigate the problem.
  • It makes it impossible to register using an email account that is not your own.

For those reasons, and based on my experience building auth systems, I think that most users will want to use :confirmable. That is why it is used by default.

But it should be as easy as possible to disable. How do you guys feel about this solution?

@jasonswett
Copy link
Contributor Author

What do you mean by "hacky things"?

Nothing super terrible. Here's my relevant user model code:

class User < ActiveRecord::Base
  def confirmed?
    true
  end

  def send_confirmation_instructions(attributes)
    # Do nothing because we're not :confirmable.
  end
end

I feel ever so slightly funny that I have to add extra code in order to disable a feature that's off by default.

I prefer defaulting to :confirmable

I can totally understand where you're coming from and I definitely don't disagree with your reasons for wanting :confirmable to be enabled. It's just that my use case doesn't fit with what :confirmable is all about.

But it should be as easy as possible to disable. How do you guys feel about this solution?

I feel fine about that solution, but it unfortunately didn't work for me. devise_token_auth (at least I think it was devise_token_auth as opposed to Devise itself) still tried to send the confirmation email, I think because of this line.

I still prefer the paradigm of opting into :confirmable rather than opting out of it, but I don't have super strong feelings and I wouldn't try to argue very hard for my view.

@lynndylanhurley
Copy link
Owner

That line shouldn't be called if the before_create :skip_confirmation! callback is in place. I have test coverage on this, and the tests are passing with the latest beta (0.1.29.beta5).

Which version are you using?

@jasonswett
Copy link
Contributor Author

Hmm, weird. I think I'm using 0.1.28. Let me see if I can nail down why exactly I was seeing what I was seeing and report back. It'll unfortunately probably be tomorrow before I can get into it again. If the skip_confirmation! solution could work for me, that would be great.

@lynndylanhurley
Copy link
Owner

Cool, no problem. I just added this like two days ago (in response to your SO post). I've updated my answer there to reflect the change. Please try the latest beta and let me know how it goes.

@jasonswett
Copy link
Contributor Author

Okay, that totally worked. Thanks.

@lynndylanhurley
Copy link
Owner

😃

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

No branches or pull requests

3 participants