FOLIO: Use shib_cql if Shib is used for login

[dltj]

Fixes VUFIND-1615.

[demiankatz]

Thanks, @dltj! I've somehow managed to have three different thoughts related to the same if statement -- see below.

Beyond that, I guess the main question is whether this is too specific of a customization for the core distribution. I definitely see the use case, but it relies on a specific combination of authentication and ILS technology which may or may not be commonly found. I'm definitely not automatically opposed to merging this, but that's a factor to consider.

Perhaps a good first step would be to refactor the code so there's a protected function getLoginCQL that returns the login CQL statement, since this would make it easier to implement these types of customizations in future, whether or not we decide to adopt this one now.

What do you think?

[demiankatz]

The Shibboleth session ID is configurable here: https://github.com/vufind-org/vufind/blob/dev/config/vufind/config.ini#L835 -- I wonder if it's worth making config.ini available to the FOLIO driver so it can check for this configuration (or if it's at least worth adding a setting to Folio.ini to allow override of the name of the parameter).

We also try to avoid directly referencing $_SERVER in our code, since Laminas wraps this up in the request object. However, that raises the question of whether it's worth creating a new dependency on the request object for this one feature. Something to think about, at least.

And just as a matter of style, I think isset($this->config['User']['shib_cql']) might be more typical of VuFind style than using array_key_exists.

[dltj]

The Shibboleth session ID is configurable here: https://github.com/vufind-org/vufind/blob/dev/config/vufind/config.ini#L835 -- I wonder if it's worth making config.ini available to the FOLIO driver so it can check for this configuration (or if it's at least worth adding a setting to Folio.ini to allow override of the name of the parameter).

Ah, good point...I thought I had seen that value somewhere. If you can point me in the right direction to "make config.ini available to the FOLIO driver" I'm happy to add that. I'll push a change for the isset() versus array_key_exists()...this is my lack of PHP experience showing through...I don't know the common idioms; I just know I need something done and I go looking for the language-specific way of doing it.

[demiankatz]

The easiest way to make the setting available to the FOLIO driver is through the factory... so in FolioFactory.php, you can add something like:

$config = $container->get(\VuFind\Config\PluginManager::class)->get('config');

and then $config will be an object representation of config.ini that you can either pass directly into the Folio constructor, provide via a setter, etc. The best approach to getting the data into the driver is probably a matter of taste. :-)

And no worries about the isset/array_key_exists thing. In PHP, the general rule is that if you can do something with either a function or a built-in keyword, the keyword approach is usually faster and more concise. But of course that requires you to know all the options available to you. :-)

[demiankatz]

I also notice that there were two array_key_exists calls here, but only one has been changed to isset. Though perhaps you're holding off on changing the other one since other parts of our discussion may impact the same line of code. :-)

[dltj]

I'm working on this because it is tangentially related to something else. (I'm setting a feature flag in the main VuFind configuration in preparation for isolating code to propose as pull requests to the upstream VuFind.) Is this a legitimate way to inject the main VuFind configuration into the ILS driver?

FolioFactory.php

<?php

namespace IndexDataShared\ILS\Driver;

use Psr\Container\ContainerInterface;

class FolioFactory extends \VuFind\ILS\Driver\FolioFactory
{
    /**
     * Create an object
     *
     * Adds the main VuFind configuration object to the ILS driver.
     *
     * @param ContainerInterface $container     Service manager
     * @param string             $requestedName Service being created
     * @param null|array         $options       Extra options (optional)
     *
     * @return object
     *
     * @throws ServiceNotFoundException if unable to resolve the service.
     * @throws ServiceNotCreatedException if an exception is raised when
     * creating a service.
     * @throws ContainerException&\Throwable if any other error occurs
     */
    public function __invoke(
        ContainerInterface $container,
        $requestedName,
        array $options = null
    ) {
        $mainConfig = $container->get(\VuFind\Config\PluginManager::class)
            ->get('config');
        $driver = parent::__invoke($container, $requestedName, $options);
        if (method_exists($requestedName, 'setMainConfig')) {
            $driver->setMainConfig($mainConfig);
        }
        return $driver;
    }
}

FOLIO driver (in part)

<?php

namespace IndexDataShared\ILS\Driver;

class Folio extends \VuFind\ILS\Driver\Folio
{
    /**
     * VuFind configuration
     *
     * @var array
     */
    protected $mainConfig = null;

