[#325] Authentication via Social Networks by ruslankhaertdinov · Pull Request #360 · fs/rails-base

ruslankhaertdinov · 2015-10-13T09:12:43Z

Related to: #325

Demo: https://auth-via-networks.herokuapp.com/

ruslankhaertdinov · 2015-10-13T09:27:40Z

@MaximLarionov I apply review comments and reopen PR into examples branch, please take a look again.

maxinspace · 2015-10-13T09:59:16Z

app/controllers/users/omniauth_callbacks_controller.rb

not really DRY to repeat one check in the every single action, WDYT about moving this check to callback before_action?

Yep, agree.

…, use before filter and use expose instead of private method

maxinspace · 2015-10-14T12:38:49Z

Moreover, it appears that we partially can't use some social services for oauth.

There is 3 cases of oauth behavior:

Sign in user via finding SocialProfile by uid and provider from auth hash.
Sign in user via finding him by email from auth hash, and create SocialProfile for user.
Create new user via auth hash, and SocialProfile for him.

Usually, auth hash should contain some data, which says, that account from that Social Profile is confirmed/verified by email. But some services doesn't have that data, like LinkedIn or GitHub.
So we can't trust this auth hash data, and we can't use it for authenticating user in second case.

So it seems, that we have to develop strategy for those cases.

ruslankhaertdinov · 2015-10-30T20:08:51Z

@MaximLarionov please take a new look

ruslankhaertdinov · 2015-11-01T19:05:51Z

@timurvafin also kindly please review

ruslankhaertdinov · 2015-11-09T09:49:18Z

@MaximLarionov ping)

maxinspace · 2015-11-09T10:13:51Z

app/interactors/fetch_oauth_user.rb

I guess, we should try to find social profile, before finding by email.

If user has already signed with this social account, then he should have Social Profile, so it's more reasonable to look for it, and if we haven't found one - find by email, and create new social profile for him.

maxinspace · 2015-11-09T10:27:56Z

@ruslankhaertdinov it seems like that's all from my side for current moment :)

ruslankhaertdinov · 2015-11-09T10:30:12Z

@MaximLarionov thanks a lot!)

ruslankhaertdinov · 2015-11-11T20:37:06Z

@MaximLarionov please take a look again, when you have time ;)

maxinspace · 2015-11-18T19:05:20Z

app/interactors/oauth_organizer.rb

&& !found_user_by_email?

do we need this check here?

@MaximLarionov good catch 👍 , seems like this check is redundant here.

…ce, remove unused checks from OauthOrganizer

ruslankhaertdinov · 2015-11-19T19:40:56Z

app/interactors/oauth_organizer.rb

@MaximLarionov what do you think, should I leave it as is, or change logic and replace if auth_verified? with if !user_found_by_email??

Hm, I doubt that we should build something from unverified auth, so auth verification should be kept

timurvafin · 2016-02-04T12:06:08Z

Moved to FlatstackSchool/rails-examples#3

Ruslan Khaertdinov added 2 commits October 13, 2015 12:11

[fs#325] Authentication via Social Networks

caa4d0d

update .ruby-verison

fd23292

maxinspace reviewed Oct 13, 2015
View reviewed changes

apply review comments: move check_omniauth content into policy object…

8d6d669

…, use before filter and use expose instead of private method

try to add services

e4e8549

ruslankhaertdinov force-pushed the authentication-via-social-networks branch 2 times, most recently from 52bef83 to 85ec5a3 Compare October 30, 2015 12:40

apply review comments: extract omniauth logic into separate services

cf69813

ruslankhaertdinov force-pushed the authentication-via-social-networks branch from 85ec5a3 to cf69813 Compare October 30, 2015 19:21

Ruslan Khaertdinov added 2 commits October 30, 2015 22:46

rework oauth_organizer and fetch_oauth_user classes

1407201

extend user_spec

2690d11

maxinspace reviewed Nov 9, 2015
View reviewed changes

apply review comments: revise code

95fa19c

ruslankhaertdinov force-pushed the authentication-via-social-networks branch from 1312fdd to e92ad7b Compare November 11, 2015 20:30

apply review comments: refactor feature specs

11a7531

ruslankhaertdinov force-pushed the authentication-via-social-networks branch from e92ad7b to 11a7531 Compare November 11, 2015 20:34

maxinspace reviewed Nov 18, 2015
View reviewed changes

apply review comments: extract user#connect_social_profile into servi…

5552594

…ce, remove unused checks from OauthOrganizer

ruslankhaertdinov reviewed Nov 19, 2015
View reviewed changes

timurvafin closed this Feb 4, 2016

Conversation

ruslankhaertdinov commented Oct 13, 2015

Uh oh!

ruslankhaertdinov commented Oct 13, 2015

Uh oh!

maxinspace Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

ruslankhaertdinov Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

maxinspace commented Oct 14, 2015

Uh oh!

ruslankhaertdinov commented Oct 30, 2015

Uh oh!

ruslankhaertdinov commented Nov 1, 2015

Uh oh!

ruslankhaertdinov commented Nov 9, 2015

Uh oh!

maxinspace Nov 9, 2015

Choose a reason for hiding this comment

Uh oh!

maxinspace commented Nov 9, 2015

Uh oh!

ruslankhaertdinov commented Nov 9, 2015

Uh oh!

ruslankhaertdinov commented Nov 11, 2015

Uh oh!

maxinspace Nov 18, 2015

Choose a reason for hiding this comment

Uh oh!

ruslankhaertdinov Nov 19, 2015

Choose a reason for hiding this comment

Uh oh!

ruslankhaertdinov Nov 19, 2015

Choose a reason for hiding this comment

Uh oh!

maxinspace Nov 25, 2015

Choose a reason for hiding this comment

Uh oh!

timurvafin commented Feb 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants