refactoring of the shibboleth authentification and authorization

[jochen-lienhard]

follow up PR 209 ... contains only the shibboleth part

[jochen-lienhard]

I need a little help. I want to add the following permission / rule. If there is no config for the staffview in the permission.ini, show it, if there is some, follow the rule (at the moment IPRange based).
When I add the isGranted check in StaffViewMARC.php, I get this error:
Call to a member function isGranted() on null

What can be wrong?

[demiankatz]

The issue here is that the authorization service will only be injected if the service manager creating the object uses the ZfcRbac\Initializer\AuthorizationServiceInitializer initializer. Right now, VuFind is only configured to use this initializer when creating controllers (see module/VuFind/config/module.config.php). To get this to work, you'll have to add the initializer to the configuration of the 'recordtab' section of module.config.php.

[demiankatz]

Let me know if you need more specific guidance/clarification!

[jochen-lienhard]

Thanks for that hint. It was a problem of the cached config files ;-)

[jochen-lienhard]

It seems there is a php warning in the IpRegEx.php:
PHP Warning: preg_match(): Delimiter must not be alphanumeric or backslash in ..../module/VuFind/src/VuFind/Role/PermissionProvider/IpRegEx.php on line 72

[jochen-lienhard]

I'm not sure if we should add a role staff and return in the legacy.StaffView a role => staff?

[demiankatz]

Regarding the warning in IpRegEx, I think this is a configuration problem rather than a bug -- if you pass in a regular expression that preg_match doesn't like, it will trigger the warning. Have you checked your configuration?

[jochen-lienhard]

I have no permission with IpRegEx active, so I'm wondering about this warning.

[demiankatz]

That's very strange. The class shouldn't even be instantiated unless something is trying to use it. Could you double-check your permissions.ini in both local/config/vufind and config/vufind just to be sure there are no uncommented lines in either referring to IpRegEx? If you don't find an obvious explanation there, you should also check for the [Auth] ip_range setting in Summon.ini and the [AdminAuth] ipRegEx setting in config.ini, since both of these are mapped to IpRegEx settings for legacy compatibility. If none of those things are set, this is a real mystery! Let me know exactly what you are doing that puts the warning into your logs, and I'll see if I can reproduce the situation on this end.

[jochen-lienhard]

I found the problem. I had a string not a regular expression in the AdminAuth. Sorry for my confusion.

[berndoberknapp]

I've implemented the permission providers as discussed in pull request 209.

Maybe we can find a better class name for Header. Shibboleth by default uses environment variables which are also included in $_SERVER. Actually Server would be more appropriate, but that's even worse that Header in my opinion.

[demiankatz]

Thanks, this is coming along nicely!

A few comments:

1.) I agree that Header isn't a great name for the class, since it sort of implies HTTP headers, which are something different. I also agree that Server isn't very helpful. What about ServerVariable? It's more verbose, but also more accurate, since we're dealing with server variables here, and I can't think of a different name for them.

2.) In permissions.ini, we should keep all the permission type definitions in alphabetical order... but we can hold off on reorganizing those until we've settled on a final name for header/servervariable/etc.

3.) In permissions.ini, you've turned off [default.EITModule], but we should leave that on as-is. I also don't think we should restrict staff view to logged in users by default, though we can certainly leave that configuration there (but commented out) as a useful example.

4.) Maybe I'm missing something, but it looks to me like some of the things you've marked as deprecated in config.ini have actually been removed completely... in which case, there's no reason to keep the corresponding configurations around. If we want to deprecate rather than remove, I think we may need some backward-compatibility logic somewhere.

5.) The most significant of these changes seems to be the handling of user attributes, which appears to be totally removed. I don't think we can totally do without this, because user attributes are used not just for authorization but also for population of VuFind's user table. For example, it should be possible to load in email address and ILS credentials from the Shibboleth attributes as part of the login process... but your changes seem to remove this capability (though again, I might be missing something).

[berndoberknapp]

1. Maybe ServerVar (less verbose 😉) oder ServerParam (Zend Request.php uses the term server params or server parameters)?

2. We had commented out default.EITModule for testing and I didn't notice that change when commiting the changes. I will revert the change and add the guest role to the staff view.

4)+5) Jochen and I will look into this on Monday. I would be in favor of removing the userattribute settings because they actually were used for authorization and we now have more fine grained permissions that have to be configured anyway if Shibboleth is used. Maybe we should remove these options but still check for them and throw an Exception if they are present?

[demiankatz]

