Improve the omniauth controller specs. Fix the email provided by CAS. Get name from CAS attributes.

* Make omniauth controller specs more robust by using shared examples for all authentication providers in controller spec. – Still passing. Yay!

* Return "casuser", instead of "casuser@" when no cas_domainname is configured.

* If no cas_domainname is configured, the CAS authentication would return "casuser@" for the users email field, because it tried to assume the email adress of the CAS user by it's username + cas_domainname.
  Now it just returns the username instead of adding an "@" if cas_domainname is not configured.
  This especially makes sense on CAS setups where the username equals the users email adress.
  The old behaviour, if cas_domainname is configured, was not changed.

* Fetch the email from CAS attributes if provided
  If the cas:authenticationSuccess (handled via omniauth-cas) response gives us an email use that.
  If not, behave as before (username or username@cas_domainname).

* Fetch the (full) name from CAS attributes if provided
  If the CAS response by omniauth provides a [:info][:name] field, prefer this over the uid, because we want the name to be a "Full Name", instead of just a "shortname"

app/controllers/users/omniauth_callbacks_controller.rb

@@ -140,9 +140,22 @@ class Users::OmniauthCallbacksController < ApplicationController
 
   def create_or_sign_on_user_using_cas(auth_token)
     logger.error "authtoken #{auth_token}"
-    email = "#{auth_token[:extra][:user]}@#{SiteSetting.cas_domainname}"
+
+    email = auth_token[:info][:email] if auth_token[:info]
+    email ||= if SiteSetting.cas_domainname.present?
+      "#{auth_token[:extra][:user]}@#{SiteSetting.cas_domainname}"
+    else
+      auth_token[:extra][:user]
+    end
+
     username = auth_token[:extra][:user]
