Installing the module causes exception and Drupal whitescreen

Same

Could this be composer installing the module and library, but no configuration files?

I tried installing via composer:

$ composer require drupal/simplesamlphp_auth && drush @uwmed.local en simplesamlphp_auth -y 

And got no config files in
vendor/simplesamlphp/simplesamlphp/config and
vendor/simplesamlphp/simplesamlphp/metadata

As a side note, Aquia and BLT has a settings autoload that will overwrites $settings['simplesamlphp_dir'].

You might try download the SimpleSAMLphp library, placing config and metadata above the docroot, and symlinking to those folders.

From composer.lock:

            "require": {
                "drupal/core": "*",
                "drupal/externalauth": "*",
                "simplesamlphp/simplesamlphp": "~1.14.11"
            },

Everytime I install SimpleSAMLphp on a vanilla site, here are the steps I have to do:

- Composer require drupal/simplesamlphp_auth
- Do not enable the module yet
- Configure SimpleSAML, the config needs to be completed otherwise you WILL get WSOD
- Enable module
- Configure module

I believe the reason we get WSOD is:

function simplesamlphp_auth_requirements($phase) {
  $requirements = [];

  if ($phase == 'install') {
    simplesamlphp_auth_check_library();

.......
.......

function simplesamlphp_auth_check_library() {
  if (!class_exists('SimpleSAML_Configuration')) {
    $dir = Settings::get('simplesamlphp_dir');
    include_once $dir . '/lib/_autoload.php';
  }
}

include_once will require a valid config. Perhaps we can check without doing include_once?

Has anyone been able to work around this issue yet?

We have simplesamlphp installed outside of Drupal's docroot and even with the Acquia suggested config override in settings.php:

...
$settings['simplesamlphp_dir'] = '/var/www/html/mysitedev/simplesamlphp';
...

I'm still seeing a "The website encountered an unexpected error. Please try again later." message. Looking at the logs, it appears that the contrib module is still trying to find a non-existent config file in the /vendor/ directory:

Uncaught PHP Exception SimpleSAML\Error\CriticalConfigurationError: "The configuration (config/config.php) is invalid: Missing configuration file" at /mnt/www/html/mysitedev/docroot/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Error/CriticalConfigurationError.php line 78

Currently this is a show stopper if one wants to install SimpleSAMLphp without composer.

With composer everything runs fine, but without composer the module gets confused about where things are.

It does seem that the settings.php configuration makes no difference.

Encountering the same issue as #6 and #7.

user composer to install the module

however, after enable the module

"The website encountered an unexpected error. Please try again later."

Disabled the module, website recovered

Make sure your saml config is updated and correct. Without this it will not work regardless of if you installed with Composer or not. See my post above.

Here's a patch to fix the issue. In the constructor for SimplesamlphpAuthManager, we can simply add a condition which allows us to not interact with the simplesamlphp library if it's disabled within the drupal admin.

As long as simplesamlphp_auth.settings.activate is set to FALSE, it won't try to load up the library.

Once the patch is in place, if you need to disable it on one environment, until that environment is set up, you can add to your settings.php:

$config['simplesamlphp_auth.settings']['activate'] = FALSE;

Here's a new patch that also prevents the fatal error if simplesamlphp_auth.settings.activate is set to TRUE, and instead outputs a message to the user.

Same as 13, but changes the error messages so they only show on admin pages, and setting them as "error" messages.

but my simplesamlphp is installed at /var/simplesamlphp and works and can be visited via http://auth.xxx.com

need to copy the /congig /metedata from /var/simplesamlphp to vender/simplesamlphp/simplesamlphp ？

Oh right, I see, in /simplesamlphp/simplesamlphp/lib/_autoload.php

// SSP is loaded as a separate project
if (file_exists(dirname(dirname(__FILE__)).'/vendor/autoload.php')) {
    require_once dirname(dirname(__FILE__)).'/vendor/autoload.php';
} else {  // SSP is loaded as a library
    if (file_exists(dirname(dirname(__FILE__)).'/../../autoload.php')) {
        require_once dirname(dirname(__FILE__)).'/../../autoload.php';
    } else {
        throw new Exception('Unable to load Composer autoloader');
    }
}

If you don't use composer, it throws an error,

in

function simplesamlphp_auth_check_library() {
  if (!class_exists('SimpleSAML_Configuration')) {
    $dir = Settings::get('simplesamlphp_dir');
    include_once $dir . '/lib/_autoload.php';
  }
}

This is actually a different issue than I was having, which is when you install it with composer, but you haven't configured it yet.

So there's two potential vectors for the WSOD, an unconfigured simplesamlphp installed with composer, and a simplesamlphp installation that Drupal can't find upon installing the simplesamlphp_auth module.

For @sealionking, have you tried a symlink to where it's looking for the library?

I'm marking this back as in review.

I think the issue with the requirements where the install is not in the usual location is a separate issue to the missing configuration files.

thank you Oknate.

Would you please tell me how to make the symlink to my installed simplesamlphp at /var/simplesamlphp ?

For @sealionking, I actually don't think you need a symlink.

I would add to your settings.php the following:

$settings['simplesamlphp_dir'] = '/var/simplesamlphp';

This is used by simplesamlphp_auth_check_library() called from simplesamlphp_auth_requirements(), and it will load up the library.

But there isn't a check if your $settings['simplesamlphp_dir'] is a valid directory of if the path to it's _autoload.php actually exists before it includes it, from simplesamlphp_auth_check_library():

    $dir = Settings::get('simplesamlphp_dir');
    include_once $dir . '/lib/_autoload.php';

Although for you, @sealionking, since your simplesamlphp library is configured, it should work.

But this will only work if the simplesamlphp library is already configured. Otherwise, the simplesamlphp_auth_requirements will register a REQUIREMENT_ERROR, and the module will fail to install.

thank you @oknate

added the line

$settings['simplesamlphp_dir'] = '/var/simplesamlphp';

then enabled the module, it still crashed and altered "The website encountered an unexpected error. Please try again later."

@sealionking Have you checked your PHP logs for the specific error?

Also, since it seems like the install requirements check is the issue, you could try temporarily removing the simplesamlphp_auth_requirements() method in the install file, once the module is installed, it would then pick up your custom library. I haven't tested this, since I installed with composer, but you could try it.

I will try them again from the ground and let you know my result

I'm pretty new to the whole "fixing" Drupal and it's module so I'm not real sure how to go about that but the problem is when the module tries to instantiate the library, the library is looking for it's config.php file which doesn't exist. The fix for the white screen when installing the module is this:

In modules/contrib/simplesamlphp_auth/src/Srvice/SimplesamlphpAuthManager.php on line 59.

Replace:
$this->instance = new Simple($auth_source);

With:

      // Get instance of SimpleSAMLphp
      try {
        $this->instance = new Simple($auth_source);
      }
      catch (\Exception $e) {
        watchdog_exception('simplesamlphp_auth', $e);
        drupal_set_message($e->getMessage(), 'error');
        return false;
      }

This will allow Drupal to load instead of getting a white screen.

The actual config that needs to be made is by adding an environment variable:
SIMPLESAMLPHP_CONFIG_DIR to the absolute path of where your simplesaml/conf directory is..

For instance, I symlink /vendor/simplesamlphp/simplesamlphp/conf to /var/www/host/domain.com/simplesaml/conf which is where I keep the config.php file. If you set this at the Apache level (ie: vhosts) then the current version of the module should work. However, if you'r elike me and have a reason to set this via .htaccess then you need version 1.6 of the SimpleSAMLphp library and this module is only allowing 1.5.x so you'll need to adjust the modules composer.json to get 1.6 (note: I haven't tested the module with 1.6 or 1.7 yet).

This patch is working well for me.

I am installing the module with composer and then I am symlinking in the config, metadata and cert directories inside the vendor/simplesamlphp/simplesamlphp to my own directories. Initially I tried with simplesamlphp in another directory but abandoned that as the use of composer keeps things a bit more compact.

I am also having the same issue as sealionking, brand new vm running php 7 and simplesamlphp installed, simplesamlphp is setup and the test page works fine with adfs with no issues, when the module gets installed on a plain vanilla drupal install I get "The website encountered an unexpected error. Please try again later." if I disable the module drupal starts working again, I checked the vendor/simplesamlphp folder and there

is nothing in the config or metadata folder, I tried it on a site with php 7.1 and the moduled installed but got similar errors, to work around it I had to symlink the folders to their proper folders in simplesamlphp, like sealionking I also installed simplesamlphp in /var/simplesamlphp, is there a default folder that the module is expecting simplesamlphp to be in?, or any ideas what could be causing the module not to grab the proper files?

I ran into the same exact issue as sealionking and hpinheiro. I had installed SimpleSAMLphp at the standard /var/simplesamlphp location and had it working at mysite.com/simplesaml without issue. When I installed this module (with Composer) and enabled it, I got the dreaded 500 error, and I noticed the vendor/simplesamlphp/simplesamlphp/config and vendor/simplesamlphp/simplesamlphp/metadata folders were empty.

I was able to resolve (by copying those from /var/simplesamlphp, but I was not entirely sure it was resolved. I didn't know what other files might be missing, so I instead deleted vendor/simplesamlphp/simplesamlphp and added a symlink inside vendor/simplesamlphp pointing to var/simplesamlphp . Now it appears to work just fine. This is not great though, because it breaks the Composer workflow, introducing a manual step.

I have not tried any of the patches on this thread. The Composer installation procedure does not copy the config and metadata files, and I was using the default/standard /var/simplesamlphp location. So, either the module does not try to copy the files, or it tried and failed.

Lastly, Mmore of a meta observation; I find it strange that this module's installation instructions mention installing SimpleSAMLphp per instructions at https://simplesamlphp.org/docs/stable/simplesamlphp-install, and then Composer also includes the same library in Drupal's vendor folder. It should really just be in one location, not two. I think the install instructions need some attention after this is all sorted.

I installed SimpleSamlPHP on a Pantheon D8 site and encountered this issue as well.

- I installed the SimpleSamlPHP library directly (without using Composer) following this Pantheon guide: https://pantheon.io/docs/shibboleth-sso/
- I installed the simplesamlphp_auth module using Composer.
- SimpleSamlPHP was working and accessible.

Using this installation method, it seems like the key observation is in #7: The $settings['simplesamlphp_dir'] value in settings.php is never used, or at least is not used all the time. At least once, the module requires files in /vendor/... for the config, when it is actually in /private/simplesaml (or wherever), and the site falls over.

I was able to get around it by symlinking the actual config folder from /vendor/simplesamlphp/simplesamlphp. Definitely a hack, but it satisfied the module and I was able to get it working.

I'm not sure why the $settings['simplesamlphp_dir'] value is documented in so many places (here, Pantheon, and Acquia all refer to it) if it didn't work at some point. Did it used to work and no longer does?

I have been using simpleSAMLphp Authentication 8.x-3.0-rc4 with /var/simplesamlphp (v1.14).

I also have $settings['simplesamlphp_dir'] = "/var/simplesamlphp" in my settings.php.

When I update to 3.0 using composer require drupal/simplesamlphp_auth:3.0, my site whitescreens, with the watchdog error:

 Type       :  php                                                                                                                                                                                                   
 Severity   :  error                                                                                                                                                                                                 
 Message    :  SimpleSAML\Error\CriticalConfigurationError: The configuration (config/config.php) is invalid: Missing configuration file in SimpleSAML\Error\CriticalConfigurationError::fromException() (line 77 of 
               /usr/share/nginx/html/docroot/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Error/CriticalConfigurationError.php).                                                                                

I added the patch in #14, the error was then:

 ID         :  11010371                                                                                                                                                                                                                                             
 Date       :  07/Feb 08:04                                                                                                                                                                                                                                         
 Type       :  php                                                                                                                                                                                                                                                  
 Severity   :  error                                                                                                                                                                                                                                                
 Message    :  Error: Call to a member function requireAuth() on null in Drupal\simplesamlphp_auth\Service\SimplesamlphpAuthManager->externalAuthenticate() (line 105 of                                                                                            
               /usr/share/nginx/html/docroot/modules/contrib/simplesamlphp_auth/src/Service/SimplesamlphpAuthManager.php) #0 /usr/share/nginx/html/docroot/modules/contrib/simplesamlphp_auth/src/Controller/SimplesamlphpAuthController.php(200):                  
               Drupal\simplesamlphp_auth\Service\SimplesamlphpAuthManager->externalAuthenticate()                                                                                                                                                                   
               #1 [internal function]: Drupal\simplesamlphp_auth\Controller\SimplesamlphpAuthController->authenticate()                                                                                                                                             
               #2 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)                                                                                         
               #3 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/Render/Renderer.php(582): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()                                                       
               #4 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))             
               #5 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) 
               #6 /usr/share/nginx/html/docroot/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()                                                      
               #7 /usr/share/nginx/html/docroot/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)                                                              
               #8 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                                    
               #9 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                                
               #10 /usr/share/nginx/html/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(184): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                     
               #11 /usr/share/nginx/html/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(121): Drupal\page_cache\StackMiddleware\PageCache->fetch(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                      
               #12 /usr/share/nginx/html/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(75): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                      
               #13 /usr/share/nginx/html/docroot/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                                         
               #14 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                                   
               #15 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                          
               #16 /usr/share/nginx/html/docroot/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                     
               #17 /usr/share/nginx/html/docroot/core/lib/Drupal/Core/DrupalKernel.php(693): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)                                                                             
               #18 /usr/share/nginx/html/docroot/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))                                                                                                                  
               #19 {main}.                                                                                                                                                                                                                                          

From my docroot, I then run:

cd vendor/simplesamlphp/simplesamlphp/
rm -rf config
rm -rf log
rm -rf metadata

ln -s /var/simplesamlphp/config ./config
ln -s /var/simplesamlphp/log ./log
ln -s /var/simplesamlphp/metadata ./metadata

The error is now:

 ID         :  11010372                                                                                                                                                                                                                                                  
 Date       :  07/Feb 08:14                                                                                                                                                                                                                                              
 Type       :  php                                                                                                                                                                                                                                                       
 Severity   :  error                                                                                                                                                                                                                                                     
 Message    :  SimpleSAML_Error_UnserializableException: [ARRAY]: The option 'Format' is not a valid string value. in SimpleSAML_Auth_Source->initLogin() (line 198 of /usr/share/nginx/html/docroot/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Auth/Source.php). 

Adding a fix for #6 and #7.

It does seem that the settings.php configuration makes no difference.

Borrowed from this patch: https://www.drupal.org/files/issues/2019-01-02/SimplesamlphpAuthManager-...

(But leaving out the apache specific line)

This fixed an installation on acquia.

1. +++ b/simplesamlphp_auth.routing.yml
   @@ -5,6 +5,8 @@ simplesamlphp_auth.admin_settings:
        _permission: 'administer simpleSAMLphp authentication'
   +  options:
   +    _admin_route: TRUE
    simplesamlphp_auth.admin_settings_local:
      path: '/admin/config/people/simplesamlphp_auth/local'
   

   This seems unrelated and unnecessary, all routes that start with /admin/ are automatically admin routes?

2. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -3,15 +3,24 @@
   -use SimpleSAML_Configuration;
   +use Drupal\Core\StringTranslation\StringTranslationTrait;
    use Drupal\simplesamlphp_auth\Exception\SimplesamlphpAttributeException;
    use Drupal\Core\Site\Settings;
    
   +if (Settings::get('simplesamlphp_dir')) {
   +  $dir = Settings::get('simplesamlphp_dir');
   +  include_once $dir . '/lib/_autoload.php';
   +}
   +
   +use SimpleSAML\Auth\Simple;
   +use SimpleSAML_Configuration;
   +use SimpleSAML\Error\CriticalConfigurationError;
   +
   

   Still trying to wrap my head around this issue, but a use statement is just an alias, it doesn't care at all if that exists or not, so there doesn't seem to be any reason to put something above use statements.

   It will only lookup the alias when it actually encounters a reference to that class.

   Which does happen in the constructor, so running it in there is too late, I see that. But at least we can keep that below the use statements.

   If I understand it correctly, the problem is that installing it through composer doesn't work on some hosting providers because you can't expose the files that need to be public?

   PS: Install through composer works fine on platform.sh btw, there's even documentation for it: https://docs.platform.sh/frameworks/drupal8/simplesaml.html#simplesaml

3. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -52,18 +61,42 @@ class SimplesamlphpAuthManager {
        $this->checkLibrary();
   

   the checkLibrary() call seems to be basically do what the new snippet above does, so if we do it outside of the class, this shouldn't be required anymore? Or we could make that static and call it from somewhere else?

It's been a few months since I did this patch, but I remember the general idea. I think there are a few issues that are mingled in this ticket.

1) When the module is not installed with composer, and the simplesamlphp library hasn't been installed or the drupal module can't find it because the $settings['simplesamlphp_dir'] is incorrectly configured, there isn't a graceful degradation where an error message is output, "You must install the simplesamlphp library", etc. Some people already have simplesamlphp on their servers and they want to use that copy of it, rather than use composer to install it. So it's just a UX improvement to tell them what's wrong rather than give the WSOD.

2) When the module is installed with composer and then the simplesamlphp is moved out of the vendor directory, the site will similarly blow up if the $settings['simplesamlphp_dir'] is incorrectly configured. It would be nice to detect a misconfiguration and output an explanation to the user.

3) If the library's service provider and IPD are incorrectly configured, it outputs a CriticalConfigurationError and this leads to a white screen of death, where it could be detected and an explanation given to the user.

When I get a chance, I'll update the patch, there's some good feedback there.

We could also perhaps break it up, as I think I handled multiple situations in the last patch.

Yeah, I get the basic idea, just not very sure about the implementation. especially because the check does seem to exist already, just apparently not early enough.

I'm don't think that failing with an exception is bad when there is a critical/breaking configuration/environment problem, but i also see that it is tricky to debug and figure out in some environments. As mentioned the main thing I'm unhappy about is that the added code duplicates two existing checks, checkLibrary() and simplesamlphp_auth_check_library() in the .install file.

I also see it has a requirements check for install, so I think the issue title doesn't seem very accurate.. it's not that the missing files break it, it's having the filters but no configuration for it. Which *is* being addressed here but also various other things.
Some ideas/thoughts:

* Stop instantiating things in the constructor, Instead add protected getInstance()/getConfig() or so methods that instantiate on demand, that does allow us to safely check

* it looks like those two arguments actually only exists for unit tests, so lazy loading it could also possibly allow to move that to setter methods and simplify the constructor. Then there could be a isConfigured() method that could be called also from a requirement hooks.

* In general, especially if we change it to no longer fail hard, the install hook should be expanded to check for the classes and valid configuration, then we can IMHO avoid this problem for most people before they even install the module.

A small (possibly inconsequential for this issue) clarification to #2 above. Acquia recommends placing /vendor in the repo root . So just above web root (or '/docroot' as Acquia requires) as documented at https://docs.acquia.com/resource/composer/d8-migrate/.

Somewhat confusingly, the Acquia docs recommend installing SimpleSAMLphp manually, rather than with Composer. Even so, they want it above the web root.

We're currently transitioning to a fully composer-managed site. Still testing, but we have composer-installed simplesamlphp_auth and SimpleSAMLphp lib working on Acquia Cloud.

I wholeheartedly agree, though, that a notice would be preferable to a WSOD! :)

Looks like #3043942: Error "Failed to start the session: already started by PHP" is a related issue that's going into the direction of lazy loading, although I'd prefer to just have getInstance() method.

Thanks for clarifying that about acquia vendor directory belonging the repo root, not the web root. I feel a little dumb, as I was told some bad information and failed to verify it, but hopefully someone else won't make the same mistake. I'm going to update our installation to move simplesamlphp library back into the vendor directory and move the vendor directory back where it belongs. I deleted that part of the comment, so as not to confuse others.

Here's a new patch that fixes the dependency injection and lazy loads the Simple instance and the Configuration instance.

There are two patches, one that combines the changes with patch #3 on #3050273: Require SimpleSAMLphp ^1.17.2 and use namespaced class names.

The tests will need to be updated to match the changes.

Fixing tests

Fixing tests, and coding standards fixes

This needs a reroll now, will get this in asap.

Here's the reroll.

There are some other things I think need changing such as calling Header directly, but I think we should handle them separately.

1. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -3,15 +3,23 @@
    
    use Drupal\Core\Config\ConfigFactoryInterface;
   -use SimpleSAML\Auth\Simple;
   -use SimpleSAML\Configuration;
   +use Drupal\Core\StringTranslation\StringTranslationTrait;
    use Drupal\simplesamlphp_auth\Exception\SimplesamlphpAttributeException;
    use Drupal\Core\Site\Settings;
   +use SimpleSAML\Configuration;
   +use SimpleSAML\Auth\Simple;
   +use SimpleSAML\Error\CriticalConfigurationError;
   +use Drupal\Core\Session\AccountInterface;
   +use Drupal\Core\Routing\AdminContext;
   +use Drupal\Core\Extension\ModuleHandlerInterface;
   +use Symfony\Component\HttpFoundation\RequestStack;
   +use Drupal\Core\Messenger\MessengerInterface;
   

   Not sure why some uses are moved around and others aren't, either we should focus on keeping the diff size minimal and easier to review or just make the whole block alphabetic.

2. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -42,41 +50,136 @@ class SimplesamlphpAuthManager {
   +  /**
   +   * Returns a SimpleSAML Simple class instance.
   +   *
   +   * @return \SimpleSAML\Auth\Simple
   +   *   The simplesaml Simple instance.
   

   Should consistently use SimpleSAML

3. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -214,7 +333,7 @@ class SimplesamlphpAuthManager {
      protected function checkLibrary() {
   -    if (!class_exists('SimpleSAML\Configuration')) {
   +    if (!class_exists('Configuration') || !class_exists('Simple')) {
          $dir = Settings::get('simplesamlphp_dir');
   

   This doesn't work, the classes have a namespace that you need to include when using in a string. The old is actually correct and it also seems extremely unlikely that Simple isn't defined when Configuration is?

4. +++ b/tests/simplesamlphp_auth_test/src/SimplesamlphpAuthTestManager.php
   @@ -21,16 +29,9 @@ class SimplesamlphpAuthTestManager extends SimplesamlphpAuthManager {
       */
   -  public function __construct(ConfigFactoryInterface $config_factory, Simple $instance = NULL, Configuration $config = NULL) {
   -    $this->config = $config_factory->get('simplesamlphp_auth.settings');
   +  public function __construct(ConfigFactoryInterface $config_factory, AccountInterface $current_user, AdminContext $admin_context, ModuleHandlerInterface $module_handler, RequestStack $request_stack, MessengerInterface $messenger) {
   +    parent::__construct($config_factory, $current_user, $admin_context, $module_handler, $request_stack, $messenger);
        $this->authenticated = FALSE;
      }
   

   You can initially $this->authenticated to FALSE then you can drop the constructor completely I think, this was only necessary because we initialized the things there, which we no longer do.

5. +++ b/tests/src/Unit/Service/SimplesamlphpAuthManagerTest.php
   @@ -80,12 +143,27 @@ class SimplesamlphpAuthManagerTest extends UnitTestCase {
   +        $this->messenger,
   +      ])
   +      ->setMethods([
   +        'instance',
   +        'simpleSamlConfig',
   +      ])
   +      ->getMock();
      }
   

   Hm, my idea was to keep those as constructor arguments, but in the constructor simply do a $this->instance = $inst ance, which is a no-op if NULL is passed.

   I think that is easier for unit testing, no need to mock the auth manager then..

   I'd also use getX for the methods and would suggest the following names:

   simpleSamlConfiguration/getSimpleSamlConfiguration()

   simpleSamlInstance/getSimpleSamlInstance()

   The second one is hard, because there doesn't really seem to be a name for "Simple" or what it represents. Nothing useful on the class docblock. And instance is super generic, especially since there's also Configuration::getInstance(), which is why I think we should prefix both properties/methods consistently.

1. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -42,41 +50,136 @@ class SimplesamlphpAuthManager {
   +   *
   +   * @return \SimpleSAML\Configuration
   +   *   The simplesaml Configuration instance.
       */
   

   Also, the @return needs include |null and document that can be returned.

2. +++ b/src/Service/SimplesamlphpAuthManager.php
   @@ -205,7 +319,12 @@ class SimplesamlphpAuthManager {
        }
   -    $this->instance->logout($redirect_path);
   +
   +    if (empty($this->instance())) {
   +      return FALSE;
   +    }
   +
   +    $this->instance()->logout($redirect_path);
      }
   

   This method isn't expected and is also implemented differently than other methods which use a $instance variable.

   Could be done for example as:

   <?php
   if ($instance = ...) {
   $instance->logout();
   }

1) move use statements back to original position
2) changed simplesaml to SimpleSAML for consistency
3) reverted the change
4) removed constructor and set $this->authenticated to FALSE by default
5) renamed methods to getSimpleSamlInstance and getSimpleSamlConfiguration

