Refactors old plugin behavior

Signed-off-by: Gabriel Silva <gabriel93.silva@gmail.com>

Refactors old plugin behavior
Signed-off-by: Gabriel Silva <gabriel93.silva@gmail.com>
Gabriel Silva
1 parent 9e014471
Showing 4 changed files with 83 additions and 57 deletions Show diff stats
plugins/oauth_client/controllers/public/oauth_client_plugin_public_controller.rb
plugins/oauth_client/db/migrate/20160720165808_add_external_profile_to_oauth_auth.rb
plugins/oauth_client/models/oauth_client_plugin/auth.rb
plugins/oauth_client/test/functional/oauth_client_plugin_public_controller_test.rb
@@ -3,14 +3,14 @@ class OauthClientPluginPublicController &lt; 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 &lt; 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
 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
 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 &lt; 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"
@@ -25,7 +25,39 @@ class OauthClientPluginPublicControllerTest &lt; 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 &lt; 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 &lt; 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 &lt; 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