Add setting to restrict ILS login by user.

[demiankatz]

This PR makes it possible to turn off the catalog login form in scenarios where end users should never have the ability to log in (e.g. because credentials are always pre-loaded via authentication system attributes).

TODO

• Add tests once approach is approved

[EreMaijala]

@demiankatz This looks good, but I wonder if we need to account for ChoiceAuth somehow. E.g. if the user logs in using Shibboleth, disable ILS user login, but if user logs in with Database auth, allow it? Or using MultiBackend and you get credentials for one but not the other. Maybe I'm just thinking too far and this is fine in either case. But one place where this should be tied at least in config file comments is library cards (library_cards and auth_based_library_cards) that would still allow one to add cards.

[demiankatz]

Thanks, @EreMaijala. If our main intention here is to prevent people from injecting credentials into VuFind in scenarios where user-entered details are unsupported, I'm not sure that we need to worry about ChoiceAuth with ILS enabled, since that's openly inviting people to log in with ILS credentials. I can't really think of a scenario where you could do harm by logging in with Shibboleth and then entering ILS credentials, as opposed to just logging in directly with the ILS credentials. Happy to discuss further, though, if you can see a compelling use case.

Regarding library card settings, is the idea that you cannot turn on library_cards if you have logins disabled, but you can with auth_based_library_cards? I don't use those settings much, but I'm happy to take a stab at the comment updates if you can confirm that my interpretation is correct.

I also notice that the GitHub Actions tests seem to have gotten stuck for some reason. Hopefully next time I push up changes, it will sort itself out.

[lahmann]

Thanks @demiankatz! I tested this code with our Shibboleth configuration and it works fine.

I wonder if in addition to throwing an exception the user should be logged out - either this is an attempt to interact with VuFind in a way the configuration prohibits or something went completely wrong prompting the user for login data. Either way ending the current session would result in a known state.

[demiankatz]

Thanks, @lahmann! It certainly wouldn't be hard to redirect to the logout action instead of throwing an exception... I'm really not sure what's best, though. Since I can only realistically see this happening if someone is taking malicious action, I don't think we care too much about the user experience. I like the exception because it ensures that the event is logged at a high level, though of course we could also log a message in combination with the logout redirect. We could also refactor the code to use a support method called something like handleUnexpectedCatalogLogin so it's easier to customize the response locally. I'm interested in whether @EreMaijala has an opinion, and happy to make some adjustments when we reach consensus.

[EreMaijala]

@demiankatz I might be overthinking it. But with ChoiceAuth+MultiILS you could have Shibboleth and ILS authentication with Shibboleth providing ILS credentials to one ILS while MultiILS allows login to the other one.

[EreMaijala]

@demiankatz And with library cards, you could (accidentally) leave them enabled while trying to restrict catalog login using the new setting.

[EreMaijala]

Nevertheless I still don't quite understand why the ILS driver cannot be configured to disallow an empty string as password. I'm probably missing a detail or two here.

[demiankatz]

Thanks, @EreMaijala -- I'll do some experiments with library cards and see how they interact with the new setting, and then I'll adjust comments and/or code appropriately.

Regarding the ChoiceAuth+MultiILS scenario, I guess the question may come down to whether or not we can reliably know the user's current authentication method at the moment that they attempt the catalog login. If that data is available, we could probably link the restriction to specific authentication methods -- maybe revising the current setting to be an array of authentication methods (with wildcard support) or else adding a second setting for more granular checking. If you think it's worth the effort, I can investigate a little more deeply. If we're going to need it, it's probably better to do it right now rather than having to change/replace settings after they've been released.

And as for configuring ILS drivers to disallow empty passwords, I think the idea of this setting is that, in some environments, there may only be a single data point available to connect a user account to the ILS, and we want to be able to utilize that without having to worry about the possibility of users impersonating each other via the catalog login form. Historically, we've avoided that problem by using a secondary data point for login, even when the underlying system might not really require that. We could do a variety of things to approach this in different ways (for example: allow a stored catalog login with a blank password, but don't allow blank passwords via POST), but it seems most straightforward to have a setting that switches off the form. If the VuFind administrator expects all users to always get credentials automatically loaded via SSO, there's no reason to allow for the possibility of a catalog login form appearing -- probably better to show an error message so the affected user can report it and reach a solution.

[EreMaijala]

@demiankatz I understand the single data point issue, and it's the same with email authentication of Alma. But since user-entered password can never be null while system-provided can, a distinction can be made based on that.

[demiankatz]

@demiankatz I understand the single data point issue, and it's the same with email authentication of Alma. But since user-entered password can never be null while system-provided can, a distinction can be made based on that.

That's true, and I'm not opposed to following up that line of thought... but that's a subtle and potentially confusing distinction, and I think bugs are likely. Since this addresses security, I think there might be more peace of mind having an "off switch" as an option, even if we also offer some additional alternatives in the future.

[demiankatz]

@EreMaijala, I just took a look at the library card functionality, and it's rather more complicated than I had remembered! Would the simplest approach here, in addition to adding an appropriate comment to config.ini, be to check if user login is disabled and ILS login method resolves to password, and throw an exception about incompatible configurations in that case?

