From 271146f6882f326d19eeb5da10e0f79332716278 Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 13 Jan 2016 11:30:57 +0100 Subject: [PATCH 01/30] Rename Identity method Remove Identity method --- app/models/identity.rb | 9 +-------- app/models/user.rb | 4 ++-- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/app/models/identity.rb b/app/models/identity.rb index 3ba19e3fa..e3704da01 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -4,14 +4,7 @@ class Identity < ActiveRecord::Base validates :provider, presence: true validates :uid, presence: true, uniqueness: { scope: :provider } - def self.find_for_oauth(auth) + def self.first_or_create_from_oauth(auth) where(uid: auth.uid, provider: auth.provider).first_or_create end - - def update_user(new_user) - return unless user != new_user - - self.user = new_user - save! - end end diff --git a/app/models/user.rb b/app/models/user.rb index 8deb423f4..6906f21c3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -55,6 +55,7 @@ class User < ActiveRecord::Base def self.find_for_oauth(auth, signed_in_resource = nil) # Get the identity and user if they exist identity = Identity.find_for_oauth(auth) + identity = Identity.first_or_create_from_oauth(auth) # If a signed_in_resource is provided it always overrides the existing user # to prevent the identity being locked with accidentally created accounts. @@ -63,8 +64,7 @@ class User < ActiveRecord::Base user = signed_in_resource ? signed_in_resource : identity.user user ||= first_or_create_for_oauth(auth) - # Associate the identity with the user if needed - identity.update_user(user) + identity.update(user: user) user end From 5ea8d7690380555426de98d904c9c145e7a296f0 Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 13 Jan 2016 11:32:29 +0100 Subject: [PATCH 02/30] Rename find_for_oauth param and refactor user expression --- app/models/user.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 6906f21c3..8a227b82c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -52,17 +52,14 @@ class User < ActiveRecord::Base before_validation :clean_document_number - def self.find_for_oauth(auth, signed_in_resource = nil) - # Get the identity and user if they exist - identity = Identity.find_for_oauth(auth) + def self.find_for_oauth(auth, current_user) identity = Identity.first_or_create_from_oauth(auth) - # If a signed_in_resource is provided it always overrides the existing user + # If a current is provided it always overrides the existing user # to prevent the identity being locked with accidentally created accounts. # Note that this may leave zombie accounts (with no associated identity) which # can be cleaned up at a later date. - user = signed_in_resource ? signed_in_resource : identity.user - user ||= first_or_create_for_oauth(auth) + user = current_user || identity.user || first_or_create_for_oauth(auth) identity.update(user: user) user From f26268602307521d2cc83e1770835f9796c53579 Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 13 Jan 2016 11:32:52 +0100 Subject: [PATCH 03/30] Allows deploying branches in preproduction Bundle install --- Gemfile.lock | 10 ---------- config/deploy/preproduction.rb | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5b5a67d7c..50d0b6ad2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -357,16 +357,6 @@ GEM sprockets (>= 2.8, < 4.0) sprockets-rails (>= 2.0, < 4.0) tilt (>= 1.1, < 3) - sassc (1.8.3) - bundler - ffi (~> 1.9.6) - sass (>= 3.3.0) - sassc-rails (1.1.0) - rails (>= 4.0.0) - sass - sassc (~> 1.6) - sprockets (> 2.11) - tilt savon (2.11.1) akami (~> 1.2) builder (>= 2.1.2) diff --git a/config/deploy/preproduction.rb b/config/deploy/preproduction.rb index a514f84eb..16d1c5bb7 100644 --- a/config/deploy/preproduction.rb +++ b/config/deploy/preproduction.rb @@ -1,7 +1,7 @@ set :deploy_to, deploysecret(:deploy_to) set :server_name, deploysecret(:server_name) set :db_server, deploysecret(:db_server) -set :branch, :master +set :branch, ENV['branch'] || :master set :ssh_options, port: deploysecret(:ssh_port) set :stage, :preproduction set :rails_env, :preproduction From d1741a2b28a62044ac2af29008df9d4acba576bc Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 13 Jan 2016 12:31:07 +0100 Subject: [PATCH 04/30] Refactor first_or_create_for_auth & its usage Conflicts: app/controllers/users/omniauth_callbacks_controller.rb Refactors first_or_initialize_for_oauth --- .../users/omniauth_callbacks_controller.rb | 42 ++++++++++++------- app/models/user.rb | 42 ++++++------------- 2 files changed, 38 insertions(+), 46 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 423ecedad..16385a5a7 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -1,23 +1,15 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController - def self.provides_callback_for(provider) - class_eval %Q{ - def #{provider} - @user = User.find_for_oauth(env["omniauth.auth"], current_user) - - if @user.persisted? - sign_in_and_redirect @user, event: :authentication - set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? - else - session["devise.#{provider}_data"] = env["omniauth.auth"] - redirect_to new_user_registration_url - end - end - } + def twitter + sign_in_with :twitter end - [:twitter, :facebook, :google_oauth2].each do |provider| - provides_callback_for provider + def facebook + sign_in_with :facebook + end + + def google_oauth2 + sign_in_with :google_oauth2 end def after_sign_in_path_for(resource) @@ -28,4 +20,22 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end end + private + + def sign_in_with(provider) + auth = env["omniauth.auth"] + + identity = Identity.first_or_create_from_oauth(auth) + @user = current_user || identity.user || User.first_or_initialize_for_oauth(auth) + + if @user.save + identity.update(user: @user) + sign_in_and_redirect @user, event: :authentication + set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? + else + session["devise.#{provider}_data"] = env["omniauth.auth"] + redirect_to new_user_registration_url + end + end + end diff --git a/app/models/user.rb b/app/models/user.rb index 8a227b82c..ffdcc1b35 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -52,39 +52,21 @@ class User < ActiveRecord::Base before_validation :clean_document_number - def self.find_for_oauth(auth, current_user) - identity = Identity.first_or_create_from_oauth(auth) - - # If a current is provided it always overrides the existing user - # to prevent the identity being locked with accidentally created accounts. - # Note that this may leave zombie accounts (with no associated identity) which - # can be cleaned up at a later date. - user = current_user || identity.user || first_or_create_for_oauth(auth) - - identity.update(user: user) - user - end - # Get the existing user by email if the provider gives us a verified email. - # If no verified email was provided we assign a temporary email and ask the - # user to verify it on the next step via RegistrationsController.finish_signup - def self.first_or_create_for_oauth(auth) - email = auth.info.email if auth.info.verified || auth.info.verified_email - user = User.where(email: email).first if email - # Create the user if it's a new registration - if user.nil? - user = User.new( - username: auth.info.nickname || auth.extra.raw_info.name.parameterize('-') || auth.uid, - email: email ? email : "#{OMNIAUTH_EMAIL_PREFIX}-#{auth.uid}-#{auth.provider}.com", - password: Devise.friendly_token[0,20], - terms_of_service: '1' - ) - user.skip_confirmation! - user.save! + def self.first_or_initialize_for_oauth(auth) + email, user = nil, nil + if auth.info.verified || auth.info.verified_email + email = auth.info.email + user = User.where(email: email).first if email end - - user + user || User.new( + username: auth.info.nickname || auth.extra.raw_info.name.parameterize('-') || auth.uid, + email: email || "#{OMNIAUTH_EMAIL_PREFIX}-#{auth.uid}-#{auth.provider}.com", + password: Devise.friendly_token[0,20], + terms_of_service: '1', + confirmed_at: Time.now + ) end def name From 981e82fb4d5c93022352b57c6cc8216031092dd1 Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 18 Jan 2016 17:28:15 +0100 Subject: [PATCH 05/30] Refactors ensure_signup_complete action --- app/controllers/application_controller.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 346f6aa34..0080e3af6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -82,10 +82,9 @@ class ApplicationController < ActionController::Base end def ensure_signup_complete - # Ensure we don't go into an infinite loop - return if action_name.in? %w(finish_signup do_finish_signup) - - if user_signed_in? && !current_user.email_provided? + if user_signed_in? && + current_user.pending_finish_signup? && + %w(finish_signup do_finish_signup).exclude?(action_name) redirect_to finish_signup_path end end From f74028e3b58c009bf92e4e9c51caa76d7d6c78ed Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 18 Jan 2016 17:30:21 +0100 Subject: [PATCH 06/30] Replaces the OMNIAUTH_REGEX logic with a boolean --- .../users/omniauth_callbacks_controller.rb | 6 ++--- app/models/user.rb | 24 +++++++++---------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 16385a5a7..eaecb7efc 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -13,10 +13,10 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end def after_sign_in_path_for(resource) - if resource.email_provided? - super(resource) - else + if resource.pending_finish_signup? finish_signup_path + else + super(resource) end end diff --git a/app/models/user.rb b/app/models/user.rb index ffdcc1b35..035c60913 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,6 +1,4 @@ class User < ActiveRecord::Base - OMNIAUTH_EMAIL_PREFIX = 'omniauth@participacion' - OMNIAUTH_EMAIL_REGEX = /\A#{OMNIAUTH_EMAIL_PREFIX}/ include Verification @@ -31,7 +29,6 @@ class User < ActiveRecord::Base validate :validate_username_length validates :official_level, inclusion: {in: 0..5} - validates_format_of :email, without: OMNIAUTH_EMAIL_REGEX, on: :update validates :terms_of_service, acceptance: { allow_nil: false }, on: :create validates :locale, inclusion: {in: I18n.available_locales.map(&:to_s), @@ -42,6 +39,7 @@ class User < ActiveRecord::Base accepts_nested_attributes_for :organization, update_only: true attr_accessor :skip_password_validation + attr_accessor :registering_with_oauth scope :administrators, -> { joins(:administrators) } scope :moderators, -> { joins(:moderator) } @@ -58,14 +56,15 @@ class User < ActiveRecord::Base email, user = nil, nil if auth.info.verified || auth.info.verified_email email = auth.info.email - user = User.where(email: email).first if email + user = User.where(email: email).first end user || User.new( + registering_with_oauth: true, username: auth.info.nickname || auth.extra.raw_info.name.parameterize('-') || auth.uid, - email: email || "#{OMNIAUTH_EMAIL_PREFIX}-#{auth.uid}-#{auth.provider}.com", + email: email, password: Devise.friendly_token[0,20], terms_of_service: '1', - confirmed_at: Time.now + confirmed_at: DateTime.now ) end @@ -149,11 +148,6 @@ class User < ActiveRecord::Base erased_at.present? end - def email_provided? - !!(email && email !~ OMNIAUTH_EMAIL_REGEX) || - !!(unconfirmed_email && unconfirmed_email !~ OMNIAUTH_EMAIL_REGEX) - end - def locked? Lock.find_or_create_by(user: self).locked? end @@ -176,11 +170,11 @@ class User < ActiveRecord::Base end def username_required? - !organization? && !erased? + !organization? && !erased? && !registering_with_oauth end def email_required? - !erased? + !erased? && !registering_with_oauth end def has_official_email? @@ -192,6 +186,10 @@ class User < ActiveRecord::Base self[:locale] ||= I18n.default_locale.to_s end + def pending_finish_signup? + email.blank? && unconfirmed_email.blank? + end + private def clean_document_number self.document_number = self.document_number.gsub(/[^a-z0-9]+/i, "").upcase unless self.document_number.blank? From b71dd5767b0c65143bb86d0277b95fe71431b204 Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 18 Jan 2016 17:31:31 +0100 Subject: [PATCH 07/30] Adds username to the finish_signup form Conflicts: app/views/users/registrations/finish_signup.html.erb --- app/views/users/registrations/finish_signup.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/users/registrations/finish_signup.html.erb b/app/views/users/registrations/finish_signup.html.erb index c57f01a8e..a1e1fd8ac 100644 --- a/app/views/users/registrations/finish_signup.html.erb +++ b/app/views/users/registrations/finish_signup.html.erb @@ -5,6 +5,7 @@ <%= form_for current_user, as: :user, url: do_finish_signup_path, html: { role: 'form'} do |f| %> <%= render 'shared/errors', resource: current_user %> + <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label"), value: nil %> <%= f.email_field :email, placeholder: t("devise_views.users.registrations.new.email_label"), value: nil %> <%= f.submit t("devise_views.users.registrations.new.submit"), class: 'button radius' %> <% end %> From 91a16142bfcec9d5446351bfb36bd984ce406168 Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 18 Jan 2016 17:32:27 +0100 Subject: [PATCH 08/30] Refactors confirm_email & uses it in user_auth_spec --- spec/features/users_auth_spec.rb | 8 +++----- spec/support/common_actions.rb | 7 ++++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index b23d956ac..9fba71df4 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -18,8 +18,7 @@ feature 'Users' do expect(page).to have_content "You have been sent a message containing a verification link. Please click on this link to activate your account." - sent_token = /.*confirmation_token=(.*)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1] - visit user_confirmation_path(confirmation_token: sent_token) + confirm_email expect(page).to have_content "Your account has been confirmed." end @@ -124,8 +123,7 @@ feature 'Users' do fill_in 'user_email', with: 'manueladelascarmenas@example.com' click_button 'Register' - sent_token = /.*confirmation_token=(.*)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1] - visit user_confirmation_path(confirmation_token: sent_token) + confirm_email expect(page).to have_content "Your email address has been successfully confirmed" @@ -134,7 +132,7 @@ feature 'Users' do scenario 'Sign in, user was already signed up with OAuth' do user = create(:user, email: 'manuela@madrid.es', password: 'judgementday') - identity = create(:identity, uid: '12345', provider: 'twitter', user: user) + create(:identity, uid: '12345', provider: 'twitter', user: user) omniauth_twitter_hash = { 'provider' => 'twitter', 'uid' => '12345', 'info' => { diff --git a/spec/support/common_actions.rb b/spec/support/common_actions.rb index 4521a1e9f..437a3c7ca 100644 --- a/spec/support/common_actions.rb +++ b/spec/support/common_actions.rb @@ -36,12 +36,13 @@ module CommonActions end def confirm_email - expect(page).to have_content "A message with a confirmation link has been sent to your email address." + body = ActionMailer::Base.deliveries.last.try(:body) + expect(body).to be_present - sent_token = /.*confirmation_token=(.*)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1] + sent_token = /.*confirmation_token=(.*)".*/.match(body.to_s)[1] visit user_confirmation_path(confirmation_token: sent_token) - expect(page).to have_content "Your email address has been successfully confirmed" + expect(page).to have_content "Your account has been confirmed" end def reset_password From c6c01fc9f286d981ed8d6720c82086d2b5f26a88 Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 19 Jan 2016 19:43:54 +0100 Subject: [PATCH 09/30] Adds 'registering_with_oauth' boolean to users Conflicts: db/schema.rb Changes title in finish_signup page Conflicts: config/locales/en.yml config/locales/es.yml --- config/locales/en.yml | 20 +++++++++++++++++++ config/locales/es.yml | 20 +++++++++++++++++++ ...320_add_registering_with_oauth_to_users.rb | 5 +++++ db/schema.rb | 1 + 4 files changed, 46 insertions(+) create mode 100644 db/migrate/20160119164320_add_registering_with_oauth_to_users.rb diff --git a/config/locales/en.yml b/config/locales/en.yml index 7e0bd1b46..be80cb8a4 100755 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -468,3 +468,23 @@ en: user_permission_verify_info: "* Only for users on Madrid Census." user_permission_verify_url: verify your account user_permission_votes: Participate on final voting + omniauth: + finish_signup: + title: "Additional details" + twitter: + sign_in: "Sign in with Twitter" + sign_up: "Sign up with Twitter" + facebook: + sign_in: "Sign in with Facebook" + sign_up: "Sign up with Facebook" + #google_oauth2: + # sign_in: "Sign in with Google" + # sign_up: "Sign up with Google" + legislation: + help: + title: "How I can comment this document?" + text_logged: "Select the text you want to comment and press the button with the pencil." + text: "To comment this document you must %{sign_in} or %{sign_up}. Then select the text you want to comment and press the button with the pencil." + text_sign_in: "login" + text_sign_up: "sign up" + alt: "Select the text you want to comment and press the button with the pencil." diff --git a/config/locales/es.yml b/config/locales/es.yml index 848a14bf9..9ba8a24ef 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -468,3 +468,23 @@ es: user_permission_verify_info: "* Sólo usuarios empadronados en el municipio de Madrid." user_permission_verify_url: verifica tu cuenta user_permission_votes: Participar en las votaciones finales* + omniauth: + finish_signup: + title: "Detalles adicionales de tu cuenta" + twitter: + sign_in: "Entra con Twitter" + sign_up: "Regístrate con Twitter" + facebook: + sign_in: "Entra con Facebook" + sign_up: "Regístrate con Facebook" + #google_oauth2: + # sign_in: "Entra con Google" + # sign_up: "Regístrate con Google" + legislation: + help: + title: "¿Cómo puedo comentar este documento?" + text_logged: "Selecciona el texto que quieres comentar y pulsa en el botón con el lápiz." + text: "Para comentar este documento debes %{sign_in} o %{sign_up}. Después selecciona el texto que quieres comentar y pulsa en el botón con el lápiz." + text_sign_in: "iniciar sesión" + text_sign_up: "registrarte" + alt: "Selecciona el texto que quieres comentar y pulsa en el botón con el lápiz." diff --git a/db/migrate/20160119164320_add_registering_with_oauth_to_users.rb b/db/migrate/20160119164320_add_registering_with_oauth_to_users.rb new file mode 100644 index 000000000..a29d310ba --- /dev/null +++ b/db/migrate/20160119164320_add_registering_with_oauth_to_users.rb @@ -0,0 +1,5 @@ +class AddRegisteringWithOauthToUsers < ActiveRecord::Migration + def change + add_column :users, :registering_with_oauth, :bool, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index df5bbc3a9..963cce889 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -397,6 +397,7 @@ ActiveRecord::Schema.define(version: 20160122153329) do t.boolean "newsletter", default: false t.integer "notifications_count", default: 0 t.string "locale" + t.boolean "registering_with_oauth", default: false end add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree From a87669840cd8d7f9ffdc4b9b67362dd7e1fd2eac Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 19 Jan 2016 19:46:22 +0100 Subject: [PATCH 10/30] Hides fields without errors in finish_signup.html.erb Conflicts: app/views/users/registrations/finish_signup.html.erb --- .../users/registrations/finish_signup.html.erb | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/app/views/users/registrations/finish_signup.html.erb b/app/views/users/registrations/finish_signup.html.erb index a1e1fd8ac..95e8a8098 100644 --- a/app/views/users/registrations/finish_signup.html.erb +++ b/app/views/users/registrations/finish_signup.html.erb @@ -5,9 +5,20 @@ <%= form_for current_user, as: :user, url: do_finish_signup_path, html: { role: 'form'} do |f| %> <%= render 'shared/errors', resource: current_user %> - <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label"), value: nil %> - <%= f.email_field :email, placeholder: t("devise_views.users.registrations.new.email_label"), value: nil %> - <%= f.submit t("devise_views.users.registrations.new.submit"), class: 'button radius' %> + + <% if current_user.errors.include? :username %> + <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label"), value: nil %> + <% else %> + <%= f.hidden_field :username %> + <% end %> + + <% if current_user.errors.include? :email %> + <%= f.email_field :email, placeholder: t("devise_views.users.registrations.new.email_label"), value: nil %> + <% else %> + <%= f.hidden_field :email %> + <% end %> + + <%= f.submit t("devise_views.users.registrations.new.submit"), class: 'button radius expand' %> <% end %> From 7615dc066ba3204ac8ec58a0c455b8ff0ab4673b Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 19 Jan 2016 19:47:07 +0100 Subject: [PATCH 11/30] Uses registering_with_oauth persistent attribute to deactivate checks in certain occasions --- .../users/omniauth_callbacks_controller.rb | 16 ++++++++++++++-- .../users/registrations_controller.rb | 10 +++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index eaecb7efc..24737777f 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -28,13 +28,25 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController identity = Identity.first_or_create_from_oauth(auth) @user = current_user || identity.user || User.first_or_initialize_for_oauth(auth) + # If there are no problems with the email/username, then they were provided by oauth or they + # correspond to an existing user. Associate the identity and sign in if @user.save identity.update(user: @user) sign_in_and_redirect @user, event: :authentication set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? else - session["devise.#{provider}_data"] = env["omniauth.auth"] - redirect_to new_user_registration_url + # If either the username or email have provoked a failure, we save the user anyway (but marked for revision) + # This mark will be detected by applicationcontroller and the user will be redirected to finish_signup + @user.registering_with_oauth = true + if @user.save + identity.update(user: @user) + sign_in_and_redirect @user, event: :authentication + set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? + else + # If the failure is because something else happens, just present the "new user" form + session["devise.#{provider}_data"] = auth + redirect_to new_user_registration_url + end end end diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index b25418db4..3a564f232 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -24,13 +24,17 @@ class Users::RegistrationsController < Devise::RegistrationsController end def finish_signup + current_user.registering_with_oauth = false + current_user.validate end def do_finish_signup + current_user.registering_with_oauth = false + current_user.validate + should_send_confirmation = current_user.errors.include? :email if current_user.update(sign_up_params) - current_user.skip_reconfirmation! - sign_in(current_user, bypass: true) - redirect_to root_url + current_user.send_confirmation_instructions if should_send_confirmation + sign_in_and_redirect current_user, event: :authentication else render :finish_signup end From 8dcb5d2cac12f73231151e3dbd5413c1ddf4c3ac Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 19 Jan 2016 19:47:28 +0100 Subject: [PATCH 12/30] Refactors self.first_or_initialize_for_oauth --- app/models/user.rb | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 035c60913..2331c0ae6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -51,20 +51,16 @@ class User < ActiveRecord::Base before_validation :clean_document_number # Get the existing user by email if the provider gives us a verified email. - def self.first_or_initialize_for_oauth(auth) - email, user = nil, nil - if auth.info.verified || auth.info.verified_email - email = auth.info.email - user = User.where(email: email).first - end - user || User.new( - registering_with_oauth: true, - username: auth.info.nickname || auth.extra.raw_info.name.parameterize('-') || auth.uid, - email: email, + auth_email = auth.info.email if auth.info.verified || auth.info.verified_email + auth_email_user = User.find_by(email: auth_email) if auth_email.present? + + auth_email_user || User.new( + username: auth.info.nickname || auth.extra.raw_info.name.parameterize('-') || auth.uid, + email: auth_email, password: Devise.friendly_token[0,20], terms_of_service: '1', - confirmed_at: DateTime.now + confirmed_at: auth_email.present? ? DateTime.now : nil ) end From 1a7ca435697bf930bf186d78512abf761c4a5a44 Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 20 Jan 2016 18:14:26 +0100 Subject: [PATCH 13/30] Simplifies username assignation when creating a new oauth user --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 2331c0ae6..bc8faa8a9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -56,7 +56,7 @@ class User < ActiveRecord::Base auth_email_user = User.find_by(email: auth_email) if auth_email.present? auth_email_user || User.new( - username: auth.info.nickname || auth.extra.raw_info.name.parameterize('-') || auth.uid, + username: auth.info.name || auth.uid, email: auth_email, password: Devise.friendly_token[0,20], terms_of_service: '1', From 9318cd8d7e3c51b6847a28452f1b2f2b5afdf842 Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 20 Jan 2016 18:15:02 +0100 Subject: [PATCH 14/30] Refactors twitter hash usage on auth specs --- spec/features/users_auth_spec.rb | 69 +++++++++++--------------------- 1 file changed, 24 insertions(+), 45 deletions(-) diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index 9fba71df4..ea677eb3a 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -46,27 +46,16 @@ feature 'Users' do xcontext 'OAuth authentication' do context 'Twitter' do - background do - #request.env["devise.mapping"] = Devise.mappings[:user] - end + + let(:twitter_hash){ {'provider' => 'twitter', 'uid' => '12345', 'info' => { 'name' => 'manuela' }} } + let(:twitter_hash_with_email) { { 'provider' => 'twitter', + 'uid' => '12345', + 'info' => { 'name' => 'manuela' , 'email' => 'manuelacarmena@example.com', 'verified' => '1' } } + } + scenario 'Sign up, when email was provided by OAuth provider' do - omniauth_twitter_hash = { 'provider' => 'twitter', - 'uid' => '12345', - 'info' => { - 'name' => 'manuela', - 'email' => 'manuelacarmena@example.com', - 'nickname' => 'ManuelaRocks', - 'verified' => '1' - }, - 'extra' => { 'raw_info' => - { 'location' => 'Madrid', - 'name' => 'Manuela de las Carmenas' - } - } - } - - OmniAuth.config.add_mock(:twitter, omniauth_twitter_hash) + OmniAuth.config.add_mock(:twitter, twitter_hash_with_email) visit '/' click_link 'Register' @@ -82,26 +71,15 @@ feature 'Users' do expect(current_path).to eq(root_path) expect_to_be_signed_in - user = User.last - expect(user.username).to eq('ManuelaRocks') - expect(user.email).to eq('manuelacarmena@example.com') - expect(user.confirmed?).to eq(true) + click_link 'My account' + expect(page).to have_field('account_username', with: 'manuela') + + visit edit_user_registration_path + expect(page).to have_field('user_email', with: 'manuelacarmena@example.com') end - scenario 'Sign up, when neither email nor nickname were provided by OAuth provider' do - omniauth_twitter_hash = { 'provider' => 'twitter', - 'uid' => '12345', - 'info' => { - 'name' => 'manuela' - }, - 'extra' => { 'raw_info' => - { 'location' => 'Madrid', - 'name' => 'Manuela de las Carmenas' - } - } - } - - OmniAuth.config.add_mock(:twitter, omniauth_twitter_hash) + scenario 'Sign up, when no email was provided by OAuth provider' do + OmniAuth.config.add_mock(:twitter, twitter_hash) visit '/' click_link 'Register' @@ -127,20 +105,21 @@ feature 'Users' do expect(page).to have_content "Your email address has been successfully confirmed" + visit '/' + click_link 'Sign in' + click_link 'Sign in with Twitter' + expect_to_be_signed_in + + click_link 'My account' + expect(page).to have_field('account_username', with: 'manuela') + expect(user.reload.email).to eq('manueladelascarmenas@example.com') end scenario 'Sign in, user was already signed up with OAuth' do user = create(:user, email: 'manuela@madrid.es', password: 'judgementday') create(:identity, uid: '12345', provider: 'twitter', user: user) - omniauth_twitter_hash = { 'provider' => 'twitter', - 'uid' => '12345', - 'info' => { - 'name' => 'manuela' - } - } - - OmniAuth.config.add_mock(:twitter, omniauth_twitter_hash) + OmniAuth.config.add_mock(:twitter, twitter_hash) visit '/' click_link 'Sign in' From 892612fe9b743ae27ade6e9d69f3174cf312d64c Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 20 Jan 2016 18:15:14 +0100 Subject: [PATCH 15/30] Adds two new scenarios to auth specs --- spec/features/users_auth_spec.rb | 57 ++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index ea677eb3a..831a2b0a5 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -132,6 +132,63 @@ feature 'Users' do expect_to_be_signed_in end + + scenario 'Try to register with the username of an already existing user' do + create(:user, username: 'manuela', email: 'manuela@madrid.es', password: 'judgementday') + OmniAuth.config.add_mock(:twitter, twitter_hash_with_email) + + visit '/' + click_link 'Register' + click_link 'Sign up with Twitter' + + expect(current_path).to eq(finish_signup_path) + + fill_in 'user_username', with: 'manuela2' + click_button 'Register' + + expect_to_be_signed_in + + click_link 'My account' + expect(page).to have_field('account_username', with: 'manuela2') + + visit edit_user_registration_path + expect(page).to have_field('user_email', with: 'manuelacarmena@example.com') + end + + scenario 'Try to register with the email of an already existing user' do + create(:user, username: 'peter', email: 'manuela@example.com') + OmniAuth.config.add_mock(:twitter, twitter_hash) + + visit '/' + click_link 'Register' + click_link 'Sign up with Twitter' + + expect(current_path).to eq(finish_signup_path) + + fill_in 'user_email', with: 'manuela@example.com' + click_button 'Register' + + expect(current_path).to eq(do_finish_signup_path) + + fill_in 'user_email', with: 'somethingelse@example.com' + click_button 'Register' + + expect(page).to have_content "You must confirm your account to continue" + + confirm_email + expect(page).to have_content "Your account has been confirmed" + + visit '/' + click_link 'Sign in' + click_link 'Sign in with Twitter' + expect_to_be_signed_in + + click_link 'My account' + expect(page).to have_field('account_username', with: 'manuela') + + visit edit_user_registration_path + expect(page).to have_field('user_email', with: 'somethingelse@example.com') + end end end From 62ac864d900131bae7def79516f00d5e924274b6 Mon Sep 17 00:00:00 2001 From: kikito Date: Wed, 20 Jan 2016 18:54:43 +0100 Subject: [PATCH 16/30] removes deprecated user specs Conflicts: spec/models/user_spec.rb --- spec/models/user_spec.rb | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7d44a202b..6b65fcb0e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -85,26 +85,6 @@ describe User do end end - describe 'OmniAuth' do - describe '#email_provided?' do - it "is false if the email matchs was temporarely assigned by the OmniAuth process" do - subject.email = 'omniauth@participacion-ABCD-twitter.com' - expect(subject.email_provided?).to eq(false) - end - - it "is true if the email is not omniauth-like" do - subject.email = 'manuelacarmena@example.com' - expect(subject.email_provided?).to eq(true) - end - - it "is true if the user's real email is pending to be confirmed" do - subject.email = 'omniauth@participacion-ABCD-twitter.com' - subject.unconfirmed_email = 'manuelacarmena@example.com' - expect(subject.email_provided?).to eq(true) - end - end - end - describe "administrator?" do it "is false when the user is not an admin" do expect(subject.administrator?).to be false @@ -245,23 +225,23 @@ describe User do end end end - + describe "has_official_email" do it "checks if the mail address has the officials domain" do # We will use empleados.madrid.es as the officials' domain # Subdomains are also accepted + Setting['email_domain_for_officials'] = 'officials.madrid.es' - user1 = create(:user, email: "john@officials.madrid.es", confirmed_at: Time.now) user2 = create(:user, email: "john@yes.officials.madrid.es", confirmed_at: Time.now) user3 = create(:user, email: "john@unofficials.madrid.es", confirmed_at: Time.now) user4 = create(:user, email: "john@example.org", confirmed_at: Time.now) - + expect(user1.has_official_email?).to eq(true) expect(user2.has_official_email?).to eq(true) expect(user3.has_official_email?).to eq(false) expect(user4.has_official_email?).to eq(false) - + # We reset the officials' domain setting Setting.find_by(key: 'email_domain_for_officials').update(value: '') end From 68e1ee2bc2b3ce9808c7c30ae95f766a2379fc92 Mon Sep 17 00:00:00 2001 From: kikito Date: Fri, 22 Jan 2016 10:34:19 +0100 Subject: [PATCH 17/30] synch Gemfile.lock with master --- Gemfile.lock | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Gemfile.lock b/Gemfile.lock index 50d0b6ad2..5b5a67d7c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -357,6 +357,16 @@ GEM sprockets (>= 2.8, < 4.0) sprockets-rails (>= 2.0, < 4.0) tilt (>= 1.1, < 3) + sassc (1.8.3) + bundler + ffi (~> 1.9.6) + sass (>= 3.3.0) + sassc-rails (1.1.0) + rails (>= 4.0.0) + sass + sassc (~> 1.6) + sprockets (> 2.11) + tilt savon (2.11.1) akami (~> 1.2) builder (>= 2.1.2) From 248bff712cb072e4988911c0800df12e431d39d5 Mon Sep 17 00:00:00 2001 From: kikito Date: Fri, 22 Jan 2016 10:56:30 +0100 Subject: [PATCH 18/30] fixes failing i18n specs Eliminates registering_with_oauth attr_accessor ELIMINATE Conflicts: app/views/users/registrations/finish_signup.html.erb improves devise confirmation message improves devise omniauth confirmation message improves devise omniauth confirmation message do not use nils on finish_signup fields Fixes auth specs after changing the i18n --- app/models/user.rb | 1 - .../registrations/finish_signup.html.erb | 36 ++++++++----------- config/locales/devise.en.yml | 2 +- config/locales/devise.es.yml | 8 ++--- config/locales/en.yml | 7 ++-- config/locales/es.yml | 7 ++-- spec/features/users_auth_spec.rb | 4 ++- 7 files changed, 29 insertions(+), 36 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index bc8faa8a9..5fe0e77ea 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,7 +39,6 @@ class User < ActiveRecord::Base accepts_nested_attributes_for :organization, update_only: true attr_accessor :skip_password_validation - attr_accessor :registering_with_oauth scope :administrators, -> { joins(:administrators) } scope :moderators, -> { joins(:moderator) } diff --git a/app/views/users/registrations/finish_signup.html.erb b/app/views/users/registrations/finish_signup.html.erb index 95e8a8098..5542ed6c3 100644 --- a/app/views/users/registrations/finish_signup.html.erb +++ b/app/views/users/registrations/finish_signup.html.erb @@ -1,25 +1,19 @@ -
-
-
-

