[Fixes #14107] Add configurable sync strategies for Social Account group memberships#14108
[Fixes #14107] Add configurable sync strategies for Social Account group memberships#14108
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable synchronization strategy for user groups during social login, allowing for full, safe, or no synchronization via the SOCIALACCOUNT_SYNC_USER_GROUPS_ON_LOGIN setting. A potential logic error was identified in the SAFE_SYNC mode where an empty group list from the provider could cause the synchronization to skip entirely instead of clearing existing local groups.
geonode/people/adapters.py
Outdated
|
|
||
| # check here if user is member already of other groups and remove it form the ones that are not declared here... | ||
| # If Azure returns no group data, skip synchronization entirely | ||
| if sync_strategy == "SAFE_SYNC" and (groups is None or groups == ""): |
There was a problem hiding this comment.
The SAFE_SYNC check (groups is None or groups == "") is problematic because groups is initialized using or "" on line 293. In Python, [] or "" evaluates to "". This means that if the social provider returns an empty list of groups (indicating the user belongs to no groups), groups will become "", triggering this condition and returning early.
Consequently, SAFE_SYNC will fail to remove a user from their groups when they no longer belong to any groups on the provider side. If the intention was to only skip synchronization when the group data is missing from the response, this logic is currently ambiguous. Additionally, groups is None is redundant here as the value will always be at least an empty string due to the assignment logic.
This PR was created according to this issue: #14107
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.