Fixes #31165 - sync usergroups only for given user #8204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ezr-ondrej
changed the title
|
Issues: #31165 |
ezr-ondrej
force-pushed
the
ldap_sync_slow_on_login
branch
from
d49bbda
to
caec689
Compare
tbrisker
suggested changes
Dec 21, 2020
| @@ -134,15 +134,14 @@ def update_usergroups(login) | |||
| .where(ExternalUsergroup.arel_table[:auth_source_id].eq(id)) | |||
| .pluck(:id) | |||
There was a problem hiding this comment.
Non blocking suggestion:
perhaps this should be called current_external_ids so it's clearer?
You could get rid of the joins and arel here with something like:
current_external_ids = user.usergroups
.where(id: ExternalUsergroup.where(auth_source_id: id).select(:usergroup_id))
.pluck(:id)| Usergroup.where(id: usergroup_ids.uniq).find_each do |usergroup| | ||
| refresh_usergroup_members(usergroup) | ||
| end | ||
| external_mapping_ids = ExternalUsergroup.where(auth_source_id: id) |
There was a problem hiding this comment.
if we rename the variables to be more descriptive, this would perhaps be updated_external_ids?
There was a problem hiding this comment.
updated dosn't sound correct as those are ALL not just UPDATED ids 🤔
There was a problem hiding this comment.
I went with actual_external_ids ... ?
There was a problem hiding this comment.
OR correct_external_ids ?
Comment on lines
141
to
142
| user.usergroup_ids -= (usergroup_ids - external_mapping_ids) | ||
| user.usergroup_ids += (external_mapping_ids - usergroup_ids) |
There was a problem hiding this comment.
How about:
Suggested change
| user.usergroup_ids -= (usergroup_ids - external_mapping_ids) | |
| user.usergroup_ids += (external_mapping_ids - usergroup_ids) | |
| user.usergroup_ids = user.usergroup_ids - current_external_ids + updated_external_ids | |
ezr-ondrej
force-pushed
the
ldap_sync_slow_on_login
branch
from
caec689
to
c1c9b57
Compare
ezr-ondrej
force-pushed
the
ldap_sync_slow_on_login
branch
5 times, most recently
from
5829d85
to
7eb201b
Compare
|
Note I've added one more test to cover the auditing (as I was not 100% sure it works without validation) :) |
In @17c4b47 we've disabled synchronization of groups for user from different auth sources. That gave us oportunity to sync the groups directly from fetched groups. This is changing the Auditing of group membership, as of now it is being audited as User update, not Usergroup update.
ezr-ondrej
force-pushed
the
ldap_sync_slow_on_login
branch
from
7eb201b
to
ca17c26
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 17c4b47 we've disabled synchronization of groups for user from different auth sources.
That gave us oportunity to sync the groups directly from fetched groups.
This is changing the Auditing of group membership, as of now it is being audited as User update, not Usergroup update.
See #6388 for previous change - we kept back there and didn't want to regress usage of synchronization of groups from two authsources for given user, but that should not be a valid case and the speedup is significat if we don't support it.