From: Galen Charlton <gmc@equinoxOLI.org>
Date: Thu, 5 Aug 2021 21:55:18 +0000 (-0400)
Subject: LP#1844121: prevent staff login by expired barcode
X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=b3d80259e25904e3580a18471ced4cb2edba127b;p=evergreen%2Fpines.git

LP#1844121: prevent staff login by expired barcode

open-ils.auth.authenticate.init accepts an identifier as its
sole parameter, then determines whether it looks like a
username or barcode and retrieves the patron's password
salt as the seed accordingly.

open-ils.auth.authenticate.complete can accept the identifier
via the 'identifier', 'username', or 'barcode' keys, but the
key used does not need to match how .init found the patron.

As a consequence, the .init/.complete dance can retrieve the
patron by barcode but handle the barcode value as if it were
a username, thereby bypassing the check of whether the barcode
was inactive. In particular, the AngularJS staff client login
process does this, meaning that staff members can log in to the
staff client via the AngularJS form using an expired barcode.

This is not good. The OPAC explicitly blocks logging in using an
inactive barcode because it checks the identifier type and sets
the key passed to .complete accordingly. The Angular staff login
page also prevents logging in using an inactive barcode because
(a) it uses open-ils.auth.login, which doesn't have the same
problem and (b) it forces the identifier to be marked as a user
name regardless.

NOTE: this means that the Angular staff login form prevents staff
from logging in via barcode, which potentially is a regression as
compared to the AngularJS side (or, alternatively, is providing
additional necessary strictness).

This patch avoids the problem by having .complete inspect the
cached seed created by .init to determine how the user was ultimately
found.

Some alternative approaches that were rejected include:

[1] Having AngularJS just mirror Angular. Problem: if some staff
    users are used to using their barcode to log in, doing
    this would cause an immediate problem. I note that because
    the staff interface URL is commonly expressed as
    https://library.example/eg/staff, is currently far more common
    for the staff interface to be logged into via the AngularJS
    form rather than the Angular one.
[2] Having AngularJS use open-ils.auth.login, but make it and
    Angular use 'identifier' as the key rather than 'username'.
    Problem: while this would have the desired effect if you
    only use native authentication, if you're using open-ils.auth_proxy,
    it won't work - open-ils.auth_proxy.login doesn't recognize an
    'identifier' parameter. While that could be changed, it
    is more invasive.

To test
-------
[1] Set up a staff user that has a username, an active barcode,
    and an inactive barcode.
[2] Log in to the AngularJS staff interface (/eg/staff) using
    the username, the active barcode, and the inactive one.
[3] Note that you are permitted to log in with all three identifiers.
[4] Apply the patch and repeat step 2.
[5] This time, logging in using the inactive barcode should
    fail.
[6] Verify that other login types continue to work as expected:
    - Angular staff login form
    - OPAC
    - SIP2 terminal login
    - SIP2 user authentication
    - operator change (Angular and AngularJS)
    - Web-based self-check
[7] Extra credit: test logging in via open-ils.auth_proxy with
    it falling back to native authentication.

Signed-off-by: Galen Charlton <gmc@equinoxOLI.org>
Signed-off-by: Shula Link <slink@gchrl.org>
Signed-off-by: Mike Rylander <mrylander@gmail.com>
---

diff --git a/Open-ILS/src/c-apps/oils_auth.c b/Open-ILS/src/c-apps/oils_auth.c
index 1836b025b8..577e725642 100644
--- a/Open-ILS/src/c-apps/oils_auth.c
+++ b/Open-ILS/src/c-apps/oils_auth.c
@@ -769,6 +769,21 @@ int oilsAuthComplete( osrfMethodContext* ctx ) {
         ctx, "open-ils.cstore.direct.actor.user.retrieve", param);
     jsonObjectFree(param);
 
+    // determine if authenticate.init had found the user by barcode,
+    // regardless of whether authenticate.complete is being passed
+    // a username or identifier key.
+    bool initFoundUserByBarcode = false;
+    jsonObject* value = NULL;
+    jsonIterator* cacheIter = jsonNewIterator(cacheObj);
+    while (value = jsonIteratorNext(cacheIter)) {
+        const char *key_name = cacheIter->key;
+        if (!strcmp(key_name, "barcode")) {
+            initFoundUserByBarcode = true;
+            break;
+        }
+    }
+    jsonIteratorFree(cacheIter);
+
     char* freeable_uname = NULL;
     if (!uname) {
         uname = freeable_uname = oilsFMGetString(userObj, "usrname");
@@ -781,7 +796,11 @@ int oilsAuthComplete( osrfMethodContext* ctx ) {
         jsonNewNumberObject(oilsFMGetObjectId(userObj)));
     jsonObjectSetKey(params,"org_unit", jsonNewNumberObject(orgloc));
     jsonObjectSetKey(params, "login_type", jsonNewObject(type));
-    if (barcode) jsonObjectSetKey(params, "barcode", jsonNewObject(barcode));
+    if (initFoundUserByBarcode) {
+         jsonObjectSetKey(params, "barcode", jsonNewObject(identifier));
+    } else if (barcode) {
+         jsonObjectSetKey(params, "barcode", jsonNewObject(barcode));
+    }
 
     jsonObject* authEvt = oilsUtilsQuickReqCtx( // freed after password test
         ctx,