Skip to content

Commit

Permalink
Set the ACS base URL (including domain) correctly
Browse files Browse the repository at this point in the history
As a work-around for a problem where ACS was read as e.g. domain/acs
instead of domain/path/acs, explicitly setting the path (via the base
url) was done. But we need the domain to match the actual loaded domain,
not the service provider entity ID.

It is important since there is already another major version of this
saml module that no changes that would require existing users to make
changes to their code be introduced (requiring a major version). This is
the reason we're now relying on Director::absoluteBaseUrl instead of
letting the SAML library do what it does (in cases where the workaround
is not needed).

When using Director::absoluteBaseUrl instead of letting the SAML library
build it's own interpretation, the use of other Util class options become
redundant - they're not used anyway, so there is no point in having any
further/alternative options. A hook point is introduced to further
manipulate things like that should anyone have the need.
  • Loading branch information
NightJar authored and satrun77 committed Apr 2, 2024
1 parent 8529d1b commit ed14d79
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 3 deletions.
9 changes: 6 additions & 3 deletions src/Control/SAMLController.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,12 @@ public function acs()
$uniqueErrorId = uniqid('SAML-');

// Force php-saml module to use the current absolute base URL (e.g. https://www.example.com/saml). This avoids
// errors that we otherwise get when having a multi-directory ACS URL like /saml/acs).
// errors that we otherwise get when having a multi-directory ACS URL (like /saml/acs).
// See https://github.com/onelogin/php-saml/issues/249
Utils::setBaseURL(Controller::join_links($auth->getSettings()->getSPData()['entityId'], 'saml'));
Utils::setBaseURL(Controller::join_links(Director::absoluteBaseURL(), 'saml'));

// Hook point to allow extensions to further modify or unset any of the above base url coersion
$this->extend('onBeforeAcs', $uniqueErrorId);

// Attempt to process the SAML response. If there are errors during this, log them and redirect to the generic
// error page. Note: This does not necessarily include all SAML errors (e.g. we still need to confirm if the
Expand Down Expand Up @@ -269,7 +272,7 @@ protected function getRedirect()
if ($relayState && Director::is_site_url($relayState)) {
return $this->redirect($relayState);
}

// Spoofing attack, redirect to homepage instead of spoofing url
if ($this->getRequest()->getSession()->get('BackURL')
&& !Director::is_site_url($this->getRequest()->getSession()->get('BackURL'))) {
Expand Down
1 change: 1 addition & 0 deletions src/Services/SAMLConfiguration.php
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ public function asArray()
$spEntityId = Injector::inst()->convertServiceProperty($sp['entityId']);
$extraAcsBaseUrl = (array)$config->get('extra_acs_base');
$currentBaseUrl = Director::absoluteBaseURL();
$count = count($extraAcsBaseUrl);
$acsBaseUrl = in_array($currentBaseUrl, $extraAcsBaseUrl) ? $currentBaseUrl : $spEntityId;

$spX509Cert = Injector::inst()->convertServiceProperty($sp['x509cert']);
Expand Down

0 comments on commit ed14d79

Please sign in to comment.