[EreMaijala]

@demiankatz Agreed, and I think that's a sound approach, and doesn't add too much complexity.

[EreMaijala]

Let's add the check that throws an exception if incompatible settings are found.

[demiankatz]

@EreMaijala, d8d14c3 adds the basic check to prevent unanticipated library card password-based logins and some additional config.ini comments. I wonder if we should also do something to the librarycard/editcard.phtml template, but if so, I'm not really sure what.

[EreMaijala]

I'm not sure if there's much to do in the editcard.phtml template, but what we could additionally do is hide the library cards entry from MyResearch menu if ILS login is disabled. Or if it's useful to still see the card, more logic could be added to hide any add/edit buttons.

In any case, this looks good to me so far.

[demiankatz]

Thanks, @EreMaijala. I think as far as hiding the option goes, I was a bit unclear about how all the settings might interact (e.g. the authentication-based library cards, or how email-based login fits into the picture), and so I wasn't sure exactly how to detect the scenario where library cards should be disabled; it seems to me like there may be some situations where you could disable user catalog login but still have some level of working library card functionality. But maybe I'm wrong!

In any case, I'll wait a little longer to see if @lahmann has any further feedback (or wants to discuss the idea of forcing a log-out in more detail); if I don't hear anything more by next week, I'll write up tests and get this merged (on the theory that we can always revise further at a future date).

[EreMaijala]

@demiankatz I think that the library card functionality will only be useful for display purposes, if even for that, if ILS login is disabled. Or maybe for removing a second card that you had added before it became forbidden. :)

[demiankatz]

@EreMaijala, I'm thinking of the auth_based_library_cards setting; doesn't that offer the option of linking multiple cards via SSO?

[EreMaijala]

@demiankatz In theory, yes, but at least the current code in Shibboleth::connectLibraryCard handles a single one. And even if the user has multiple automatically connected cards, it doesn't require the library card actions, just the possibility to change the active card.

[lahmann]

Thanks @demiankatz and @EreMaijala for pushing things forward and I have no strong objections. I agree that exceptions allow a great way of logging and therefore reporting potentially malicious actions and would definitely keep the exception. Regarding the forced logout on providing POST data when allowUserLogin is false my initial thought was that this only happens when someone tries malicious actions by utilizing a valid session. In order to prevent any other malicious actions by using the same valid session, invalidating it would be an extra security layer in addition to logging the whole attempt. Kind of an intrusion detection counter measure - but maybe I am just over-engineering this. :)

[demiankatz]

I'm not sure if there's much to do in the editcard.phtml template, but what we could additionally do is hide the library cards entry from MyResearch menu if ILS login is disabled. Or if it's useful to still see the card, more logic could be added to hide any add/edit buttons.

In any case, this looks good to me so far.

I like the idea of removing add/edit buttons rather than hiding the entire feature. It looks to me like this should be compatible with the scenario where user login is disabled, but creating cards via SSO logins is enabled. See commit 3d24f65 for the latest additions.

[demiankatz]

Thanks @demiankatz and @EreMaijala for pushing things forward and I have no strong objections. I agree that exceptions allow a great way of logging and therefore reporting potentially malicious actions and would definitely keep the exception. Regarding the forced logout on providing POST data when allowUserLogin is false my initial thought was that this only happens when someone tries malicious actions by utilizing a valid session. In order to prevent any other malicious actions by using the same valid session, invalidating it would be an extra security layer in addition to logging the whole attempt. Kind of an intrusion detection counter measure - but maybe I am just over-engineering this. :)

If this were a scenario where user X was attempting to hijack a session belonging to user Y, I would definitely strongly support the idea of invalidating the session to prevent further abuse... but I think if somebody were trying to abuse the catalog login, they would be using their own credentials to log in, and then attempting to attach their own VuFind account with somebody else's ILS account. Invalidating their VuFind session might mildly inconvenience them, but I don't think it would really make a meaningful difference, because they could just log back in and try again -- they didn't need to steal credentials or abuse a session to get to this point, and trying to do this to somebody else's account wouldn't really make much sense. Please correct me if my line of thinking is off-base, but I think it's probably better to keep things simple and understandable rather than adding extra logic here.

[demiankatz]

I've also added unit test coverage for the Auth\Manager code. I don't think the controllers are currently in a particularly easy-to-test state. The question, then, is whether a Mink test is useful/worth the effort here, or if we should just merge this for now. Any thoughts/preferences?

[lahmann]

Thanks @demiankatz, you are absolutely right that invalidating the session would most likely only be a nuisance - I didn't look at it from this perspective... sorry.

Thanks also for implementing the tests - regarding mink test coverage I have no preference and am good with this pull request as it is.

[demiankatz]

Thanks again, @lahmann and @EreMaijala. I've added a Mink test to exercise the basic "disable ILS login form" behavior.

It would probably also be useful to add a separate test for the library card functionality, but I see that we don't seem to have any library card tests yet. I've opened VUFIND-1585 to track this.