Merge pull request #2033 from birarda/master

add option to override user attributes from SSO payload

app/models/discourse_single_sign_on.rb

@@ -39,13 +39,37 @@ class DiscourseSingleSignOn < SingleSignOn
     "SSO_NONCE_#{nonce}"
   end
 
-
   def lookup_or_create_user
     sso_record = SingleSignOnRecord.where(external_id: external_id).first
-    if sso_record && sso_record.user
+    
+    if sso_record && user = sso_record.user
       sso_record.last_payload = unsigned_payload
-      sso_record.save
     else
+      user = match_email_or_create_user
+      sso_record = user.single_sign_on_record
+    end
+    
+    # if the user isn't new or it's attached to the SSO record we might be overriding username or email
+    unless user.new_record?
+      change_external_attributes_and_override(sso_record, user)
+    end
+
+    if sso_record && (user = sso_record.user) && !user.active
+      user.active = true
+      user.save
+      user.enqueue_welcome_message('welcome_user')
+    end
+    
+    # optionally save the user and sso_record if they have changed
+    user.save!
+    sso_record.save!
+
+    sso_record && sso_record.user
+  end
+  
+  private
+  
+  def match_email_or_create_user
     user = User.where(email: Email.downcase(email)).first
 
     user_params = {
@@ -58,21 +82,39 @@ class DiscourseSingleSignOn < SingleSignOn
       if sso_record = user.single_sign_on_record
         sso_record.last_payload = unsigned_payload
         sso_record.external_id = external_id
-          sso_record.save!
       else
         sso_record = user.create_single_sign_on_record(last_payload: unsigned_payload,
-                                            external_id: external_id)
+                                          external_id: external_id,
-        end
+                                          external_username: username,
-      end
+                                          external_email: email,
-    end
+                                          external_name: name)
-
-    if sso_record && (user = sso_record.user) && !user.active
-      user.active = true
-      user.save
-      user.enqueue_welcome_message('welcome_user')
-    end
-
-    sso_record && sso_record.user
       end
     end
     
+    user
+  end
+  
+  def change_external_attributes_and_override(sso_record, user)
+    if SiteSetting.sso_overrides_email && email != sso_record.external_email
+      # set the user's email to whatever came in the payload
+      user.email = email
+    end
+    
+    if SiteSetting.sso_overrides_username && username != sso_record.external_username && user.username != username
+      # we have an external username change, and the user's current username doesn't match
+      # run it through the UserNameSuggester to override it
+      user.username = UserNameSuggester.suggest(username || name || email)  
+    end
+    
+    if SiteSetting.sso_overrides_name && name != sso_record.external_name && user.name != name
+      # we have an external name change, and the user's current name doesn't match
+      # run it through the name suggester to override it
+      user.name = User.suggest_name(name || username || email)
+    end
+    
+    # change external attributes for sso record
+    sso_record.external_username = username
+    sso_record.external_email = email
+    sso_record.external_name = name
+  end
+end

config/locales/server.en.yml

@@ -679,6 +679,9 @@ en:
     enable_sso: "Enable single sign on via an external site"
     sso_url: "URL of single sign on endpoint"
     sso_secret: "Secret string used to encrypt/decrypt SSO information, be sure it is 10 chars or longer"
+    sso_overrides_email: "Overrides local email with external site email from SSO payload (WARNING: discrepancies can occur due to normalization of local emails)"
+    sso_overrides_username: "Overrides local username with external site username from SSO payload (WARNING: discrepancies can occur due to differences in username length/requirements)"
+    sso_overrides_name: "Overrides local name with external site name from SSO payload (WARNING: discrepancies can occur due to normalization of local names)"
 
     enable_local_logins: "Enable traditional, local username and password authentication"
     enable_local_account_create: "Enable creating new local accounts"

config/site_settings.yml

@@ -78,6 +78,15 @@ users:
     default: ''
   sso_secret:
     defalt: ''
+  sso_overrides_email:
+    client: true
+    default: false
+  sso_overrides_username:
+    client: true
+    default: false
+  sso_overrides_name:
+    client: true
+    default: false
   enable_local_logins:
     client: true
     default: true

db/migrate/20140228005443_add_external_username_to_single_sign_on_record.rb

@@ -0,0 +1,5 @@
+class AddExternalUsernameToSingleSignOnRecord < ActiveRecord::Migration
+  def change
+    add_column :single_sign_on_records, :external_username, :string
+  end
+end

db/migrate/20140228173431_add_external_email_and_external_name_to_single_sign_on_record.rb

