|
Description
Tomás Cohen Arazi (tcohen)
2021-10-19 13:25:43 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> 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> Good catch, Jonathan! 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> 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> 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. 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. (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! From the last comments I understand that another change is required - moving to FQA for the follow-up. 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> 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> 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> 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> All works as expected now, good catch. Passing QA Pushed to master for 21.11, thanks to everybody involved! It is in 21.05.05 Does not apply on 20.11.x Please provide a rebase if needed It will be in 21.05.06 |