Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overwrite the NameID when specified in use_as_nameid ARP setting #1308

Merged
merged 6 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions library/EngineBlock/Application/DiContainer.php
Original file line number Diff line number Diff line change
Expand Up @@ -572,4 +572,20 @@ public function getAuthLogAttributes()
{
return $this->container->getParameter('auth.log.attributes');
}

/**
* @return EngineBlock_Saml2_NameIdResolver
*/
public function getNameIdResolver()
{
return new EngineBlock_Saml2_NameIdResolver($this->container->get('engineblock.compat.logger'));
}

/**
* @return EngineBlock_Arp_NameIdSubstituteResolver
*/
public function getNameIdSubstituteResolver()
{
return new EngineBlock_Arp_NameIdSubstituteResolver($this->container->get('engineblock.compat.logger'));
}
}
48 changes: 48 additions & 0 deletions library/EngineBlock/Arp/NameIdSubstituteResolver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php

use OpenConext\EngineBlock\Metadata\AttributeReleasePolicy;
use Psr\Log\LoggerInterface;

/**
* Copyright 2024 SURFnet B.V.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

class EngineBlock_Arp_NameIdSubstituteResolver
{
/**
* @var LoggerInterface
*/
private $logger;

public function __construct(LoggerInterface $logger)
{
$this->logger = $logger;
}

public function findNameIdSubstitute(AttributeReleasePolicy $arp, array $responseAttributes): ?string
{
$substituteAttribute = $arp->findNameIdSubstitute();
if ($substituteAttribute !== null && array_key_exists($substituteAttribute, $responseAttributes)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to encapsulate this complex if in a descriptive private method like isValidAttibute

$this->logger->notice(
sprintf(
'Found a NameId substitute ("use_as_nameid", %s will be used as NameID)',
$substituteAttribute
)
);
return reset($responseAttributes[$substituteAttribute]);
}
return null;
}
}
83 changes: 64 additions & 19 deletions library/EngineBlock/Corto/Filter/Command/AddIdentityAttributes.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
* limitations under the License.
*/

use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
use Psr\Log\LoggerInterface;
use SAML2\Constants;
use SAML2\XML\saml\NameID;

class EngineBlock_Corto_Filter_Command_AddIdentityAttributes extends EngineBlock_Corto_Filter_Command_Abstract
implements EngineBlock_Corto_Filter_Command_ResponseAttributesModificationInterface
Expand All @@ -27,8 +29,23 @@ class EngineBlock_Corto_Filter_Command_AddIdentityAttributes extends EngineBlock
*/
private $logger;

public function __construct(LoggerInterface $logger)
{
/**
* @var EngineBlock_Saml2_NameIdResolver
*/
private $nameIdResolver;

/**
* @var EngineBlock_Arp_NameIdSubstituteResolver
*/
private $substituteResolver;

public function __construct(
EngineBlock_Saml2_NameIdResolver $nameIdResolver,
EngineBlock_Arp_NameIdSubstituteResolver $resolver,
LoggerInterface $logger
) {
$this->nameIdResolver = $nameIdResolver;
$this->substituteResolver = $resolver;
$this->logger = $logger;
}

Expand All @@ -54,28 +71,56 @@ public function execute()
$this->_server->getRepository()
);

// Resolve what NameID we should send the destination.
$resolver = new EngineBlock_Saml2_NameIdResolver($this->logger);
$nameId = $resolver->resolve(
$this->_request,
$this->_response,
$destinationMetadata,
$this->_collabPersonId
);
$isResolved = false;

$this->logger->info('Setting the NameId on the Assertion');
$this->_response->getAssertion()->setNameId($nameId);
$arp = $destinationMetadata->getAttributeReleasePolicy();
if (!is_null($arp)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we go through the trouble to resolve a nameid above if we're going to overwrite it anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at it like this:

  1. We need to set the nameId to the resolved value.
  2. 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.

Copy link
Member

@thijskh thijskh Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, there's two ways to look at this, you do what you think is best

// Now check if we should update the NameID value according to the 'use_as_nameid' directive in the ARP.
$arpSubstitute = $this->substituteResolver->findNameIdSubstitute($arp, $this->getResponseAttributes());
if ($arpSubstitute !== null) {
$nameId = new NameID();
$nameId->setFormat(Constants::NAMEID_UNSPECIFIED);
$nameId->setValue($arpSubstitute);
$isResolved = true;
$this->_response->getAssertion()->setNameId($nameId);
}

// If there's an ARP, but it does not contain the EPTI, we're done now.
if (!$arp->hasAttribute(Constants::EPTI_URN_MACE)) {
return;
}
}

