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.

Sign up for GitHub

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

FOLIO: Use shib_cql if Shib is used for login #2902

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

FOLIO: Use shib_cql if Shib is used for login #2902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2d689c
Use shib_cql if Shib is used for login
dltj Jul 12, 2024
707de58
Add method to set config.ini in driver
dltj Jul 12, 2024
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
3 changes: 3 additions & 0 deletions config/vufind/Folio.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ password_field = false
; %%username_field%% = The username_field config setting (above)
; %%password_field%% = The password_field config setting (above)
;cql = '%%username_field%% == "%%username%%" and %%password_field%% == "%%password%%"'
; If this CQL statement is uncommented, it will be used when a user is logged in
; via Shibboleth (e.g., the $_SERVER['Shib-Session-ID'] array value exists).
;shib_cql = 'externalSystemId == "%%username%%"'

; Should we try to log the user into the Okapi API (true) or just look them
; up in the database using [API] credentials above (false). If set to true,
Expand Down
38 changes: 35 additions & 3 deletions module/VuFind/src/VuFind/ILS/Driver/Folio.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ class Folio extends AbstractAPI implements
'Open - Awaiting delivery',
];

/**
* VuFind configuration, config.ini
Copy link
Member

Choose a reason for hiding this comment

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

I'd change this to "Configuration loaded from config.ini"

*
* @var array
*/
protected $mainConfig = null;

/**
* Constructor
*
Expand Down Expand Up @@ -150,6 +157,20 @@ public function setConfig($config)
$this->tenant = $this->config['API']['tenant'];
}

/**
* Set main configuration
*
* Store a reference to the config.ini configuration
*
* @param array $mainConfig Configuration array loaded from the main VuFind configuration
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need both of the lines above, and I'd adjust the @param to reference config.ini.

This is obviously not a big deal, but I'm trying to avoid unnecessary references to VuFind® since if we mention it, we should include the ®, and it's less of a hassle to just avoid mentioning the name. ;-)

*
* @return void
*/
public function setMainConfig($mainConfig)
{
$this->mainConfig = $mainConfig;
}

/**
* Get the type of FOLIO ID used to match up with VuFind's bib IDs.
*
Expand Down Expand Up @@ -995,6 +1016,10 @@ protected function patronLoginWithOkapi($username, $password)
* Support method for patronLogin(): authenticate the patron with a CQL looup.
* Returns the CQL query for retrieving more information about the user.
*
* NOTE: this method looks for the existence of a $SERVER['Shib-Session-ID'] variable
* and, if found, looks for a `shib-cql` configuration stanza to use instead of the
* standard `cql` stanza.
*
* @param string $username The patron username
* @param string $password The patron password
*
Expand All @@ -1005,9 +1030,16 @@ protected function getUserWithCql($username, $password)
// Construct user query using barcode, username, etc.
$usernameField = $this->config['User']['username_field'] ?? 'username';
$passwordField = $this->config['User']['password_field'] ?? false;
$cql = $this->config['User']['cql']
?? '%%username_field%% == "%%username%%"'
. ($passwordField ? ' and %%password_field%% == "%%password%%"' : '');
if (
isset($this->config['User']['shib_cql'])
&& array_key_exists('Shib-Session-ID', $_SERVER)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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. :-)

Copy link
Member

Choose a reason for hiding this comment

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

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. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
    }

Copy link
Member

Choose a reason for hiding this comment

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

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). :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

) {
$cql = $this->config['User']['shib_cql'];
} else {
$cql = $this->config['User']['cql']
?? '%%username_field%% == "%%username%%"'
. ($passwordField ? ' and %%password_field%% == "%%password%%"' : '');
}
$placeholders = [
'%%username_field%%',
'%%password_field%%',
Expand Down
7 changes: 6 additions & 1 deletion module/VuFind/src/VuFind/ILS/Driver/FolioFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ public function __invoke(
$manager = $container->get(\Laminas\Session\SessionManager::class);
return new \Laminas\Session\Container("Folio_$namespace", $manager);
};
return parent::__invoke($container, $requestedName, [$sessionFactory]);
$mainConfig = $container->get(\VuFind\Config\PluginManager::class)->get('config');
$driver = parent::__invoke($container, $requestedName, [$sessionFactory]);
if (method_exists($requestedName, 'setMainConfig')) {
$driver->setMainConfig($mainConfig);
}
return $driver;
}
}
Loading