Merge pull request #63 from NightJar/main

silverstripe · Oct 29, 2024 · 297de70 · 297de70
2 parents 6b788c6 + c57397f
commit 297de70
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 25 deletions.
diff --git a/src/Control/SAMLController.php b/src/Control/SAMLController.php
@@ -73,9 +73,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

diff --git a/src/Services/SAMLConfiguration.php b/src/Services/SAMLConfiguration.php
@@ -3,9 +3,10 @@
 namespace SilverStripe\SAML\Services;
 
 use OneLogin\Saml2\Constants;
-use SilverStripe\Core\Injector\Injectable;
-use SilverStripe\Core\Config\Configurable;
+use SilverStripe\Control\Controller;
 use SilverStripe\Control\Director;
+use SilverStripe\Core\Config\Configurable;
+use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\Core\Injector\Injector;
 
 /**
@@ -27,26 +28,31 @@ class SAMLConfiguration
     use Configurable;
 
     /**
+     * @config
      * @var bool
      */
     private static $strict;
 
     /**
+     * @config
      * @var bool
      */
     private static $debug;
 
     /**
+     * @config
      * @var array
      */
     private static $SP;
 
     /**
+     * @config
      * @var array
      */
     private static $IdP;
 
     /**
+     * @config
      * @var array List of valid AuthN contexts that the IdP can use to authenticate a user. Will be passed to the IdP in
      * every AuthN request (e.g. every login attempt made by a user). The default values should work for ADFS 2.0, but
      * can be overridden if needed.
@@ -99,17 +105,54 @@ class SAMLConfiguration
     private static $login_persistent = false;
 
     /**
+     * Set other base urls (e.g. subdomains) that may also request Authn from the IdP.
+     *
+     * As with the instruction for SP.entityId it must include protocol (which is always https://), but in this case
+     * always include a trailing slash too.
+     *
+     * In a Silverstripe CMS context this could be e.g. language oriented domains (fr.example.org)
+     * or subdomains for the silverstripe/subsites module
+     * or a pathed URL if your site lives in a subdirectory (example.org/website/) which doesn't match the SP entityId
+     *
+     * If not set the IdP will always redirect to the main site ACS url, ending in user confusion in the least.
+     * An infinite loop (automated or manual) when then redirecting to the RelayState (if cookies aren't shared),
+     * or simply being sent to the main site homepage (leaving the subsite inaccessible if SAMLMiddleware is in use)
+     *
+     * Having a setting that allows certain bases to be used gives a more defined behaviour than simply relying on
+     * {@see Director::absoluteBaseURL} directly
+     *
+     * @see SilverStripe\SAML\Middleware\SAMLMiddleware
+     *
+     * @config
+     * @var array
+     */
+    private static $extra_acs_base = [];
+
+    /**
+     * Build the SAML configuration array for use with OneLogin\Saml2\Auth
+     * The use of Injector allows yaml config to refer to environment variables
+     * @see Injector::convertServiceProperty
+     * @see OneLogin\Saml2\Auth
+     *
      * @return array
      */
     public function asArray()
     {
-        $conf = [];
+        $samlConf = [];
 
-        $conf['strict'] = $this->config()->get('strict');
-        $conf['debug'] = $this->config()->get('debug');
+        $config = $this->config();
+
+        $samlConf['strict'] = $config->get('strict');
+        $samlConf['debug'] = $config->get('debug');
 
         // SERVICE PROVIDER SECTION
-        $sp = $this->config()->get('SP');
+        $sp = $config->get('SP');
+
+        $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']);
         $spCertPath = Director::is_absolute($spX509Cert)
@@ -120,25 +163,24 @@ public function asArray()
             ? $spPrivateKey
             : sprintf('%s/%s', BASE_PATH, $spPrivateKey);
 
-        $conf['sp']['entityId'] = Injector::inst()->convertServiceProperty($sp['entityId']);
-        $conf['sp']['assertionConsumerService'] = [
-            'url' => Injector::inst()->convertServiceProperty($sp['entityId']) . '/saml/acs',
+        $samlConf['sp']['entityId'] = $spEntityId;
+        $samlConf['sp']['assertionConsumerService'] = [
+            'url' => Controller::join_links($acsBaseUrl, '/saml/acs'),
             'binding' => Constants::BINDING_HTTP_POST
         ];
-        $conf['sp']['NameIDFormat'] = isset($sp['nameIdFormat']) ?
-            $sp['nameIdFormat'] : Constants::NAMEID_TRANSIENT;
-        $conf['sp']['x509cert'] = file_get_contents($spCertPath);
-        $conf['sp']['privateKey'] = file_get_contents($spKeyPath);
+        $samlConf['sp']['NameIDFormat'] = $sp['nameIdFormat'] ?? Constants::NAMEID_TRANSIENT;
+        $samlConf['sp']['x509cert'] = file_get_contents($spCertPath);
+        $samlConf['sp']['privateKey'] = file_get_contents($spKeyPath);
 
         // IDENTITY PROVIDER SECTION
-        $idp = $this->config()->get('IdP');
-        $conf['idp']['entityId'] = Injector::inst()->convertServiceProperty($idp['entityId']);
-        $conf['idp']['singleSignOnService'] = [
+        $idp = $config->get('IdP');
+        $samlConf['idp']['entityId'] = Injector::inst()->convertServiceProperty($idp['entityId']);
+        $samlConf['idp']['singleSignOnService'] = [
             'url' => Injector::inst()->convertServiceProperty($idp['singleSignOnService']),
             'binding' => Constants::BINDING_HTTP_REDIRECT,
         ];
         if (isset($idp['singleLogoutService'])) {
-            $conf['idp']['singleLogoutService'] = [
+            $samlConf['idp']['singleLogoutService'] = [
                 'url' => Injector::inst()->convertServiceProperty($idp['singleLogoutService']),
                 'binding' => Constants::BINDING_HTTP_REDIRECT,
             ];
@@ -148,14 +190,14 @@ public function asArray()
         $idpCertPath = Director::is_absolute($idpX509Cert)
             ? $idpX509Cert
             : sprintf('%s/%s', BASE_PATH, $idpX509Cert);
-        $conf['idp']['x509cert'] = file_get_contents($idpCertPath);
+        $samlConf['idp']['x509cert'] = file_get_contents($idpCertPath);
 
         // SECURITY SECTION
-        $security = $this->config()->get('Security');
+        $security = $config->get('Security');
         $signatureAlgorithm = $security['signatureAlgorithm'];
 
-        $authnContexts = $this->config()->get('authn_contexts');
-        $disableAuthnContexts = $this->config()->get('disable_authn_contexts');
+        $authnContexts = $config->get('authn_contexts');
+        $disableAuthnContexts = $config->get('disable_authn_contexts');
 
         if ((bool)$disableAuthnContexts) {
             $authnContexts = false;
@@ -170,7 +212,7 @@ public function asArray()
             }
         }
 
-        $conf['security'] = [
+        $samlConf['security'] = [
             /** signatures and encryptions offered */
             // Indicates that the nameID of the <samlp:logoutRequest> sent by this SP will be encrypted.
             'nameIdEncrypted' => true,
@@ -214,6 +256,6 @@ public function asArray()
             'wantXMLValidation' => true,
         ];
 
-        return $conf;
+        return $samlConf;
     }
 }
diff --git a/tests/php/Services/SAMLConfigurationTest.php b/tests/php/Services/SAMLConfigurationTest.php
@@ -0,0 +1,80 @@
+<?php
+
+namespace SilverStripe\SAML\Tests\Services;
+
+use SilverStripe\Control\Director;
+use SilverStripe\Core\Config\Config;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\SAML\Services\SAMLConfiguration;
+
+class SAMLConfigurationTest extends SapphireTest
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $config = Config::modify();
+        $config->set(Director::class, 'alternate_base_url', 'https://running.test');
+
+        $config->set(SAMLConfiguration::class, 'extra_acs_base', [
+            'https://example.running.test/'
+        ]);
+
+        $config->set(SAMLConfiguration::class, 'SP', [
+            'entityId' => "https://running.test",
+            'privateKey' => __DIR__ . '/fakeCertificate.pem',
+            'x509cert' => __DIR__ . '/fakeCertificate.pem',
+        ]);
+        $config->set(SAMLConfiguration::class, 'IdP', [
+            'entityId' => "idp.example.com",
+            'singleSignOnService' => "https://idp.example.com/test/saml2",
+            'x509cert' => __DIR__ . '/fakeCertificate.pem',
+        ]);
+
+        $config->set(SAMLConfiguration::class, 'strict', true);
+        $config->set(SAMLConfiguration::class, 'debug', false);
+        $config->set(SAMLConfiguration::class, 'Security', [
+            'signatureAlgorithm' => "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256",
+        ]);
+    }
+
+    public function provideBaseUrls(): array
+    {
+        return [
+            [
+                null,
+                'https://running.test/saml/acs',
+                'SP.EntityId should be used by default'
+            ],
+            [
+                'https://example.running.test/',
+                'https://example.running.test/saml/acs',
+                'Extra ACS should work when the loaded (or specified) domain matches'
+            ],
+            [
+                'https://not-legit.running.test/',
+                'https://running.test/saml/acs',
+                'Unlisted ACS base should result in the SP.EntityId being used instead',
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideBaseUrls
+     *
+     * @param string $baseUrl
+     * @param string $expectedOut
+     * @return void
+     */
+    public function testAcsBaseIsSetCorrectly($baseUrl, $expectedOut, $message)
+    {
+        if (isset($baseUrl)) {
+            Config::modify()->set(Director::class, 'alternate_base_url', $baseUrl);
+        }
+        $samlConfig = (new SAMLConfiguration())->asArray();
+        $this->assertSame(
+            $expectedOut,
+            $samlConfig['sp']['assertionConsumerService']['url'],
+            $message
+        );
+    }
+}
diff --git a/tests/php/Services/fakeCertificate.pem b/tests/php/Services/fakeCertificate.pem