Bug 21190 - Add logging of successful/unsuccessful login attempts
Summary: Add logging of successful/unsuccessful login attempts
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P2 enhancement with 2 votes (vote)
Assignee: Marcel de Rooy
QA Contact: Jonathan Druart
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 22794
  Show dependency treegraph
 
Reported: 2018-08-09 13:30 UTC by Vitor Fernandes
Modified: 2020-05-20 09:03 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This enhancement adds two new logging preferences `AuthFailureLog` and `AuthSuccessLog` in order to keep track of bad login attempts and successful ones. NOTE: In some countries, this may be a requirement as a local application of GDPR legislation. **New system preferences**: `AuthFailureLog` and `AuthSuccessLog` both default to disabled.
Version(s) released in:
20.05.00


Attachments
Bug 21190: Prepare Log.t for further changes (4.86 KB, patch)
2019-10-25 07:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add subtest to Log.t for GDPR logging (3.07 KB, patch)
2019-10-25 08:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add interfaces parameter to POD in C4::Logs (892 bytes, patch)
2019-10-25 08:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Remove t/Log.t (816 bytes, patch)
2019-10-25 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.44 KB, patch)
2019-10-25 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.37 KB, patch)
2019-10-25 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.50 KB, patch)
2019-10-25 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.43 KB, patch)
2019-10-25 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Allow browsing the system logs for AUTH events (1.99 KB, patch)
2019-10-25 10:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add tests for authentication success and failure (2.66 KB, patch)
2019-10-28 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Prepare Log.t for further changes (4.86 KB, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add subtest to Log.t for GDPR logging (3.07 KB, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add interfaces parameter to POD in C4::Logs (892 bytes, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Remove t/Log.t (816 bytes, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.50 KB, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.43 KB, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Allow browsing the system logs for AUTH events (1.99 KB, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add tests for authentication success and failure (2.66 KB, patch)
2020-01-29 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Prepare Log.t for further changes (4.87 KB, patch)
2020-01-30 09:16 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Add subtest to Log.t for GDPR logging (3.08 KB, patch)
2020-01-30 09:16 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Add interfaces parameter to POD in C4::Logs (904 bytes, patch)
2020-01-30 09:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Remove t/Log.t (828 bytes, patch)
2020-01-30 09:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.51 KB, patch)
2020-01-30 09:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.44 KB, patch)
2020-01-30 09:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Allow browsing the system logs for AUTH events (2.00 KB, patch)
2020-01-30 09:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: Add tests for authentication success and failure (2.67 KB, patch)
2020-01-30 09:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21190: (follow-up) Save patron id in failure when available (1.27 KB, patch)
2020-01-30 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Prepare Log.t for further changes (4.92 KB, patch)
2020-01-30 09:50 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Add subtest to Log.t for GDPR logging (3.13 KB, patch)
2020-01-30 09:50 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Add interfaces parameter to POD in C4::Logs (953 bytes, patch)
2020-01-30 09:50 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Remove t/Log.t (877 bytes, patch)
2020-01-30 09:50 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.56 KB, patch)
2020-01-30 09:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.49 KB, patch)
2020-01-30 09:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Allow browsing the system logs for AUTH events (2.05 KB, patch)
2020-01-30 09:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Add tests for authentication success and failure (2.72 KB, patch)
2020-01-30 09:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: (follow-up) Save patron id in failure when available (1.28 KB, patch)
2020-01-30 09:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21190: Prepare Log.t for further changes (4.82 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add subtest to Log.t for GDPR logging (3.78 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add interfaces parameter to POD in C4::Logs (993 bytes, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Remove t/Log.t (917 bytes, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.60 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.53 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Allow browsing the system logs for AUTH events (2.12 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Add tests for authentication success and failure (2.76 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: (follow-up) Save patron id in failure when available (1.32 KB, patch)
2020-03-27 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21190: Prepare Log.t for further changes (4.85 KB, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Add subtest to Log.t for GDPR logging (3.80 KB, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Add interfaces parameter to POD in C4::Logs (1.00 KB, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Remove t/Log.t (951 bytes, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Introduce preferences AuthFailureLog and AuthSuccessLog (5.61 KB, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Add authentication logging to checkpw (1.56 KB, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Allow browsing the system logs for AUTH events (2.40 KB, patch)
2020-04-07 10:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: Add tests for authentication success and failure (2.79 KB, patch)
2020-04-07 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21190: (follow-up) Save patron id in failure when available (1.35 KB, patch)
2020-04-07 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vitor Fernandes 2018-08-09 13:30:46 UTC
Under the auspices of the recently issued European legislation regarding data privacy (GDPR), the Portuguese government has issued a series of mandatory requirements, as well as general recommendations, for software applications that are implemented under the umbrella of public bodies (RCM 41/2018).

Since Koha is mostly used by municipalities and universities in Portugal, some of these mandatory requirements need to be address by Koha implementers in Portugal.

We believe that this requirement is also useful for the community at large. Here’s a description of the requirement.

*** 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.

*** Scope ***

Applies in all cases.
Comment 1 Magnus Enger 2018-08-13 07:47:37 UTC
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.
Comment 2 Marcel de Rooy 2018-09-12 11:16:18 UTC
(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 ?
Comment 3 Vitor Fernandes 2018-09-12 11:30:57 UTC
(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");
Comment 4 Agnes Rivers-Moore 2019-02-15 20:54:15 UTC
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.
Comment 5 Marcel de Rooy 2019-10-25 07:13:33 UTC
Trying to give this a little start here :)
Comment 6 Marcel de Rooy 2019-10-25 07:17:23 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2019-10-25 08:08:50 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2019-10-25 08:11:56 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2019-10-25 08:23:44 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2019-10-25 09:23:05 UTC
Moving 21191 to see also
Comment 11 Marcel de Rooy 2019-10-25 10:07:42 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2019-10-25 10:07:46 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2019-10-25 10:08:30 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2019-10-25 10:08:35 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2019-10-25 10:14:08 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2019-10-25 10:15:37 UTC
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.
Comment 17 Marcel de Rooy 2019-10-28 10:12:35 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2019-10-28 10:13:42 UTC
Please get this further now by testing and signing off !
Comment 19 Joonas Kylmälä 2019-10-28 10:32:26 UTC
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.
Comment 20 Marcel de Rooy 2019-10-28 10:57:42 UTC
(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.
Comment 21 Katrin Fischer 2019-10-29 06:55:51 UTC
(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.
Comment 22 Marcel de Rooy 2019-10-29 15:44:07 UTC
(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.
Comment 23 Joonas Kylmälä 2019-10-30 05:19:49 UTC
(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.
Comment 24 Katrin Fischer 2019-10-30 07:05:59 UTC
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).
Comment 25 Marcel de Rooy 2020-01-29 13:14:15 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2020-01-29 13:14:19 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2020-01-29 13:14:24 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2020-01-29 13:14:28 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2020-01-29 13:14:33 UTC Comment hidden (obsolete)
Comment 30 Marcel de Rooy 2020-01-29 13:14:37 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2020-01-29 13:14:41 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2020-01-29 13:14:46 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2020-01-29 13:16:49 UTC
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!
Comment 34 Agnes Rivers-Moore 2020-01-30 00:11:05 UTC
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.
Comment 35 Magnus Enger 2020-01-30 08:55:32 UTC
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.
Comment 36 ByWater Sandboxes 2020-01-30 09:16:54 UTC Comment hidden (obsolete)
Comment 37 ByWater Sandboxes 2020-01-30 09:16:57 UTC Comment hidden (obsolete)
Comment 38 ByWater Sandboxes 2020-01-30 09:17:00 UTC Comment hidden (obsolete)
Comment 39 ByWater Sandboxes 2020-01-30 09:17:04 UTC Comment hidden (obsolete)
Comment 40 ByWater Sandboxes 2020-01-30 09:17:07 UTC Comment hidden (obsolete)
Comment 41 ByWater Sandboxes 2020-01-30 09:17:10 UTC Comment hidden (obsolete)
Comment 42 ByWater Sandboxes 2020-01-30 09:17:13 UTC Comment hidden (obsolete)
Comment 43 ByWater Sandboxes 2020-01-30 09:17:16 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2020-01-30 09:22:53 UTC
(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.
Comment 45 Marcel de Rooy 2020-01-30 09:23:14 UTC
(In reply to ByWater Sandboxes from comment #43)
> Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Thanks Jon
Comment 46 Marcel de Rooy 2020-01-30 09:31:56 UTC Comment hidden (obsolete)
Comment 47 Michal Denar 2020-01-30 09:50:43 UTC
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>
Comment 48 Michal Denar 2020-01-30 09:50:48 UTC
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>
Comment 49 Michal Denar 2020-01-30 09:50:52 UTC
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>
Comment 50 Michal Denar 2020-01-30 09:50:57 UTC
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>
Comment 51 Michal Denar 2020-01-30 09:51:01 UTC
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>
Comment 52 Michal Denar 2020-01-30 09:51:06 UTC
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>
Comment 53 Michal Denar 2020-01-30 09:51:10 UTC
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>
Comment 54 Michal Denar 2020-01-30 09:51:14 UTC
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>
Comment 55 Michal Denar 2020-01-30 09:51:18 UTC
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>
Comment 56 Marcel de Rooy 2020-01-30 09:58:03 UTC
(In reply to Michal Denar from comment #55)
> Signed-off-by: Michal Denar <black23@gmail.com>

Thanks Michal too!
Comment 57 Magnus Enger 2020-01-30 10:42:27 UTC
(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. :-)
Comment 58 Marcel de Rooy 2020-03-27 09:13:06 UTC
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>
Comment 59 Marcel de Rooy 2020-03-27 09:13:11 UTC
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>
Comment 60 Marcel de Rooy 2020-03-27 09:13:17 UTC
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>
Comment 61 Marcel de Rooy 2020-03-27 09:13:22 UTC
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>
Comment 62 Marcel de Rooy 2020-03-27 09:13:27 UTC
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>
Comment 63 Marcel de Rooy 2020-03-27 09:13:32 UTC
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>
Comment 64 Marcel de Rooy 2020-03-27 09:13:38 UTC
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>
Comment 65 Marcel de Rooy 2020-03-27 09:13:43 UTC
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>
Comment 66 Marcel de Rooy 2020-03-27 09:13:48 UTC
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>
Comment 67 Marcel de Rooy 2020-03-27 09:14:49 UTC
Rebased after last signoff on 30-01. Please QA
Comment 68 Jonathan Druart 2020-03-27 10:14:32 UTC
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.
Comment 69 Marcel de Rooy 2020-03-27 10:46:53 UTC
(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.
Comment 70 Jonathan Druart 2020-03-27 10:59:32 UTC
(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.
Comment 71 Katrin Fischer 2020-03-27 13:17:00 UTC
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.
Comment 72 Jonathan Druart 2020-04-07 09:41:39 UTC
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?
Comment 73 Vitor Fernandes 2020-04-07 10:11:00 UTC
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).
Comment 74 Jonathan Druart 2020-04-07 10:27:19 UTC
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>
Comment 75 Jonathan Druart 2020-04-07 10:27:24 UTC
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>
Comment 76 Jonathan Druart 2020-04-07 10:27:30 UTC
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>
Comment 77 Jonathan Druart 2020-04-07 10:27:37 UTC
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>
Comment 78 Jonathan Druart 2020-04-07 10:27:44 UTC
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>
Comment 79 Jonathan Druart 2020-04-07 10:27:49 UTC
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>
Comment 80 Jonathan Druart 2020-04-07 10:27:55 UTC
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>
Comment 81 Jonathan Druart 2020-04-07 10:28:01 UTC
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>
Comment 82 Jonathan Druart 2020-04-07 10:28:06 UTC
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>
Comment 83 Marcel de Rooy 2020-04-07 11:35:28 UTC
Thanks Vitor and Jonathan for further follow-up
Comment 84 Martin Renvoize 2020-04-14 15:07:44 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 85 Martin Renvoize 2020-04-14 15:15:11 UTC
Please fix the bug title if there is now [part 2] bug reported or in the works.
Comment 86 Marcel de Rooy 2020-04-15 11:18:45 UTC
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 :)
Comment 87 Joy Nelson 2020-05-05 20:45:16 UTC
enhancement not backported to 19.11.x