From ddc5125d8d3ec0a5dcd8087ec806e95ee515bff6 Mon Sep 17 00:00:00 2001 From: Gabriel Silva Date: Tue, 9 Aug 2016 00:22:32 +0000 Subject: [PATCH] Refactors old plugin behavior --- plugins/oauth_client/controllers/public/oauth_client_plugin_public_controller.rb | 73 +++++++++++++++++++++++++++++++++++-------------------------------------- plugins/oauth_client/db/migrate/20160720165808_add_external_profile_to_oauth_auth.rb | 8 +++++++- plugins/oauth_client/models/oauth_client_plugin/auth.rb | 8 ++++++-- plugins/oauth_client/test/functional/oauth_client_plugin_public_controller_test.rb | 51 +++++++++++++++++++++++++++++++++++---------------- 4 files changed, 83 insertions(+), 57 deletions(-) diff --git a/plugins/oauth_client/controllers/public/oauth_client_plugin_public_controller.rb b/plugins/oauth_client/controllers/public/oauth_client_plugin_public_controller.rb index 81d0d20..c60819a 100644 --- a/plugins/oauth_client/controllers/public/oauth_client_plugin_public_controller.rb +++ b/plugins/oauth_client/controllers/public/oauth_client_plugin_public_controller.rb @@ -3,14 +3,14 @@ class OauthClientPluginPublicController < PublicController skip_before_filter :login_required def callback - auth = request.env["omniauth.auth"] - + auth_data = request.env["omniauth.auth"] oauth_params = request.env["omniauth.params"] + if oauth_params && oauth_params["action"] == "external_login" - external_person_login(auth) + external_person_login(auth_data) else - auth_user = environment.users.where(email: auth.info.email).first - if auth_user then login(auth_user.person) else signup(auth) end + auth_user = environment.users.where(email: auth_data.info.email).first + if auth_user then login(auth_user.person) else signup(auth_data) end end end @@ -26,48 +26,45 @@ class OauthClientPluginPublicController < PublicController protected - def external_person_login(auth) + def external_person_login(auth_data) provider = OauthClientPlugin::Provider.find(session[:provider_id]) - if provider.enabled? - user = User.new(email: auth.info.email, login: auth.info.name.to_slug) - webfinger = OpenStruct.new( - identifier: auth.info.nickname || user.login, - name: auth.info.name, - created_at: Time.now, - domain: provider.site || auth.provider, - email: user.email) - person = ExternalPerson.get_or_create(webfinger) - user.external_person_id = person.id + user = User.new(email: auth_data.info.email, login: auth_data.info.name.to_slug) + person_data = OpenStruct.new( + identifier: auth_data.info.nickname || user.login, + name: auth_data.info.name, + created_at: Time.now, + domain: provider.site || auth_data.provider, + email: user.email) + person = ExternalPerson.get_or_create(person_data) + user.external_person_id = person.id - oauth_auth = person.oauth_auth - if oauth_auth.nil? - auth_data = { profile: person, provider: provider, enabled: true, - external_person_uid: auth.uid, external_person_image_url: auth.info.image } - oauth_auth = OauthClientPlugin::Auth.create_for_strategy(provider.strategy, auth_data) - end - self.current_user = user if oauth_auth.enabled? - else - session[:notice] = _("Can't login with %s") % provider.name - end + oauth_auth = person.oauth_auth + oauth_data = { profile: person, provider: provider, enabled: true, + external_person_uid: auth_data.uid, external_person_image_url: auth_data.info.image } + oauth_auth ||= OauthClientPlugin::Auth.create_for_strategy(provider.strategy, oauth_data) + create_session(user, oauth_auth) + end - redirect_to :controller => :account, :action => :login + def signup(auth_data) + session[:oauth_data] = auth_data + username = auth_data.info.email.split('@').first + name = auth_data.info.name + name ||= auth_data.extra && auth_data.extra.raw_info ? auth_data.extra.raw_info.name : '' + redirect_to :controller => :account, :action => :signup, :user => {:login => username, :email => auth_data.info.email}, :profile_data => {:name => name} end - def signup(auth) - login = auth.info.email.split('@').first - session[:oauth_data] = auth - name = auth.info.name - name ||= auth.extra && auth.extra.raw_info ? auth.extra.raw_info.name : '' - redirect_to :controller => :account, :action => :signup, :user => {:login => login, :email => auth.info.email}, :profile_data => {:name => name} + def login(person) + auth = person.oauth_auths.find_or_create_by(profile: person, + provider_id: session[:provider_id]) + create_session(person.user, auth) end - def login person + def create_session(user, oauth_auth) provider = OauthClientPlugin::Provider.find(session[:provider_id]) - auth = person.oauth_auths.where(provider_id: provider.id).first - auth ||= person.oauth_auths.create! profile: person, provider: provider, enabled: true - if auth.enabled? && provider.enabled? - self.current_user = person.user + + if oauth_auth.allow_login? + self.current_user = user else session[:notice] = _("Can't login with %s") % provider.name end diff --git a/plugins/oauth_client/db/migrate/20160720165808_add_external_profile_to_oauth_auth.rb b/plugins/oauth_client/db/migrate/20160720165808_add_external_profile_to_oauth_auth.rb index c803855..72d21b5 100644 --- a/plugins/oauth_client/db/migrate/20160720165808_add_external_profile_to_oauth_auth.rb +++ b/plugins/oauth_client/db/migrate/20160720165808_add_external_profile_to_oauth_auth.rb @@ -1,15 +1,21 @@ class AddExternalProfileToOauthAuth < ActiveRecord::Migration def up add_column :oauth_client_plugin_auths, :profile_type, :string + add_index :oauth_client_plugin_auths, :profile_type + add_column :oauth_client_plugin_auths, :external_person_uid, :string add_column :oauth_client_plugin_auths, :external_person_image_url, :string - add_index :oauth_client_plugin_auths, :profile_type + + change_column_default :oauth_client_plugin_auths, :enabled, true end def down remove_index :oauth_client_plugin_auths, :profile_type remove_column :oauth_client_plugin_auths, :profile_type + remove_column :oauth_client_plugin_auths, :external_person_uid remove_column :oauth_client_plugin_auths, :external_person_image_url + + change_column_default :oauth_client_plugin_auths, :enabled, nil end end diff --git a/plugins/oauth_client/models/oauth_client_plugin/auth.rb b/plugins/oauth_client/models/oauth_client_plugin/auth.rb index fdee8a8..15197f4 100644 --- a/plugins/oauth_client/models/oauth_client_plugin/auth.rb +++ b/plugins/oauth_client/models/oauth_client_plugin/auth.rb @@ -1,7 +1,7 @@ class OauthClientPlugin::Auth < ApplicationRecord - attr_accessible :profile, :provider, :enabled, :access_token, - :expires_in, :type, :external_person_uid, + attr_accessible :profile, :provider, :provider_id, :enabled, + :access_token, :expires_in, :type, :external_person_uid, :external_person_image_url belongs_to :profile, polymorphic: true @@ -27,6 +27,10 @@ class OauthClientPlugin::Auth < ApplicationRecord not self.expired? end + def allow_login? + self.enabled? && self.provider.enabled? + end + def self.create_for_strategy(strategy, args = {}) namespace = self.name.split("::")[0] class_name = "#{namespace}::#{strategy.camelize}Auth" diff --git a/plugins/oauth_client/test/functional/oauth_client_plugin_public_controller_test.rb b/plugins/oauth_client/test/functional/oauth_client_plugin_public_controller_test.rb index f7b7bfe..a4d0f95 100644 --- a/plugins/oauth_client/test/functional/oauth_client_plugin_public_controller_test.rb +++ b/plugins/oauth_client/test/functional/oauth_client_plugin_public_controller_test.rb @@ -25,7 +25,39 @@ class OauthClientPluginPublicControllerTest < ActionController::TestCase assert_match /.*\/account\/signup/, @response.redirect_url end - should 'login using provider when url param is present' do + should 'login when user already signed up' do + create_user(@auth.info.name, email: @auth.info.email) + + get :callback + assert session[:user].present? + end + + should 'not login when user already signed up and the provider is disabled' do + create_user(@auth.info.name, email: @auth.info.email) + provider.update_attribute(:enabled, false) + + get :callback + assert session[:user].nil? + end + + should 'not login when user already signed up and the provider is disabled for him' do + create_user(@auth.info.name, email: @auth.info.email) + OauthClientPlugin::Auth.any_instance.stubs(:enabled?).returns(false) + + get :callback + assert session[:user].nil? + end + + should 'not duplicate oauth_auths when the same provider is used several times' do + user = create_user(@auth.info.name, email: @auth.info.email) + + get :callback + assert_no_difference 'user.oauth_auths.count' do + 3.times { get :callback } + end + end + + should 'perform external login using provider when url param is present' do request.env["omniauth.params"] = {"action" => "external_login"} get :callback @@ -33,7 +65,7 @@ class OauthClientPluginPublicControllerTest < ActionController::TestCase assert session[:external].present? end - should 'not login when the provider is disabled' do + should 'not perform external login when the provider is disabled' do request.env["omniauth.params"] = {"action" => "external_login"} provider.update_attribute(:enabled, false) @@ -42,7 +74,7 @@ class OauthClientPluginPublicControllerTest < ActionController::TestCase assert session[:external].nil? end - should 'not login when the provider is disabled for a user' do + should 'not perform external login when the provider is disabled for a user' do request.env["omniauth.params"] = {"action" => "external_login"} OauthClientPlugin::GithubAuth.any_instance.stubs(:enabled?).returns(false) @@ -58,17 +90,4 @@ class OauthClientPluginPublicControllerTest < ActionController::TestCase external_person = ExternalPerson.find_by(identifier: auth.info.nickname) assert_equal provider, external_person.oauth_auth.provider end - -# should 'do not duplicate relations between an user and a provider when the same provider was used again in a login' do -# user = create_user -# auth.info.stubs(:email).returns(user.email) -# auth.info.stubs(:name).returns(user.name) -# session[:provider_id] = provider.id -# -# get :callback -# assert_no_difference 'user.oauth_auths.count' do -# 3.times { get :callback } -# end -# end -# end -- libgit2 0.21.2