    /**
     * Set main configuration
     *
     * Store a reference to the main VuFind configuration
     *
     * @param array $mainConfigArray Configuration array loaded from the main VuFind configuration
     */
    public function setMainConfig($mainConfig)
    {
        $this->mainConfig = $mainConfig;
    }

[demiankatz]

Yes, @dltj, that is an appropriate way to inject the main configuration! A few random thoughts and comments:

1.) In the comments, I would suggest just referring to config.ini instead of saying "main VuFind configuration" -- it's both shorter and more explicit.

2.) It looks like you may still be enforcing an 80-character wrap in some places (or maybe you just copied and pasted some old wrapped code). If the tools are still forcing this on you, you should merge the latest dev branch here, since we raised the limit to 120 characters quite a while ago... but this PR has been sitting around long enough that it might predate that change.

3.) Since you're setting the configuration with a setter instead of via the constructor, you'll need to be careful to account for the possibility of a null value anywhere you read the property. That's not a problem, and you probably already know that, but just mentioning it because it's a detail that can get easily overlooked in the heat of coding. ;-)

4.) When working with ILS drivers, it's always important to remember that there's the possibility of having multiple instances of the same driver via the MultiBackend driver (e.g. in case a consortium needs to connect to multiple FOLIO instances). This is why it's generally preferable to inject settings via the ILS driver configuration instead of the main configuration, since that allows separate instances to be customized individually. In this instance, since you're looking for a global Shibboleth setting, and that seems to me like a higher-level concern than the ILS driver, I think the config.ini injection is probably appropriate (which is why I proposed it in the first place). There's also the option in the future of adding an override in Folio.ini that falls back to config.ini if some crazy use case comes up where we need the ability to differentiate further. But in any case, I'm just mentioning this as an important consideration because once config.ini is injected, it can get increasingly tempting to use it for things, and some of those things may not be the best way of achieving whatever goals are being worked on.

I hope this helps (and sorry for wasting time with anything that didn't need to be said... but I figured better to over-warn than under-warn). :-)

[dltj]

Ah, very good discussion discussion...

1. Changed the wording in the setter.
2. I've been fighting getting the right PHP formatter to work in VS-Code. I've given up for now and am running vendor/bin/phping phpcbf from the command line at the moment.
3. Frankly, I couldn't figure out how to do it via a constructor, but that is my ignorance of the PHP OOP style. Thank you for the warning!
4. You saved the most important for last. I hadn't considered the impact of the MultiBackend driver, and I was already considering using settings in the config.ini for feature flags. To be safe, I guess I'll need to put feature flags in two places...in Folio.ini for ILS driver flags and config.ini for everything else. It'll probably be rare that we need a MultiBackend installation, but just as I say that the need will probably come up right around the corner.

THANKYOU THANKYOU for all of the comments. I'm about to push a new version of the branch that is rebased on the current dev branch.

[demiankatz]

Glad to help! Regarding style fixes, note that now that you're up to date with the latest dev branch, you can run composer fix to do all of the automatic fixes (php-cs-fixer and phpcbf), and phpcbf should be much faster due to some adjustments to its caching configuration. Hopefully that will make everything more convenient!

If you're interested in learning how to do constructor injection, we can certainly discuss. (I don't think there's a need to change anything, but if you have any questions about the PHP syntax, etc., I'm happy to answer them for your future reference).

[dltj]

Beyond that, I guess the main question is whether this is too specific of a customization for the core distribution. I definitely see the use case, but it relies on a specific combination of authentication and ILS technology which may or may not be commonly found. I'm definitely not automatically opposed to merging this, but that's a factor to consider.

I can't speak to the other VuFind drivers, but FOLIO's use of CQL and the customizable capabilities it brings to connecting VuFind to the ILS are quite useful. I know, at least in the NCIP space, it is quite difficult to use combinations of matching identifiers (with one unnamed system allowing for only barcode to be used). This might be a bit of an edge case on one specific driver—and the work-around is not all that hard—but I thought it useful to add it here so someone else doesn't need to work out something equivalent.

Perhaps a good first step would be to refactor the code so there's a protected function getLoginCQL that returns the login CQL statement, since this would make it easier to implement these types of customizations in future, whether or not we decide to adopt this one now.

Ah, now that is a level of abstraction I'm not feeling qualified to comment on. Related to your question above, I'm trying to imagine other combinations of ChoiceAuth authentication selections other than "Shibboleth-ILS" where this would be useful. Maybe it is just that I haven't used the other options.

