expired staff accounts can login to the staff client

[Chris Sharp]

Creating a new security bug for this issue. We got a report from one of our libraries that expired staff accounts are able to login to the staff client. I have so far been unable to find an expired staff account than cannot login, so I'm thinking this is a code level problem that emerged in 2.7.

Evergreen 2.7.2+
OpenSRF 2.4.0
PostgreSQL 9.3
Ubuntu LTS

[Chris Sharp (chrissharp123)]

Okay, after clarifications by Jeff Godin and Thomas Berezansky, I can see that expired staff accounts have always been able to log in. I have been under the mistaken assumption for years that expiring an account prevented login. I still believe we have a security concern here, however. Do we want expired users to be able to login and view privileged user information and possibly be able to manipulate things? It seems to me that we would want a big NO ENTRY policy for expired staff accounts. Thoughts?

[Kathy Lussier (klussier)]

I agree that expired staff accounts should not be able to view user information. My preference would be to prohibit these accounts from logging into the client.

[Thomas Berezansky (tsbere)]

I think it depends on what that expiration date is supposed to be.

If it is only supposed to be a "You are denied circ/hold access" thing then it works as intended, no changes needed.

If it is supposed to be "you are denied all privileges" then you wouldn't even be able to log in as that has a permission attached, regardless of how you log in, which would stop patrons from logging into the OPAC as well.

If it is supposed/desired to be "you are denied *some* privileges" then we may need a new column on permission.perm_list or the permission map tables to say "this is allowed when expired". Then you could decide if STAFF_LOGIN is on that list or not as a local change. And, we should also consider how should "Everything" work with all of this?

[Chris Sharp (chrissharp123)]

Tom,

I think our assumption at PINES is that this is "you are denied all privileges" until the account is renewed. I think users with the "Everything" permission should be excluded.

Thanks,

Chris

[Thomas Berezansky (tsbere)]

I would assume that if it is implemented as "denied ALL" then "Everything" would fall under "ALL". Denied SOME would be a different thing with Everything.

And, thinking about it again, the check for "allow/deny when expired" could be on the mapping instead of the permission itself. Thus you could have entire groups who keep permissions when expired, but other groups lose those same permissions. This could even apply to the Everything permission in that case.

[Terran McCanna (tmccanna)]

In the discussion at https://bugs.launchpad.net/evergreen/+bug/1870548, having a library setting or a global flag for this was suggested, and that makes sense to me.

[Joan Kranich (jkranich)]

I agree that if expired staff accounts are prevented from logging into Evergreen, it should be controlled by a setting.

There are times at our libraries that only one staff member is working and there may not be another staff member available to renew the registration.

We control Evergreen access by changing the permission group in staff accounts of staff that have left the library.

[Galen Charlton (gmc)]

I've pushed the following branch to the working repository:

user/gmcharlt/lp1474029_staff_expiry_prevents_login / https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/gmcharlt/lp1474029_staff_expiry_prevents_login

This adds a global flag that, when turned on, blocks expired staff members from logging into all Evergreen interfaces, including the public catalog.

I won't slap a pull request on this just yet, as I'm curious about whether there's any feedback on the general approach. Some notes on decisions I made:

* I went with a global flag rather than a library setting on the basis that this sort of security decision really ought to be a consortial policy.
* Expired staff users are locked out of /all/ interfaces to prevent creative use of the Evergreen login API to acquire an authtoken that would let the user into the staff interface anyway. This is in contrast to normal patrons, who remain allowed to log into the public catalog even if their account has expired.
* As a consequence of the above, a staff user is defined as somebody who has the STAFF_LOGIN permission.

[Terran McCanna (tmccanna)]

We have not tested this yet, but we at PINES are in favor of the global flag.

We don't have strong feelings one way or another whether or not the expired staff account should be able to log into the OPAC, but we're fine with it as long as it prevents logging into the staff client.

[Galen Charlton (gmc)]

Thanks for the feedback, Terran. I will go ahead and apply the pullrequest tag now.

[Mike Risher (mrisher)]

I'm trying to test this, but am not clear on the steps to go through. After I applied this patch I tested logging on to the staff client with an expired System Administrator account and it's successful. This leads me to believe I might be missing something.

What steps do testers need to go through to activate the new global flag? I plan on trying to log into the staff interface and public catalog. Any additional areas that should be tested?

[Jane Sandberg (sandbergja)]

Hi Mike. To turn on the global flag, you can follow these steps:

1) Log into the staff client.
2) Under the Administration menu, choose Server Administration.
3) Click Global Flags
4) Find the appropriate row, double click it, enable, and save.

[Mike Risher (mrisher)]

Re: step 4. Galen's commit tells us the global flag's name is "auth.block_expired_staff_login". It's not on the list of global flags on my end. I've switched to his branch, run the usual scripts, cleared my cache & done a hard reload with Chrome, but still don't see it.

[Terran McCanna (tmccanna)]

I tested this against current master and was able to enable/disable the flag and successfully test it with expired accounts both ways. I'll be really happy when this one is in our production environment! My sign off:

I have tested this code and consent to signing off on it with my name, Terran McCanna, and my email address, <email address hidden>.

[Bill Erickson (berick)]

Works as advertised. Merged to rel_3_6 and master. Thanks, All!