1. I think I prefer ServerParam, but if you want to be as short as possible, I wouldn't object to ServerVar.

4/5) I'm okay with eliminating authorization-related functionality and throwing exceptions if legacy settings are still found. However, I definitely don't want to lose the ability to pre-populate the user database from attributes, since this is important for a good user experience (e.g. it allows us to pre-fill cat_username/cat_password, so I user can access ILS functions without having to manually type in a barcode later).

Thanks!

[berndoberknapp]

If the server param names differ depending on the web server setup, we can of course make them configurable (as discussed a week ago). The permission provider shoudn't require a separate configuration, it could reuse the settings from the config.ini [Shibboleth] section.

The user always is redirected to /MyResearch/Login for the login, but the permssion providers indeed seem to run on every page that contains protected content. I'm wondering why that is the case, wouldn't it be sufficient (and more performant) to compute the permissions only once during the login?

[demiankatz]

It sounds to me like we have a legitimate use case for configurable variable names, so it seems worth implementing... though of course it's also worth adding a comment discouraging people from changing the settings unless they really know what they're doing!

Regarding the permission providers running on every page, it may be possible to add some sort of caching mechanism to, for example, save generated permissions in the session. However, since most of these checks are fairly inexpensive, it is probably more secure to leave them alone -- otherwise a user might be able to, for example, bypass an IP restriction through a session hijack. Also, if we did caching, we would have to figure out under which circumstances the cache must be cleared, since sometimes changes of state will require new permissions to be generated.

[berndoberknapp]

I didn't expect that, because most more complex applications I know only use the Shibboleth SP session to setup an application session. Running the permission provider everytime the permission information is needed will work fine for Shibboleth - if the SP session length is configured accordingly.

[jochen-lienhard]

Hello Demian,

Franck implemented the user_attributes to allow login for users, who may use Ebsco or Summon.
With the new permission.ini this is not longer necessary. All users that can login, may use the
persistent favorites, and only the users with the correct attribute may use Ebsco or Summon.

The problem with temporary ID does not effect the user_attributes. So you can add the deprecated comment. The 'username' is the critical point for temporary ID, but I think we will discuss this soon.

[demiankatz]

So if a user with a temporary ID clicks on the account link, what happens? Or is your point that Shibboleth can assign attributes to users without actually allowing them to log in to VuFind?

[berndoberknapp]

The Shibboleth SP usually maps transient and persistent IDs to different server params, so both cases can be distinguished. If we keep the current implementation (and just add a deprecation notice for the userattribute options and a configuration option for the Shib-Identity-Provider server param) the login with a transient ID can easily be blocked by not mapping that ID to the server param specified in username.

