The /auth/password/validation route as proposed on bug 30962 does support validation with the username/user_id + password combination, but doesn't allow for using cardnumber. We should have a way to mimick what the login form/ILS-DI/SIP do and have a way for checking username and cardnumber at the same time/with the same request.
One option is to do a user search using the Patrons API using userid/cardnumber/email address or whatever match point(s) you want. And then you can validate using the fetched userid and provided password. This is what I do for a couple systems using this API endpoint. But I'm not opposed to someone adding cardnumber to this API endpoint either.
I think adding it to the endpoint would be the way to go, so it matches who Koha authenticates in all other spots. We were just not sure how it should present in the endpoint when discussing this. The user of the endpoint might not know if the login information entered was the cardnumber or the username. The simplest route might be to extend userid to check for both internally without changing how the API is used.
Created attachment 153574 [details] [review] Bug 32739: Unit tests
Created attachment 153575 [details] [review] Bug 32739: Allow other patron identifier on pwd validation This patch takes a step forward on the password validation endpoint, by renaming the `userid` attribute to `identifier` and making it be allowed to be the patron's cardnumber. The implementation relies on `C4::Auth::checkpw` to query for the patron. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! 3. Sign off :-D
After talking with Martin, I decided to submit this patches the way they are written. I decided to rename 'userid' => 'identifier' to make it generic enough. I'm open to discuss if you *also* want to be able to specify *userid* and *cardnumber* explicitly, but I think this is the way to go and thus my proposal. Best regards
(In reply to Tomás Cohen Arazi from comment #5) > After talking with Martin, I decided to submit this patches the way they are > written. > > I decided to rename 'userid' => 'identifier' to make it generic enough. I'm > open to discuss if you *also* want to be able to specify *userid* and > *cardnumber* explicitly, but I think this is the way to go and thus my > proposal. > > Best regards I'd be happy for "identifier" to be added in addition to "userid", but I think renaming it is a problematic idea, since "userid" is already in use by third-party systems using the production API. Koha has a bit of a habit of making backwards incompatible changes to its "v1" API...
(In reply to David Cook from comment #6) > (In reply to Tomás Cohen Arazi from comment #5) > > After talking with Martin, I decided to submit this patches the way they are > > written. > > > > I decided to rename 'userid' => 'identifier' to make it generic enough. I'm > > open to discuss if you *also* want to be able to specify *userid* and > > *cardnumber* explicitly, but I think this is the way to go and thus my > > proposal. > > > > Best regards > > I'd be happy for "identifier" to be added in addition to "userid", but I > think renaming it is a problematic idea, since "userid" is already in use by > third-party systems using the production API. > > Koha has a bit of a habit of making backwards incompatible changes to its > "v1" API... It's a fair point. In this case we could keep the legacy behavior with no harm. We thought being released a couple months ago made it not such a big deal. About V1 and breaking changes, I don't think there have been many, but truth is we are just reaching a point in which we can call the API stable.
(In reply to Tomás Cohen Arazi from comment #7) > It's a fair point. In this case we could keep the legacy behavior with no > harm. We thought being released a couple months ago made it not such a big > deal. I would very much appreciate that. > About V1 and breaking changes, I don't think there have been many, but truth > is we are just reaching a point in which we can call the API stable. That's true. I suppose we haven't quite reached the full stable "v1" yet.
Tomas, I was really happy to see the patch, but I also understand David's argument. I think identifier would work great for our use cases where we sometimes don't know what is entered by the user. Could be having all 3 options, userid, cardnumber and identifier, be a solution?
Created attachment 153683 [details] [review] Bug 32739: Unit tests
Created attachment 153684 [details] [review] Bug 32739: Allow other patron identifier on pwd validation This patch takes a step forward on the password validation endpoint, by renaming the `userid` attribute to `identifier` and making it be allowed to be the patron's `cardnumber`. The implementation relies on `C4::Auth::checkpw` to query for the patron. Note the original implementation verified the `userid` was really a `userid`, and with this patch, `userid` is just an alias for `identifier`. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! 3. Sign off :-D
(In reply to David Cook from comment #8) > (In reply to Tomás Cohen Arazi from comment #7) > > It's a fair point. In this case we could keep the legacy behavior with no > > harm. We thought being released a couple months ago made it not such a big > > deal. > > I would very much appreciate that. Done!(In reply to Katrin Fischer from comment #9) > Could be having all 3 options, userid, cardnumber and identifier, be a > solution? I decided to only keep `userid`(legacy) and `identifier`. I felt like a real use case for that would be required to justify it, so leaving for later/when someone really needs it for a specialized use case. I filed bug 34313 while thinking on possible reasons to have a `cardnumber` option. Happy to discuss there if needed.
Created attachment 153776 [details] [review] Bug 32739: Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 153777 [details] [review] Bug 32739: Allow other patron identifier on pwd validation This patch takes a step forward on the password validation endpoint, by renaming the `userid` attribute to `identifier` and making it be allowed to be the patron's `cardnumber`. The implementation relies on `C4::Auth::checkpw` to query for the patron. Note the original implementation verified the `userid` was really a `userid`, and with this patch, `userid` is just an alias for `identifier`. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
There is documentation in the patch claiming that userid with cardnumber should not work, but it does in my testing: Optionally, you can specify the `userid` attribute if you don't want it to be checked against the patron cardnumbers. { "userid": "42", "password": "koha" } Gives: 204
(In reply to Katrin Fischer from comment #15) > There is documentation in the patch claiming that userid with cardnumber > should not work, but it does in my testing: > > Optionally, you can specify the `userid` attribute if you don't want it > to be checked against the patron cardnumbers. > > { "userid": "42", "password": "koha" } > > Gives: 204 I don't insist on this behaviur, only on adjusting the docs.
Nick, can you retest? I decided to restore the original behavior for userid. We will make it better [1] once we move password checking into Koha::*. But the original functionality should remain as-is, and it was a mistake to simplify it like I did. Sorry for that [1] We are fetching the Koha::Patron from the DB, and then checkpw will re-fetch it. This is how the original code works in master BTW.
Created attachment 153816 [details] [review] Bug 32739: Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 153817 [details] [review] Bug 32739: Allow other patron identifier on pwd validation This patch takes a step forward on the password validation endpoint, by adding the `identifier` parameter and making it be allowed to be the patron's `cardnumber` or the `userid`. The current `userid` only validation option is kept as-is. The implementation relies on `C4::Auth::checkpw` to query for the patron. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Katrin Fischer from comment #16) > (In reply to Katrin Fischer from comment #15) > > There is documentation in the patch claiming that userid with cardnumber > > should not work, but it does in my testing: I regretted about making such change. I'll deal with the thing that bothered me about it on a separate bug.
Created attachment 153841 [details] [review] Bug 32739: Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 153842 [details] [review] Bug 32739: Allow other patron identifier on pwd validation This patch takes a step forward on the password validation endpoint, by adding the `identifier` parameter and making it be allowed to be the patron's `cardnumber` or the `userid`. The current `userid` only validation option is kept as-is. The implementation relies on `C4::Auth::checkpw` to query for the patron. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 153885 [details] [review] Bug 32739: Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153886 [details] [review] Bug 32739: Allow other patron identifier on pwd validation This patch takes a step forward on the password validation endpoint, by adding the `identifier` parameter and making it be allowed to be the patron's `cardnumber` or the `userid`. The current `userid` only validation option is kept as-is. The implementation relies on `C4::Auth::checkpw` to query for the patron. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153887 [details] [review] Bug 32739: (follow-up) QA Cleanup Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Martin Renvoize from comment #25) > Created attachment 153887 [details] [review] [review] > Bug 32739: (follow-up) QA Cleanup > > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Oops. Thanks!
Pushed to master for 23.11. Nice work everyone, thanks!
I prefer not to backport since it is about passwords ;)
(In reply to Fridolin Somers from comment #28) > I prefer not to backport since it is about passwords ;) It would be nice if you could reconsider. This brings this API in line and is more a bug fix than an enhancement I feel. The API is also very new still, so better to change it earlier than later.
(In reply to Katrin Fischer from comment #29) > (In reply to Fridolin Somers from comment #28) > > I prefer not to backport since it is about passwords ;) > > It would be nice if you could reconsider. This brings this API in line and > is more a bug fix than an enhancement I feel. The API is also very new > still, so better to change it earlier than later. OK on it. I looks solid ;)
Pushed to 23.05.x for 23.05.03
Thank you!
@Tomas I know this is rel_22_11_candidate but I believe this bug requires bug 33556. And it seems bug 33556 requires bug 21043. There may be other bugs missing here but this is as far as I went with this. Do we drop this or invest and backport the whole required tree for this?
(In reply to Pedro Amorim from comment #33) > @Tomas I know this is rel_22_11_candidate but I believe this bug requires > bug 33556. And it seems bug 33556 requires bug 21043. > > There may be other bugs missing here but this is as far as I went with this. > > Do we drop this or invest and backport the whole required tree for this? Nevermind, the team helped me rebase this without anything else being pushed.
Nice work everyone! Pushed to 22.11.x for next release