-
Notifications
You must be signed in to change notification settings - Fork 41
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
Facebook login #92
Facebook login #92
Conversation
Addresses #68 |
Also we need to decide how to handle login collisions when a user signs up through Facebook, etc. Right now we take their name on Facebook and do this, but that could obviously result in a uniqueness violation. |
@camallen any other concerns related to this? |
@@ -27,7 +27,7 @@ def respond_to_request(action_status) | |||
|
|||
def able_to_reset_password? | |||
if user = resource_class.find_for_authentication(resource_params) | |||
disabled_or_omni_auth = user.disabled? || user.encrypted_password.blank? | |||
disabled_or_omni_auth = user.disabled? || user.email.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're not forcing a user to have an email, perhaps we can use provider field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my reasoning behind this line was to allow omniauth users who allow access their email to "reset" their password and generate a new password to allow themselves to login with a login/password or their facebook account.
I defintely think we'll have some people who forgot how they signed up and click on 'Forgot password' by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would they have set a login name as well? merging accounts through different authentication providers could get tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It creates a login here since we need to have that to give them a URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fine then. All looks good.
Adds basic support for Omniauth login using Facebook.
We still need to separate Omniauth authorization into a separate table to allow Users to authorize access to multiple social networks (or login or other interactions we'd like to support).