We must then rememeber to log all kinds of login attempts, including ILS-DI, SIP2, the REST API (if that can be used for autnetication, not sure) etc.

(In reply to Vitor Fernandes from comment #0)
> *** Requirement description ***
> 
> The application MUST log successful and unsuccessful authentication
> operations. 
> This is useful, for example, to detect that a user account is being hacked.

How extensive is this requirement? Koha already allows you to lock accounts after x failed login attempts. Could this be considered as meeting this requirement already?

Testing the lockout feature I also noticed that the counter is being incremented too even if the account has been locked out. So each successful and each unsuccessful authentication triggers a database action. What would be the use of storing date, time and ip address additionally ?

(In reply to Marcel de Rooy from comment #2)
> How extensive is this requirement? Koha already allows you to lock accounts
> after x failed login attempts. Could this be considered as meeting this
> requirement already?
> 
> Testing the lockout feature I also noticed that the counter is being
> incremented too even if the account has been locked out. So each successful
> and each unsuccessful authentication triggers a database action. What would
> be the use of storing date, time and ip address additionally ?

Marcel, the account lockout after a number of failed login attempts is an important enhancement but isn't what is requested by the requirement.

The application must log successful and unsuccessful authentication operations.
This can be done by modifying Auth.pm.
When the authentication fails we can add something like:

logaction("MEMBERS", "LOGIN", 0, "Login failed: ($userid) Invalid username or password" ) if C4::Context->preference("BorrowersLog");

When the authentication is successful we can add something like:

my $login_info = sprintf "Login successful: (%s) %s %s - %s\n", map { $session->param($_) } qw(cardnumber firstname surname branch);
logaction("MEMBERS", "LOGIN", $borrowernumber, $login_info ) if C4::Context->preference("BorrowersLog");

We have had some accounts apparently fail to allow repeat login attempts to the limit in our preferences.
In investigating this, support noted that the database counter resets back to zero after a successful login, leaving no record of the failed attempts.
We would really like Koha to keep the log of successful and unsuccessful account login attempts - with timestamp. Even if we don't have it logged and visible to staff it would be helpful to support for troubleshooting.

Trying to give this a little start here :)

Moving 21191 to see also

Status: I will only extend the test for the changes in checkpw. Exploring further non-regular authentication events might be a part 2. We cover the most now via checkpw.

Please get this further now by testing and signing off !

I suggest that instead of adding two new sysprefs we could use the already existing syspref TrackLastPatronActivity? Or at least just add one syspref instead of two since I don't think such granularity is needed to log either failed or successful logins.

(In reply to Joonas Kylmälä from comment #19)
> I suggest that instead of adding two new sysprefs we could use the already
> existing syspref TrackLastPatronActivity? Or at least just add one syspref
> instead of two since I don't think such granularity is needed to log either
> failed or successful logins.

Hi Jonaas,
Thx for asking. I agree that they are related. But I do think that granularity of logging success and/or failure is useful. Although Vitor is interested in both, I can imagine that others only would be interested in the failures. There might be a difference in numbers there too.

(In reply to Marcel de Rooy from comment #20)
> (In reply to Joonas Kylmälä from comment #19)
> > I suggest that instead of adding two new sysprefs we could use the already
> > existing syspref TrackLastPatronActivity? Or at least just add one syspref
> > instead of two since I don't think such granularity is needed to log either
> > failed or successful logins.
> 
> Hi Jonaas,
> Thx for asking. I agree that they are related. But I do think that
> granularity of logging success and/or failure is useful. Although Vitor is
> interested in both, I can imagine that others only would be interested in
> the failures. There might be a difference in numbers there too.

I think TrackLastPatronActivity has a different goal. Could we shrink it to one additional pref using a multi-select approach? Check what you want to track.

(In reply to Katrin Fischer from comment #21)
> (In reply to Marcel de Rooy from comment #20)
> > (In reply to Joonas Kylmälä from comment #19)
> > > I suggest that instead of adding two new sysprefs we could use the already
> > > existing syspref TrackLastPatronActivity? Or at least just add one syspref
> > > instead of two since I don't think such granularity is needed to log either
> > > failed or successful logins.
> > 
> > Hi Jonaas,
> > Thx for asking. I agree that they are related. But I do think that
> > granularity of logging success and/or failure is useful. Although Vitor is
> > interested in both, I can imagine that others only would be interested in
> > the failures. There might be a difference in numbers there too.
> 
> I think TrackLastPatronActivity has a different goal. Could we shrink it to
> one additional pref using a multi-select approach? Check what you want to
> track.

Thx for your feedback.
Agree about TrackLastPatronActivity. The multi-select approach could of course be done; would that mean a sign-off? :)
I do not think it is a requirement to get this further btw. Currently, we have all log preferences on one tab, all in YesNo style. So this is consistent; adding another way would be a style change.

(In reply to Marcel de Rooy from comment #22)
> (In reply to Katrin Fischer from comment #21)
> > I think TrackLastPatronActivity has a different goal. Could we shrink it to
> > one additional pref using a multi-select approach? Check what you want to
> > track.
> 
> Thx for your feedback.
> Agree about TrackLastPatronActivity. The multi-select approach could of
> course be done; would that mean a sign-off? :)
> I do not think it is a requirement to get this further btw. Currently, we
> have all log preferences on one tab, all in YesNo style. So this is
> consistent; adding another way would be a style change.

I agree to not go out of style, so doing the multi-select approach could be something to be done in a separate bug report for all the logging options.

I was just trying to offer a compromise here :)

The other log options are simple yes/no questions, so there is no need to change that unless we want to make it more granular in what they log. I am NOT suggesting to combine them into one big multi-select, I think that would be more confusing than helpful.

But a LoginLog (or similar) with 2 options in a select would still make sense to me. One pref per "data type" + options. Not insisting tho, just to explain more what I meant in the first place. I am not in the Anti-YASP group.

Examples for a multi-select would be: MarkLostItemsAsReturned or SocialNetworks (on master).

Last action before I obsolete this report. If you ask for a change, please be ready to support its development by testing it too. Sending a signoff request to the mailing list. No response means closing this report and wasted time!

I added comments as an end-user, my way of supporting work on this - will be sad to see it die. I appreciate the work done.

Tested this, and everything looks good. To me it makes perfect sense to have two regular sysprefs, as Marcel says, some libraries might want to log only success or failure. 

I have one question, though. After doing one failed login and some successful ones I have this in the database: 

+-----------+---------------------+------+--------+---------+--------+--------------------------+-----------+
| action_id | timestamp           | user | module | action  | object | info                     | interface |
+-----------+---------------------+------+--------+---------+--------+--------------------------+-----------+
|      1676 | 2020-01-29 22:17:06 |   51 | AUTH   | SUCCESS |     51 | Valid password for admin | intranet  |
|      1678 | 2020-01-29 22:17:17 |    0 | AUTH   | FAILURE |      0 | Wrong password for admin | intranet  |
|      1680 | 2020-01-29 22:17:20 |   51 | AUTH   | SUCCESS |     51 | Valid password for admin | intranet  |
|      1681 | 2020-01-29 22:18:11 |   51 | AUTH   | SUCCESS |     51 | Valid password for admin | intranet  |
+-----------+---------------------+------+--------+---------+--------+--------------------------+-----------+

The successfull logins are tied to a user (51), but the failed one is not (0). Couldn't the failed one also be tied to the user, as long as the username provided is the username of an actual user in the db? Or is there some reason for not doing this? 

Apart from this question I'm ready to sign off.

(In reply to Magnus Enger from comment #35)
> Tested this, and everything looks good. To me it makes perfect sense to have
> two regular sysprefs, as Marcel says, some libraries might want to log only
> success or failure. 
> 
> I have one question, though. After doing one failed login and some
> successful ones I have this in the database: 
> 
> +-----------+---------------------+------+--------+---------+--------+-------
> -------------------+-----------+
> | action_id | timestamp           | user | module | action  | object | info 
> | interface |
> +-----------+---------------------+------+--------+---------+--------+-------
> -------------------+-----------+
> |      1676 | 2020-01-29 22:17:06 |   51 | AUTH   | SUCCESS |     51 | Valid
> password for admin | intranet  |
> |      1678 | 2020-01-29 22:17:17 |    0 | AUTH   | FAILURE |      0 | Wrong
> password for admin | intranet  |
> |      1680 | 2020-01-29 22:17:20 |   51 | AUTH   | SUCCESS |     51 | Valid
> password for admin | intranet  |
> |      1681 | 2020-01-29 22:18:11 |   51 | AUTH   | SUCCESS |     51 | Valid
> password for admin | intranet  |
> +-----------+---------------------+------+--------+---------+--------+-------
> -------------------+-----------+
> 
> The successfull logins are tied to a user (51), but the failed one is not
> (0). Couldn't the failed one also be tied to the user, as long as the
> username provided is the username of an actual user in the db? Or is there
> some reason for not doing this? 
> 
> Apart from this question I'm ready to sign off.

Thanks, Magnus.
I do not really remember a specific reason. I could register it if it is available. People could try to hack an existing and a not-existing account.
So we might have a $patron, we might not.

(In reply to ByWater Sandboxes from comment #43)
> Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Thanks Jon

Created attachment 98133 [details] [review]
Bug 21190: Prepare Log.t for further changes

This patch only makes cosmetic changes:
[1] It moves the existing tests at 'root level' inside a new subtest.
[2] It obviously adds indentation for step 1.
[3] It fixes some whitespace (tabs, space before newline)

Test plan:
Run t/db../Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98134 [details] [review]
Bug 21190: Add subtest to Log.t for GDPR logging

Apart from a new subtest, doing some more cleanup:
[1] Copyright statement
[2] Removing the (unneeded) flush
[3] Removing C4::Log from the BEGIN block; we seem to skip that nowadays.
[4] $succes goes to the subtest, $schema and $dbh are global our vars.
    Not required but just what we should do if it would run under Plack.

Test plan:
Run t/db_dependent/Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98135 [details] [review]
Bug 21190: Add interfaces parameter to POD in C4::Logs

Just fixing documentation along the way.
No test plan, just read the patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98136 [details] [review]
Bug 21190: Remove t/Log.t

Since we have t/db../Log.t and t/Log.t simply does nothing, we would
better remove it.

No test plan either :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98137 [details] [review]
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog

Test plan:
Run atomic update.
Check the Logs tab of preferences.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98138 [details] [review]
Bug 21190: Add authentication logging to checkpw

Add optional logging for successful and failing login attempts in
checkpw.

Test plan:
Enable the preferences
Perform a good login and a bad attempt
Check action_logs

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98139 [details] [review]
Bug 21190: Allow browsing the system logs for AUTH events

Trivial change, adding AUTH to viewlog template only.

Test plan:
Look for the Authentication log lines in viewlog.pl (intranet tools).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98140 [details] [review]
Bug 21190: Add tests for authentication success and failure

Test plan:
Run t/db_dependent/Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98141 [details] [review]
Bug 21190: (follow-up) Save patron id in failure when available

The wrong password might belong to an existing user. If that is the case,
we have a $patron.
Note that logaction will save the object info but has no user in the
context environment for a failure.

Test plan:
Login with good user, bad pw and bad user, bad pw. Check logviewer.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

(In reply to Michal Denar from comment #55)
> Signed-off-by: Michal Denar <black23@gmail.com>

Thanks Michal too!

(In reply to Marcel de Rooy from comment #46)
> Created attachment 98125 [details] [review] [review]
> Bug 21190: (follow-up) Save patron id in failure when available
> 
> The wrong password might belong to an existing user. If that is the case,
> we have a $patron.
> Note that logaction will save the object info but has no user in the
> context environment for a failure.
> 
> Test plan:
> Login with good user, bad pw and bad user, bad pw. Check logviewer.

Thanks, Marcel! Looks like people beat me to the signoff, though. :-)

Created attachment 101971 [details] [review]
Bug 21190: Prepare Log.t for further changes

This patch only makes cosmetic changes:
[1] It moves the existing tests at 'root level' inside a new subtest.
[2] It obviously adds indentation for step 1.
[3] It fixes some whitespace (tabs, space before newline)

Test plan:
Run t/db../Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101972 [details] [review]
Bug 21190: Add subtest to Log.t for GDPR logging

Apart from a new subtest, doing some more cleanup:
[1] Copyright statement
[2] Removing the (unneeded) flush
[3] Removing C4::Log from the BEGIN block; we seem to skip that nowadays.
[4] $succes goes to the subtest, $schema and $dbh are global our vars.
    Not required but just what we should do if it would run under Plack.

Test plan:
Run t/db_dependent/Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101973 [details] [review]
Bug 21190: Add interfaces parameter to POD in C4::Logs

Just fixing documentation along the way.
No test plan, just read the patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101974 [details] [review]
Bug 21190: Remove t/Log.t

Since we have t/db../Log.t and t/Log.t simply does nothing, we would
better remove it.

No test plan either :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101975 [details] [review]
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog

Test plan:
Run atomic update.
Check the Logs tab of preferences.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101976 [details] [review]
Bug 21190: Add authentication logging to checkpw

Add optional logging for successful and failing login attempts in
checkpw.

Test plan:
Enable the preferences
Perform a good login and a bad attempt
Check action_logs

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101977 [details] [review]
Bug 21190: Allow browsing the system logs for AUTH events

Trivial change, adding AUTH to viewlog template only.

Test plan:
Look for the Authentication log lines in viewlog.pl (intranet tools).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101978 [details] [review]
Bug 21190: Add tests for authentication success and failure

Test plan:
Run t/db_dependent/Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 101979 [details] [review]
Bug 21190: (follow-up) Save patron id in failure when available

The wrong password might belong to an existing user. If that is the case,
we have a $patron.
Note that logaction will save the object info but has no user in the
context environment for a failure.

Test plan:
Login with good user, bad pw and bad user, bad pw. Check logviewer.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

Rebased after last signoff on 30-01. Please QA

Can we have a link to the European legislation? I would like to correctly understand the need we are trying to answer with this patch set.

(In reply to Jonathan Druart from comment #68)
> Can we have a link to the European legislation? I would like to correctly
> understand the need we are trying to answer with this patch set.

Comment1. Vitor ?
Note that it is preference controlled.

(In reply to Marcel de Rooy from comment #69)
> (In reply to Jonathan Druart from comment #68)
> > Can we have a link to the European legislation? I would like to correctly
> > understand the need we are trying to answer with this patch set.
> 
> Comment1. Vitor ?
> Note that it is preference controlled.

Sorry, I do not read Portuguese.

If we are going to implement/release a GDPR recommendation we should at least point to the legislation.

The problem is, that GDPR does not describe technical measures - so all we might have here is the Portugese guidelines. We can check if other jurisdictions have done similar, but we will probably not find something specific in the law itself.

To me, the relevant part of the document (https://dre.pt/application/conteudo/114937034) is page 1429:

"""
Capacidade de monitorização, registo e análise de toda a atividade de acessos de modo a procurar ameaças prováveis.


Deve ser guardado registo de atividade (log) de todas as ações que um utiliza-dor efetue sobre dados pessoais, independentemente do seu perfil e função.Obrigatório.

[1] Todos os registos de atividade (log) devem ser armazenados apenas em modo de leitura, devendo, com uma periodicidade máxima de 1 mês, ser englo-bados  num  único  bloco  de  registos  e  assinado  digitalmente  (garantia  de  integridade).Obrigatório.

[2] Deve ser guardado registo de atividade (log) de todos os acessos e tentativas falhadas de acesso, obedecendo aos requisitos anteriores.Obrigatório.

Garantir que os registos de atividade provenientes dos diversos subsistemas (Sistemas Operativos, aplicações, browsers, Sistema de Gestão de Base de Dados — SGBD, etc.) são inequivocamente associados à sua origem.Obrigatório.

Os registos de atividade (log) devem conter, no mínimo, o endereço de acesso (IP e Porto), Host, HASH da conta do utilizador que efetuou a ação, ação efe-tuada (CRUD), Tipo de Dado Pessoal onde a ação foi efetuada, data/hora/mi-nuto/segundo (TimeStamp) da ação, alteração efetuada sobre o dado pessoal.Obrigatório.
"""

Trying a translation of the block [1] that we need here:
"""
All logs should be stored in read-only mode and kept for a maximum of 1 month.
"""
But I do not understand the second part (Vitor, could you help? "ser englo-bados  num  único  bloco  de  registos  e  assinado  digitalmente")

Block [2]:
"""
Must bbe kept all activity log of all accesses and failed access attempts must be saved, in compliance with the previous requirements.
"""

So I think we must comply with the "maximum of 1 month". What about having a new switch to cleanup_database to get a module-name and action-name parameter for --logs? Or maybe we already deal with this part on a separate bug report?

Hi Jonathan,

I'll try to explain what the portuguese legislation says about this case.

(In reply to Jonathan Druart from comment #72)
> To me, the relevant part of the document
> (https://dre.pt/application/conteudo/114937034) is page 1429:
> 
> """
> Capacidade de monitorização, registo e análise de toda a atividade de
> acessos de modo a procurar ameaças prováveis.
> 
> 
> Deve ser guardado registo de atividade (log) de todas as ações que um
> utiliza-dor efetue sobre dados pessoais, independentemente do seu perfil e
> função.Obrigatório.
> 
> [1] Todos os registos de atividade (log) devem ser armazenados apenas em
> modo de leitura, devendo, com uma periodicidade máxima de 1 mês, ser
> englo-bados  num  único  bloco  de  registos  e  assinado  digitalmente 
> (garantia  de  integridade).Obrigatório.
> 
> [2] Deve ser guardado registo de atividade (log) de todos os acessos e
> tentativas falhadas de acesso, obedecendo aos requisitos
> anteriores.Obrigatório.
> 
> Garantir que os registos de atividade provenientes dos diversos subsistemas
> (Sistemas Operativos, aplicações, browsers, Sistema de Gestão de Base de
> Dados — SGBD, etc.) são inequivocamente associados à sua origem.Obrigatório.
> 
> Os registos de atividade (log) devem conter, no mínimo, o endereço de acesso
> (IP e Porto), Host, HASH da conta do utilizador que efetuou a ação, ação
> efe-tuada (CRUD), Tipo de Dado Pessoal onde a ação foi efetuada,
> data/hora/mi-nuto/segundo (TimeStamp) da ação, alteração efetuada sobre o
> dado pessoal.Obrigatório.
> """
> 

That's the correct points :)


> Trying a translation of the block [1] that we need here:
> """
> All logs should be stored in read-only mode and kept for a maximum of 1
> month.
> """
> But I do not understand the second part (Vitor, could you help? "ser
> englo-bados  num  único  bloco  de  registos  e  assinado  digitalmente")

The second phrase says that we should do one of two things:
1) Dump the month entries to a file and sign the file digitally
2) Add a column in action_logs that saves the signature used for each authentication entry

The first option is easy to implement using the atual BD and code of Koha.

> 
> Block [2]:
> """
> Must bbe kept all activity log of all accesses and failed access attempts
> must be saved, in compliance with the previous requirements.
> """
> 
> So I think we must comply with the "maximum of 1 month". What about having a
> new switch to cleanup_database to get a module-name and action-name
> parameter for --logs? Or maybe we already deal with this part on a separate
> bug report?

The legislation doesn't cleary say that authentications logs should be removed. 
It says that every month we should do one of the two options above.
If the first option is implemented and if the file is backed up, then we can clean the authentication logs (because we have a backup of them).

Created attachment 102493 [details] [review]
Bug 21190: Prepare Log.t for further changes

This patch only makes cosmetic changes:
[1] It moves the existing tests at 'root level' inside a new subtest.
[2] It obviously adds indentation for step 1.
[3] It fixes some whitespace (tabs, space before newline)

Test plan:
Run t/db../Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102494 [details] [review]
Bug 21190: Add subtest to Log.t for GDPR logging

Apart from a new subtest, doing some more cleanup:
[1] Copyright statement
[2] Removing the (unneeded) flush
[3] Removing C4::Log from the BEGIN block; we seem to skip that nowadays.
[4] $succes goes to the subtest, $schema and $dbh are global our vars.
    Not required but just what we should do if it would run under Plack.

Test plan:
Run t/db_dependent/Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102496 [details] [review]
Bug 21190: Add interfaces parameter to POD in C4::Logs

Just fixing documentation along the way.
No test plan, just read the patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102497 [details] [review]
Bug 21190: Remove t/Log.t

Since we have t/db../Log.t and t/Log.t simply does nothing, we would
better remove it.

No test plan either :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102498 [details] [review]
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog

Test plan:
Run atomic update.
Check the Logs tab of preferences.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102499 [details] [review]
Bug 21190: Add authentication logging to checkpw

Add optional logging for successful and failing login attempts in
checkpw.

Test plan:
Enable the preferences
Perform a good login and a bad attempt
Check action_logs

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102500 [details] [review]
Bug 21190: Allow browsing the system logs for AUTH events

Trivial change, adding AUTH to viewlog template only.

Test plan:
Look for the Authentication log lines in viewlog.pl (intranet tools).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102501 [details] [review]
Bug 21190: Add tests for authentication success and failure

Test plan:
Run t/db_dependent/Log.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 102502 [details] [review]
Bug 21190: (follow-up) Save patron id in failure when available

The wrong password might belong to an existing user. If that is the case,
we have a $patron.
Note that logaction will save the object info but has no user in the
context environment for a failure.

Test plan:
Login with good user, bad pw and bad user, bad pw. Check logviewer.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Thanks Vitor and Jonathan for further follow-up

Nice work everyone!

Pushed to master for 20.05

Please fix the bug title if there is now [part 2] bug reported or in the works.

From an earlier comment:
Exploring further non-regular authentication events might be a part 2. We cover the most now via checkpw.

I will remove "part 1" from the title, because I do not think I will be addressing that topic very soon. But if some one is interested to go down that road (deeper in Auth.pm), I wont mind and will certainly follow it. Open up a new report and put me in CC :)

enhancement not backported to 19.11.x

Created attachment 123540 [details]
Picture of log showing successful SIP logins incrementing 'attempts'

Ah - there might be a problem with Koha counting SIP logins as 'failed' attempts. I could not see it before, but thanks to this enhancement I do see that the total of login attempts goes up by one when I log in to Overdrive Libby, and it is not reset to zero again - even though the login is successful.  We had found more patrons complained of being locked out of the OPAC when they had not had 8 failed login attempts. If SIP login adds one every time, and does not clear back to zero, people using SIP products can come back to the Koha OPAC and find themselves locked out. I am on Koha 20.11
It may be this needs to be a separate new bug - anyone interested to describe what's not working as expected with SIP logins? (and possibly other kinds of external login too...)

(In reply to Agnes Rivers-Moore from comment #89)
> Ah - there might be a problem with Koha counting SIP logins as 'failed'
> attempts. I could not see it before, but thanks to this enhancement I do see
> that the total of login attempts goes up by one when I log in to Overdrive
> Libby, and it is not reset to zero again - even though the login is
> successful.  We had found more patrons complained of being locked out of the
> OPAC when they had not had 8 failed login attempts. If SIP login adds one
> every time, and does not clear back to zero, people using SIP products can
> come back to the Koha OPAC and find themselves locked out. I am on Koha 20.11
> It may be this needs to be a separate new bug - anyone interested to
> describe what's not working as expected with SIP logins? (and possibly other
> kinds of external login too...)

Thx for reporting. This problem should go on its own. Will open a new report now.

See bug 28822