Bug 36420 - REST API Basic Auth does not support cardnumbers, only userid
Summary: REST API Basic Auth does not support cardnumbers, only userid
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-25 18:16 UTC by Kyle M Hall
Modified: 2024-05-28 19:36 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12


Attachments
Bug 36420: Unit tests (4.90 KB, patch)
2024-03-27 12:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36420: Allow Basic authentication using `cardnumber` (2.06 KB, patch)
2024-03-27 12:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36420: Allow Basic authentication using `cardnumber` (2.11 KB, patch)
2024-03-27 12:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36420: Unit tests (4.95 KB, patch)
2024-03-27 12:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36420: Allow Basic authentication using `cardnumber` (2.11 KB, patch)
2024-03-27 12:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36420: Unit tests (5.01 KB, patch)
2024-04-02 11:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36420: Allow Basic authentication using `cardnumber` (2.17 KB, patch)
2024-04-02 11:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36420: Unit tests (5.07 KB, patch)
2024-04-17 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36420: Allow Basic authentication using `cardnumber` (2.23 KB, patch)
2024-04-17 12:45 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 Kyle M Hall 2024-03-25 18:16:24 UTC
Every other part of Koha supports logging in either a userid or cardnumber. The REST API should support this for basic auth as well.
Comment 1 Tomás Cohen Arazi 2024-03-27 12:05:29 UTC
Created attachment 163984 [details] [review]
Bug 36420: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2024-03-27 12:05:32 UTC
Created attachment 163985 [details] [review]
Bug 36420: Allow Basic authentication using `cardnumber`

This patch makes the API Basic authentication work with cardnumbers.
The used `checkpw_internal` method already does the fallback check.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests explode! [1]
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] This highlights an underlying bug! Instead of getting 403, etc we get a 500. This is because of the internal fallback mechanism allowing the cardnumber+password combination, but then the Koha::Patron->find... not finding :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Kyle M Hall 2024-03-27 12:34:07 UTC
Created attachment 163991 [details] [review]
Bug 36420: Allow Basic authentication using `cardnumber`

This patch makes the API Basic authentication work with cardnumbers.
The used `checkpw_internal` method already does the fallback check.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests explode! [1]
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] This highlights an underlying bug! Instead of getting 403, etc we get a 500. This is because of the internal fallback mechanism allowing the cardnumber+password combination, but then the Koha::Patron->find... not finding :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2024-03-27 12:34:39 UTC
Created attachment 163992 [details] [review]
Bug 36420: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2024-03-27 12:34:45 UTC
Created attachment 163993 [details] [review]
Bug 36420: Allow Basic authentication using `cardnumber`

This patch makes the API Basic authentication work with cardnumbers.
The used `checkpw_internal` method already does the fallback check.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests explode! [1]
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] This highlights an underlying bug! Instead of getting 403, etc we get a 500. This is because of the internal fallback mechanism allowing the cardnumber+password combination, but then the Koha::Patron->find... not finding :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2024-04-02 11:26:03 UTC
Created attachment 164256 [details] [review]
Bug 36420: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens (kidclamp) 2024-04-02 11:26:05 UTC
Created attachment 164257 [details] [review]
Bug 36420: Allow Basic authentication using `cardnumber`

This patch makes the API Basic authentication work with cardnumbers.
The used `checkpw_internal` method already does the fallback check.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests explode! [1]
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] This highlights an underlying bug! Instead of getting 403, etc we get a 500. This is because of the internal fallback mechanism allowing the cardnumber+password combination, but then the Koha::Patron->find... not finding :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Martin Renvoize 2024-04-17 12:45:39 UTC
Created attachment 165006 [details] [review]
Bug 36420: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2024-04-17 12:45:42 UTC
Created attachment 165007 [details] [review]
Bug 36420: Allow Basic authentication using `cardnumber`

This patch makes the API Basic authentication work with cardnumbers.
The used `checkpw_internal` method already does the fallback check.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests explode! [1]
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] This highlights an underlying bug! Instead of getting 403, etc we get a 500. This is because of the internal fallback mechanism allowing the cardnumber+password combination, but then the Koha::Patron->find... not finding :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2024-04-17 12:46:35 UTC
This works in the same way our other auth fallbacks work. QA scripts are happy, we have unit tests.

Passing QA
Comment 11 Katrin Fischer 2024-04-18 14:52:07 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 12 Fridolin Somers 2024-05-23 13:31:28 UTC
Maybe we should create a central method Koha::Patrons->find_by_identifier, managing userid/cadnumber. Would make it easier to disable.
Comment 13 Fridolin Somers 2024-05-23 13:31:48 UTC
Pushed to 23.11.x for 23.11.06
Comment 14 Lucas Gass 2024-05-28 19:36:08 UTC
Backported to 23.05.x for upcoming 23.05.12