Authentication via social networks #3
Conversation
ruslankhaertdinov
commented
Jan 19, 2016
ruslankhaertdinov
commented
- Related to: [#325] Authentication via social networks (with schema) fs/rails-base#399
- Markdown preview
|
@timurvafin @MaximLarionov take a look please) |
There was a problem hiding this comment.
Retrieve providers list from OmniAuth.strategies
ruslankhaertdinov
force-pushed
the
authentication-via-social-networks
branch
2 times, most recently
from
2837724 to
01814ea
Compare
|
👌 good work on this |
| class UsersController < ApplicationController | ||
| before_action :authenticate_user!, only: :home | ||
|
|
||
| expose(:user, attributes: :user_params) |
There was a problem hiding this comment.
@MaximLarionov Main issue that we pass direct id into url. In this case user can manually change id param and update foreign users email and password attributes.
What do you think, maybe we should pass some token (e.g. already generated confirmation_token or something else) instead of id?
There was a problem hiding this comment.
@ruslankhaertdinov right, I guess we can use devise generated tokens for this
|
@MaximLarionov And another one question: When Oauth confirmed, when user not found by UID or email, we don't create an identity, could you please clarify why (don't create)? |
ruslankhaertdinov
changed the title
|
@ruslankhaertdinov we actually should create both User and identity, if Oauth is confirmed, and user is not found. Moreover, User account should be already confirmed after creation in this case. |
|
Ok, thanks. I'll update PR accordingly. |
|
@MaximLarionov This is updated part of Schema. Could you please confirm that it is correct?
|
|
@ruslankhaertdinov yup, it's correct. Everything else seems good. p.s. - wdyt, maybe we shouldn't allow unconfirmed OAuth to be used anywhere? |
|
@MaximLarionov Agreed. Handling unconfirmed OAuth is very complex and hard to understand. Lets try to simplify this case and only show alert message for the user (with unconfirmed oauth). |
|
@ruslankhaertdinov In case of creating "totally new" user from confirmed OAuth, his password at the moment of creating is just a random set of symbols, because Devise won't allow us to create user with empty password. And user doesn't know this password. I see 2 ways of working with this case:
First is more user-friendly, second is much more simple. |
|
@MaximLarionov In case when Devise configured to prompt current password on password changing, I think sending "change forgotten password" email is more universal approach. What do you think? |
|
@ruslankhaertdinov Yup, this seems to be more universal, guess that will work 👍 |
|
Thank you!)) |
|
|
|
@MaximLarionov Может "Change password email" вообще не нужно отправлять? Ведь пользователь всегда может нажать ссылку "Forgot password" и получить инструкцию на почту? |
|
@ruslankhaertdinov по сути, можно и так сделать. p.s. Диаграмма норм 👍 |
ruslankhaertdinov
force-pushed
the
authentication-via-social-networks
branch
from
a57c899 to
8367272
Compare
|
@MaximLarionov переделал под новую схему, посмотри как будет время, пожалуйста ;) |
ruslankhaertdinov
force-pushed
the
authentication-via-social-networks
branch
from
8367272 to
78f3ef7
Compare
|
@ruslankhaertdinov еще пара мест и и все классно) |
| include OmniauthHelper | ||
|
|
||
| Identity::PROVIDERS.each do |provider| | ||
| define_method(provider.to_s) do |
There was a problem hiding this comment.
String-cast is unnessesary
define_method provider do
|
@MaximLarionov исправил) |
|
@ruslankhaertdinov теперь все классно) |
|
@ruslankhaertdinov let's fix conflicts, I guess it's already time to merge it |
|
@MaximLarionov sure, I'll do it today |
| class ConnectIdentity | ||
| attr_reader :user, :auth | ||
| private :user, :auth | ||
|
|
There was a problem hiding this comment.
Seems we should include Interactor here
| end | ||
|
|
||
| def update_identity | ||
| identity.update_attribute(:user, user) |
There was a problem hiding this comment.
Why do you use #update_attribute here?
There was a problem hiding this comment.
I think I should bethink whole logic here)
|
|
||
| Identity::PROVIDERS.each do |provider| | ||
| define_method(provider) do | ||
| show_verification_notice and return unless auth_verified? |
There was a problem hiding this comment.
What's the reason to write all this logic in one line? It's so difficult to understand (at least for me) what is going on here.
There was a problem hiding this comment.
Yes, this one is not simple. Let me to refactor it.
|
|
||
| def destroy | ||
| action = indentity.destroy ? :notice : :alert | ||
| flash[action] = t "flash.actions.destroy.#{action}", resource_name: Identity.model_name.human |
There was a problem hiding this comment.
Can we use flash responder here instead of manually set flash message?
There was a problem hiding this comment.
Thanks, I'll revise it.
There was a problem hiding this comment.
Done (this is my own reminder)
| end | ||
|
|
||
| def create_identity | ||
| user.identities.create!(provider: auth.provider, uid: auth.uid) |
There was a problem hiding this comment.
I think we should handle result of this operation.
There was a problem hiding this comment.
Could you please tell me why should we handle the result?
| end | ||
|
|
||
| def call | ||
| user = User.new(user_params) |
There was a problem hiding this comment.
def call
User.create!(user_params)
end
def user_params
{
email: auth.info.email,
full_name: auth.info.name,
password: Devise.friendly_token.first(8),
confirmed_at: Time.current
}
endThere was a problem hiding this comment.
Also auth hashes for each provider are different. I suggest to have parsers for each provider to properly get data from auth
There was a problem hiding this comment.
For given providers seems like it works well. May be add parser only when it necessary?
There was a problem hiding this comment.
@KirillKayumov I believe you've had an example of implemented and well-structured architecture, which provides parsers for corresponding providers.
Could you please share a link? :)
From what I remember, that solution was quite good and worthy to implement here
| end | ||
|
|
||
| def user_found_by_email | ||
| FindUserByEmail.new(auth).call |
There was a problem hiding this comment.
Here and in #new_user context of interactor is returned. I guess we should return a user.
There was a problem hiding this comment.
There are POROs which return user instance. I'll redo it when implement them as interactors.
| def call | ||
| return unless user | ||
|
|
||
| create_identity |
There was a problem hiding this comment.
This interactor is called FindUserByEmail. But it does additional things, such as Identity creating and user confirming. Seems it should be converted to an organizer.
| ```ruby | ||
| # app/models/identity.rb | ||
| class Identity < ActiveRecord::Base | ||
| PROVIDERS = OmniAuth.strategies.map { |s| s.to_s.demodulize.underscore }.drop(1) |
There was a problem hiding this comment.
I don't think we should do such complex converting of strategies. Instead let's explicitly specify list of available providers.
There was a problem hiding this comment.
I'm not sure how exactly it should be. Current case not so complex, I guess. @MaximLarionov what do you think?)
There was a problem hiding this comment.
Guess that the best way would be, if oauth gem could fetch all conventional names of used strategies simple way.
If gem can't do that, guess we'll have to go with explicit way, especially if strategies naming doesn't follow some conventions(could possibly lead to name errors).
|
|
||
| ```ruby | ||
| # app/views/identities/_provider_link.html.slim | ||
| = link_to "#{provider_name(identity.provider)} (#{identity.uid.truncate(9)}). Unauthorize?", |
There was a problem hiding this comment.
Do we really need to include uid in link text?
There was a problem hiding this comment.
This is only for demo cases when we have multiple oauth accounts with 1 provider (e.g. Facebook)
There was a problem hiding this comment.
Just wondering if it's a good idea - to attach multiple identities from single kind of provider, to one account?
It feels to be an excessive feature, actually :)
Also, bare text in views should be preferably replaced with I18n translations
| ```ruby | ||
| # config/initializers/devise.rb | ||
| ... | ||
| config.omniauth :google_oauth2, ENV["GOOGLE_CLIENT_ID"], ENV["GOOGLE_CLIENT_SECRET"] |
There was a problem hiding this comment.
Please use #fetch to get env variables
| class CreateIdentities < ActiveRecord::Migration | ||
| def change | ||
| create_table :identities do |t| | ||
| t.belongs_to :user, index: true |
There was a problem hiding this comment.
I think we need a foreign key here
| def change | ||
| create_table :identities do |t| | ||
| t.belongs_to :user, index: true | ||
| t.string :provider, index: true, null: false, default: "" |
There was a problem hiding this comment.
Value of provider and uid columns should never be empty string. Let's get rid of default values for these columns.
| ```ruby | ||
| # spec/factories/users.rb | ||
| ... | ||
| trait :from_auth_hashie do |
| end | ||
|
|
||
| def click_connect_fb | ||
| login_as(user, scope: :user) |
There was a problem hiding this comment.
This helper does SO MUCH. Can we move login and visit to background?
| let(:user) { create(:user, email: auth_hashie.info.email, confirmed_at: nil) } | ||
|
|
||
| it "confirms user" do | ||
| expect(user.confirmed?).to be_falsey |
There was a problem hiding this comment.
Can we use expect {}.to change {} here?
|
|
||
| it "raises Exception" do | ||
| expect { subject } | ||
| .to raise_error(AuthVerificationPolicy::OauthError, I18n.t("omniauth.verification.not_implemented", kind: provider)) |
There was a problem hiding this comment.
yes... need to refactor it
ruslankhaertdinov
left a comment
ruslankhaertdinov
left a comment
There was a problem hiding this comment.
Thanks a lot for detailed review 👍
|
|
||
| def destroy | ||
| action = indentity.destroy ? :notice : :alert | ||
| flash[action] = t "flash.actions.destroy.#{action}", resource_name: Identity.model_name.human |
There was a problem hiding this comment.
Thanks, I'll revise it.
|
|
||
| Identity::PROVIDERS.each do |provider| | ||
| define_method(provider) do | ||
| show_verification_notice and return unless auth_verified? |
There was a problem hiding this comment.
Yes, this one is not simple. Let me to refactor it.
| class ConnectIdentity | ||
| attr_reader :user, :auth | ||
| private :user, :auth | ||
|
|
| end | ||
|
|
||
| def create_identity | ||
| user.identities.create!(provider: auth.provider, uid: auth.uid) |
There was a problem hiding this comment.
Could you please tell me why should we handle the result?
| end | ||
|
|
||
| def call | ||
| user = User.new(user_params) |
There was a problem hiding this comment.
For given providers seems like it works well. May be add parser only when it necessary?
| def change | ||
| create_table :identities do |t| | ||
| t.belongs_to :user, index: true | ||
| t.string :provider, index: true, null: false, default: "" |
| ```ruby | ||
| # spec/factories/users.rb | ||
| ... | ||
| trait :from_auth_hashie do |
| end | ||
|
|
||
| def click_connect_fb | ||
| login_as(user, scope: :user) |
| let(:user) { create(:user, email: auth_hashie.info.email, confirmed_at: nil) } | ||
|
|
||
| it "confirms user" do | ||
| expect(user.confirmed?).to be_falsey |
|
|
||
| it "raises Exception" do | ||
| expect { subject } | ||
| .to raise_error(AuthVerificationPolicy::OauthError, I18n.t("omniauth.verification.not_implemented", kind: provider)) |
There was a problem hiding this comment.
yes... need to refactor it
|
|
||
| private | ||
|
|
||
| def show_verification_notice |
There was a problem hiding this comment.
can be named as show_verification_error, since you actually return an error notice
|
@MaximLarionov @KirillKayumov there are no enough time to apply all comments rapidly, so I'll refactor app step by step, thanks advance for understanding. |
|
@ruslankhaertdinov sure, that's not a rush, or something, take your time surely. Also, you might add me as a collaborator to your fork, so I can do some pull-requests straight to your fork with fixes. |
3 participants
