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

Return helpful error message when attempting to login/signup w Twitter oauth with no email #8734

Merged
merged 12 commits into from
Dec 1, 2020

Conversation

jywarren
Copy link
Member

@jywarren jywarren commented Nov 11, 2020

Fixes #8325 with a test - we may need to remove uid as well!

Part 1

We need to modify this logic to account for not having a Twitter email:

Both of these pathways could lead to this same error:

if User.where(email: auth["info"]["email"]).empty?
# Create a new user as email provided is not present in PL database
user = User.create_with_omniauth(auth)
WelcomeMailer.notify_newcomer(user).deliver_now
@identity = UserTag.create_with_omniauth(auth, user.id)
key = user.generate_reset_key
@user_session = UserSession.create(@identity.user)
@user = user
# send key to user email
PasswordResetMailer.reset_notify(user, key).deliver_now unless user.nil? # respond the same to both successes and failures; security
if session[:openid_return_to] # for openid login, redirects back to openid auth process
return_to = session[:openid_return_to]
session[:openid_return_to] = nil
redirect_to return_to + hash_params
elsif params[:return_to] && params[:return_to].split('/')[0..3] == ["", "subscribe", "multiple", "tag"]
flash[:notice] = "You are now following '#{params[:return_to].split('/')[4]}'."
subscribe_multiple_tag(params[:return_to].split('/')[4])
redirect_to '/dashboard', notice: "You have successfully signed in. Please change your password using the link sent to you via e-mail."
else
redirect_to "/dashboard", notice: "You have successfully signed in. Please change your password using the link sent to you via e-mail."
end
else # email exists so link the identity with existing user and log in the user
user = User.where(email: auth["info"]["email"])
# If no identity was found, create a brand new one here
@identity = UserTag.create_with_omniauth(auth, user.ids.first)

And if there's no email provided, perhaps we should divert away here too:

else # not signed in
# User U has Provider P linked to U. U has email E1 while P has email E2. So, User table can't find E2 provided
# from auth hash, hence U is found by the user of identity having E2 as email
@user = User.where(email: auth["info"]["email"]) ? User.find_by(email: auth["info"]["email"]) : @identity.user
if @user&.status&.zero?

I think this section should divert to the final else if there is no email in the provided Twitter response.

Part 2

We then should try solving for the scenario with the allocator undefined for Proc error from #8325:

if create(value: "oauth:" + auth['provider'] + ":" + auth['uid'], uid: uid, data: auth.to_hash) fails, maybe it's the non-existence of auth['uid'] OR it could be the .to_hash and it's failing to hash something that is too complex to hash? That might be a more subtle one.

def self.create_with_omniauth(auth, uid)
create(value: "oauth:" + auth['provider'] + ":" + auth['uid'],
uid: uid, data: auth.to_hash)
end

@gitpod-io
Copy link

gitpod-io bot commented Nov 11, 2020

@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #8734 (f24df93) into main (4fadc45) will decrease coverage by 0.03%.
The diff coverage is 86.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8734      +/-   ##
==========================================
- Coverage   81.96%   81.93%   -0.04%     
==========================================
  Files         100      100              
  Lines        5901     5933      +32     
==========================================
+ Hits         4837     4861      +24     
- Misses       1064     1072       +8     
Impacted Files Coverage Δ
app/controllers/user_sessions_controller.rb 67.48% <76.92%> (+0.60%) ⬆️
app/models/node.rb 91.47% <92.30%> (+0.01%) ⬆️
app/controllers/notes_controller.rb 84.46% <100.00%> (+0.93%) ⬆️
app/models/drupal_content_type_map.rb 92.59% <100.00%> (ø)
app/models/revision.rb 88.29% <100.00%> (+0.12%) ⬆️
app/services/execute_search.rb 88.88% <0.00%> (-5.56%) ⬇️
app/api/srch/search.rb 66.87% <0.00%> (-3.83%) ⬇️

assert_not_nil request.env['omniauth.auth']
#Sign Up for a new user
post :create
assert_equal "You have successfully signed in. Please change your password by editing your profile in the upper right menu.", flash[:notice]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, we'll have to change this too, from assert_equal "You have successfully signed in. Please change your password using the link sent to you via e-mail.", flash[:notice]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redirect_to '/dashboard', notice: "You have successfully signed in. Please change your password using the link sent to you via e-mail."
else
redirect_to "/dashboard", notice: "You have successfully signed in. Please change your password using the link sent to you via e-mail."

@jywarren jywarren requested a review from a team as a code owner November 17, 2020 16:35
@@ -89,9 +89,17 @@ def handle_social_login_flow(auth)
elsif params[:return_to] && params[:return_to].split('/')[0..3] == ["", "subscribe", "multiple", "tag"]
flash[:notice] = "You are now following '#{params[:return_to].split('/')[4]}'."
subscribe_multiple_tag(params[:return_to].split('/')[4])
redirect_to '/dashboard', notice: "You have successfully signed in. Please change your password using the link sent to you via e-mail."
if user.email
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid deeply nested control flow statements.

else
redirect_to "/dashboard", notice: "You have successfully signed in. Please change your password using the link sent to you via e-mail."
if user.email
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid deeply nested control flow statements.

