https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30962 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (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) -- You are receiving this mail because: You are watching all bug changes.