Bug 29272 - API not respecting $category->effective_change_password
Summary: API not respecting $category->effective_change_password
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 10796 22061
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-19 13:25 UTC by Tomás Cohen Arazi
Modified: 2021-11-10 23:58 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.06


Attachments
Bug 29272: Regression tests (2.39 KB, patch)
2021-10-19 13:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29272: Make public password changing honour category constraints (1.68 KB, patch)
2021-10-19 13:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29272: Regression tests (2.44 KB, patch)
2021-10-20 23:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 29272: Make public password changing honour category constraints (1.73 KB, patch)
2021-10-20 23:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 29272: (QA follow-up) Category setup overrides syspref (1.88 KB, patch)
2021-10-24 22:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29272: Regression tests (2.50 KB, patch)
2021-10-27 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29272: Make public password changing honour category constraints (1.79 KB, patch)
2021-10-27 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29272: (QA follow-up) Category setup overrides syspref (1.95 KB, patch)
2021-10-27 12:05 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 Tomás Cohen Arazi 2021-10-19 13:25:43 UTC
The API public routes for changing passwords were developed before bug 10796, and when it was pushed (before this routes), we forgot to adapt things in the API.

It only checks OpacPasswordChange, but doesn't consider $category->effective_change_password
Comment 1 Tomás Cohen Arazi 2021-10-19 13:32:10 UTC
Created attachment 126501 [details] [review]
Bug 29272: Regression tests

This patch introduces regression tests for the described bug.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_password.t
=> FAIL: Tests fail, excepted failures are considered success

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-10-19 13:32:13 UTC
Created attachment 126502 [details] [review]
Bug 29272: Make public password changing honour category constraints

This patch makes the public API routes validate
$user->category->effective_change_password before allowing the change.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_password.t
=> FAIL: Tests fail, it allows the first change instead of returning
         403.
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-10-19 13:33:18 UTC
Good catch, Jonathan!
Comment 4 David Nind 2021-10-20 23:20:06 UTC
Created attachment 126632 [details] [review]
Bug 29272: Regression tests

This patch introduces regression tests for the described bug.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_password.t
=> FAIL: Tests fail, excepted failures are considered success

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2021-10-20 23:20:11 UTC
Created attachment 126633 [details] [review]
Bug 29272: Make public password changing honour category constraints

This patch makes the public API routes validate
$user->category->effective_change_password before allowing the change.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_password.t
=> FAIL: Tests fail, it allows the first change instead of returning
         403.
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Jonathan Druart 2021-10-22 12:16:23 UTC
Are you sure it's how it's suppose to work?

I think you should remove the short-circuit part (checking for the syspref).
You should only rely on effective_change_password return.
Comment 7 Jonathan Druart 2021-10-22 12:17:28 UTC
Using the GUI you can have the pref OFF and the setting ON at category level, and the feature is enabled.

See also discussion on bug 29211.
Comment 8 Tomás Cohen Arazi 2021-10-22 14:14:24 UTC
(In reply to Jonathan Druart from comment #6)
> Are you sure it's how it's suppose to work?
> 
> I think you should remove the short-circuit part (checking for the syspref).
> You should only rely on effective_change_password return.

I just checked, and agree!
Comment 9 Katrin Fischer 2021-10-24 21:31:40 UTC
From the last comments I understand that another change is required - moving to FQA for the follow-up.
Comment 10 Tomás Cohen Arazi 2021-10-24 22:56:26 UTC
Created attachment 126814 [details] [review]
Bug 29272: (QA follow-up) Category setup overrides syspref

This patch removes an early short-circuit we had, which wasn't correct
regarding the OpacPasswordChange syspref. If a patron category is
allowed to change password, it overrides the syspref.

To test:
1. Tests still pass.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Martin Renvoize 2021-10-27 12:05:36 UTC
Created attachment 126976 [details] [review]
Bug 29272: Regression tests

This patch introduces regression tests for the described bug.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_password.t
=> FAIL: Tests fail, excepted failures are considered success

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-10-27 12:05:40 UTC
Created attachment 126977 [details] [review]
Bug 29272: Make public password changing honour category constraints

This patch makes the public API routes validate
$user->category->effective_change_password before allowing the change.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons_password.t
=> FAIL: Tests fail, it allows the first change instead of returning
         403.
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-10-27 12:05:43 UTC
Created attachment 126978 [details] [review]
Bug 29272: (QA follow-up) Category setup overrides syspref

This patch removes an early short-circuit we had, which wasn't correct
regarding the OpacPasswordChange syspref. If a patron category is
allowed to change password, it overrides the syspref.

To test:
1. Tests still pass.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-10-27 12:10:16 UTC
All works as expected now, good catch.

Passing QA
Comment 15 Jonathan Druart 2021-10-28 10:30:02 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Fridolin Somers 2021-11-10 23:46:41 UTC Comment hidden (obsolete)
Comment 17 Fridolin Somers 2021-11-10 23:47:58 UTC
Does not apply on 20.11.x
Please provide a rebase if needed
Comment 18 Fridolin Somers 2021-11-10 23:58:01 UTC
It will be in 21.05.06