@@ -0,0 +1,6 @@
+class AddExternalEmailAndExternalNameToSingleSignOnRecord < ActiveRecord::Migration
+  def change
+    add_column :single_sign_on_records, :external_email, :string
+    add_column :single_sign_on_records, :external_name, :string
+  end
+end

lib/single_sign_on.rb

@@ -1,5 +1,5 @@
 class SingleSignOn
-  ACCESSORS = [:nonce, :name, :username, :email, :about_me, :external_id]
+  ACCESSORS = [:nonce, :name, :username, :email, :about_me, :external_email, :external_username, :external_name, :external_id]
   FIXNUMS = []
   NONCE_EXPIRY_TIME = 10.minutes
 

spec/controllers/session_controller_spec.rb

@@ -35,7 +35,8 @@ describe SessionController do
       sso = get_sso("/")
       user = Fabricate(:user)
       sso.email = user.email
-      sso.external_id = "abc"
+      sso.external_id = 'abc'
+      sso.username = 'sam'
 
       get :sso_login, Rack::Utils.parse_query(sso.payload)
 
@@ -44,6 +45,7 @@ describe SessionController do
       logged_on_user.email.should == user.email
       
       logged_on_user.single_sign_on_record.external_id.should == "abc"
+      logged_on_user.single_sign_on_record.external_username.should == 'sam'
     end
 
     it 'allows you to create an account' do
@@ -63,11 +65,11 @@ describe SessionController do
       logged_on_user.username.should == 'sam'
 
       logged_on_user.single_sign_on_record.external_id.should == "666"
+      logged_on_user.single_sign_on_record.external_username.should == 'sam'
       logged_on_user.active.should == true
     end
     
     it 'allows login to existing account with valid nonce' do
-
       sso = get_sso('/hello/world')
       sso.external_id = '997'
 
@@ -87,7 +89,78 @@ describe SessionController do
       # nonce is bad now
       get :sso_login, Rack::Utils.parse_query(sso.payload)
       response.code.should == '500'
+    end
     
+    describe 'local attribute ovveride from SSO payload' do
+      before do
+        SiteSetting.stubs("sso_overrides_email").returns(true)
+        SiteSetting.stubs("sso_overrides_username").returns(true)
+        SiteSetting.stubs("sso_overrides_name").returns(true)
+        
+        @user = Fabricate(:user)
+        
+        @sso = get_sso('/hello/world')
+        @sso.external_id = '997'
+        
+        @reversed_username = @user.username.reverse
+        @sso.username = @reversed_username
+        
+        @sso.email = "#{@reversed_username}@garbage.org"
+        @reversed_name = @user.name.reverse
+        
+        @sso.name = @reversed_name
+        
+        @suggested_username = UserNameSuggester.suggest(@sso.username || @sso.name || @sso.email)
+        @suggested_name = User.suggest_name(@sso.name || @sso.username || @sso.email) 
+        
+        @user.create_single_sign_on_record(external_id: '997', last_payload: '')
+      end
+      
+      it 'stores the external attributes' do
+        get :sso_login, Rack::Utils.parse_query(@sso.payload)
+        
+        @user.single_sign_on_record.reload
+        @user.single_sign_on_record.external_username.should == @sso.username
+        @user.single_sign_on_record.external_email.should == @sso.email
+        @user.single_sign_on_record.external_name.should == @sso.name
+      end
+      
+      it 'overrides attributes' do
+        get :sso_login, Rack::Utils.parse_query(@sso.payload)
+        
+        logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+      
+        logged_on_user.username.should == @suggested_username
+        logged_on_user.email.should == "#{@reversed_username}@garbage.org"
+        logged_on_user.name.should == @suggested_name
+      end
+    
+      it 'does not change matching attributes for an existing account' do
+        @sso.username = @user.username
+        @sso.name = @user.name
+        @sso.email = @user.email
+        
+        get :sso_login, Rack::Utils.parse_query(@sso.payload)
+        
+        logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+        logged_on_user.username.should == @user.username
+        logged_on_user.name.should == @user.name
+        logged_on_user.email.should == @user.email
+      end
+      
+      it 'does not change attributes for unchanged external attributes' do
+        @user.single_sign_on_record.external_username = @sso.username
+        @user.single_sign_on_record.external_email = @sso.email
+        @user.single_sign_on_record.external_name = @sso.name
+        @user.single_sign_on_record.save
+        
+        get :sso_login, Rack::Utils.parse_query(@sso.payload)
+    
+        logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+        logged_on_user.username.should == @user.username
+        logged_on_user.email.should == @user.email
+        logged_on_user.name.should == @user.name
+      end
     end  
   end