commit 1e8a50848d3636241f12ae584183ba19934cd120
Author: Jan Pazdziora <jpazdziora@redhat.com>
Date:   Thu Apr 17 16:44:20 2014 +0200

    fixes #5242 - Update external user attributes and group membership on every logon

diff --git a/app/models/user.rb b/app/models/user.rb
index beb5d05e1..f4a605a66 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -200,18 +200,32 @@ class User < ActiveRecord::Base
   end
 
   def self.find_or_create_external_user(attrs, auth_source_name)
+    external_groups = attrs.delete(:groups)
+    auth_source = AuthSource.find_by_name(auth_source_name)
+
+    # existing user, we'll update them
     if (user = unscoped.find_by_login(attrs[:login]))
+      # we know this auth source and it's user's auth source, we'll update user attributes
+      if auth_source && (user.auth_source_id == auth_source.id)
+        auth_source_external_groups = auth_source.external_usergroups.pluck(:usergroup_id)
+        new_usergroups = user.usergroups.includes(:external_usergroups).where('usergroups.id NOT IN (?)', auth_source_external_groups)
+
+        new_usergroups += auth_source.external_usergroups.includes(:usergroup).where(:name => external_groups).map(&:usergroup)
+        user.update_attributes(Hash[attrs.select { |k, v| v.present? }])
+        user.usergroups = new_usergroups.uniq
+      end
+
       return true
+    # not existing user and creating is disabled by settings
     elsif auth_source_name.nil?
       return false
+    # not existing user and auth source is set, we'll create the user and auth source if needed
     else
       User.as :admin do
-        options = { :name => auth_source_name }
-        auth_source = AuthSource.where(options).first || AuthSourceExternal.create!(options)
-        external_groups = attrs.delete(:groups)
+        auth_source = AuthSourceExternal.create!(:name => auth_source_name) if auth_source.nil?
         user = User.create!(attrs.merge(:auth_source => auth_source))
         if external_groups.present?
-          user.usergroups = ExternalUsergroup.where(:auth_source_id => auth_source, :name => external_groups).map(&:usergroup).uniq
+          user.usergroups = auth_source.external_usergroups.where(:name => external_groups).map(&:usergroup).uniq
         end
         user.post_successful_login
       end
diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index a6c81e0b5..f80a4f526 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -413,16 +413,34 @@ class UserTest < ActiveSupport::TestCase
     # with existing user groups that are assigned
     apache_source = AuthSourceExternal.find_or_create_by_name('apache_module')
     usergroup = FactoryGirl.create :usergroup
-    FactoryGirl.create :external_usergroup, :usergroup => usergroup, 
-                                            :auth_source => apache_source, 
-                                            :name => usergroup.name
+    external = FactoryGirl.create :external_usergroup, :usergroup => usergroup,
+                                  :auth_source => apache_source,
+                                  :name => usergroup.name
     assert User.find_or_create_external_user({:login => 'not_existing_user_4',
-                                              :groups => [usergroup.name, 'does-not-exists-for-sure-123']},
+                                              :groups => [external.name, 'does-not-exists-for-sure-123']},
                                              apache_source.name)
     user = User.find_by_login('not_existing_user_4')
     assert_equal [usergroup], user.usergroups
   end
 
+  test ".find_or_create_external_user updates external groups" do
+    apache_source = AuthSourceExternal.find_or_create_by_name('apache_module')
+    user = FactoryGirl.create(:user, :auth_source => apache_source)
+    external1 = FactoryGirl.create(:external_usergroup, :auth_source => apache_source)
+    external2 = FactoryGirl.create(:external_usergroup, :auth_source => apache_source)
+    usergroup = FactoryGirl.create(:usergroup)
+    user.usergroups << [external1.usergroup, usergroup]
+
+    refute_equal 'foo@example.com', user.mail
+    assert User.find_or_create_external_user({:login => user.login,
+                                              :groups => [external2.name],
+                                              :mail => 'foo@example.com'},
+                                             apache_source.name)
+    user.reload
+    assert_includes user.usergroups, external2.usergroup
+    assert_includes user.usergroups, usergroup
+    assert_equal 'foo@example.com', user.mail
+  end
 
   test ".try_to_auto_create_user" do
     AuthSourceLdap.any_instance.stubs(:authenticate).returns({ :firstname => "Foo", :lastname => "Bar", :mail => "baz@qux.com" })
