Hotfix twitter login

[ebihara99999]

This is moved from NoamB/sorcery pull request Hotfix twitter login #802. I copy the same request here for readability. If it's not easy to read, leave a comment to delete it.

Thank you for reading, looking forward to a comment.

About the bug

I found a bug with Twitter login with external module reported at the issue #800

How to fix

I tried to fix it by myself, then I add username property to and remove not null constraint of email from users in lib/generators/sorcery/templates/migration/core.rb.

Username is needed when you login with Twitter, but email isn't needed.

Without username property, it raises error(mentioned in issue #800).

Thinking about email, not null constraint in a database layer raises an error
at user.sorcery_adapter.save(:validate => false) in #create_from_provider in lib/sorcery/model/submodules/external.rb.
The error is:

Mysql2::Error: Field 'email' doesn't have a default value: INSERT INTO `users` (`username`, `created_at`, `updated_at`) VALUES ('blablabla', '2016-09-22 15:56:22', '2016-09-22 15:56:22')

Using #save with validate option, expected that a validation of email is ignored. The validation should be imposed only in model.

I wonder why tests don't fail, looking into specs.
In spec/rails_app/db/migrate/core/20101224223620_create_users.rb, different from the migration template mentioned above, defined are columns username and email without not null constraint.

I think users defined in tests is expected, and I fix in this way.

How to test

I run all tests by bundle exec rpsec.
I also confirmed expected migration template as follows:

class SorceryCore < ActiveRecord::Migration
  def change
    create_table :users do |t|
      t.string :email
      t.string :username
      t.string :crypted_password
      t.string :salt

      t.timestamps
    end

    add_index :users, :email, unique: true
  end
end

I'm looking forward to a comment. Thank you for reading.

[Ch4s3]

I think it might make sense to not require email, but I need to think this through. @athix what do you think?

[joshbuker]

Personally I think email should always be required by default, as it's how you verify who someone is for the most part, and send them important messages like account resets. I would first investigate the documentation and specs to make sure they make sense before removing the not null constraint.

[Ch4s3]

Yeah, lets look into the Twitter docs and see what we can/should do.

[ebihara99999]

I should've needed unique index on username. The last commit is the correction.

[Ch4s3]

I'm still up in the air about how best for us to approach username and e-mail. @athix do you have any further thoughts?

[joshbuker]

As far as I'm aware, twitter oauth shouldn't care about your application's usernames (user logs in via twitter and oauth returns your app the UID & meta on success, sorcery associates the UID with an auth record then logs the appropriate user in). I think there probably is a bug somewhere in here, but I think it's with the documentation or the twitter implementation rather than the user migration. I would look into the problem myself, but unfortunately i'm pretty tied up between work and school at the moment and don't know if I have the time.

As @ebihara99999 pointed out, sorcery's test suites are pretty inconsistent at the moment, and I know for a fact that the external providers aren't being properly tested. (I broke linkedin and the suite didn't fail because it only tests twitter for oauth1 providers for example) The process I would follow is:

1. Try to replicate the issue @ebihara99999 is seeing by following his procedure.
2. Go through the test suite and correct inconsistencies

Sorcery is pretty flexible, in @ebihara99999's case, it might be a thing where they really do need the not-null constraint removed. In that case, I would recommend installing sorcery then removing the constraint from his application's migration, rather than changing sorcery's generator.

EDIT: undefined method 'username=' for #<User:0x007f01e80e6c08> this error is likely just the column missing from the user migration, and Sorcery trying to set username via @current_user.username = params[:username] (or similar). Not sure on the email not being set properly, but I believe this reaffirms my thoughts on it being a documentation or twitter provider problem.

[ebihara99999]

Thank you @athix for the explanation.
I've looked over the config file (config/initializers/sorcery.rb) again, and I notice setting user_info_mapping improperly.
In wiki it says config.twitter.user_info_mapping = {:username => "screen_name"}, but it is needed to set config.twitter.user_info_mapping = {:email => "screen_name"} if just after implementing "Simple Password Authentication" in wiki.

So I think it is a documentation problem as @athix said. I will fix the external page in wiki and send a pull request.

In addition to documentation, I'm wondering how user activation works when a screen_name is set to email. I think I need to check it.

Try to replicate the issue @ebihara99999 is seeing by following his procedure.

About this I create a rails app to reproduce a bug in sorcery-presentation-app on the replicate-bug branch. I'm happy if it's easier to trace the bug.

EDIT:
I would like to correct test inconsistencies if I can (I'm not yet a well-trained programmer, it would take long or at worst I can't...)

[ebihara99999]

Thinking about test inconsistencies, I need suggestion about where to go.
Now in specs, constraints on username and email are as follows:

t.string :username,         :null => false
t.string :email,            :default => nil

For now, I tested in 3 ways:

• email with not null constraint
• username removed (in the way I modified the external page in wiki)
• username without not null constraint

In the firs case, the results are "354 examples, 27 failures, 2 pending", and these errors in the 3 files:

• ./spec/shared_examples/user_oauth_shared_examples.rb
• ./spec/controllers/controller_oauth2_spec.rb
• ./spec/shared_examples/user_shared_examples.rb

The messages are like this:

Failure/Error: create_new_external_user provider
     ActiveRecord::StatementInvalid:
       SQLite3::ConstraintException: NOT NULL constraint failed: users.email: INSERT INTO "users" ("username", "created_at", "updated_at") VALUES (?, ?, ?)
     # ./lib/sorcery/adapters/active_record_adapter.rb:14:in `save'
     # ./lib/sorcery/test_helpers/internal.rb:40:in `create_new_external_user'
     # ./spec/controllers/controller_oauth2_spec.rb:268:in `block (4 levels) in <top (required)>'

The second case, the results are "354 examples, 138 failures, 214 passed, 2 pending" caused by
ActiveRecord::UnknownAttributeError: unknown attribute 'username' for User..

The last case: No failure.

Which case do you think the schema is expected to be?
Considering discussion above, email needs to be required. But how about username?

[joshbuker]

I'd say username is an optional field for most applications. If we could get it working without a not null constraint, I think that would be ideal.

[Ch4s3]

I agree.

[ebihara99999]

I fix the test inconsistency about username and email constraints on users table

• Email: not null constraint
• Username: optional

If screen_name maps to username and the user model has the email attribute, then it saves an empty string to email.
This makes it possible for users to choose screen_name mapping type flexibly: choose screen_name mapping to email as before , or to username.
Personally I think it's better than mapping screen name to email (the email attribute should not be saved by username).

[Ch4s3]

@athix I think this is good to go, do you agree?

[tetsuya]

Hey guys, sorry for interrupting. I've been waiting for this change.

Just noticed the first user can be created with email blank but not for the second one because of the unique constraint on email column.

Take a look at this case.

context 'without email' do
  it 'sets an empty string to email' do
    expect do
      User.create_from_provider('twitter', '123', username: 'first user') { true }
    end.to change { User.count }.by(1)

    expect(User.first.email).to eq ''

    expect do
      User.create_from_provider('twitter', '456', username: 'second user') { true }
    end.to change { User.count }.by(1)

    expect(User.first.email).to eq ''
  end
end

This will fail as follow

Failures:

  1) User with no submodules (core) external users it should behave like external_user .create_from_provider without email sets an empty string to email
     Failure/Error: @model.send(mthd, options)

     ActiveRecord::RecordNotUnique:
       SQLite3::ConstraintException: UNIQUE constraint failed: users.email: INSERT INTO "users" ("username", "email", "created_at", "updated_at") VALUES (?, ?, ?, ?)

[Ch4s3]

@tetsuya Interesting. This seems like a tricky issue. The model validations need to include allow_nil: true.

[joshbuker]

Seems like email should be set to nil rather than '', although there's still the issue of nothing being mapped to email. You could always use the build_from method and ask for email after the callback, but it would break create_from for any providers that don't have a mapping for email..

[ebihara99999]

I agree that it's not good to set '', only to avoid errors caused by not-null constraint.

Another plan to deal with this problem, seen in case of devise, is to prepare a method that sets a random unique value like "#{uuid}@example.com" to email.

But none of them, including my solution, are best. Setting null causes an error, and setting unique random email is confusing because which of record's email is valid, really able to use email to send notification.

My solution is not best but less confusing: making sure which email is really valid and avoiding an error.

[ebihara99999]

Introducing a user to register email after callbacks seems a very good idea, but it's not role of sorcery...
Could you explain more about using #build_from, @athix ?

[joshbuker]

Yeah, here's a (admittedly bad, I still need to clean this up) example of using build_from in your oauth controller: oauth_controller.rb

[ebihara99999]

@athix
Thanks, I read the oauth_controller.rb and the #build_from definition. #build_from can't be used for the first registration(creation), and always using #build_from seems difficult, if I understand correctly what you meant by "You could always use the build_from method...".

But asking for email registration is very good idea, setting "" to email makes it realize.
It makes it easier to recognize which email is not valid and which user needs to be asked for email.
Also it enables to prompt sorcery users to implement conduction to update email in a rails application if email is "". This looks more convenient than setting random unique email and forcing to judge which email is not valid, needless to say, and than setting null and causing an error.

Of course, in the external documentation, need to write about asking for email after registration if email is "".

[ebihara99999]

I'm sorry I made mistake: '' breaks the unique contraint. This change doesn't work in some cases, and don't merge this change now please.
I will offer an alternative solution soon.

[Ch4s3]

@ebihara99999 let us know when you have a fix

[ebihara99999]

@Ch4s3
I would like to discuss something.

I will make a method that generates a random email, not making up which to implement email as
"#{SecureRandom.uuid}@example.com" or "#{provider}-#{uid}@example.com".
It might be the better way to use "#{provider}-#{uid}@example.com", but the problem is, in wiki #create_from_provider doesn't have the argument of uid. If I use uid to generate a random email, it breaks backward compatibility.

Then I would like to adopt "#{SecureRandom.uuid}@example.com". Do you have some suggestion?

[Ch4s3]

I could probably live with "#{SecureRandom.uuid}@example.com" if @athix is ok with it.

[ebihara99999]

I'd appreciate it if someone could give suggestion. One test case failed, but it isn't replicated on my local computer. And it doesn't seem to be relevant to what I changed.

[Ch4s3]

@ebihara99999 I'll try to take a look in the next day or so.

[joshbuker]

@ebihara99999 You can use build_from for creation of new accounts, it's just that the flow would require the user first authenticating with the oauth provider (i.e. twitter), then being returned to a form where they can enter in the rest of their data (e.g. email, username, etc). The user account is created after submitting that last form.

Personally, I'm against generating fake emails to satisfy the null: false constraint on email. I believe applications should do one of the following: override email to be null: true, or require all oauth providers to supply email (for twitter, see #83), or use the build_from method and an appropriate registration flow. For an example of how this flow works in practice, see: CCSK registration.

[joshbuker]

@ebihara99999 I think this still isn't quite ready for being merged, and unfortunately have run low on time to spend working on Sorcery. I'll probably leave this for a later release, but if you want to jump on IM or a video call to discuss this I could set aside some time to help with the issue.

[ebihara99999]

@athix It's OK to step this onto a later release. I don't have time to spend on this issue, either. Need time to catch up this issue...

[joshbuker]

@ebihara99999 is this still an issue with the codebase, or can this be fixed with a documentation update? For the v1 update I plan on refreshing all of the documentation to be current with Rails 6 and other current best practices.

[ebihara99999]

@athix
Sorry it's been long.

It's the issue of both codebase and documentation.

As for codebase, it's needed to implement a feature to set unique value into email column in '#create_from_provider. Omniauth-twitter gem requires user to implement the feature by themselves, but in case of Sorcery, it must be done in '#create_from_provider because a user is created there.

About documentation, uid must be passed to #create_from_provider at the callback phase ( wiki says @user = create_from(provider) ). uid will be used for constructing an email value.

ref: #6 (comment)

[joshbuker]

Looking at the omniauth-twitter implementation, it looks like they require the email when authenticating to Twitter. This is inline with what I was thinking previously, so what I'll do is update the Twitter implementation on v1 to require email by default. This should resolve the issue of using create_from_provider via Twitter going forward.

See: Line 19 and Line 35 of the omniauth-twitter implementation.

&include_email=true