<%= t('omniauth.finish_signup.title') %>

+

<%= t('omniauth.finish_signup.title') %>

- <%= form_for current_user, as: :user, url: do_finish_signup_path, html: { role: 'form'} do |f| %> - <%= render 'shared/errors', resource: current_user %> +<%= form_for current_user, as: :user, url: do_finish_signup_path, html: { role: 'form'} do |f| %> + <%= render 'shared/errors', resource: current_user %> - <% if current_user.errors.include? :username %> - <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label"), value: nil %> - <% else %> - <%= f.hidden_field :username %> - <% end %> + <% if current_user.errors.include? :username %> + <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label") %> + <% else %> + <%= f.hidden_field :username %> + <% end %> - <% if current_user.errors.include? :email %> - <%= f.email_field :email, placeholder: t("devise_views.users.registrations.new.email_label"), value: nil %> - <% else %> - <%= f.hidden_field :email %> - <% end %> + <% if current_user.errors.include? :email %> + <%= f.email_field :email, placeholder: t("devise_views.users.registrations.new.email_label") %> + <% else %> + <%= f.hidden_field :email %> + <% end %> - <%= f.submit t("devise_views.users.registrations.new.submit"), class: 'button radius expand' %> - <% end %> -
-
-
+ <%= f.submit t("devise_views.users.registrations.new.submit"), class: 'button radius expand' %> +<% end %> diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index 473181f02..a7504ff3d 100755 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -15,7 +15,7 @@ en: not_found_in_database: "Invalid %{authentication_keys} or password." timeout: "Your session has expired. Please sign in again to continue." unauthenticated: "You must sign in or register to continue." - unconfirmed: "You must confirm your account to continue." + unconfirmed: "To continue, please click on the confirmation link that we have sent you via email" mailer: confirmation_instructions: subject: "Confirmation instructions" diff --git a/config/locales/devise.es.yml b/config/locales/devise.es.yml index bf81e4199..9e5ad1f36 100644 --- a/config/locales/devise.es.yml +++ b/config/locales/devise.es.yml @@ -1,7 +1,7 @@ es: devise: confirmations: - confirmed: "Tu cuenta ha sido confirmada." + confirmed: "Tu cuenta ha sido confirmada. Por favor autentifícate con tu red social o tu usuario y contraseña" send_instructions: "Recibirás un correo electrónico en unos minutos con instrucciones sobre cómo restablecer tu contraseña." send_paranoid_instructions: "Si tu correo electrónico existe en nuestra base de datos recibirás un correo electrónico en unos minutos con instrucciones sobre cómo restablecer tu contraseña." failure: @@ -13,7 +13,7 @@ es: not_found_in_database: "%{authentication_keys} o contraseña inválidos." timeout: "Tu sesión ha expirado, por favor inicia sesión nuevamente para continuar." unauthenticated: "Necesitas iniciar sesión o registrarte para continuar." - unconfirmed: "Debes confirmar tu cuenta para continuar." + unconfirmed: "Para continuar, por favor pulsa en el enlace de confirmación que hemos enviado a tu cuenta de correo." mailer: confirmation_instructions: subject: "Instrucciones de confirmación" @@ -22,8 +22,8 @@ es: unlock_instructions: subject: "Instrucciones de desbloqueo" omniauth_callbacks: - failure: "No se te ha podido autorizar de %{kind} debido a \"%{reason}\"." - success: "Identificado correctamente de %{kind}." + failure: "No se te ha podido identificar via %{kind} por el siguiente motivo: \"%{reason}\"." + success: "Identificado correctamente via %{kind}." passwords: no_token: "No puedes acceder a esta página si no es a través de un enlace para restablecer la contraseña. Si has accedido desde el enlace para restablecer la contraseña, asegúrate de que la URL esté completa." send_instructions: "Recibirás un correo electrónico con instrucciones sobre cómo restablecer tu contraseña en unos minutos." diff --git a/config/locales/en.yml b/config/locales/en.yml index be80cb8a4..555b47413 100755 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -477,13 +477,12 @@ en: facebook: sign_in: "Sign in with Facebook" sign_up: "Sign up with Facebook" - #google_oauth2: - # sign_in: "Sign in with Google" - # sign_up: "Sign up with Google" + google_oauth2: + sign_in: "Sign in with Google" + sign_up: "Sign up with Google" legislation: help: title: "How I can comment this document?" - text_logged: "Select the text you want to comment and press the button with the pencil." text: "To comment this document you must %{sign_in} or %{sign_up}. Then select the text you want to comment and press the button with the pencil." text_sign_in: "login" text_sign_up: "sign up" diff --git a/config/locales/es.yml b/config/locales/es.yml index 9ba8a24ef..87de46def 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -477,13 +477,12 @@ es: facebook: sign_in: "Entra con Facebook" sign_up: "Regístrate con Facebook" - #google_oauth2: - # sign_in: "Entra con Google" - # sign_up: "Regístrate con Google" + google_oauth2: + sign_in: "Entra con Google" + sign_up: "Regístrate con Google" legislation: help: title: "¿Cómo puedo comentar este documento?" - text_logged: "Selecciona el texto que quieres comentar y pulsa en el botón con el lápiz." text: "Para comentar este documento debes %{sign_in} o %{sign_up}. Después selecciona el texto que quieres comentar y pulsa en el botón con el lápiz." text_sign_in: "iniciar sesión" text_sign_up: "registrarte" diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index 831a2b0a5..7e874b06c 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -101,6 +101,8 @@ feature 'Users' do fill_in 'user_email', with: 'manueladelascarmenas@example.com' click_button 'Register' + expect(page).to have_content "To continue, please click on the confirmation link that we have sent you via email" + confirm_email expect(page).to have_content "Your email address has been successfully confirmed" @@ -173,7 +175,7 @@ feature 'Users' do fill_in 'user_email', with: 'somethingelse@example.com' click_button 'Register' - expect(page).to have_content "You must confirm your account to continue" + expect(page).to have_content "To continue, please click on the confirmation link that we have sent you via email" confirm_email expect(page).to have_content "Your account has been confirmed" From 98f99954e7e88ac58f1f7420e40f448906f44fc9 Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 25 Jan 2016 12:13:25 +0100 Subject: [PATCH 19/30] Corrects the logic dealing with confirmations of users via oauth --- app/controllers/users/registrations_controller.rb | 12 +++++++++--- app/models/user.rb | 12 +++++++----- ...160125100637_add_confirmed_oauth_email_to_user.rb | 5 +++++ db/schema.rb | 3 ++- 4 files changed, 23 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20160125100637_add_confirmed_oauth_email_to_user.rb diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index 3a564f232..ad5f6abf7 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -30,10 +30,16 @@ class Users::RegistrationsController < Devise::RegistrationsController def do_finish_signup current_user.registering_with_oauth = false - current_user.validate - should_send_confirmation = current_user.errors.include? :email if current_user.update(sign_up_params) - current_user.send_confirmation_instructions if should_send_confirmation + + if current_user.confirmed_oauth_email != current_user.email + current_user.update(confirmed_at: nil) + current_user.send_confirmation_instructions + end + if current_user.confirmed_oauth_email.present? + current_user.update(confirmed_oauth_email: nil) + end + sign_in_and_redirect current_user, event: :authentication else render :finish_signup diff --git a/app/models/user.rb b/app/models/user.rb index 5fe0e77ea..44b13e52a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -51,15 +51,17 @@ class User < ActiveRecord::Base # Get the existing user by email if the provider gives us a verified email. def self.first_or_initialize_for_oauth(auth) - auth_email = auth.info.email if auth.info.verified || auth.info.verified_email - auth_email_user = User.find_by(email: auth_email) if auth_email.present? + oauth_email = auth.info.email + confirmed_oauth_email = oauth_email if auth.info.verified || auth.info.verified_email + oauth_user = User.find_by(email: confirmed_oauth_email) if confirmed_oauth_email.present? - auth_email_user || User.new( + oauth_user || User.new( username: auth.info.name || auth.uid, - email: auth_email, + email: oauth_email, + confirmed_oauth_email: confirmed_oauth_email, password: Devise.friendly_token[0,20], terms_of_service: '1', - confirmed_at: auth_email.present? ? DateTime.now : nil + confirmed_at: confirmed_oauth_email.present? ? DateTime.now : nil ) end diff --git a/db/migrate/20160125100637_add_confirmed_oauth_email_to_user.rb b/db/migrate/20160125100637_add_confirmed_oauth_email_to_user.rb new file mode 100644 index 000000000..660735ab2 --- /dev/null +++ b/db/migrate/20160125100637_add_confirmed_oauth_email_to_user.rb @@ -0,0 +1,5 @@ +class AddConfirmedOauthEmailToUser < ActiveRecord::Migration + def change + add_column :users, :confirmed_oauth_email, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 963cce889..2fdd31706 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160122153329) do +ActiveRecord::Schema.define(version: 20160125100637) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -398,6 +398,7 @@ ActiveRecord::Schema.define(version: 20160122153329) do t.integer "notifications_count", default: 0 t.string "locale" t.boolean "registering_with_oauth", default: false + t.string "confirmed_oauth_email" end add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree From d5eab64568dc1bd95b13db777826e2a4e8baff04 Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 25 Jan 2016 16:15:02 +0100 Subject: [PATCH 20/30] adds feature flags for omniauth login buttons/controls Conflicts: app/controllers/users/omniauth_callbacks_controller.rb app/views/devise/_omniauth_form.html.erb --- .../users/omniauth_callbacks_controller.rb | 10 ++-- app/views/devise/_omniauth_form.html.erb | 47 ++++++++++++++----- db/dev_seeds.rb | 3 ++ db/seeds.rb | 3 ++ 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 24737777f..07f37ee13 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -1,15 +1,15 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController def twitter - sign_in_with :twitter + sign_in_with :twitter_login, :twitter end def facebook - sign_in_with :facebook + sign_in_with :facebook_login, :facebook end def google_oauth2 - sign_in_with :google_oauth2 + sign_in_with :google_login, :google_oauth2 end def after_sign_in_path_for(resource) @@ -22,7 +22,9 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController private - def sign_in_with(provider) + def sign_in_with(feature, provider) + raise ActionController::RoutingError.new('Not Found') unless Setting["feature.#{feature}"] + auth = env["omniauth.auth"] identity = Identity.first_or_create_from_oauth(auth) diff --git a/app/views/devise/_omniauth_form.html.erb b/app/views/devise/_omniauth_form.html.erb index 343743ff3..3167cc1d6 100644 --- a/app/views/devise/_omniauth_form.html.erb +++ b/app/views/devise/_omniauth_form.html.erb @@ -1,12 +1,35 @@ -<% if current_page?(new_user_session_path) %> - <%= link_to t("omniauth.twitter.sign_in"), user_omniauth_authorize_path(:twitter), class: "button-twitter button radius expand" %> - <%= link_to t("omniauth.facebook.sign_in"), user_omniauth_authorize_path(:facebook), class: "button-facebook button radius expand" %> - <%= link_to t("omniauth.google_oauth2.sign_in"), user_omniauth_authorize_path(:google_oauth2), class: "button-google button radius expand" %> -
-<% elsif current_page?(new_user_registration_path) %> - <%= link_to t("omniauth.twitter.sign_up"), user_omniauth_authorize_path(:twitter), class: "button-twitter button radius expand" %> - <%= link_to t("omniauth.facebook.sign_up"), user_omniauth_authorize_path(:facebook), class: "button-facebook button radius expand" %> - <%= link_to t("omniauth.google_oauth2.sign_up"), user_omniauth_authorize_path(:google_oauth2), class: "button-google button radius expand" %> -

