Allow user group mapping #51
Conversation
Jianbinzhu
force-pushed
the
feature/user-group-mapping
branch
from
0ff8f4d to
7936e96
Compare
| /** | ||
| * @var string | ||
| */ | ||
| private static $group_claims_field; |
There was a problem hiding this comment.
| private static $group_claims_field; | |
| private static string $group_claims_field; |
| /** | ||
| * @var array | ||
| */ | ||
| private static $dependencies = [ |
There was a problem hiding this comment.
| private static $dependencies = [ | |
| private static array $dependencies = [ |
| * @param Member $member | ||
| * @return Member | ||
| */ | ||
| public function map($attributes, $member): Member |
There was a problem hiding this comment.
| public function map($attributes, $member): Member | |
| public function map(array $attributes, Member $member): Member |
| */ | ||
| public function map($attributes, $member): Member | ||
| { | ||
| $groups = $this->config()->get('group_claims_field'); |
There was a problem hiding this comment.
config() is a static method
| $groups = $this->config()->get('group_claims_field'); | |
| $groups = self::config()->get('group_claims_field'); |
There was a problem hiding this comment.
not a big deal but this style is valid and in our docs :) https://docs.silverstripe.org/en/5/developer_guides/configuration/configuration/#configuration-masks
| // Get groups from saml response | ||
| $groupTitles = $attributes[$groups]; | ||
|
|
||
| foreach ($groupTitles as $groupTitle) { |
There was a problem hiding this comment.
Move the query outside the loop, so we are executing one query that fetches everything into Arraylist. then you can look at the titles and check if we have the group or if we need a new one.
| $group->write(); | ||
| } | ||
|
|
||
| $member->Groups()->add($group); |
There was a problem hiding this comment.
I think we also want to remove users from groups they are no longer part of - this probably requires marking groups as 'synced' in some way so we know which ones to remove them from. Its quite likely that a site would have some groups synced with saml but others that are not (we've got at least one project with this requirement) - we wouldn't want to remove users from these 'manual' groups.
| */ | ||
| public function map($attributes, $member): Member | ||
| { | ||
| $groups = $this->config()->get('group_claims_field'); |
There was a problem hiding this comment.
not a big deal but this style is valid and in our docs :) https://docs.silverstripe.org/en/5/developer_guides/configuration/configuration/#configuration-masks
|
Hey @Jianbinzhu, are you still keen to merge this in? There are a few merge conflicts now, but I'd be happy to merge once those are resolved. |
|
I've taken a look at this recently @madmatt - there is another branch that is more up to date, and I'm looking to combine it into some work that's set to come after #68 gets merged ;) |
This will allow group mapping enable if needed via
and then specify the group claims field