|
Description
Martin Renvoize (ashimema)
2019-03-25 18:24:24 UTC
Created attachment 86991 [details] [review] Bug 22577: Prevent deletion of system users Created attachment 86992 [details] [review] Bug 22577: Add 'cron' system user Created attachment 86993 [details] [review] Bug 22577: Set 'cron' user for fines job t/lib/Mocks.pm:sub mock_userenv should probably be set to default to 0 instead of 51 - or create it's own user? raising severity as some crons are currently broken by this (longoverdue for example) Created attachment 87007 [details] [review] Bug 22577: Update cronjobs to set userenv 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? Created attachment 87008 [details] [review] Bug 22577: Retrospectively assign '-1' for cronscripts (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? Created attachment 87013 [details] [review] Bug 22577: Less typing is good! I'm working on a branch here: https://github.com/PTFS-Europe/koha/compare/master...PTFS-Europe:bug_22577 Created attachment 87014 [details] [review] Bug 22577: Less typing is good! Created attachment 87015 [details] [review] Bug 22577: Less typing is good! Created attachment 87017 [details] [review] Bug 22577: Add test for Koha::Cron base class Created attachment 87028 [details] [review] Bug 22577: Prevent deletion of system users Created attachment 87029 [details] [review] Bug 22577: Add 'cron' system user Created attachment 87030 [details] [review] Bug 22577: Set 'cron' user for cron scripts Created attachment 87031 [details] [review] Bug 22577: Add test for Koha::Cron base class Created attachment 87032 [details] [review] Bug 22577: Retrospectively assign '-1' for cronscripts 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) Created attachment 87053 [details] [review] Bug 22577: Prevent deletion of system users Created attachment 87054 [details] [review] Bug 22577: Add 'cron' system user Created attachment 87055 [details] [review] Bug 22577: Retrospectively assign '-1' for cronscripts Created attachment 87056 [details] [review] Bug 22577: Set 'cron' user for cron scripts Created attachment 87057 [details] [review] Bug 22577: Add test for Koha::Cron base class Created attachment 87058 [details] [review] Bug 22577: Test patron delete restriction Created attachment 87059 [details] [review] Bug 22577: Prevent system users from returning in searches Created attachment 87060 [details] [review] Bug 22577: Add test for Koha::Patrons->search (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 (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) For distinguiging who did something, maybe it would be better to go via interface in the log tables anyway? 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). (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. I do like the general idea of Koha::Cron module. What I do not like is using some hardcoded "system" user. 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 * Bug 26170 added protected patrons |