[Koha-bugs] [Bug 30962] REST API: Add endpoint /patrons/:patron_id/check_password

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30962

--- Comment #12 from David Cook <dcook at 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.