Bug 32739 - REST API: Extend endpoint /auth/password/validation for cardnumber
Summary: REST API: Extend endpoint /auth/password/validation for cardnumber
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords: rel_22_11_candidate
Depends on: 30962
Blocks: 34313
  Show dependency treegraph
 
Reported: 2023-01-27 16:00 UTC by Katrin Fischer
Modified: 2023-12-28 20:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adds a new attribute for identifying the patron for password validation: `identifier`. It expects to be passed a `userid` or a `cardnumber` in it. It the `identifier` doesn't match a `userid`, then Koha will try matching a `cardnumber`.
Version(s) released in:
23.11.00,23.05.03,22.11.09


Attachments
Bug 32739: Unit tests (4.66 KB, patch)
2023-07-17 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32739: Allow other patron identifier on pwd validation (3.06 KB, patch)
2023-07-17 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32739: Unit tests (7.05 KB, patch)
2023-07-19 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32739: Allow other patron identifier on pwd validation (7.39 KB, patch)
2023-07-19 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32739: Unit tests (7.10 KB, patch)
2023-07-21 12:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32739: Allow other patron identifier on pwd validation (7.45 KB, patch)
2023-07-21 12:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32739: Unit tests (7.41 KB, patch)
2023-07-21 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32739: Allow other patron identifier on pwd validation (7.57 KB, patch)
2023-07-21 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32739: Unit tests (7.41 KB, patch)
2023-07-24 15:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32739: Allow other patron identifier on pwd validation (7.42 KB, patch)
2023-07-24 15:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32739: Unit tests (7.47 KB, patch)
2023-07-25 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32739: Allow other patron identifier on pwd validation (7.48 KB, patch)
2023-07-25 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32739: (follow-up) QA Cleanup (997 bytes, patch)
2023-07-25 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-01-27 16:00:43 UTC
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.
Comment 1 David Cook 2023-05-24 23:38:35 UTC
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.
Comment 2 Katrin Fischer 2023-07-16 10:57:36 UTC
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.
Comment 3 Tomás Cohen Arazi 2023-07-17 18:52:47 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2023-07-17 18:52:51 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2023-07-17 18:56:36 UTC
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
Comment 6 David Cook 2023-07-18 00:36:31 UTC
(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...
Comment 7 Tomás Cohen Arazi 2023-07-18 01:24:26 UTC
(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.
Comment 8 David Cook 2023-07-18 01:38:22 UTC
(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.
Comment 9 Katrin Fischer 2023-07-18 07:43:15 UTC
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?
Comment 10 Tomás Cohen Arazi 2023-07-19 19:06:57 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2023-07-19 19:07:06 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2023-07-19 19:09:17 UTC
(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.
Comment 13 Nick Clemens 2023-07-21 12:30:46 UTC Comment hidden (obsolete)
Comment 14 Nick Clemens 2023-07-21 12:30:48 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2023-07-21 12:55:28 UTC
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
Comment 16 Katrin Fischer 2023-07-21 13:09:51 UTC
(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.
Comment 17 Tomás Cohen Arazi 2023-07-21 19:34:25 UTC
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.
Comment 18 Tomás Cohen Arazi 2023-07-21 19:36:30 UTC
Created attachment 153816 [details] [review]
Bug 32739: Unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2023-07-21 19:36:34 UTC
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>
Comment 20 Tomás Cohen Arazi 2023-07-21 19:38:33 UTC
(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.
Comment 21 Nick Clemens 2023-07-24 15:19:13 UTC
Created attachment 153841 [details] [review]
Bug 32739: Unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens 2023-07-24 15:19:17 UTC
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>
Comment 23 Martin Renvoize 2023-07-25 15:30:53 UTC
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>
Comment 24 Martin Renvoize 2023-07-25 15:30:56 UTC
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>
Comment 25 Martin Renvoize 2023-07-25 15:30:59 UTC
Created attachment 153887 [details] [review]
Bug 32739: (follow-up) QA Cleanup

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Tomás Cohen Arazi 2023-07-25 15:57:41 UTC
(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!
Comment 27 Tomás Cohen Arazi 2023-07-25 19:26:23 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 28 Fridolin Somers 2023-08-04 02:26:01 UTC
I prefer not to backport since it is about passwords ;)
Comment 29 Katrin Fischer 2023-08-04 17:01:16 UTC
(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.
Comment 30 Fridolin Somers 2023-08-04 18:52:16 UTC
(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 ;)
Comment 31 Fridolin Somers 2023-08-04 18:53:47 UTC
Pushed to 23.05.x for 23.05.03
Comment 32 Katrin Fischer 2023-08-04 18:58:50 UTC
Thank you!
Comment 33 Pedro Amorim 2023-08-18 12:03:17 UTC
@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?
Comment 34 Pedro Amorim 2023-08-28 10:04:58 UTC
(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.
Comment 35 Pedro Amorim 2023-08-28 10:22:51 UTC
Nice work everyone!

Pushed to 22.11.x for next release