There are use cases where we might want to allow a login with just a transient ID, for example if the user is entitled to access licensed content (I think that's the use case Jochen had in mind). And there are use cases where a user has a persistent ID but should not have access to persistent personalization features, for example for group accounts. With the permissions we could support these use cases, but of course that would require significant changes, so we should look at/discuss this (and other possible new features) on the mailinglist after the release.

[demiankatz]

Okay, that makes sense to me. I'll hold off on doing anything more here until you have time to add the configurable server parameter feature. After that, I'll be happy to put in a deprecation warning if you prefer that I define the wording there.

I'm going to miss the next VuFind developers call due to a conference, but perhaps on the following call in mid-April I can put the topic of permissions for social features (and, more broadly, future goals for permissioning) on the agenda to get a discussion started.

[berndoberknapp]

How can I access a setting from config.ini (name of the server param with the identity provider entityID, added for the Shibboleth auth module) in the Shibboleth permission provider?

[demiankatz]

You could pass the config.ini settings to the constructor of the permission provider from within the VuFind\Role\PermissionProvider\Factory (either passing in the whole configuration object or using some of the settings, as appropriate). To access the configuration in the factory, use:

$sm->getServiceLocator()->get('VuFind\Config')->get('config')

(this will give you a Zend\Config\Config object)

[berndoberknapp]

I've added the configuration option for the identity provider entityID server param with default Shib-Identity-Provider.

While testing the changes I noticed that getServer()->get(...) has to be tested for null instead of false to detect missing server params. The bugfix commit unfortunately only contains the fix for one check, the fixes for the Shibboleth permission provider and the unit tests are included in the second commit.

[berndoberknapp]

I'm wondering why the result of VuFind\Auth\Shibboleth::isExpired depends on whether the configuration contains a logout URL. The Shibboleth SP session expired after the lifetime or timeout configured in shibboleth2.xml, and when it expires the Shibboleth server params are gone. So it should be sufficient to check if the identity provider entityID server param exists, or am I missing something?

[demiankatz]

I seem to recall some conversation long ago about some Shibboleth setups offering users a mechanism to manually log out, and others failing to do so. Checks related to the logout URL setting may be an attempt to accommodate those two scenarios, but I'm not sure why/how that would affect expiration.

[berndoberknapp]

Logout is one of the Shibboleth features we probably should discuss on the mailinglist. The Shibboleth SP supports both local logout and single logout (SLO). Local logout doesn't make much sense in my opinion because the user can simply login again without entering credentials if the IdP session is still valid - we only could tell the user to close the browser to really log out. Implementing SLO is difficult, especially on the IdP side (see https://wiki.shibboleth.net/confluence/display/CONCEPT/SLOIssues ), nevertheless I think we should be prepared to support SLO. The Shibboleth SP supports front channel SLO via redirects (we have a working implementation for VuFind 1.3) and back channel SLO via SOAP requests and application notification (this should also be feasible).

[EreMaijala]

In our federation there's a local (to Finland) logout implementation where on logout the app must redirect back to the IdP using a link provided in Shibboleth attributes. Not much different from a staticly configured logout URL, but something we have to implement locally, at least.

[demiankatz]

Definitely an issue worth discussing further among actual Shibboleth users. Is there anything I can do to help facilitate this discussion? Would a wiki page or JIRA ticket be helpful for summarizing issues and collecting opinions? Would you like me to get this onto the developers call agenda for a particular date? (Due to the fact that many Shib users are not regular attendees of the dev call, it might be best to start the discussion on the mailing list and then plan on a future dev call date if people would benefit from a real-time discussion).

[jochen-lienhard]

Bernd and I discussed about the next step. We will send a mail to the vufind-tech list (after eastern) describing the changes and new features of the new Shibboleth authentication and authorization.
We think that the mailing list would be the best platform to discuss this.

[demiankatz]

Sounds like a good plan!

[berndoberknapp]

I would like to clean up the remaining changes in this pull request before adding more changes to the Shibboleth authentication module. There are different changes which unfortunately are mixed.

The result of request->getServer()->get(...) is compared against false instead of null in two places, see bea0065 and line 80 resp. 94 in PermissionProvider/Shibboleth.php 42c33d1. These changes should be included in the next bugfix release.

The remaining changes make the name of the server param with the IdP entityID configurable as requested. I didn't want to duplicate the option in the authentication module and the permission provider, therefore I've also used the option/default from the authentication module in the permission provider. I hope that's OK?

Regarding the code coverage of the unit tests for the PermissionProvider, some of the code marked as not executed in ServerParam.php is actually tested by Shibboleth.php, but that doesn't seem to count...

What's the best way to proceed with theses changes?

[demiankatz]

Bernd,

I've manually applied the false ==> null corrections to the release-2.4 branch and merged that both to master and back into this branch.

The remainder of the code looks reasonable to me and can probably be committed to master soon in order to close out this PR (we can start a new one for the next phase).

Just one question first: I see that you've changed the providerId parameter to entityId. What is the significance of this? Are they synonyms, or do they potentially serve different purposes? If they're different, maybe we need two different config options. If they're the same, we should probably adjust some of the terminology to be more consistent (right now the config.ini comment says we're going to use the providerId parameter -- at very least, maybe that needs to change).

Thanks as always for all the work on this!

[demiankatz]

Regarding the code coverage, note that a method will not show as green until every line has been covered by tests, and it looks like there are some edge cases that are not covered in the case of ServerParam, such as the $escape === '' case in splitString(). Are there really some lines that are showing as uncovered that you're sure are executing, or does this explain what you are seeing?

[demiankatz]

It's been over a month since the last comment, so I'm just checking in to see where we are with this. There are still a couple of unanswered questions in my last two comments. As always, please let me know if I can be of any further assistance... and no rush on this; I just want to make sure nobody has missed anything.

[berndoberknapp]

I'm sorry for the delay. I was out of office for five weeks and currently I'm busy prepraring the move to a new library building next week...

providerId is an alias for entityID for backward compatibility, the setting/parameter was renamed in Shibboleth 2. I think we currently should leave the ini setting as is and change it when we improve the Shibboleth module. So you could merge the changes and close the PR.

[demiankatz]

Thanks for the clarification. I've gone ahead and manually merged this to master. Please let me know if you still need any help with the testing situation.