@jywarren
Copy link
Member Author

So one big unknown is whether we can create User records with no email in the first place. Many things on the site may break, i suppose... maybe we should enter a dummy email?

@jywarren
Copy link
Member Author

I'll write a unit test for it...

@jywarren
Copy link
Member Author

Other than the user email issue, i see:

ERROR["test_sign_up_and_login_via_provider_basic_flow_for_twitter_user_with_no_email", #<Minitest::Reporters::Suite:0x0000000010adeb10 @name="UserSessionsControllerTest">, 26.474472665999997]
 test_sign_up_and_login_via_provider_basic_flow_for_twitter_user_with_no_email#UserSessionsControllerTest (26.47s)
Minitest::UnexpectedError:         NoMethodError: undefined method `tr' for nil:NilClass
            app/models/user.rb:445:in `create_with_omniauth'
            app/controllers/user_sessions_controller.rb:77:in `handle_social_login_flow'
            app/controllers/user_sessions_controller.rb:11:in `create'
            test/functional/user_sessions_controller_test.rb:180:in `block in <class:UserSessionsControllerTest>'

Indeed, this line means we need an email address! https://github.com/publiclab/plots2/blob/main/app/models/user.rb#L445

      email_prefix = auth["info"]["email"].tr('.', '_').split('@')[0]
      email_prefix = auth["info"]["email"].tr('.', '_').split('@')[0] + random_chars until User.where(username: email_prefix).empty?

Maybe we should just throw an error saying we require an email in your Twitter account to use it to create an account?

@jywarren
Copy link
Member Author

Confirming that removing email address for a test user broke lots of notifications stuff. We'd have to come up with alternative ways for all those features to work...

@jywarren
Copy link
Member Author

OK we are clear (from PL staff) to refusing Twitter OAuth account creation from Twitter accounts that don't have emails - and prompting people to create an email to get around this. Thanks!

@user_session = UserSession.create(@identity.user)
@user = user
# send key to user email
PasswordResetMailer.reset_notify(user, key).deliver_now unless user.nil? # respond the same to both successes and failures; security
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid deeply nested control flow statements.

@user = user
# send key to user email
PasswordResetMailer.reset_notify(user, key).deliver_now unless user.nil? # respond the same to both successes and failures; security
if session[:openid_return_to] # for openid login, redirects back to openid auth process
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid deeply nested control flow statements.

@user = user
# log in them
@user_session = UserSession.create(@identity.user)
if session[:openid_return_to] # for openid login, redirects back to openid auth process
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid deeply nested control flow statements.

@jywarren
Copy link
Member Author

ERROR["test_sign_up_and_login_via_provider_basic_flow_for_twitter_user_with_no_email", #<Minitest::Reporters::Suite:0x00000000105b26a0 @name="UserSessionsControllerTest">, 87.508377427]
 test_sign_up_and_login_via_provider_basic_flow_for_twitter_user_with_no_email#UserSessionsControllerTest (87.51s)
Minitest::UnexpectedError:         NameError: undefined local variable or method `be_nil' for #<UserSessionsControllerTest:0x00000000105ad8a8>
            test/functional/user_sessions_controller_test.rb:179:in `block in <class:UserSessionsControllerTest>'

@codeclimate
Copy link

codeclimate bot commented Nov 25, 2020

Code Climate has analyzed commit 6dd94ab and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3

View more on Code Climate.

post :destroy
assert_equal "Successfully logged out.", flash[:notice]
#auth hash is present so login via a provider
test 'sign up and login via provider basic flow for twitter user with no email' do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting this is now protected by a specific test for the Twitter-login-with-no-email scenario!

@jywarren jywarren changed the title Attempt a test of Twitter oauth with no email Return helpful error message when attempting to login/signup w Twitter oauth with no email Dec 1, 2020
@jywarren jywarren merged commit a23a3f9 into main Dec 1, 2020
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
…r oauth with no email (publiclab#8734)

* Attempt a test of Twitter oauth with no email

* Update test.rb

* Update user_sessions_controller_test.rb

* change alerts to prompt for email

* try removing email address from a user

* revert removing email from user

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* use assert_nil session[:user_session]
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
…r oauth with no email (publiclab#8734)

* Attempt a test of Twitter oauth with no email

* Update test.rb

* Update user_sessions_controller_test.rb

* change alerts to prompt for email

* try removing email address from a user

* revert removing email from user

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* use assert_nil session[:user_session]
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
…r oauth with no email (publiclab#8734)

* Attempt a test of Twitter oauth with no email

* Update test.rb

* Update user_sessions_controller_test.rb

* change alerts to prompt for email

* try removing email address from a user

* revert removing email from user

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* use assert_nil session[:user_session]
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
…r oauth with no email (publiclab#8734)

* Attempt a test of Twitter oauth with no email

* Update test.rb

* Update user_sessions_controller_test.rb

* change alerts to prompt for email

* try removing email address from a user

* revert removing email from user

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* Update user_sessions_controller.rb

* Update user_sessions_controller_test.rb

* use assert_nil session[:user_session]
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

Successfully merging this pull request may close these issues.

Trouble with Twitter Signup/Login
1 participant