providers/scim: modify filtergroup(s) behavior to allow (multi-)group filtering#13550
providers/scim: modify filtergroup(s) behavior to allow (multi-)group filtering#13550ImmanuelVonNeumann wants to merge 8 commits intogoauthentik:mainfrom
Conversation
✅ Deploy Preview for authentik-storybook canceled.
|
✅ Deploy Preview for authentik-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #13550 +/- ##
==========================================
+ Coverage 92.77% 92.82% +0.05%
==========================================
Files 794 797 +3
Lines 40545 40857 +312
==========================================
+ Hits 37614 37926 +312
Misses 2931 2931
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi, thanks for the PR! When I added the There is also an already existing option for doing this more dynamically which is having a custom user/group mapping that dynamically raises |
Thanks for your fast response, @BeryJu! From my understanding the final solution could be that:
Is this correct? One further question: I could not find documentation on the implications of the field "filterset_fields" within the model. |
Correct, I think having One potential issue that could arise when this is merged is what happens with groups that were previously provisioned and are now outside of the group filter, they would still exist in authentik and the remote system but would not get further updates (and currently would not be correctly deletable iirc)
|
…iltering Replaces the scim provider's argument filter_group with filter_groups. Allows multiple filter_groups to be selected. Only syncs groups within filter_groups and users which are members of at least one filter_group for scim.
10c87cb to
6ee9884
Compare
I believe so, if the previously sent state is not currently being tracked. To solve this, the sent state would need to be tracked so that differences to it can be identified and remediated on the next sync.
Thanks! |
|
I believe the checks failed due to issues in uv, can we do a rerun of failed jobs? |
|
@ImmanuelVonNeumann you might need to |
tanberry
left a comment
There was a problem hiding this comment.
Thank you @ImmanuelVonNeumann for this PR (and for the detailed description, much appreciated!). From a Docs perspective, the only comment/question is whether we should tell users how to configure the group filters, but maybe that is outside of this PR's scope, and/or super-simple and doesn't need further explanation? Otherwise it looks great to me, and thanks for remembering to update the docs!
I agree.
I can work on an updated version where:
|
by removing default users and groups which were interfering with counts
| # Get a queryset of all roups with consistent ordering | ||
| # according to the provider's settings | ||
| base = Group.objects.all() | ||
| if self.filter_groups.exists(): | ||
| base = base.filter(pk__in=self.filter_groups.all()) | ||
| return base.order_by("pk") |
There was a problem hiding this comment.
Additional feedback from existing users of the feature, this is actually a breaking change, and would prevent all groups from being synced, even if filtering users.
There was a problem hiding this comment.
Additional feedback from existing users of the feature, this is actually a breaking change, and would prevent all groups from being synced, even if filtering users.
Thank you very much for your feedback @rissson
However, I am not sure if I understand you correctly.
If you are saying that previously synced groups will not be synced when using a filter group, then that is indeed correct.
While the same users will be synced, with this change, the groups fill also be filtered by the selected filter groups.
However, from my understanding this is more in line with the filtering logic of most IdP solutions.
I have also noted this in the PR:
Since this does change group-syncing behavior (i.e. not syncing previously synced groups anymore) I suggest this change to be implemented in a major release. Additionally, blueprints for scim_providers need to adapt to the new name
filter_groups.
Some of the benefits I see here:
- Only groups that are designed for a specific scim sync are being evaluated (currently syncing can fail due to the scim-server. For example if the server expects a specific group attribute)
- Currently, groups cannot be isolated in the scim sync which can lead to information exposure when authentik is used with multi-tenancy (group from clientA will be synced to the scim-server of clientB)
- More in line with other IdP solutions (as noted above)
Alternatively, another attribute could be used for group-filtering.
However, I chose not to do this as it would be confusing to have an attribute named "filter group" and an additional attribute used to filter groups.
There was a problem hiding this comment.
@rissson I think I understand your point better now (please correct me if wrong).
As long as user-filtering is tied to filter groups there are use-cases (especially when groups shared among multiple "filter groups" are involved) which cannot be implemented with this solution.
Example:
There are two SCIM servers for it- and non-it personnel. However there is a group "first responder" which intersects both groups and should be available on both servers.
The only solution is to add this group to both filter groups, which in turn now syncs all members of "first responder", (even non-it/it personnel) to both servers.
I think the best solution here would be as mentioned by @BeryJu
When I added the
filter_groupit was actually more so thought of as a "temporary" solution with the intended solution being that the SCIM provider would use the policies bound to the application the provider is a backchannel provider of to figure out which users to sync. However this would still leave room for either always syncing all groups or syncing all groups the synced users in.
Where the sync of users is determined by their permissions to "view" the application of the backchannel provider and sync of groups is determined by the filter groups attribute.
|
Closing this in favor of #13947 |
Details
This PR aims to change add group filtering to the SCIMProvider by incorporating the following changes:
filter_grouptofilter_groups(including django migrations to keep a previously selected filter_group present)filter_groupsfilter_groupsare synced to the SCIM Serverfilter_groupsare synced to the SCIM ServerReason:
Most SCIM Clients allow groups to be filtered, however authentik currently does not.
This issue has been mentioned in #6065 which has been given the
enhancement/confirmed-label.Among the many use-cases one would be a multi-tenant setup where groups of one tenant should not be visible to another tenant.
Note
Since this does change group-syncing behavior (i.e. not syncing previously synced groups anymore) I suggest this change to be implemented in a major release.
Additionally, blueprints for scim_providers need to adapt to the new name
filter_groups.Fixes #6065
Checklist
ak test authentik/)make lint-fix)If an API change has been made
make gen-build)If changes to the frontend have been made
make web)If applicable
make website)