Bug 30962 - Add POST endpoint for validating a user password
Summary: Add POST endpoint for validating a user password
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: David Cook
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 35204 36575 32739
  Show dependency treegraph
 
Reported: 2022-06-15 06:16 UTC by David Cook
Modified: 2024-04-12 03:38 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03


Attachments
Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password (5.66 KB, patch)
2022-06-17 04:55 UTC, David Cook
Details | Diff | Splinter Review
Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password (5.71 KB, patch)
2022-06-17 19:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password (6.75 KB, patch)
2022-12-23 00:34 UTC, David Cook
Details | Diff | Splinter Review
Bug 30962: REST API: Add endpoint /auth/password/validation (6.66 KB, patch)
2023-01-04 01:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 30962: Add unit tests for /auth/password/validation endpoint (8.87 KB, patch)
2023-01-04 01:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 30962: REST API: Add endpoint /auth/password/validation (6.71 KB, patch)
2023-01-17 07:45 UTC, David Nind
Details | Diff | Splinter Review
Bug 30962: Add unit tests for /auth/password/validation endpoint (8.92 KB, patch)
2023-01-17 07:45 UTC, David Nind
Details | Diff | Splinter Review
Bug 30962: REST API: Add endpoint /auth/password/validation (6.77 KB, patch)
2023-01-27 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30962: Add unit tests for /auth/password/validation endpoint (8.97 KB, patch)
2023-01-27 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30962: (QA follow-up) Rename attribute and simplify tests (12.05 KB, patch)
2023-01-27 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30962: Add unit tests for /auth/password/validation endpoint (9.03 KB, patch)
2023-01-31 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30962: (QA follow-up) Rename attribute and simplify tests (12.10 KB, patch)
2023-01-31 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2022-06-15 06:16:53 UTC
This endpoint would check that the POSTed password matches the Koha database password.

In theory, you could have other validations here as well (e.g. check if patron is expired, restricted, etc.). But maybe those should be their own endpoints. 

Adding this to support a Keycloak User Storage SPI as per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17004#c39
Comment 1 Katrin Fischer 2022-06-15 07:46:25 UTC
+1
Comment 2 Tomás Cohen Arazi 2022-06-15 12:28:33 UTC
How will we prevent abuse for this password-guessing service? Hehe
Comment 3 David Cook 2022-06-16 05:40:04 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> How will we prevent abuse for this password-guessing service? Hehe

