Bug 22577 - Cronjobs (and other scripts) should be attributed to a 'real' user
Summary: Cronjobs (and other scripts) should be attributed to a 'real' user
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 22593
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-25 18:24 UTC by Martin Renvoize
Modified: 2019-04-23 15:08 UTC (History)
7 users (show)

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


Attachments
Bug 22577: Prevent deletion of system users (757 bytes, patch)
2019-03-25 19:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add 'cron' system user (965 bytes, patch)
2019-03-25 19:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Set 'cron' user for fines job (671 bytes, patch)
2019-03-25 19:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Update cronjobs to set userenv (20.49 KB, patch)
2019-03-26 10:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Retrospectively assign '-1' for cronscripts (1.26 KB, patch)
2019-03-26 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Less typing is good! (24.99 KB, patch)
2019-03-26 12:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Less typing is good! (27.38 KB, patch)
2019-03-26 12:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Less typing is good! (29.75 KB, patch)
2019-03-26 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add test for Koha::Cron base class (1.76 KB, patch)
2019-03-26 14:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Prevent deletion of system users (757 bytes, patch)
2019-03-26 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add 'cron' system user (965 bytes, patch)
2019-03-26 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Set 'cron' user for cron scripts (20.60 KB, patch)
2019-03-26 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add test for Koha::Cron base class (1.76 KB, patch)
2019-03-26 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Retrospectively assign '-1' for cronscripts (1.26 KB, patch)
2019-03-26 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Prevent deletion of system users (757 bytes, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add 'cron' system user (2.56 KB, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Retrospectively assign '-1' for cronscripts (1.40 KB, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Set 'cron' user for cron scripts (20.58 KB, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add test for Koha::Cron base class (1.76 KB, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Test patron delete restriction (1.45 KB, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Prevent system users from returning in searches (915 bytes, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22577: Add test for Koha::Patrons->search (1.46 KB, patch)
2019-03-27 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-03-25 18:24:24 UTC
Currently the userenv for cronjobs is set to '0' for the userid. This user does not exist and breaks/prevents foreign key constraints from working as expected.

We should really add a hard coded default user to identify such actions.
Comment 1 Martin Renvoize 2019-03-25 19:05:39 UTC
Created attachment 86991 [details] [review]
Bug 22577: Prevent deletion of system users
Comment 2 Martin Renvoize 2019-03-25 19:05:41 UTC
Created attachment 86992 [details] [review]
Bug 22577: Add 'cron' system user
Comment 3 Martin Renvoize 2019-03-25 19:05:44 UTC
Created attachment 86993 [details] [review]
Bug 22577: Set 'cron' user for fines job
Comment 4 Nick Clemens 2019-03-25 19:45:56 UTC
t/lib/Mocks.pm:sub mock_userenv should probably be set to default to 0 instead of 51 - or create it's own user?
Comment 5 Nick Clemens 2019-03-25 19:47:01 UTC
raising severity as some crons are currently broken by this (longoverdue for example)
Comment 6 Martin Renvoize 2019-03-26 10:18:12 UTC
Created attachment 87007 [details] [review]
Bug 22577: Update cronjobs to set userenv
Comment 7 Martin Renvoize 2019-03-26 10:42:42 UTC
Question time.. I've added a 'CRON' user account at borrowernumber '-1' and updated all the relevant scripts in /misc/cronjobs. I'm now wondering about the other scripts in that directory.. they're a mishmash of things.. should we have a 'command line' user to assign these to or use the cron user again?
Comment 8 Martin Renvoize 2019-03-26 11:17:36 UTC
Created attachment 87008 [details] [review]
Bug 22577: Retrospectively assign '-1' for cronscripts
Comment 9 Martin Renvoize 2019-03-26 11:47:25 UTC
(In reply to Nick Clemens from comment #4)
> t/lib/Mocks.pm:sub mock_userenv should probably be set to default to 0
> instead of 51 - or create it's own user?

As much as I agree to this.. as there's a user added in that slot by default for Jenkins in the default data we're OK without it being done here.. perhaps we could add that to a further bug which works towards having the ability to run tests without that default data?
Comment 10 Martin Renvoize 2019-03-26 12:39:13 UTC
Created attachment 87013 [details] [review]
Bug 22577: Less typing is good!
Comment 11 Martin Renvoize 2019-03-26 12:40:27 UTC
I'm working on a branch here: https://github.com/PTFS-Europe/koha/compare/master...PTFS-Europe:bug_22577
Comment 12 Martin Renvoize 2019-03-26 12:44:06 UTC
Created attachment 87014 [details] [review]
Bug 22577: Less typing is good!
Comment 13 Martin Renvoize 2019-03-26 12:47:34 UTC
Created attachment 87015 [details] [review]
Bug 22577: Less typing is good!
Comment 14 Martin Renvoize 2019-03-26 14:01:28 UTC
Created attachment 87017 [details] [review]
Bug 22577: Add test for Koha::Cron base class
Comment 15 Martin Renvoize 2019-03-26 16:50:44 UTC
Created attachment 87028 [details] [review]
Bug 22577: Prevent deletion of system users
Comment 16 Martin Renvoize 2019-03-26 16:50:47 UTC
Created attachment 87029 [details] [review]
Bug 22577: Add 'cron' system user
Comment 17 Martin Renvoize 2019-03-26 16:50:50 UTC
Created attachment 87030 [details] [review]
Bug 22577: Set 'cron' user for cron scripts
Comment 18 Martin Renvoize 2019-03-26 16:50:53 UTC
Created attachment 87031 [details] [review]
Bug 22577: Add test for Koha::Cron base class
Comment 19 Martin Renvoize 2019-03-26 16:50:56 UTC
Created attachment 87032 [details] [review]
Bug 22577: Retrospectively assign '-1' for cronscripts
Comment 20 Katrin Fischer 2019-03-27 06:47:15 UTC
I wonder if having a real user is the right way to do this. If we don't want to differentiate between cronjob and command line, maybe using NULL could work? I am worried that system users will add a lot of other problems (which branch to assign them to? especially in multi-branch systems with the new 'independent branches' features it seems like it could get confusing really quick)
Comment 21 Martin Renvoize 2019-03-27 10:31:16 UTC
Created attachment 87053 [details] [review]
Bug 22577: Prevent deletion of system users
Comment 22 Martin Renvoize 2019-03-27 10:31:19 UTC
Created attachment 87054 [details] [review]
Bug 22577: Add 'cron' system user
Comment 23 Martin Renvoize 2019-03-27 10:31:22 UTC
Created attachment 87055 [details] [review]
Bug 22577: Retrospectively assign '-1' for cronscripts
Comment 24 Martin Renvoize 2019-03-27 10:31:26 UTC
Created attachment 87056 [details] [review]
Bug 22577: Set 'cron' user for cron scripts
Comment 25 Martin Renvoize 2019-03-27 10:31:29 UTC
Created attachment 87057 [details] [review]
Bug 22577: Add test for Koha::Cron base class
Comment 26 Martin Renvoize 2019-03-27 10:31:32 UTC
Created attachment 87058 [details] [review]
Bug 22577: Test patron delete restriction
Comment 27 Martin Renvoize 2019-03-27 10:31:35 UTC
Created attachment 87059 [details] [review]
Bug 22577: Prevent system users from returning in searches
Comment 28 Martin Renvoize 2019-03-27 10:31:38 UTC
Created attachment 87060 [details] [review]
Bug 22577: Add test for Koha::Patrons->search
Comment 29 Nick Clemens 2019-03-27 11:23:17 UTC
(In reply to Katrin Fischer from comment #20)
> I wonder if having a real user is the right way to do this. If we don't want
> to differentiate between cronjob and command line, maybe using NULL could
> work? I am worried that system users will add a lot of other problems (which
> branch to assign them to? especially in multi-branch systems with the new
> 'independent branches' features it seems like it could get confusing really
> quick)

An issue with using NULL is that we would then not have a way to distinguish between system/cron and deleted user actions.

I added this bug to agenda for next dev meeting
Comment 30 Katrin Fischer 2019-03-27 11:34:00 UTC
(In reply to Nick Clemens from comment #29)
> (In reply to Katrin Fischer from comment #20)
> > I wonder if having a real user is the right way to do this. If we don't want
> > to differentiate between cronjob and command line, maybe using NULL could
> > work? I am worried that system users will add a lot of other problems (which
> > branch to assign them to? especially in multi-branch systems with the new
> > 'independent branches' features it seems like it could get confusing really
> > quick)
> 
> An issue with using NULL is that we would then not have a way to distinguish
> between system/cron and deleted user actions.
> 
> I added this bug to agenda for next dev meeting

It just appears to me like thsi approach will open anoher can of worms.

- Which homelibrary to use for those users?
- Which patron category?
- How to keep people from deleting or changing them?
- How to prevent that they change statistics in an unwanted way? (if they have a branch and a patron category)
Comment 31 Katrin Fischer 2019-03-27 11:34:58 UTC
For distinguiging who did something, maybe it would be better to go via interface in the log tables anyway?
Comment 32 Martin Renvoize 2019-03-27 19:35:29 UTC
Well, I've de-escalated this one by resolving the constraint problem a simpler way in bug 22593.

So, this now serves as proof of concept code which could let us further constrain some things in the database and allow for other uses of system type users (anonymous user seems a good case, splitting up Cron, command line scripts).
Comment 33 Martin Renvoize 2019-03-27 19:40:51 UTC
(In reply to Katrin Fischer from comment #30)
> 
> It just appears to me like thsi approach will open anoher can of worms.
> 
> - Which homelibrary to use for those users?
> - Which patron category?
> - How to keep people from deleting or changing them?
> - How to prevent that they change statistics in an unwanted way? (if they
> have a branch and a patron category)

No homelibrary, see patches
No categorycode, see patches
We can't if they're doing so directly in the db or a home grown script. But the patch does attemp to prevent it if they're using Koha object as they should.
We also prevent these users from being searched at a pretty low level in code, see patches.

As I saw, this is all now proof of concept code open for opinion and discussion.
Comment 34 Josef Moravec 2019-03-28 08:21:00 UTC
I do like the general idea of Koha::Cron module.

What I do not like is using some hardcoded "system" user.
Comment 35 Martin Renvoize 2019-04-23 15:08:14 UTC
We now get around the need for a 'real' user for cron scripts (though I still like the idea of having system users allowing for stricter constraints).

Bug 22600 adds an interface field to the accountlines and introduces a base class for scripts so it's always set which accomplishes some of what this patch introduced.

Dropping the assignee to open this one up for adoption should anyone want to rebase/rework it for future use