Bug 35204 - REST API: POST endpoint /auth/password/validation dies on patron with expired password
Summary: REST API: POST endpoint /auth/password/validation dies on patron with expired...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Matt Blenkinsop
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 30962
Blocks:
  Show dependency treegraph
 
Reported: 2023-10-31 15:59 UTC by Nick Clemens
Modified: 2024-01-19 11:08 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.02,23.05.08


Attachments
Bug 35204: Prevent an expired password from throwing a 500 error (1.60 KB, patch)
2023-12-06 10:07 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35204: Prevent an expired password from throwing a 500 error (1.68 KB, patch)
2023-12-18 11:01 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35204: Add unit tests (2.02 KB, patch)
2023-12-18 11:01 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35204: Add unit tests (2.04 KB, patch)
2023-12-18 12:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35204: Prevent an expired password from throwing a 500 error (2.01 KB, patch)
2023-12-18 12:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-10-31 15:59:07 UTC
The REST module relies on return of cardnumber/userid from C4::Auth::checkpw, however a patron with an expired password returns only -2

This causes a 500 Error from the API route
Comment 1 Matt Blenkinsop 2023-12-06 10:07:58 UTC
Created attachment 159612 [details] [review]
Bug 35204: Prevent an expired password from throwing a 500 error

Currently when a patron with an expired password is authenticated via the API a 500 error is returned rather than a 400 "Validation failed" error. This patch catches the return value for an expired password and returns the validation failure before the patron search is attempted.

Test plan:
1) Choose a patron and set their password expiry date to a date in the past
2) Send a request to auth/password/validation with that patron's details
3) The response should be a 500 error
4) Apply patch
5) Repeat steps 1-3 and this time the response should be a 400 code with an error message of  "Validation failed"
Comment 2 David Nind 2023-12-06 21:01:39 UTC
I'm happy to test, but how to send a request is not something I'm great at.

Could you provide an example of how to send a request to generate the error?

Thanks!
Comment 3 David Cook 2023-12-07 05:59:13 UTC
(In reply to David Nind from comment #2)
> I'm happy to test, but how to send a request is not something I'm great at.
> 
> Could you provide an example of how to send a request to generate the error?
> 
> Thanks!

curl -u AMINDUSER:ADMINPASSWORD -H "Content-Type: application/json" http://localhost:8081/api/v1/auth/password/validation -d '{"identifier":"patron","password":"$uper Duper 4wesome Password"}'
Comment 4 David Cook 2023-12-07 06:14:32 UTC
While this works... I don't think it's the right solution.

It looks like we should change "unless ($status)" to "unless ($status && $status > 0)".

I'm basing that off these lines

2004     # INTERNAL AUTH
2005     if ( $check_internal_as_fallback ) {
2006         @return = checkpw_internal( $userid, $password, $no_set_userenv);
2007         $passwd_ok = 1 if $return[0] > 0; # 1 or 2
2008     }

I don't think the -2 for an expired password actually gets used anywhere so that's fun. The "password_expired" message at login time seems to come from $patron->password_expired in a different auth function. 

I don't know what a return value of 2 means either... 

(One of these days auth in Koha won't be a nightmare...)
Comment 5 David Cook 2023-12-07 06:15:07 UTC
Sorry but I think I'm going to mark this one as Failed QA even though I'm the one who made the original mistake 😅

I can fix this tomorrow afternoon if you'd prefer.
Comment 6 David Nind 2023-12-07 07:02:19 UTC
(In reply to David Cook from comment #3)
> (In reply to David Nind from comment #2)
> > I'm happy to test, but how to send a request is not something I'm great at.
> > 
> > Could you provide an example of how to send a request to generate the error?
> > 
> > Thanks!
> 
> curl -u AMINDUSER:ADMINPASSWORD -H "Content-Type: application/json"
> http://localhost:8081/api/v1/auth/password/validation -d
> '{"identifier":"patron","password":"$uper Duper 4wesome Password"}'

Thanks David!
Comment 7 Tomás Cohen Arazi 2023-12-07 12:31:51 UTC
I was going to fail this one yesterday but had a flight delayed.

- We need tests (they should highlight what dcook is saying)
- My opinion is we should return `error_code` => 'password_expired' or similar, so API consumers can really act correctly on further steps for the end user.
Comment 8 Matt Blenkinsop 2023-12-18 11:01:10 UTC
Created attachment 159939 [details] [review]
Bug 35204: Prevent an expired password from throwing a 500 error

Currently when a patron with an expired password is authenticated via the API a 500 error is returned rather than a 400 "Validation failed" error. This patch catches the return value for an expired password and returns the validation failure before the patron search is attempted.

Test plan:
1) Choose a patron and set their password expiry date to a date in the past
2) Send a request to auth/password/validation as an authenticated user with that patron's details
3) The response should be a 500 error
4) Apply patch
5) Repeat steps 1-3 and this time the response should be a 400 code with an error message of  "Password expired"
Comment 9 Matt Blenkinsop 2023-12-18 11:01:13 UTC
Created attachment 159940 [details] [review]
Bug 35204: Add unit tests

prove t/db_dependent/api/v1/password_validation.t
Comment 10 Matt Blenkinsop 2023-12-18 11:02:00 UTC
Attempt two - I've added a unit test and changed the error code to now show a password expiration rather than just "Validation failed". Let me know if this needs tweaking at all
Comment 11 Tomás Cohen Arazi 2023-12-18 12:13:03 UTC
Created attachment 159956 [details] [review]
Bug 35204: Add unit tests

prove t/db_dependent/api/v1/password_validation.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

bsoletes: 159940 - Bug 35204: Add unit tests
Comment 12 Tomás Cohen Arazi 2023-12-18 12:13:09 UTC
Created attachment 159957 [details] [review]
Bug 35204: Prevent an expired password from throwing a 500 error

Currently when a patron with an expired password is authenticated via the API a 500 error is returned rather than a 400 "Validation failed" error. This patch catches the return value for an expired password and returns the validation failure before the patron search is attempted.

Test plan:
1) Choose a patron and set their password expiry date to a date in the past
2) Send a request to auth/password/validation as an authenticated user with that patron's details
3) The response should be a 500 error
4) Apply patch
5) Repeat steps 1-3 and this time the response should be a 400 code with an error message of  "Password expired"

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2023-12-18 12:14:16 UTC
Looks good. Amended the patches so they don't have tidy issues.
Comment 14 Katrin Fischer 2023-12-18 14:25:18 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 15 Fridolin Somers 2024-01-03 14:51:48 UTC
Pushed to 23.11.x for 23.11.02
Comment 16 Lucas Gass 2024-01-04 20:25:22 UTC
Backported to 23.05.x for upcoming 23.05.08
Comment 17 Frédéric Demians 2024-01-19 11:08:28 UTC
Pushed to 22.11.x for 23.11.14