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
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"
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!
(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"}'
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...)
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 [U+1F605] I can fix this tomorrow afternoon if you'd prefer.
(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!
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.
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"
Created attachment 159940 [details] [review] Bug 35204: Add unit tests prove t/db_dependent/api/v1/password_validation.t
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
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
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>
Looks good. Amended the patches so they don't have tidy issues.
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.02
Backported to 23.05.x for upcoming 23.05.08
Pushed to 22.11.x for 23.11.14
The REST API is self-documenting (up to QA to pick up any issues), so no changes to the manual are required.