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

PLANNING ISSUE:Facebook Authentication #2388

Closed
12 of 15 tasks
SidharthBansal opened this issue Feb 23, 2018 · 16 comments
Closed
12 of 15 tasks

PLANNING ISSUE:Facebook Authentication #2388

SidharthBansal opened this issue Feb 23, 2018 · 16 comments
Assignees
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration enhancement explains that the issue is to improve upon one of our existing features feature explains that the issue is to add a new feature has-pull-request issues which already have a pull request solving it in-progress more-detail-please issue lacks proper description and perhaps needs code links or the location of the problem planning Planning issues! summer-of-code

Comments

@SidharthBansal
Copy link
Member

SidharthBansal commented Feb 23, 2018

Planning issues for #683

  • Add omniauth gem
  • Add omniauth-facebook gem
  • Add fiagro gem
  • Define routes
  • Add OmniAuth Configuration to initializer
  • Install Openssl
  • Add OmniAuth Capability to User Model
  • User_session_controller update
  • Views update for login
  • Views update for sign in
  • Write tests for the login through fb
  • Write Documentation for the fiagro gem
  • Write Documentation for omniauth
  • Write Documentaion for the omniauth-facebook
  • Write Documentaion for openssl
@jywarren jywarren added enhancement explains that the issue is to improve upon one of our existing features break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration labels Feb 23, 2018
@jywarren jywarren added this to the Social media integration milestone Feb 23, 2018
@jywarren
Copy link
Member

This is awesome!!! 👍 👍 👍 @ebarry

What is the minimum chunk of this we could do in a single PR? Perhaps the gems, plus OmniAuth in initializer and user model, and routes, but not changing the existing login sequence or views YET. What do you think? All the back-end stuff.

@jywarren
Copy link
Member

Sorry, i don't mean put it all back into one big PR. Just that we can do some things that don't affect current workflows (like login, signup) and merge them in with tests, and know that they work well. Then in a followup PR we can start "Turning it on" for people logging into the site for real -- exposing the back-end work. Does this make sense?

@jywarren
Copy link
Member

Like, we could potentially enable this for login first, but not for signup. That would let us test this progressively in chunks.

One question is -- will existing users be able to log in with Facebook to their pre-existing accounts? Just wondering.

Thanks so much!

@SidharthBansal
Copy link
Member Author

Sorry I could not do much last two weeks due to my mid sems.
Yeah, we will be doing this into chunks of separate prs so that it will be a quick, easy and a small merge.

@SidharthBansal
Copy link
Member Author

Regarding the tests. I think we will be able to write them at the end of the full functioning of login and signup

@SidharthBansal
Copy link
Member Author

First of all, I will make a simple facebook login compatible with the current publiclab.org
Then I shall add functionality so that the users who have the facebook account or the login on public labs directly can fetch their same profile in either way around.
I hope this makes sense.

@SidharthBansal
Copy link
Member Author

Then in a followup PR we can start "Turning it on" for people logging into the site for real -- exposing the back-end work
Yeah first of all we will test all the functionality on the local then on production, then we will make the frontend so that new users will be able to start logging in from there.

@jywarren
Copy link
Member

jywarren commented Mar 12, 2018 via email

@SidharthBansal
Copy link
Member Author

Yeah

@jywarren
Copy link
Member

Just noting I think we could add some unit tests possibly after Add OmniAuth Capability to User Model -- what do you think?

Love this!

@SidharthBansal
Copy link
Member Author

SidharthBansal commented Mar 23, 2018

@jywarren one important thing I like to discuss which is written in my proposal. If we have a direct sign-up page through provider without registering through the basic sign up form, we will run into many errors. I have listed all of them at https://www.publiclab.org/notes/bansal_sidharth2996/02-20-2018/gsoc-proposal-oauth-authentication. Thus, registration of the users through /signup is compulsory.

@SidharthBansal
Copy link
Member Author

SidharthBansal commented Mar 23, 2018

If a user is able to directly sign up through a provider without registration through /signup and then he deletes his provider account then he will not be able to access his public lab account. Also, Twitter is not providing us with the email addresses, so we can't directly write all stuff into the user model. Another reason is if two providers give you same uid then there would be a conflict hard to resolve. If the public lab would not have any login in the past then it would be much simpler. But that is not the case. We already have a lot of usernames flushed into the production database so there are potential chances of conflicts of the usernames given by the provider and already present in the database.
So, I came up at only one solution that is to make identities which are associated with the user model.
If a user wishes to sign up through provider then first he needs to sign up through the registration form available at /signup then he needs to link his provider's account to the user model through edit settings. Then the person can log in directly to the public labs via the provider. Direct signup via a provider without signing through /signup is not possible.

A user has many identities. An identity belongs to a user. An identity holds information of the provider. So, no need to worry about all the erroneous situations.
This implementation just minimises the fallout.

@SidharthBansal
Copy link
Member Author

SidharthBansal commented Mar 23, 2018

Also, I will like to mention that there is not a fixed solution to multi party login system . we can have multi party login (that does not have any problem ) but multi party sign up (without a registration form)have many issues. Just we can discuss and adopt the strategy which is best over the rest.

I have gone through many websites which already have signed up through multiple providers. Yes, they do suffer from the anomalies which I have list above/ proposal. So, I don't want that those will happen here. Eg. At codechef.com you can login through fb and then unlink your fb account. That will result in an account present in the database but not accessible by anyone. Because the user has destroyed the identity through which he logs in to his account.

@SidharthBansal
Copy link
Member Author

Also, I will like to tell that the integration part needs to be implemented simultaneously through out the project. We could not have integration at the last stage for multi party authentication

@jywarren
Copy link
Member

jywarren commented Mar 30, 2018

Hi, all - in relation to @SidharthBansal 's proposal and discussion of "without a password" state, check out these two lines in the login sequence, which redirect to ask someone to create a password!

flash[:warning] = I18n.t('user_sessions_controller.create_password_for_new_site')
redirect_to '/profile/edit'

@SidharthBansal SidharthBansal added the has-pull-request issues which already have a pull request solving it label Apr 6, 2018
@jywarren jywarren added the planning Planning issues! label May 3, 2018
@SidharthBansal SidharthBansal added summer-of-code more-detail-please issue lacks proper description and perhaps needs code links or the location of the problem feature explains that the issue is to add a new feature labels May 9, 2018
@SidharthBansal SidharthBansal changed the title Facebook Authentication PLANNING ISSUE:Facebook Authentication May 20, 2018
@SidharthBansal SidharthBansal self-assigned this Jun 5, 2018
@SidharthBansal
Copy link
Member Author

Moved to #2676 as code is highly interdependent and interlinked for different providers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration enhancement explains that the issue is to improve upon one of our existing features feature explains that the issue is to add a new feature has-pull-request issues which already have a pull request solving it in-progress more-detail-please issue lacks proper description and perhaps needs code links or the location of the problem planning Planning issues! summer-of-code
Projects
None yet
Development

No branches or pull requests

2 participants