-    name = auth_token["uid"]
+
+    name = if auth_token[:info] && auth_token[:info][:name]
+      auth_token[:info][:name]
+    else
+      auth_token["uid"]
+    end
+
     cas_user_id = auth_token["uid"]
 
     session[:authentication] = {

spec/controllers/omniauth_callbacks_controller_spec.rb

@@ -3,7 +3,49 @@ require 'spec_helper'
 describe Users::OmniauthCallbacksController do
 
   let(:auth) { {info: {email: 'eviltrout@made.up.email', name: 'Robin Ward', uid: 123456789}, "extra" => {"raw_info" => {} } } }
-  let(:cas_auth) {{ uid: "caluser2", extra: {user: "caluser2"}  }  }
+  let(:cas_auth) { { 'uid' => 'casuser', extra: { user: 'casuser'}  }  }
+
+  shared_examples_for "an authenticaton provider" do |provider|
+    context "when #{provider} logins are disabled" do
+      before do
+        SiteSetting.stubs("enable_#{provider}_logins?").returns(false)
+      end
+
+      it "fails" do
+        get :complete, provider: provider
+        response.should_not be_success
+      end
+
+    end
+
+    context "when #{provider} logins are enabled" do
+      before do
+        SiteSetting.stubs("enable_#{provider}_logins?").returns(true)
+      end
+
+      it "succeeds" do
+        get :complete, provider: provider
+        response.should be_success
+      end
+
+      context "and 'invite only' site setting is enabled" do
+        before do
+          SiteSetting.stubs(:invite_only?).returns(true)
+        end
+
+        it "informs the user they are awaiting approval" do
+          xhr :get, :complete, provider: provider, format: :json
+
+          expect(
+            JSON.parse(response.body)['awaiting_approval']
+          ).to be_true
+        end
+      end
+
+    end
+
+  end
+
   describe 'invalid provider' do
 
     it "fails" do
@@ -20,29 +62,8 @@ describe Users::OmniauthCallbacksController do
       request.env["omniauth.auth"] = auth
     end
 
-    it "fails when twitter logins are disabled" do
+    it_behaves_like "an authenticaton provider", 'twitter'
-      SiteSetting.stubs(:enable_twitter_logins?).returns(false)
-      get :complete, provider: 'twitter'
-      response.should_not be_success
-    end
 
-    it "succeeds when twitter logins are enabled" do
-      SiteSetting.stubs(:enable_twitter_logins?).returns(true)
-      get :complete, provider: 'twitter'
-      response.should be_success
-    end
-
-    context "when 'invite only' site setting is enabled" do
-      before { SiteSetting.stubs(:invite_only?).returns(true) }
-
-      it 'informs the user they are awaiting approval' do
-        xhr :get, :complete, provider: 'twitter', format: :json
-
-        expect(
-          JSON.parse(response.body)['awaiting_approval']
-        ).to be_true
-      end
-    end
   end
 
   describe 'facebook' do
@@ -51,17 +72,7 @@ describe Users::OmniauthCallbacksController do
       request.env["omniauth.auth"] = auth
     end
 
-    it "fails when facebook logins are disabled" do
+    it_behaves_like "an authenticaton provider", 'facebook'
-      SiteSetting.stubs(:enable_facebook_logins?).returns(false)
-      get :complete, provider: 'facebook'
-      response.should_not be_success
-    end
-
-    it "succeeds when facebook logins are enabled" do
-      SiteSetting.stubs(:enable_facebook_logins?).returns(true)
-      get :complete, provider: 'facebook'
-      response.should be_success
-    end
 
   end
 
@@ -71,16 +82,47 @@ describe Users::OmniauthCallbacksController do
       request.env["omniauth.auth"] = cas_auth
     end
 
-    it "fails when cas logins are disabled" do
+    it_behaves_like "an authenticaton provider", 'cas'
-      SiteSetting.stubs(:enable_cas_logins?).returns(false)
+
-      get :complete, provider: 'cas'
+    describe "extracted user data" do
-      response.should_not be_success
+      before do
+        SiteSetting.stubs(:enable_cas_logins?).returns(true)
+      end
+
+      subject {
+        xhr :get, :complete, provider: 'cas', format: :json
+        OpenStruct.new(JSON.parse(response.body))
+      }
+
+      context "when no user infos are returned by cas" do
+        its(:username) { should eq 'casuser' }
+        its(:name) { should eq 'casuser' }
+        its(:email) { should eq 'casuser' } # No cas_domainname configured!
+
+        context "when cas_domainname is configured" do
+          before do
+            SiteSetting.stubs(:cas_domainname).returns("example.com")
+          end
+
+          its(:email) { should eq 'casuser@example.com' }
+        end
+      end
+
+      context "when user infos are returned by cas" do
+        before do
+          request.env["omniauth.auth"] = cas_auth.merge({
+            info: {
+              name: 'Proper Name',
+              email: 'public@example.com'
+              }
+          })
+        end
+
+        its(:username) { should eq 'casuser' }
+        its(:name) { should eq 'Proper Name' }
+        its(:email) { should eq 'public@example.com' }
       end
 
-    it "succeeds when cas logins are enabled" do
-      SiteSetting.stubs(:enable_cas_logins?).returns(true)
-      get :complete, provider: 'cas'
-      response.should be_success
     end
 
   end
@@ -93,31 +135,11 @@ describe Users::OmniauthCallbacksController do
     end
 
     describe "google" do
-      it "fails when google logins are disabled" do
+      it_behaves_like "an authenticaton provider", 'google'
-        SiteSetting.stubs(:enable_google_logins?).returns(false)
-        get :complete, provider: 'google'
-        response.should_not be_success
-      end
-
-      it "succeeds when google logins are enabled" do
-        SiteSetting.stubs(:enable_google_logins?).returns(true)
-        get :complete, provider: 'google'
-        response.should be_success
-      end
     end
 
     describe "yahoo" do
-      it "fails when yahoo logins are disabled" do
+      it_behaves_like "an authenticaton provider", 'yahoo'
-        SiteSetting.stubs(:enable_yahoo_logins?).returns(false)
-        get :complete, provider: 'yahoo'
-        response.should_not be_success
-      end
-
-      it "succeeds when yahoo logins are enabled" do
-        SiteSetting.stubs(:enable_yahoo_logins?).returns(true)
-        get :complete, provider: 'yahoo'
-        response.should be_success
-      end
     end
 
   end
@@ -128,17 +150,7 @@ describe Users::OmniauthCallbacksController do
       request.env["omniauth.auth"] = auth
     end
 
-    it "fails when github logins are disabled" do
+    it_behaves_like "an authenticaton provider", 'github'
-      SiteSetting.stubs(:enable_github_logins?).returns(false)
-      get :complete, provider: 'github'
-      response.should_not be_success
-    end
-
-    it "succeeds when github logins are enabled" do
-      SiteSetting.stubs(:enable_github_logins?).returns(true)
-      get :complete, provider: 'github'
-      response.should be_success
-    end
 
   end
 
@@ -148,17 +160,7 @@ describe Users::OmniauthCallbacksController do
       request.env["omniauth.auth"] = auth
     end
 
-    it "fails when persona logins are disabled" do
+    it_behaves_like "an authenticaton provider", 'persona'
-      SiteSetting.stubs(:enable_persona_logins?).returns(false)
-      get :complete, provider: 'persona'
-      response.should_not be_success
-    end
-
-    it "succeeds when persona logins are enabled" do
-      SiteSetting.stubs(:enable_persona_logins?).returns(true)
-      get :complete, provider: 'persona'
-      response.should be_success
-    end
 
   end