second comment
1) Added |NULL
2) Fixed logout to call getSimpleSamlInstance() rather than accessing instance directly and fixed the logic to load instance in conditional statement per your suggestion.

I changed the unit test per your suggestion so we don't need to use a mock object, and updated the constructor args.

It seems odd to have those in the constructor if they're not used, but it sure does allow for easier unit testing.

Made a few minor improvements like removing all the unused uses in the test implemenation and committed.

diff --git a/src/Service/SimplesamlphpAuthManager.php b/src/Service/SimplesamlphpAuthManager.php
index 3b99708..3e4df21 100644
--- a/src/Service/SimplesamlphpAuthManager.php
+++ b/src/Service/SimplesamlphpAuthManager.php
@@ -85,7 +85,7 @@ class SimplesamlphpAuthManager {
   protected $messenger;
 
   /**
-   * {@inheritdoc}
+   * Constructor for SimplesamlphpAuthManager.
    *
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The configuration factory.
@@ -179,7 +179,7 @@ class SimplesamlphpAuthManager {
         return $this->simplesamlConfig;
       }
       catch (CriticalConfigurationError $e) {
-        if (\Drupal::currentUser()->hasPermission('administer simplesamlphp authentication')
+        if ($this->currentUser->hasPermission('administer simplesamlphp authentication')
           && $this->currentUser->isAdminRoute()) {
           $this->messenger->addError($this->t('There is a Simplesamlphp configuration problem. ' . $e->getMessage()), 'error');
         }
diff --git a/tests/simplesamlphp_auth_test/src/SimplesamlphpAuthTestManager.php b/tests/simplesamlphp_auth_test/src/SimplesamlphpAuthTestManager.php
index ec38302..512852e 100644
--- a/tests/simplesamlphp_auth_test/src/SimplesamlphpAuthTestManager.php
+++ b/tests/simplesamlphp_auth_test/src/SimplesamlphpAuthTestManager.php
@@ -3,17 +3,6 @@
 namespace Drupal\simplesamlphp_auth_test;
 
 use Drupal\simplesamlphp_auth\Service\SimplesamlphpAuthManager;
-use Drupal\Core\Config\ConfigFactoryInterface;
-use SimpleSAML\Auth\Simple;
-use SimpleSAML\Configuration;
-use Drupal\Core\StringTranslation\StringTranslationTrait;
-use Drupal\Core\Site\Settings;
-use SimpleSAML\Error\CriticalConfigurationError;
-use Drupal\Core\Session\AccountInterface;
-use Drupal\Core\Routing\AdminContext;
-use Drupal\Core\Extension\ModuleHandlerInterface;
-use Symfony\Component\HttpFoundation\RequestStack;
-use Drupal\Core\Messenger\MessengerInterface;
 
 /**
  * Mock SimplesamlphpAuthManager class for testing purposes.

Great! Thank you.

I created a follow-up issue:

#3052222: [needs follow-up] checkLibrary() ignores alternate simplesamlphp library location if simplesamlphp already found

This is for the use case where:
1) You have installed via composer so, there is a simplesamlphp folder within the /vendor directory
2) you have set an alternate install location within settings.php
3) You have configs within your alternate location, but not within the copy within your /vendor directory