Overwrite the NameID when specified in use_as_nameid ARP setting #1308
Conversation
Boyscout work. This was done to make testing easier, but mostly to follow the best practice
This resolver actually searches the ARP for a given SP and resolves if NameID substitution should take place. If it decides it should, it returns the attributename of which attr to take the new NameId value
The output filter will handle the name id substitution logic. As it already juggles the correctly setting of the desired identity (nameid, collabpersonid) values.
MKodde
force-pushed
the
feature/use_as_nameid
branch
from
12257be to
b6f59b0
Compare
| public function findNameIdSubstitute(AttributeReleasePolicy $arp, array $responseAttributes): ?string | ||
| { | ||
| $substituteAttribute = $arp->findNameIdSubstitute(); | ||
| if ($substituteAttribute !== null && array_key_exists($substituteAttribute, $responseAttributes)) { |
There was a problem hiding this comment.
I would suggest to encapsulate this complex if in a descriptive private method like isValidAttibute
| if (isset($rule['use_as_nameid']) && $rule['use_as_nameid'] === true) { | ||
| if (array_key_exists('release_as', $rule)) { | ||
| return $rule['release_as']; | ||
| } |
There was a problem hiding this comment.
Can this be combined in a local method like ruleHasReleaseAs or so?
if ($this->ruleHasReleaseAs($rule)) {
return $rule['release_as'];
}
|
|
||
| // Find out if the EduPersonTargetedId is in the ARP of the destination SP. | ||
| // If the ARP is NULL this means no ARP = let everything through including ePTI. | ||
| // Otherwise, only add ePTI if it's actually in the ARP. |
There was a problem hiding this comment.
I think this comment needs to be reworded because the logic is now spread to other places.
Maybe here something like: if there's an ARP, but it does not contain the EPTI, we're done now.
And below: We arrive here if either: 1) the ARP is NULL, this means no ARP = let everything through including ePTI; or 2) the ARP is not null and does contain the EPTI attribute. In both cases, set the EPTI attribute.
An alternative is to change the logic and keep everything related to EPTI together. So remove this condition and return here, but check for ARP === null OR ARP contains EPTI below.
There was a problem hiding this comment.
Thanks for the suggestion. I think improving the inline doc is a nice improvement. Moving the logic to the input filter will not result in the desired outcome. This is what I tried first. But then the AddIdentityAttributes output filter would overwrite the NameId again.
Or do you suggest another solution?
There was a problem hiding this comment.
I think just a very clear comment like there's now suffices to alleviate the concern of understandability.
| $arp = $destinationMetadata->getAttributeReleasePolicy(); | ||
| if (!is_null($arp) && !$arp->hasAttribute(Constants::EPTI_URN_MACE)) { | ||
| return; | ||
| if (!is_null($arp)) { |
There was a problem hiding this comment.
Should we go through the trouble to resolve a nameid above if we're going to overwrite it anyway?
There was a problem hiding this comment.
I looked at it like this:
- We need to set the nameId to the resolved value.
- We might need to overwrite it with the substitute value.
What could be done is to move the NameIdResolver logic down below, and only resolve the nameId if we did not use the substitute.
There was a problem hiding this comment.
Alright, there's two ways to look at this, you do what you think is best
Thanks @thijskh for suggesting this improvement
Prevent possible unnecesary resolving of the NameId This makes the logic slightly less intuitive (INHM) but prevents the resolver from doing it's work without reason.
MKodde
force-pushed
the
feature/use_as_nameid
branch
from
fb00eed to
2acb8c9
Compare
This PR aims to close the feature described in issue: #1301 For functional details, please study the well described feature there.
For this PR some noteworthy things passed: