Yes, that is correct. Every user is explicitly assigned 1 auth source which is used during authentication. But user can have multiple external usergroup mappings potentially from different auth sources. So we should make sure we only apply changes from those mappings that are relevant, in other words we’ll use only mappings for user’s auth source.
If I understand the patch correctly, you’re replacing the logic from ExternalUsergroup#refresh with simply adding/removing user who logs in to/from external user group. I think that’s good improvement, few thoughts:
- you’ll need to
next unless auth_source.respond_to?(:users_in_group) since some auth sources don’t support fetching users from groups.
- you may also want to skip this entirely if user’s authsource is not ldap one (there’s internal that uses credetials in DB, there’s external which only relies on REMOTE_USER env variable, there’s hidden for hidden system users, and there’s at least one introduced in some plugin inheriting from hidden)
- we need to make sure the change creates audit record
I think it would be better to continue discussion in a PR 