Al hacer login con una red social, usted está aceptando los términos legales.

-
-<% end %> \ No newline at end of file +<% if feature?(:twitter_login) || feature?(:facebook_login) || feature?(:google_login) %> + + <% if current_page?(new_user_session_path) %> + + <% if feature? :twitter_login %> + <%= link_to t("omniauth.twitter.sign_in"), user_omniauth_authorize_path(:twitter), class: "button-twitter button radius expand" %> + <% end %> + + <% if feature? :facebook_login %> + <%= link_to t("omniauth.facebook.sign_in"), user_omniauth_authorize_path(:facebook), class: "button-facebook button radius expand" %> + <% end %> + + <% if feature? :google_login %> + <%= link_to t("omniauth.google_oauth2.sign_in"), user_omniauth_authorize_path(:google_oauth2), class: "button-google button radius expand" %> + <% end %> + +
+ <% elsif current_page?(new_user_registration_path) %> + + <% if feature? :twitter_login %> + <%= link_to t("omniauth.twitter.sign_up"), user_omniauth_authorize_path(:twitter), class: "button-twitter button radius expand" %> + <% end %> + + <% if feature? :facebook_login %> + <%= link_to t("omniauth.facebook.sign_up"), user_omniauth_authorize_path(:facebook), class: "button-facebook button radius expand" %> + <% end %> + + <% if feature? :google_login %> + <%= link_to t("omniauth.google_oauth2.sign_up"), user_omniauth_authorize_path(:google_oauth2), class: "button-google button radius expand" %> + <% end %> + +
+ <% end %> + +<% end %> diff --git a/db/dev_seeds.rb b/db/dev_seeds.rb index 4f638758a..0bee91cb2 100644 --- a/db/dev_seeds.rb +++ b/db/dev_seeds.rb @@ -20,6 +20,9 @@ Setting.create(key: 'org_name', value: 'Consul') Setting.create(key: 'place_name', value: 'City') Setting.create(key: 'feature.debates', value: "true") Setting.create(key: 'feature.spending_proposals', value: "true") +Setting.create(key: 'feature.twitter_login', value: "true") +Setting.create(key: 'feature.facebook_login', value: "true") +Setting.create(key: 'feature.google_login', value: "true") puts "Creating Geozones" ('A'..'Z').each{ |i| Geozone.create(name: "District #{i}") } diff --git a/db/seeds.rb b/db/seeds.rb index 6a6ae6671..659b3b2c0 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -53,3 +53,6 @@ Setting["place_name"] = "Consul-land" # Feature flags Setting['feature.debates'] = true Setting['feature.spending_proposals'] = true +Setting['feature.twitter_login'] = true +Setting['feature.facebook_login'] = true +Setting['feature.google_login'] = true From 1617f7295cc5b638a9f0c64aadc41dd2b496b969 Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 25 Jan 2016 17:20:42 +0100 Subject: [PATCH 21/30] requests omniauth facebook email --- config/initializers/devise.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index ad5f76fb6..31623e4cf 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -240,7 +240,7 @@ Devise.setup do |config| # up on your models and hooks. # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' config.omniauth :twitter, Rails.application.secrets.twitter_key, Rails.application.secrets.twitter_secret - config.omniauth :facebook, Rails.application.secrets.facebook_key, Rails.application.secrets.facebook_secret + config.omniauth :facebook, Rails.application.secrets.facebook_key, Rails.application.secrets.facebook_secret, scope: 'email', info_fields: 'email' config.omniauth :google_oauth2, Rails.application.secrets.google_oauth2_key, Rails.application.secrets.google_oauth2_secret # ==> Warden configuration From 55216ba4315ca122386927033813fc4834a9e4ea Mon Sep 17 00:00:00 2001 From: kikito Date: Mon, 25 Jan 2016 18:55:08 +0100 Subject: [PATCH 22/30] allows creating email duplicates while registering with omniauth --- app/models/user.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 44b13e52a..808b433bc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -174,6 +174,11 @@ class User < ActiveRecord::Base !erased? && !registering_with_oauth end + # Deactivates the email uniqueness validation when registering with oauth + def email_changed? + !registering_with_oauth && super + end + def has_official_email? domain = Setting['email_domain_for_officials'] !email.blank? && ( (email.end_with? "@#{domain}") || (email.end_with? ".#{domain}") ) From 92786764b7b9e4e86c944d340e185a947e990aff Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 12:44:05 +0100 Subject: [PATCH 23/30] Adds new auth tests Conflicts: spec/features/users_auth_spec.rb --- spec/features/users_auth_spec.rb | 67 +++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index 7e874b06c..f2a8b3283 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -48,13 +48,31 @@ feature 'Users' do context 'Twitter' do let(:twitter_hash){ {'provider' => 'twitter', 'uid' => '12345', 'info' => { 'name' => 'manuela' }} } - let(:twitter_hash_with_email) { { 'provider' => 'twitter', + let(:twitter_hash_with_email){ { 'provider' => 'twitter', 'uid' => '12345', 'info' => {'name' => 'manuela', 'email' => 'manuelacarmena@example.com' } } } + let(:twitter_hash_with_verified_email) { { 'provider' => 'twitter', 'uid' => '12345', 'info' => { 'name' => 'manuela' , 'email' => 'manuelacarmena@example.com', 'verified' => '1' } } } - scenario 'Sign up, when email was provided by OAuth provider' do + scenario 'Sign up when Oauth provider has a verified email' do + OmniAuth.config.add_mock(:twitter, twitter_hash_with_verified_email) + + visit '/' + click_link 'Register' + + click_link 'Sign up with Twitter' + + expect_to_be_signed_in + + click_link 'My account' + expect(page).to have_field('account_username', with: 'manuela') + + visit edit_user_registration_path + expect(page).to have_field('user_email', with: 'manuelacarmena@example.com') + end + + scenario 'Sign up when Oauth provider has an unverified email' do OmniAuth.config.add_mock(:twitter, twitter_hash_with_email) visit '/' @@ -68,7 +86,15 @@ feature 'Users' do end.to change { Identity.count }.by(1) end.to change { User.count }.by(1) - expect(current_path).to eq(root_path) + expect(current_path).to eq(new_user_session_path) + expect(page).to have_content "To continue, please click on the confirmation link that we have sent you via email" + + confirm_email + expect(page).to have_content "Your account has been confirmed" + + visit '/' + click_link 'Sign in' + click_link 'Sign in with Twitter' expect_to_be_signed_in click_link 'My account' @@ -137,7 +163,7 @@ feature 'Users' do scenario 'Try to register with the username of an already existing user' do create(:user, username: 'manuela', email: 'manuela@madrid.es', password: 'judgementday') - OmniAuth.config.add_mock(:twitter, twitter_hash_with_email) + OmniAuth.config.add_mock(:twitter, twitter_hash_with_verified_email) visit '/' click_link 'Register' @@ -157,7 +183,7 @@ feature 'Users' do expect(page).to have_field('user_email', with: 'manuelacarmena@example.com') end - scenario 'Try to register with the email of an already existing user' do + scenario 'Try to register with the email of an already existing user, when no email was provided by oauth' do create(:user, username: 'peter', email: 'manuela@example.com') OmniAuth.config.add_mock(:twitter, twitter_hash) @@ -191,6 +217,37 @@ feature 'Users' do visit edit_user_registration_path expect(page).to have_field('user_email', with: 'somethingelse@example.com') end + + scenario 'Try to register with the email of an already existing user, when an unconfirmed email was provided by oauth' do + create(:user, username: 'peter', email: 'manuelacarmena@example.com') + OmniAuth.config.add_mock(:twitter, twitter_hash_with_email) + + visit '/' + click_link 'Register' + click_link 'Sign up with Twitter' + + expect(current_path).to eq(finish_signup_path) + + expect(page).to have_field('user_email', with: 'manuelacarmena@example.com') + fill_in 'user_email', with: 'somethingelse@example.com' + click_button 'Register' + + expect(page).to have_content "To continue, please click on the confirmation link that we have sent you via email" + + confirm_email + expect(page).to have_content "Your account has been confirmed" + + visit '/' + click_link 'Sign in' + click_link 'Sign in with Twitter' + expect_to_be_signed_in + + click_link 'My account' + expect(page).to have_field('account_username', with: 'manuela') + + visit edit_user_registration_path + expect(page).to have_field('user_email', with: 'somethingelse@example.com') + end end end From a606c7aa8d5d42eb3a44a48ef00730bff90ee10c Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 12:44:55 +0100 Subject: [PATCH 24/30] Renames confirmed_oauth_email to oauth_email Refactors the way oauth_email is used to cover more cases (fixes pending specs) --- .../users/omniauth_callbacks_controller.rb | 29 ++++++++++--------- .../users/registrations_controller.rb | 7 +++-- app/models/user.rb | 13 +++------ ...me_confirmed_oauth_email_to_oauth_email.rb | 5 ++++ db/schema.rb | 4 +-- 5 files changed, 31 insertions(+), 27 deletions(-) create mode 100644 db/migrate/20160126090634_rename_confirmed_oauth_email_to_oauth_email.rb diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 07f37ee13..15a41a395 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -32,23 +32,26 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController # If there are no problems with the email/username, then they were provided by oauth or they # correspond to an existing user. Associate the identity and sign in - if @user.save + unless @user.save + # If either the username or email have provoked a failure, we save the user anyway (but marked for revision) + # This mark will be detected by applicationcontroller and the user will be redirected to finish_signup + @user.registering_with_oauth = true + unless @user.save + # If we still can't save the user, the email might be invalidating devise's validatable "unique" + # constraint. Set email to nil and try again (we'll reset later using oauth_email) + @user.email = nil + @user.save + end + end + + if @user.persisted? identity.update(user: @user) sign_in_and_redirect @user, event: :authentication set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? else - # If either the username or email have provoked a failure, we save the user anyway (but marked for revision) - # This mark will be detected by applicationcontroller and the user will be redirected to finish_signup - @user.registering_with_oauth = true - if @user.save - identity.update(user: @user) - sign_in_and_redirect @user, event: :authentication - set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? - else - # If the failure is because something else happens, just present the "new user" form - session["devise.#{provider}_data"] = auth - redirect_to new_user_registration_url - end + # If the failure is because something else happens, just present the "new user" form + session["devise.#{provider}_data"] = auth + redirect_to new_user_registration_url end end diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index ad5f6abf7..0d32e725a 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -25,6 +25,7 @@ class Users::RegistrationsController < Devise::RegistrationsController def finish_signup current_user.registering_with_oauth = false + current_user.email = current_user.oauth_email if current_user.email.blank? current_user.validate end @@ -32,12 +33,12 @@ class Users::RegistrationsController < Devise::RegistrationsController current_user.registering_with_oauth = false if current_user.update(sign_up_params) - if current_user.confirmed_oauth_email != current_user.email + if current_user.oauth_email != current_user.email current_user.update(confirmed_at: nil) current_user.send_confirmation_instructions end - if current_user.confirmed_oauth_email.present? - current_user.update(confirmed_oauth_email: nil) + if current_user.oauth_email.present? + current_user.update(oauth_email: nil) end sign_in_and_redirect current_user, event: :authentication diff --git a/app/models/user.rb b/app/models/user.rb index 808b433bc..4d1aaf27e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -52,16 +52,16 @@ class User < ActiveRecord::Base # Get the existing user by email if the provider gives us a verified email. def self.first_or_initialize_for_oauth(auth) oauth_email = auth.info.email - confirmed_oauth_email = oauth_email if auth.info.verified || auth.info.verified_email - oauth_user = User.find_by(email: confirmed_oauth_email) if confirmed_oauth_email.present? + oauth_email_confirmed = auth.info.verified || auth.info.verified_email + oauth_user = User.find_by(email: oauth_email) if oauth_email_confirmed oauth_user || User.new( username: auth.info.name || auth.uid, email: oauth_email, - confirmed_oauth_email: confirmed_oauth_email, + oauth_email: oauth_email, password: Devise.friendly_token[0,20], terms_of_service: '1', - confirmed_at: confirmed_oauth_email.present? ? DateTime.now : nil + confirmed_at: oauth_email_confirmed ? DateTime.now : nil ) end @@ -174,11 +174,6 @@ class User < ActiveRecord::Base !erased? && !registering_with_oauth end - # Deactivates the email uniqueness validation when registering with oauth - def email_changed? - !registering_with_oauth && super - end - def has_official_email? domain = Setting['email_domain_for_officials'] !email.blank? && ( (email.end_with? "@#{domain}") || (email.end_with? ".#{domain}") ) diff --git a/db/migrate/20160126090634_rename_confirmed_oauth_email_to_oauth_email.rb b/db/migrate/20160126090634_rename_confirmed_oauth_email_to_oauth_email.rb new file mode 100644 index 000000000..3b3f027fe --- /dev/null +++ b/db/migrate/20160126090634_rename_confirmed_oauth_email_to_oauth_email.rb @@ -0,0 +1,5 @@ +class RenameConfirmedOauthEmailToOauthEmail < ActiveRecord::Migration + def change + rename_column :users, :confirmed_oauth_email, :oauth_email + end +end diff --git a/db/schema.rb b/db/schema.rb index 2fdd31706..a9cce3582 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160125100637) do +ActiveRecord::Schema.define(version: 20160126090634) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -398,7 +398,7 @@ ActiveRecord::Schema.define(version: 20160125100637) do t.integer "notifications_count", default: 0 t.string "locale" t.boolean "registering_with_oauth", default: false - t.string "confirmed_oauth_email" + t.string "oauth_email" end add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree From 1e8d46d83a8f26674eb656518df0cda2196beb0c Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 13:26:35 +0100 Subject: [PATCH 25/30] fixes failing email specs Conflicts: app/models/user.rb fixes missing i18n entry fixes badly done merge fixes i18n issues force build to start --- README.md | 2 +- app/controllers/application_controller.rb | 2 +- app/controllers/users/omniauth_callbacks_controller.rb | 2 +- app/models/user.rb | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 5e841baae..7d3f6842a 100644 --- a/README.md +++ b/README.md @@ -61,4 +61,4 @@ Code published under AFFERO GPL v3 (see [LICENSE-AGPLv3.txt](LICENSE-AGPLv3.txt) ## Contributions -See [CONTRIBUTING_EN.md](CONTRIBUTING_EN.md) +See [CONTRIBUTING_EN.md](CONTRIBUTING_EN.md) \ No newline at end of file diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0080e3af6..3fa634898 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -83,7 +83,7 @@ class ApplicationController < ActionController::Base def ensure_signup_complete if user_signed_in? && - current_user.pending_finish_signup? && + current_user.registering_with_oauth && %w(finish_signup do_finish_signup).exclude?(action_name) redirect_to finish_signup_path end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 15a41a395..f5682d97b 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -13,7 +13,7 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end def after_sign_in_path_for(resource) - if resource.pending_finish_signup? + if resource.registering_with_oauth finish_signup_path else super(resource) diff --git a/app/models/user.rb b/app/models/user.rb index 4d1aaf27e..bf85babed 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -183,8 +183,8 @@ class User < ActiveRecord::Base self[:locale] ||= I18n.default_locale.to_s end - def pending_finish_signup? - email.blank? && unconfirmed_email.blank? + def confirmation_required? + super && !registering_with_oauth end private From 9e0494a82de88d9fcf1a8e6f1a7d577a7dbfba27 Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 17:20:24 +0100 Subject: [PATCH 26/30] Activates omniauth functionality --- app/views/devise/sessions/new.html.erb | 2 +- app/views/users/registrations/new.html.erb | 2 +- spec/features/users_auth_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/devise/sessions/new.html.erb b/app/views/devise/sessions/new.html.erb index e3cfc1201..56783bcb5 100644 --- a/app/views/devise/sessions/new.html.erb +++ b/app/views/devise/sessions/new.html.erb @@ -1,7 +1,7 @@ <% provide :title do %><%= t("devise_views.sessions.new.title") %><% end %>