It's not a public/anonymous endpoint. Only an authenticated and authorized user could use it.
Comment 4 David Cook 2022-06-16 07:10:00 UTC
(In reply to David Cook from comment #3)
> (In reply to Tomás Cohen Arazi from comment #2)
> > How will we prevent abuse for this password-guessing service? Hehe
> 
> It's not a public/anonymous endpoint. Only an authenticated and authorized
> user could use it.

But happy to add more security to lock accounts on too many bad password checks.
Comment 5 Katrin Fischer 2022-06-16 15:54:34 UTC
(In reply to David Cook from comment #4)
> (In reply to David Cook from comment #3)
> > (In reply to Tomás Cohen Arazi from comment #2)
> > > How will we prevent abuse for this password-guessing service? Hehe
> > 
> > It's not a public/anonymous endpoint. Only an authenticated and authorized
> > user could use it.
> 
> But happy to add more security to lock accounts on too many bad password
> checks.

I think it would make sense to use the existing lock feature here, also consistent.

ILS-DI locks too.
Comment 6 David Cook 2022-06-17 02:31:00 UTC
(In reply to Katrin Fischer from comment #5)
> (In reply to David Cook from comment #4)
> > (In reply to David Cook from comment #3)
> > > (In reply to Tomás Cohen Arazi from comment #2)
> > > > How will we prevent abuse for this password-guessing service? Hehe
> > > 
> > > It's not a public/anonymous endpoint. Only an authenticated and authorized
> > > user could use it.
> > 
> > But happy to add more security to lock accounts on too many bad password
> > checks.
> 
> I think it would make sense to use the existing lock feature here, also
> consistent.
> 
> ILS-DI locks too.

Done!
Comment 7 David Cook 2022-06-17 04:55:57 UTC
Created attachment 136215 [details] [review]
Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password

This patch adds an endpoint for /patrons/:patron_id/check_password

This allows a third-party, using an authenticated and authorized Koha
API user, to check if the username and password given by a user is
correct in Koha.

For example, a Keycloak extension can be created using its
User Storage SPI to use Koha as the user database for Keycloak. This
API allows us to authenticate the user as a particular Koha user - without
creating a Koha user session for them.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth
2. Enable "RESTBasicAuth"
3. Run the following commands while substituting correct values for <koha_user> and <koha_password>
3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "<koha_password>" }' -v
4. Note "200 OK" response
5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "this is definitely not the password" }' -v
6. Note "400 Bad Request" response and error message {"error":"Invalid password"}
Comment 8 David Nind 2022-06-17 19:09:33 UTC
Created attachment 136300 [details] [review]
Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password

This patch adds an endpoint for /patrons/:patron_id/check_password

This allows a third-party, using an authenticated and authorized Koha
API user, to check if the username and password given by a user is
correct in Koha.

For example, a Keycloak extension can be created using its
User Storage SPI to use Koha as the user database for Keycloak. This
API allows us to authenticate the user as a particular Koha user - without
creating a Koha user session for them.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth
2. Enable "RESTBasicAuth"
3. Run the following commands while substituting correct values for <koha_user> and <koha_password>
3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "<koha_password>" }' -v
4. Note "200 OK" response
5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "this is definitely not the password" }' -v
6. Note "400 Bad Request" response and error message {"error":"Invalid password"}

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Cook 2022-07-29 01:22:31 UTC
Regarding security, I've been thinking more about this, and in theory you could lock down all non-public API routes by IP address, if your organisation has static IP addresses and requires VPNs for working from home (WFH).

We do this on other systems that have admin APIs. 

It's possible that you might need to provide a third-party access to an admin API (like this one in bug 30962), but then you can add their IP address to the allow list. 

It just adds another layer of security over top of the existing security measures. 

We could promote the idea by adding some configuration directives to Apache that allow all IP addresses for both public and non-public API routes and include some comments about how they can lock down the non-public API routes by doing X, Y, and Z.
Comment 10 David Cook 2022-08-02 02:55:11 UTC
I was thinking about this again, and really it's very similar to an LDAP lookup. 

It requires staff-level authorization, and with the account lockout functionality, it wouldn't be any different from someone trying to brute force the OPAC login.
Comment 11 Jonathan Druart 2022-08-09 08:27:09 UTC
1. Missing tests (you must provide tons of tests to cover the different situations)
2. Route's name should not be a verb (/password/validation maybe?)
3. Routes that returns empty should return 204
4. It's always returning "Invalid password" even for other failures (like too many attempts)
5. It allows you to check for pwd validation for a user you don't know their userid (you can force brute only by knowing the patron's id). I don't think it's a security concern as userid could be guessed anyway (?)
6. following 5, you can lock any accounts if FailedLoginAttempts is set, no need to know the userid list. How bad is that?
Comment 12 David Cook 2022-08-10 01:41:23 UTC
(In reply to Jonathan Druart from comment #11)
> 1. Missing tests (you must provide tons of tests to cover the different
> situations)

Are all the existing API tests in "t/db_dependent/api/v1/"?

> 2. Route's name should not be a verb (/password/validation maybe?)

I did struggle to come up with a noun for this one so I take the point. 

I think "validation" or "authentication" instead of "check_password" makes sense. It also makes things more flexible for the future. 

I suppose the concept would be that we're creating the patron validation or authentication by supplying these details...

> 4. It's always returning "Invalid password" even for other failures (like
> too many attempts)

I suppose a generic error like "Authentication failed" or "Validation failed" would be better.

> 5. It allows you to check for pwd validation for a user you don't know their
> userid (you can force brute only by knowing the patron's id). I don't think
> it's a security concern as userid could be guessed anyway (?)
> 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no
> need to know the userid list. How bad is that?

Remember that this isn't a public endpoint. Depending on an attacker's goal, it would be silly to try to brute force  /patrons/:patron_id/check_password when you could just use /patrons/:patron_id/password to set the password to whatever you want, and then use /patrons/:patron_id to get the userid in order to login. Both password API endpoints require the same level of staff-level authorization from an authenticated API user.

That said, I do take the point. I'm not opposed to changing this to "/patrons/authentication" and sending userid and password (like http://localhost:8080/cgi-bin/koha/ilsdi.pl/ilsdi.pl?service=Describe&verb=AuthenticatePatron)
Comment 13 David Cook 2022-08-10 01:48:31 UTC
While I originally created this to support a Keycloak User Storage SPI extension, I've had libraries interested in it as an alternative to the ILS-DI API "AuthenticatePatron" verb. 

It's essentially the 1 thing that the ILS-DI API can do that the REST API can't do, which means that libraries use the ILS-DI API (or SIP2) to authenticate patrons against the Koha database for third-parties software.

So it would be great to get this into Koha core, so that we can continue to move away from the ILS-DI API.

--

We've actually had more and more libraries using Keycloak with the Koha user storage extension, because it means they get to-purpose software for SSO, while still getting to manage all their patrons in Koha. It's super useful. 

(I also have a goal of Koha using Keycloak Oauth2 access tokens to bring SSO to Koha's REST API, so that third-parties could call public API endpoints on behalf of Koha users without third-parties needing dedicated API users with staff-level permissions, which would be a massive improvement on security/trust relationships, but one step at a time...)
Comment 14 David Cook 2022-09-07 23:22:00 UTC
I'm keen to keep working on this but without any additional feedback I don't really have any work to do...
Comment 15 David Cook 2022-09-12 02:02:47 UTC
I'm planning on making a plugin for this, and it's got me thinking more about what people would accept in the longer term...

I'm thinking /api/v1/auth/ will be the route, and it'll take "username" and "password" JSON POST parameters. 

I've noticed FortiAuthenticator works this way; it's the way that the ILS-DI API works; it would be difficult to abuse. 

I suppose we could argue about the route name. FortiAuthenticator uses that exact same route, but I suppose it could be /api/v1/patrons/auth, so it makes it more obvious that it's a route not for authenticating to use the API but rather just for authenticating patrons.
Comment 16 Tomás Cohen Arazi 2022-09-16 19:10:02 UTC
(In reply to David Cook from comment #12)
> (In reply to Jonathan Druart from comment #11)
> > 1. Missing tests (you must provide tons of tests to cover the different
> > situations)
> 
> Are all the existing API tests in "t/db_dependent/api/v1/"?

Not sure what you mean here, but we try to enforce 100% code coverage on the tests.

> > 2. Route's name should not be a verb (/password/validation maybe?)
> 
> I did struggle to come up with a noun for this one so I take the point. 

I'd go with 'validation'.

> > 4. It's always returning "Invalid password" even for other failures (like
> > too many attempts)
> 
> I suppose a generic error like "Authentication failed" or "Validation
> failed" would be better.

If you think generic is fine, go for it. If you think there are use cases in which the caller could take advantage on the information, please catch those exceptions and return something meaningful.

> > 5. It allows you to check for pwd validation for a user you don't know their
> > userid (you can force brute only by knowing the patron's id). I don't think
> > it's a security concern as userid could be guessed anyway (?)
> > 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no
> > need to know the userid list. How bad is that?

Maybe not the right bug to talk about how to prevent brute force attacks, as you said: this is not a public route.
Comment 17 David Cook 2022-12-20 05:59:10 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> (In reply to David Cook from comment #12)
> > Are all the existing API tests in "t/db_dependent/api/v1/"?
> 
> Not sure what you mean here, but we try to enforce 100% code coverage on the
> tests.

Are good examples of API tests found in that location?
 
> > > 2. Route's name should not be a verb (/password/validation maybe?)
> > 
> > I did struggle to come up with a noun for this one so I take the point. 
> 
> I'd go with 'validation'.

In light of the /api/v1/auth/* routes that we have these days... /api/v1/auth/validation?  

Or do we want another path prefix in there like "password" for /api/v1/auth/password/validation?

And we'd be posting userid and password to that endpoint. 

--

I'm currently using /patrons/:patron_id/check_password and when I implemented bug 31982 locally it did cause a problem because I was using the OPAC interface. I switched to the Staff Interface API and then it was all good. 

In my case, the consumer of this API is Keycloak, which also runs locally. If it was a third-party system, I think I'd need to create an security exception to allow it through to that endpoint. 

I keep thinking about public vs non-public APIs... and I figure public APIs are things that every day people and front-end systems should be able to use. They offer the same functionality to the public as the OPAC but they do so in a machine-friendly way. If an action is only allowed by staff, it's part of the non-public admin API. And if there's a third-party system that needs to access that, because we trust it, then we make an explicit exception for it, and ideally keep the scope narrowed to just what it needs.
Comment 18 David Cook 2022-12-23 00:34:58 UTC
Created attachment 144810 [details] [review]
Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password

This patch adds an endpoint for /patrons/:patron_id/check_password

This allows a third-party, using an authenticated and authorized Koha
API user, to check if the username and password given by a user is
correct in Koha.

For example, a Keycloak extension can be created using its
User Storage SPI to use Koha as the user database for Keycloak. This
API allows us to authenticate the user as a particular Koha user - without
creating a Koha user session for them.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth
2. Enable "RESTBasicAuth"
3. Run the following commands while substituting correct values for <koha_user> and <koha_password>
3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v
4. Note "204 No Content" response
5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v
6. Note "400 Bad Request" response and error message {"error":"Validation failed"}
Comment 19 David Cook 2022-12-23 00:36:55 UTC
I think the updated patch includes everything except the unit tests. I've just run out of time for that today, as holidays start in 5 minutes...

In the new year, I'll try to model some tests using t/db_dependent/api/v1/idp.t as a template.

I think that should be what's needed...
Comment 20 David Cook 2023-01-04 01:04:38 UTC
Created attachment 144964 [details] [review]
Bug 30962: REST API: Add endpoint /auth/password/validation

This patch adds an endpoint for /auth/password/validation

This allows a third-party, using an authenticated and authorized Koha
API user, to check if the username and password given by a user is
correct in Koha.

For example, a Keycloak extension can be created using its
User Storage SPI to use Koha as the user database for Keycloak. This
API allows us to authenticate the user as a particular Koha user - without
creating a Koha user session for them.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth
2. Enable "RESTBasicAuth"
3. Run the following commands while substituting correct values for <koha_user> and <koha_password>
3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v
4. Note "204 No Content" response
5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v
6. Note "400 Bad Request" response and error message {"error":"Validation failed"}
Comment 21 David Cook 2023-01-04 01:04:43 UTC
Created attachment 144965 [details] [review]
Bug 30962: Add unit tests for /auth/password/validation endpoint

Test plan:
0. Apply patch
1. prove -v t/db_dependent/api/v1/password_validation.t
Comment 22 David Cook 2023-01-04 01:05:29 UTC
(In reply to Jonathan Druart from comment #11)
> 1. Missing tests (you must provide tons of tests to cover the different
> situations)
> 2. Route's name should not be a verb (/password/validation maybe?)
> 3. Routes that returns empty should return 204
> 4. It's always returning "Invalid password" even for other failures (like
> too many attempts)
> 5. It allows you to check for pwd validation for a user you don't know their
> userid (you can force brute only by knowing the patron's id). I don't think
> it's a security concern as userid could be guessed anyway (?)
> 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no
> need to know the userid list. How bad is that?

I think that I've addressed all these points now :)
Comment 23 David Nind 2023-01-17 07:45:19 UTC
Created attachment 145350 [details] [review]
Bug 30962: REST API: Add endpoint /auth/password/validation

This patch adds an endpoint for /auth/password/validation

This allows a third-party, using an authenticated and authorized Koha
API user, to check if the username and password given by a user is
correct in Koha.

For example, a Keycloak extension can be created using its
User Storage SPI to use Koha as the user database for Keycloak. This
API allows us to authenticate the user as a particular Koha user - without
creating a Koha user session for them.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth
2. Enable "RESTBasicAuth"
3. Run the following commands while substituting correct values for <koha_user> and <koha_password>
3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v
4. Note "204 No Content" response
5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v
6. Note "400 Bad Request" response and error message {"error":"Validation failed"}

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2023-01-17 07:45:25 UTC
Created attachment 145351 [details] [review]
Bug 30962: Add unit tests for /auth/password/validation endpoint

Test plan:
0. Apply patch
1. prove -v t/db_dependent/api/v1/password_validation.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 Katrin Fischer 2023-01-27 14:47:50 UTC
Giving this one a try...
Comment 26 Katrin Fischer 2023-01-27 16:01:27 UTC
1) Unit tests pass
2) QA test tools pass

3) Test plan

I followed the test plan and while I have the replies 204 and 400, I don't see the error as described:

error message {"error":"Validation failed"}

4) Postman

I also tried to test this with Postman.

* Verified that BasicAuth worked as expected by listing patrons
* Tried: localhost:8081/api/v1/auth/password/validation
* Params: username, password 
* Verb: POST
* Body: { "username": "...", "password": "..." }

a) Matching username + password

* cardnumber + correct password = 400 - Bad request
* username + correct password = 204 - No Content (that's a success?)
* username + incorrect password = 400 - Bad Request - error: Validation failed
* username + incorrect password so many times to make the account lock: 400 - Bad Request - error: Validation failed

Notes:

* The login page in Koha allows for cardnumber + password AND userid + password at the same time. I think we should extend this route in a separate bug to also support cardnumber/both to make this easier to use and also mimick what ILS-DI and SIP do as well. 
I've filed: Bug 32739 - REST API: Extend endpoint /auth/password/validation for cardnumber

* We do want the account to lock with too many attempts, which it does. The error stays the same, but I think that's good too and matches what we do on the OPAC, we don't want to give away too much information. *thumbs up*

QA fail:

* The route users username, but the patrons api uses user_id. We should make things match and use user_id here as well.

Almost ready to PQA, please fix!
Comment 27 Tomás Cohen Arazi 2023-01-27 17:44:26 UTC
Created attachment 145765 [details] [review]
Bug 30962: REST API: Add endpoint /auth/password/validation

This patch adds an endpoint for /auth/password/validation

This allows a third-party, using an authenticated and authorized Koha
API user, to check if the username and password given by a user is
correct in Koha.

For example, a Keycloak extension can be created using its
User Storage SPI to use Koha as the user database for Keycloak. This
API allows us to authenticate the user as a particular Koha user - without
creating a Koha user session for them.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth
2. Enable "RESTBasicAuth"
3. Run the following commands while substituting correct values for <koha_user> and <koha_password>
3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v
4. Note "204 No Content" response
5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v
6. Note "400 Bad Request" response and error message {"error":"Validation failed"}

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2023-01-27 17:44:39 UTC
Created attachment 145766 [details] [review]
Bug 30962: Add unit tests for /auth/password/validation endpoint

Test plan:
0. Apply patch
1. prove -v t/db_dependent/api/v1/password_validation.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2023-01-27 17:44:48 UTC
Created attachment 145767 [details] [review]
Bug 30962: (QA follow-up) Rename attribute and simplify tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2023-01-27 17:48:31 UTC
(In reply to Katrin Fischer from comment #26)
> * The login page in Koha allows for cardnumber + password AND userid +
> password at the same time. I think we should extend this route in a separate
> bug to also support cardnumber/both to make this easier to use and also
> mimick what ILS-DI and SIP do as well. 
> I've filed: Bug 32739 - REST API: Extend endpoint /auth/password/validation
> for cardnumber

Nice idea!

> QA fail:
> 
> * The route users username, but the patrons api uses user_id. We should make
> things match and use user_id here as well.

I submitted a follow-up to help this bug get pushed soon! Hope it covers your concern.

dcook: hope you're fine with my tests simplification, it is much easier to maintain and similar to what we do everywhere (with two exceptions I think).
Comment 31 David Cook 2023-01-31 02:57:52 UTC
(In reply to Tomás Cohen Arazi from comment #30)
> dcook: hope you're fine with my tests simplification, it is much easier to
> maintain and similar to what we do everywhere (with two exceptions I think).

I've taken a quick look and if you're happy with it, I'm happy with it.
Comment 32 Katrin Fischer 2023-01-31 15:10:36 UTC
Created attachment 145883 [details] [review]
Bug 30962: Add unit tests for /auth/password/validation endpoint

Test plan:
0. Apply patch
1. prove -v t/db_dependent/api/v1/password_validation.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 33 Katrin Fischer 2023-01-31 15:10:41 UTC
Created attachment 145884 [details] [review]
Bug 30962: (QA follow-up) Rename attribute and simplify tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 34 Tomás Cohen Arazi 2023-02-03 13:32:40 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 35 Jacob O'Mara 2023-02-06 16:27:57 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 36 Lucas Gass 2023-02-14 22:10:10 UTC
Enhancement will not be backported to 22.05.x
Comment 37 Caroline Cyr La Rose 2023-05-24 19:46:25 UTC
Back end stuff, doesn't affect the end user so there's nothing to add/amend in the manual.

I'm told the API docs is automated, so there's no need to put those bugs as Needs documenting.