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.
Created attachment 93512 [details] [review] Bug23011 : 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.
Created attachment 93517 [details] [review] Bug23011 : 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
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 ?
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.
Created attachment 96560 [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
Created attachment 96561 [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.
Created attachment 96562 [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
Created attachment 100423 [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>
Created attachment 100424 [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>
Created attachment 100425 [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>
Created attachment 100426 [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>
Forgot to obsolete last patch when signing off first time. But patch works as intended.
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)
Yeay, thanks for signing off this patch Emmi =) Arthur
Waiting for an answer.
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>
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>
Created attachment 106567 [details] [review] Bug 23011: QA follow-up
(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
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.