<%= t("devise_views.sessions.new.title") %>

-<%# render 'devise/omniauth_form' %> +<%= render 'devise/omniauth_form' %>

<%= t("devise_views.shared.links.signup", diff --git a/app/views/users/registrations/new.html.erb b/app/views/users/registrations/new.html.erb index faeff384a..61ac4031b 100644 --- a/app/views/users/registrations/new.html.erb +++ b/app/views/users/registrations/new.html.erb @@ -1,7 +1,7 @@ <% provide :title do %><%= t("devise_views.users.registrations.new.title") %><% end %>

<%= t("devise_views.users.registrations.new.title") %>

-<%# render 'devise/omniauth_form' %> +<%= render 'devise/omniauth_form' %> <%= form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f| %> <%= render 'shared/errors', resource: resource %> diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index f2a8b3283..0105a8dbd 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -44,7 +44,7 @@ feature 'Users' do end end - xcontext 'OAuth authentication' do + context 'OAuth authentication' do context 'Twitter' do let(:twitter_hash){ {'provider' => 'twitter', 'uid' => '12345', 'info' => { 'name' => 'manuela' }} } From 646ca8a686a51b6845175b713b38a0d5f19db1ec Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 17:21:24 +0100 Subject: [PATCH 27/30] Splits omniauth_callbacks_controller#login_with method in two smaller ones Uses new hash syntax in specs --- .../users/omniauth_callbacks_controller.rb | 34 ++++++++++--------- spec/features/users_auth_spec.rb | 11 +++--- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index f5682d97b..ca0dfc667 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -30,29 +30,31 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController identity = Identity.first_or_create_from_oauth(auth) @user = current_user || identity.user || User.first_or_initialize_for_oauth(auth) - # If there are no problems with the email/username, then they were provided by oauth or they - # correspond to an existing user. Associate the identity and sign in - unless @user.save - # If either the username or email have provoked a failure, we save the user anyway (but marked for revision) - # This mark will be detected by applicationcontroller and the user will be redirected to finish_signup - @user.registering_with_oauth = true - unless @user.save - # If we still can't save the user, the email might be invalidating devise's validatable "unique" - # constraint. Set email to nil and try again (we'll reset later using oauth_email) - @user.email = nil - @user.save - end - end - - if @user.persisted? + if save_user(@user) identity.update(user: @user) sign_in_and_redirect @user, event: :authentication set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? else - # If the failure is because something else happens, just present the "new user" form + # If saving the user was not possible (weird errors, etc) just present the "new user" form session["devise.#{provider}_data"] = auth redirect_to new_user_registration_url end end + def save_user(user) + # If there are no problems with the email/username, then they were provided by oauth or they + # correspond to an existing user. Associate the identity and sign in + return true if @user.save + + # If either the username or email have provoked a failure, we save the user anyway (but marked for revision) + # This mark will be detected by applicationcontroller and the user will be redirected to finish_signup + @user.registering_with_oauth = true + return true if @user.save + + # If we still can't save the user, the email might be invalidating devise's validatable "unique" + # constraint. Set email to nil and try again (we'll reset later using oauth_email) + @user.email = nil + @user.save + end + end diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index 0105a8dbd..c757c1c90 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -47,12 +47,11 @@ feature 'Users' do context 'OAuth authentication' do context 'Twitter' do - let(:twitter_hash){ {'provider' => 'twitter', 'uid' => '12345', 'info' => { 'name' => 'manuela' }} } - let(:twitter_hash_with_email){ { 'provider' => 'twitter', 'uid' => '12345', 'info' => {'name' => 'manuela', 'email' => 'manuelacarmena@example.com' } } } - let(:twitter_hash_with_verified_email) { { 'provider' => 'twitter', - 'uid' => '12345', - 'info' => { 'name' => 'manuela' , 'email' => 'manuelacarmena@example.com', 'verified' => '1' } } - } + let(:twitter_hash){ {provider: 'twitter', uid: '12345', info: {name: 'manuela'}} } + let(:twitter_hash_with_email){ {provider: 'twitter', uid: '12345', info: {name: 'manuela', email: 'manuelacarmena@example.com'}} } + let(:twitter_hash_with_verified_email){ {provider: 'twitter', + uid: '12345', + info: {name: 'manuela', email: 'manuelacarmena@example.com', verified: '1'}} } scenario 'Sign up when Oauth provider has a verified email' do From a23d0ebbd8fddc62c328f5115b92e8d3108f4587 Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 18:19:18 +0100 Subject: [PATCH 28/30] fixes issues after squashing --- spec/features/users_auth_spec.rb | 42 ++++++++++---------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index c757c1c90..3526ce97c 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -77,13 +77,7 @@ feature 'Users' do visit '/' click_link 'Register' - expect do - expect do - expect do - click_link 'Sign up with Twitter' - end.not_to change { ActionMailer::Base.deliveries.size } - end.to change { Identity.count }.by(1) - end.to change { User.count }.by(1) + click_link 'Sign up with Twitter' expect(current_path).to eq(new_user_session_path) expect(page).to have_content "To continue, please click on the confirmation link that we have sent you via email" @@ -108,29 +102,16 @@ feature 'Users' do visit '/' click_link 'Register' - - expect do - expect do - expect do - click_link 'Sign up with Twitter' - end.not_to change { ActionMailer::Base.deliveries.size } - end.to change { Identity.count }.by(1) - end.to change { User.count }.by(1) + click_link 'Sign up with Twitter' expect(current_path).to eq(finish_signup_path) - - user = User.last - expect(user.username).to eq('manuela-de-las-carmenas') - expect(user.email).to eq("omniauth@participacion-12345-twitter.com") - fill_in 'user_email', with: 'manueladelascarmenas@example.com' click_button 'Register' expect(page).to have_content "To continue, please click on the confirmation link that we have sent you via email" confirm_email - - expect(page).to have_content "Your email address has been successfully confirmed" + expect(page).to have_content "Your account has been confirmed" visit '/' click_link 'Sign in' @@ -140,7 +121,8 @@ feature 'Users' do click_link 'My account' expect(page).to have_field('account_username', with: 'manuela') - expect(user.reload.email).to eq('manueladelascarmenas@example.com') + visit edit_user_registration_path + expect(page).to have_field('user_email', with: 'manueladelascarmenas@example.com') end scenario 'Sign in, user was already signed up with OAuth' do @@ -150,14 +132,16 @@ feature 'Users' do visit '/' click_link 'Sign in' - - expect do - expect do - click_link 'Sign in with Twitter' - end.not_to change { Identity.count } - end.not_to change { User.count } + click_link 'Sign in with Twitter' expect_to_be_signed_in + + click_link 'My account' + expect(page).to have_field('account_username', with: user.username) + + visit edit_user_registration_path + expect(page).to have_field('user_email', with: user.email) + end scenario 'Try to register with the username of an already existing user' do From a796dade7aab64f4994e47c9e0c3c6fb7e32db8f Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 19:48:01 +0100 Subject: [PATCH 29/30] extracts methods into user.rb --- .../users/omniauth_callbacks_controller.rb | 16 +++------------- .../users/registrations_controller.rb | 10 +--------- app/models/user.rb | 16 ++++++++++++++++ 3 files changed, 20 insertions(+), 22 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index ca0dfc667..f3229bee1 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -42,19 +42,9 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end def save_user(user) - # If there are no problems with the email/username, then they were provided by oauth or they - # correspond to an existing user. Associate the identity and sign in - return true if @user.save - - # If either the username or email have provoked a failure, we save the user anyway (but marked for revision) - # This mark will be detected by applicationcontroller and the user will be redirected to finish_signup - @user.registering_with_oauth = true - return true if @user.save - - # If we still can't save the user, the email might be invalidating devise's validatable "unique" - # constraint. Set email to nil and try again (we'll reset later using oauth_email) - @user.email = nil - @user.save + @user.save || + @user.save_requiring_finish_signup || + @user.save_requiring_finish_signup_without_email end end diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index 0d32e725a..8a4aca2a8 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -32,15 +32,7 @@ class Users::RegistrationsController < Devise::RegistrationsController def do_finish_signup current_user.registering_with_oauth = false if current_user.update(sign_up_params) - - if current_user.oauth_email != current_user.email - current_user.update(confirmed_at: nil) - current_user.send_confirmation_instructions - end - if current_user.oauth_email.present? - current_user.update(oauth_email: nil) - end - + current_user.send_oauth_confirmation_instructions sign_in_and_redirect current_user, event: :authentication else render :finish_signup diff --git a/app/models/user.rb b/app/models/user.rb index bf85babed..a79e8acc2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -187,6 +187,22 @@ class User < ActiveRecord::Base super && !registering_with_oauth end + def send_oauth_confirmation_instructions + if oauth_email != email + self.update(confirmed_at: nil) + self.send_confirmation_instructions + end + self.update(oauth_email: nil) if oauth_email.present? + end + + def save_requiring_finish_signup + self.update(registering_with_oauth: true) + end + + def save_requiring_finish_signup_without_email + self.update(registering_with_oauth: true, email: nil) + end + private def clean_document_number self.document_number = self.document_number.gsub(/[^a-z0-9]+/i, "").upcase unless self.document_number.blank? From 957e07489aeb729c8180c398cd4a4b5f9c237970 Mon Sep 17 00:00:00 2001 From: kikito Date: Tue, 26 Jan 2016 19:59:20 +0100 Subject: [PATCH 30/30] remove unuseful comment --- app/controllers/users/omniauth_callbacks_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index f3229bee1..0bdd2a801 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -35,7 +35,6 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController sign_in_and_redirect @user, event: :authentication set_flash_message(:notice, :success, kind: "#{provider}".capitalize) if is_navigational_format? else - # If saving the user was not possible (weird errors, etc) just present the "new user" form session["devise.#{provider}_data"] = auth redirect_to new_user_registration_url end