-
Notifications
You must be signed in to change notification settings - Fork 395
FOLIO: Use shib_cql if Shib is used for login #2902
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.
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
base: dev
Are you sure you want to change the base?
Changes from all commits
262b71e
9cfbbd1
9f10f51
87049e0
ea80cf6
d356c27
65dbc4c
73efee5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -97,6 +97,12 @@ 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%%"' | ||||||
| ; The `cql_by_auth_method` setting allows you to specify different CQL queries | ||||||
| ; for different VuFind authentication methods. The keys should be the names | ||||||
| ; of the authentication methods as defined in your VuFind configuration, and | ||||||
| ; the values should be CQL queries that can use the same placeholders as | ||||||
| ; described above for the general `cql` setting. For instance, for Shibboleth: | ||||||
| ;cql_by_auth_method[Shibboleth] = '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, | ||||||
|
|
@@ -105,6 +111,10 @@ okapi_login = true | |||||
| ; Should we override the Okapi token created using [API] credentials with the | ||||||
| ; user's credentials after they log in? (Only valid if okapi_login = true) | ||||||
| use_user_token = false | ||||||
| ; If set to true, `debug_login` will log the CQL query (with password redated), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo?
Suggested change
|
||||||
| ; the environment variables, and the response from the FOLIO `/users` API call. | ||||||
| ;debug_login = false | ||||||
|
|
||||||
|
|
||||||
| ; This section controls hold behavior; note that you must also ensure that Holds are | ||||||
| ; enabled in the [Catalog] section of config.ini in order to take advantage of these | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -115,6 +115,13 @@ class Folio extends AbstractAPI implements | |||||
| */ | ||||||
| protected $dateConverter; | ||||||
|
|
||||||
| /** | ||||||
| * Auth method | ||||||
| * | ||||||
| * @var string | ||||||
| */ | ||||||
| protected $authMethod = null; | ||||||
|
|
||||||
| /** | ||||||
| * Default availability messages, in case they are not defined in Folio.ini | ||||||
| * | ||||||
|
|
@@ -145,13 +152,16 @@ class Folio extends AbstractAPI implements | |||||
| * @param \VuFind\Date\Converter $dateConverter Date converter object | ||||||
| * @param callable $sessionFactory Factory function returning | ||||||
| * SessionContainer object | ||||||
| * @param str $authMethod Authentication method | ||||||
| */ | ||||||
| public function __construct( | ||||||
| \VuFind\Date\Converter $dateConverter, | ||||||
| $sessionFactory | ||||||
| $sessionFactory, | ||||||
| string $authMethod | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth using constructor property promotion to simplify code. Also maybe worth defaulting to null for increased back-compatibility (if we want to make it optional). What do you think?
Suggested change
If you want to make the value mandatory, we should probably push this to dev-12.0 as a breaking change. |
||||||
| ) { | ||||||
| $this->dateConverter = $dateConverter; | ||||||
| $this->sessionFactory = $sessionFactory; | ||||||
| $this->authMethod = $authMethod; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -1270,6 +1280,41 @@ protected function patronLoginWithOkapi($username, $password) | |||||
| return $query; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Get the CQL query template for retrieving the patron's information. | ||||||
| * | ||||||
| * This supports both a single CQL query configured for all auth methods, and | ||||||
| * method-specific CQL queries, which can be configured by including the auth | ||||||
| * method name as a key in the `cql_by_auth_method` array in the User section of | ||||||
| * the config file. If method-specific CQL is configured, the driver will use | ||||||
| * the query for the selected auth method if it exists, and fall back to the | ||||||
| * general CQL query if not. If neither is configured, it will fall back to a | ||||||
| * default query that looks for the username and password in the fields | ||||||
| * specified by username_field and password_field in the config file, or username | ||||||
| * and password if those fields are not specified. | ||||||
| * | ||||||
| * @param string $usernameField The field to use for the username in the CQL query | ||||||
| * @param string $passwordField The field to use for password in the CQL query | ||||||
| * | ||||||
| * @return string The patron lookup CQL query with placeholders. | ||||||
| */ | ||||||
| protected function getLoginCql($usernameField, $passwordField) | ||||||
| { | ||||||
| $cql = $this->config['User']['cql_by_auth_method'][$this->authMethod] | ||||||
| ?? $this->config['User']['cql'] | ||||||
| ?? '%%username_field%% == "%%username%%"' . | ||||||
| ($passwordField ? ' and %%password_field%% == "%%password%%"' : ''); | ||||||
| $placeholders = [ | ||||||
| '%%username_field%%', | ||||||
| '%%password_field%%', | ||||||
| ]; | ||||||
| $values = [ | ||||||
| $usernameField, | ||||||
| $passwordField, | ||||||
| ]; | ||||||
| return str_replace($placeholders, $values, $cql); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Support method for patronLogin(): authenticate the patron with a CQL looup. | ||||||
| * Returns the CQL query for retrieving more information about the user. | ||||||
|
|
@@ -1284,28 +1329,32 @@ 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%%"' : ''); | ||||||
| $cql = $this->getLoginCql($usernameField, $passwordField); | ||||||
| $placeholders = [ | ||||||
| '%%username_field%%', | ||||||
| '%%password_field%%', | ||||||
| '%%username%%', | ||||||
| '%%password%%', | ||||||
| ]; | ||||||
| $values = [ | ||||||
| $usernameField, | ||||||
| $passwordField, | ||||||
| $this->escapeCql($username), | ||||||
| $this->escapeCql($password), | ||||||
| ]; | ||||||
| return str_replace($placeholders, $values, $cql); | ||||||
| $cql_expanded = str_replace($placeholders, $values, $cql); | ||||||
|
|
||||||
| $cql_redacted = str_replace($this->escapeCql($password), 'XXXX', $cql_expanded); | ||||||
| if ($this->config['User']['debug_login'] ?? false) { | ||||||
| $this->debug('FOLIO patron login CQL: ' . $cql_redacted); | ||||||
| $this->debug('Environment: ' . json_encode($_SERVER)); | ||||||
| } | ||||||
| return $cql_expanded; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Given a CQL query, fetch a single user; if we get an unexpected count, treat | ||||||
| * that as an unsuccessful login by returning null. | ||||||
| * | ||||||
| * Note that the `json_decode` then the `json_encode` in the debug statement | ||||||
| * is used to ensure that the JSON output appears in on line of the log file. | ||||||
| * | ||||||
| * @param string $query CQL query | ||||||
| * | ||||||
| * @return object | ||||||
|
|
@@ -1314,6 +1363,9 @@ protected function fetchUserWithCql($query) | |||||
| { | ||||||
| $response = $this->makeRequest('GET', '/users', compact('query')); | ||||||
| $json = json_decode($response->getBody()); | ||||||
| if ($this->config['User']['debug_login'] ?? false) { | ||||||
| $this->debug('FOLIO response body: ' . json_encode($json)); | ||||||
| } | ||||||
| return count($json->users ?? []) === 1 ? $json->users[0] : null; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,8 @@ public function __invoke( | |
| $manager = $container->get(\Laminas\Session\SessionManager::class); | ||
| return new \Laminas\Session\Container("Folio_$namespace", $manager); | ||
| }; | ||
| return parent::__invoke($container, $requestedName, [$sessionFactory]); | ||
| $authManager = $container->get(\VuFind\Auth\Manager::class); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| $selectedAuthMethod = $authManager->getSelectedAuthMethod(); | ||
| return parent::__invoke($container, $requestedName, [$sessionFactory, $selectedAuthMethod]); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a history of using backticks, so maybe we don't need them here; I also wonder if we should avoid saying "VuFind" repeatedly, since it may be implicit. Feel free to disagree if you think my suggested edits reduce clarity, though!