if (!$isResolved || !isset($nameId)){
// Resolve what NameID we should send the destination.
$resolver = new EngineBlock_Saml2_NameIdResolver($this->logger);
$nameId = $resolver->resolve(
$this->_request,
$this->_response,
$destinationMetadata,
$this->_collabPersonId
);

// 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 acutally in the ARP.
$arp = $destinationMetadata->getAttributeReleasePolicy();
if (!is_null($arp) && !$arp->hasAttribute(Constants::EPTI_URN_MACE)) {
return;
$this->logger->info('Setting the NameId on the Assertion');
$this->_response->getAssertion()->setNameId($nameId);
}

// 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.
$this->logger->info('Adding the EduPersonTargetedId on the Assertion');
$this->_responseAttributes[Constants::EPTI_URN_MACE] = [ $nameId ];
$this->_responseAttributes[Constants::EPTI_URN_MACE] = [$nameId];
}

private function resolveNameId(ServiceProvider $destinationMetadata): NameID
{
// Resolve what NameID we should send the destination.
return $this->nameIdResolver->resolve(
$this->_request,
$this->_response,
$destinationMetadata,
$this->_collabPersonId
);
}
}
14 changes: 12 additions & 2 deletions library/EngineBlock/Corto/Filter/Command/ProvisionUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,24 @@
* limitations under the License.
*/

use OpenConext\EngineBlockBridge\Authentication\Repository\UserDirectoryAdapter;
use SAML2\Constants;
use SAML2\XML\saml\NameID;

