Bug 23011 - AuthenticatePatron could alert if password is not safe
Summary: AuthenticatePatron could alert if password is not safe
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
Hardware: All All
: P4 enhancement (vote)
Assignee: Arthur Suzuki
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-29 14:33 UTC by Arthur Suzuki
Modified: 2022-04-22 21:56 UTC (History)
5 users (show)

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


Attachments
Bug23011 : Add new tests (AuthenticatePatron error codes) (2.76 KB, patch)
2019-10-03 07:24 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug23011 : Have AuthenticatePatron method send more information (2.11 KB, patch)
2019-10-03 07:24 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23011: Have AuthenticatePatron method send more information (2.14 KB, patch)
2019-12-22 05:21 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23011: Add new tests (AuthenticatePatron error codes) (2.96 KB, patch)
2019-12-22 05:21 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23011: Have AuthenticatePatron method send more information (2.14 KB, patch)
2019-12-22 05:22 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23011: Add new tests (AuthenticatePatron error codes) (3.02 KB, patch)
2020-03-10 09:31 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23011: Have AuthenticatePatron method send more information (2.20 KB, patch)
2020-03-10 09:31 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23011: Add new tests (AuthenticatePatron error codes) (3.02 KB, patch)
2020-03-10 09:33 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23011: Have AuthenticatePatron method send more information (2.20 KB, patch)
2020-03-10 09:33 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23011: Add new tests (AuthenticatePatron error codes) (2.82 KB, patch)
2020-07-05 21:06 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23011: Have AuthenticatePatron method send more information (2.20 KB, patch)
2020-07-05 21:06 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23011: QA follow-up (2.99 KB, patch)
2020-07-05 21:06 UTC, Arthur Suzuki
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2019-05-29 14:33:56 UTC
I work on a portal solution called Bokeh and we need to get the information from the webService that the patron's password is not safe and should be renewed.

Reply from the webservice could look like this :

<?xml version="1.0" encoding="UTF-8"?>
<AuthenticatePatron>
 <patronId>4</patronId>
 <error>PatronPasswordNotSecure</error>
 <securePasswordPattern>^(?=.*[A-Za-z])(?=.*\d).{6,}$</securePasswordPattern>
 <securePasswordLabel>Password must be at least 6 character long and include a letter and a digit</securePasswordLabel>
</AuthenticatePatron>

That would allow the borrower to authenticate but provide the portal software with information about the rules to define a new password.
Comment 1 Arthur Suzuki 2019-10-03 07:24:07 UTC Comment hidden (obsolete)
Comment 2 Arthur Suzuki 2019-10-03 07:24:52 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2019-10-03 08:35:49 UTC
Tests are failing, but it's just the number of tests that is wrong

>  Parse errors: Bad plan.  You planned 9 tests but ran 10.

QA tool reports some errors as well:

>  FAIL   C4/ILSDI/Services.pm
>  FAIL   forbidden patterns
>               forbidden pattern: tab char (line 382)
>               forbidden pattern: tab char (line 386)
>               forbidden pattern: tab char (line 384)
>               forbidden pattern: tab char (line 383)
>               forbidden pattern: tab char (line 385)
> 
> Commit title does not start with 'Bug XXXXX: ' - de967f9fc0
> Commit title does not start with 'Bug XXXXX: ' - 23cd5391bc

Otherwise it works well, but I'm a bit concerned by the fact that the patch duplicates the password regexp. If someone modify it in is_password_valid, there are high chances that they will forgot to modify it in ILSDI code. Also, it's not the only thing checked in is_password_valid (there is also a check on whitespaces). Is it really needed to return the regexp ?
Comment 4 Arthur Suzuki 2019-12-22 05:15:42 UTC
The regex is needed by the client app we use to provide some client side input validation before sending any new (and possibly wrong) password to koha.

i'm thinking of the following solution :
is_password_valid could return the regex in addition to the boolean and error.
Comment 5 Arthur Suzuki 2019-12-22 05:21:07 UTC Comment hidden (obsolete)
Comment 6 Arthur Suzuki 2019-12-22 05:21:32 UTC Comment hidden (obsolete)
Comment 7 Arthur Suzuki 2019-12-22 05:22:13 UTC Comment hidden (obsolete)
Comment 8 Emmi Takkinen 2020-03-10 09:31:16 UTC Comment hidden (obsolete)
Comment 9 Emmi Takkinen 2020-03-10 09:31:20 UTC Comment hidden (obsolete)
Comment 10 Emmi Takkinen 2020-03-10 09:33:25 UTC Comment hidden (obsolete)
Comment 11 Emmi Takkinen 2020-03-10 09:33:29 UTC Comment hidden (obsolete)
Comment 12 Emmi Takkinen 2020-03-10 09:37:01 UTC
Forgot to obsolete last patch when signing off first time. 

But patch works as intended.
Comment 13 Jonathan Druart 2020-03-10 12:29:45 UTC
I have the feeling that what you need here is a password_policy method that would return the different rules.

For instance only checking for minPasswordLength is wrong, there is a default to 3 if the pref is not set (or < 3)
Comment 14 Arthur Suzuki 2020-03-10 13:08:51 UTC
Yeay, thanks for signing off this patch Emmi =)
Arthur
Comment 15 Jonathan Druart 2020-03-24 10:39:57 UTC
Waiting for an answer.
Comment 16 Arthur Suzuki 2020-07-05 21:06:17 UTC
Created attachment 106565 [details] [review]
Bug 23011: Add new tests (AuthenticatePatron error codes)

This bug add new tests to the AuthenticatePatron ILS-DI service.
It tests new error codes patron might get when successfully connecting
but with a password which doesn't match Koha security rules.

Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 17 Arthur Suzuki 2020-07-05 21:06:20 UTC
Created attachment 106566 [details] [review]
Bug 23011: Have AuthenticatePatron method send more information

We want AuthenticatePatron to send some more information upon
successful login, especially in the case where password is not safe
and should be renewed.
The goal is to let a third-party app catch thoses codes and
display a warning to the users.

Test plan :
1 / apply tests patch
2 / run tests, verify failure
3 / apply C4/ILSDI/Service.pm patch
4 / run tests again, verify green

Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 18 Arthur Suzuki 2020-07-05 21:06:24 UTC
Created attachment 106567 [details] [review]
Bug 23011: QA follow-up
Comment 19 Arthur Suzuki 2020-07-05 21:09:12 UTC
(In reply to Jonathan Druart from comment #15)
> Waiting for an answer.

Bonsoir Jonathan :)
I've implemented a password_policy method in charge of submitting the regex.
KR,
Arthur
Comment 20 David Cook 2020-07-06 01:27:03 UTC
I think that sending the securePasswordPattern is not a good idea, as it's not the API consumer's job to handle the password. Sending "code" and "securePasswordLabel" is fine as that communicates the message from Koha to the API consumer.

On my TODO list, I'm actually planning more complex password security than just a regex. I want to add a customizable list of passwords that users cannot use (e.g. "password", "123456", "Password1", "koha", etc).

We could also add rules like the password and username cannot match. 

These aren't "patterns" per se, but would rather be communicated by "code" and "securePasswordLabel" I think.