[demiankatz]

I guess what I'm thinking about here is that this customization is specialized for two different reasons: first of all, it behaves differently depending on the authentication method, and secondly, it refers to implementation details of a specific authentication method. Ideally, I'd love to set things up so that we could achieve both of these things in a more generic way, so that this behavior is one possible configuration, but more things are also possible.

One possible improvement could be to fetch the \VuFind\Authentication\Manager service out of the container and use it to determine the active authentication method so we don't have to "sniff" for Shibboleth at all.

I also wonder if there's some advantage to be gained from refactoring code that extracts attributes from authentication methods so that we could fetch those attributes from the authentication manager and use them in situations like this. There are at least two other authentication methods beyond Shibboleth (LDAP and CAS) that can provide user attributes that could be useful in situations like this, so having a generic way to obtain them without caring about the authentication implementation details would be valuable. But I think that's a trickier problem to solve since in different scenarios, the access to the attributes may be more limited, so we might need to cache them in the session or database to ensure that we have them when we need them. I can give this some deeper thought if you think it's a road potentially worth following!

[demiankatz]

@dltj, I just noticed that this PR hasn't gone anywhere in quite a while, so I thought I'd follow up and see if this is something you still want to pursue. I'm happy to collaborate as needed -- just checking if this is something we need to revive, or if it is no longer needed.

[demiankatz]

@dltj, just doing another review of pull requests without milestones, and I see that this hasn't moved in a while. Is it still needed?

[dltj]

Yup —still needed...we have this in place for a number of our libraries, so I think it is generally useful. It is on my development list.

…

On Thu, Jun 20, 2024 at 3:36 PM Demian Katz ***@***.***> wrote: @dltj <https://github.com/dltj>, just doing another review of pull requests without milestones, and I see that this hasn't moved in a while. Is it still needed? — Reply to this email directly, view it on GitHub <#2902 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFLAFEXHE2GB5WXRJYIADZIMVLVAVCNFSM6AAAAABJUSLW6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBRGM4TKOJTGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[demiankatz]

Thanks for the confirmation, @dltj! I'm putting an 11.0 milestone on this so we don't lose track of it indefinitely, but also so you don't feel huge pressure to complete it right away. Obviously, if you get to it sooner, we can move the milestone earlier, and if you need more time, we can move it later... but I feel better having it on the schedule somewhere so it doesn't slip through the cracks.

[demiankatz]

A couple more suggested tweaks to comments. I see that you also still need to finish implementing code to use the config.ini settings now that they are accessible. Please request a review from me once you're ready for me to take another look -- I don't want to bug you with any further suggestions before you're actually ready for them. ;-)

[demiankatz]

Reassigning milestone as per discussion on the 1 July Community Call. I'm assuming that @dltj does not have time to get this done sooner, but of course we can complete it ahead of the milestone if the opportunity arises -- I just don't want to treat it as a critical requirement for the 11.0 release to be considered "done."

[dltj]

This is still a draft. I've tested it against an actual FOLIO server for ILS authentication, but I haven't been able to hook it up to our local Shib IdP (yet).

[demiankatz]

Thanks, @dltj -- see below for a few suggestions based on a quick review!

[demiankatz]

What if we made this cql_by_auth_method[Shibboleth] instead of hard-coding things to Shibboleth? That would make the feature more flexible and avoid hard-coded assumptions about one specific Auth method.

[demiankatz]

And just to be clear, this would let you refactor the config logic down to something like:

        $cql = $this->config['User']['cql_by_auth_method'][$this->authManager->getSelectedAuthMethod()]
            ?? $this->config['User']['cql']
            ?? '%%username_field%% == "%%username%%"' . ($passwordField ? ' and %%password_field%% == "%%password%%"' : '');

[demiankatz]

If the only thing we're using the $authManager for in the driver is to get the current auth method, is it worth considering passing only that value, rather than the whole object? That would make the dependency coupling a little looser.

[demiankatz]

If you take my above suggestion, you won't need this at all, but if you decide to keep it you can simplify this to:

Suggested change

	$authManager = $this->getMockBuilder(\VuFind\Auth\Manager::class)
	->disableOriginalConstructor()
	->getMock();
	$authManager = $this->createMock(\VuFind\Auth\Manager::class);