class EngineBlock_Corto_Filter_Command_ProvisionUser extends EngineBlock_Corto_Filter_Command_Abstract
implements EngineBlock_Corto_Filter_Command_ResponseModificationInterface,
EngineBlock_Corto_Filter_Command_CollabPersonIdModificationInterface
{
/**
* @var UserDirectoryAdapter
*/
private $userDirectory;

public function __construct(UserDirectoryAdapter $userDirectory)
{
$this->userDirectory = $userDirectory;
}

/**
* {@inheritdoc}
*/
Expand All @@ -41,8 +52,7 @@ public function getCollabPersonId()

public function execute()
{
$userDirectory = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer()->getUserDirectory();
$user = $userDirectory->identifyUser($this->_responseAttributes);
$user = $this->userDirectory->identifyUser($this->_responseAttributes);

$collabPersonIdValue = $user->getCollabPersonId()->getCollabPersonId();
$this->setCollabPersonId($collabPersonIdValue);
Expand Down
4 changes: 3 additions & 1 deletion library/EngineBlock/Corto/Filter/Input.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ public function getCommands()
new EngineBlock_Corto_Filter_Command_AddGuestStatus(),

// Figure out the collabPersonId
new EngineBlock_Corto_Filter_Command_ProvisionUser(),
new EngineBlock_Corto_Filter_Command_ProvisionUser(
$diContainer->getUserDirectory()
),

// Aggregate additional attributes for this Service Provider
new EngineBlock_Corto_Filter_Command_AttributeAggregator(
Expand Down
3 changes: 1 addition & 2 deletions library/EngineBlock/Corto/Filter/Output.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ public function getCommands()
{
$diContainer = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer();
$logger = EngineBlock_ApplicationSingleton::getLog();

return array(
// If EngineBlock is in Processing mode (redirecting to it's self)
// Then don't continue with the rest of the modifications
Expand All @@ -84,7 +83,7 @@ public function getCommands()
new EngineBlock_Corto_Filter_Command_ApplyTrustedProxyBehavior($logger),

// Add the appropriate NameID to the 'eduPeronTargetedID' and the Assertions NameId.
new EngineBlock_Corto_Filter_Command_AddIdentityAttributes($logger),
new EngineBlock_Corto_Filter_Command_AddIdentityAttributes($diContainer->getNameIdResolver(), $diContainer->getNameIdSubstituteResolver(), $logger),

// Convert all attributes to their OID format (if known) and add these.
new EngineBlock_Corto_Filter_Command_DenormalizeAttributes(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
namespace OpenConext\EngineBlock\Metadata;

use InvalidArgumentException;
use function array_key_exists;

class AttributeReleasePolicy
{
Expand Down Expand Up @@ -139,6 +140,9 @@ public function findNameIdSubstitute(): ?string
foreach ($this->attributeRules as $name => $rules) {
foreach ($rules as $rule) {
if (isset($rule['use_as_nameid']) && $rule['use_as_nameid'] === true) {
if (array_key_exists('release_as', $rule)) {
return $rule['release_as'];
}
Comment on lines 142 to +145
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be combined in a local method like ruleHasReleaseAs or so?

if ($this->ruleHasReleaseAs($rule)) {
    return $rule['release_as'];
}

return $name;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ Feature:
And a Service Provider named "Stepup Gateway"
And a Service Provider named "Stepup SelfService"
And a Service Provider named "Release As"
And a Service Provider named "Use as NameID"
And a Service Provider named "Use as NameID and Release As"
And SP "Empty ARP" allows no attributes
And SP "Wildcard ARP" allows an attribute named "urn:mace:dir:attribute-def:uid"
And SP "Wrong Value ARP" allows an attribute named "urn:mace:terena.org:attribute-def:schacHomeOrganization" with value "example.edu"
Expand All @@ -31,6 +33,10 @@ Feature:
And SP "Stepup Gateway" allows an attribute named "urn:mace:terena.org:attribute-def:eduPersonAffiliation"
And SP "Stepup SelfService" allows an attribute named "urn:mace:dir:attribute-def:uid"
And SP "Release As" allows an attribute named "urn:mace:dir:attribute-def:uid" released as "Kustom-UiD"
And SP "Use as NameID" allows an attribute named "urn:mace:terena.org:attribute-def:schacHomeOrganization"
And SP "Use as NameID" uses the value of attribute "urn:mace:terena.org:attribute-def:schacHomeOrganization" as the NameId
And SP "Use as NameID and Release As" allows an attribute named "urn:mace:terena.org:attribute-def:schacHomeOrganization" released as "Kustom-schacHomeOrganization"
And SP "Use as NameID and Release As" uses the value of attribute "urn:mace:terena.org:attribute-def:schacHomeOrganization" as the NameId
And feature "eb.run_all_manipulations_prior_to_consent" is disabled

Scenario: As a user for an Idp SP without ARPs I get all attributes
Expand Down Expand Up @@ -89,6 +95,32 @@ Feature:
Then the response should not contain "urn:mace:dir:attribute-def:uid"
And the response should contain "Kustom-UiD"

Scenario: As a user for an SP the ARP can overwrite the NameId with a given attribute value
When I log in at "Use as NameID"
And I pass through EngineBlock
And I pass through the IdP
Then the response should contain "urn:mace:terena.org:attribute-def:schacHomeOrganization"
When I give my consent
And I pass through EngineBlock
# The NameID is overwritten with the value of the schacHomeOrganization
# The IdP always releases the value: engine-test-stand.openconext.org for this schacHomeOrganization
# See: MockIdentityProviderFactory::generateDefaultResponse
Then the response should contain "urn:mace:terena.org:attribute-def:schacHomeOrganization"
# The name id always becomes unspecified after substitution
And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" and text()="engine-test-stand.openconext.org"]'

Scenario: As a user for an SP the ARP can overwrite the NameId with a given attribute value and rename the attribute at the same time
When I log in at "Use as NameID and Release As"
And I pass through EngineBlock
And I pass through the IdP
Then the response should contain "urn:mace:terena.org:attribute-def:schacHomeOrganization"
And the response should not contain "Kustom-schacHomeOrganization"
When I give my consent
And I pass through EngineBlock
Then the response should not contain "urn:mace:terena.org:attribute-def:schacHomeOrganization"
And the response should contain "Kustom-schacHomeOrganization"
And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" and text()="engine-test-stand.openconext.org"]'

Scenario: As a user for an SP with a specific value ARP I do see the attribute if it has the right value
When I log in at "Right Value ARP"
And I pass through EngineBlock
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,19 @@ public function spAllowsAnAttributeNamedReleasedAs($spName, $arpAttribute, $rele
->save();
}

/**
* @Given /^SP "([^"]*)" uses the value of attribute "([^"]*)" as the NameId$/
**/
public function spOverridesNameId($spName, $attributeName)
{
/** @var MockServiceProvider $sp */
$sp = $this->mockSpRegistry->get($spName);

$this->serviceRegistryFixture
->substituteNameIdWithAttributeValue($sp->entityId(), $attributeName)
->save();
}

/**
* @Given /^SP "([^"]*)" allows an attribute named "([^"]*)" and configures it for aggregation from "([^"]*)"$/
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
use OpenConext\EngineBlock\Metadata\X509\X509CertificateLazyProxy;
use ReflectionClass;
use SAML2\Constants;
use function array_key_exists;

/**
* @SuppressWarnings("PMD")
Expand Down Expand Up @@ -401,11 +402,9 @@ public function allowAttributeReleasedAsForSp($entityId, $arpAttribute, $release
$rules = $arp->getAttributeRules();
}

$attributeSource = 'idp';

$arpRule = [
'value' => "*",
'source' => $attributeSource,
'source' => 'idp',
'release_as' => $releasedAs,
];

Expand All @@ -416,6 +415,37 @@ public function allowAttributeReleasedAsForSp($entityId, $arpAttribute, $release
return $this;
}


public function substituteNameIdWithAttributeValue(string $entityId, $attributeName)
{
/** @var AttributeReleasePolicy $arp */
$arp = $this->getServiceProvider($entityId)->attributeReleasePolicy;

$rules = [];

if (!empty($arp)) {
$rules = $arp->getAttributeRules();
}

$arpRule = [
'value' => "*",
'source' => 'idp',
'use_as_nameid' => true,
];
// It could be the rule was already added (for example to set the release_as directive)
// in that case, load the existing rule and add the 'use_as_nameid'
if (array_key_exists($attributeName, $rules)) {
$arpRule = $rules[$attributeName][0];
$arpRule['use_as_nameid'] = true;
}

$rules[$attributeName] = [$arpRule];

$this->getServiceProvider($entityId)->attributeReleasePolicy = new AttributeReleasePolicy($rules);

return $this;
}

public function setSpWorkflowState($entityId, $workflowState)
{
$this->getServiceProvider($entityId)->workflowState = $workflowState;
Expand Down
Loading