Bug 36822

Summary: When creating a new patron via LDAP or Shibboleth 0000-00-00 is inserted for invalid updated_on
Product: Koha Reporter: betka
Component: AuthenticationAssignee: Pedro Amorim <pedro.amorim>
Status: Pushed to oldstable --- QA Contact: Aleisha Amohia <aleisha>
Severity: major    
Priority: P5 - low CC: aleisha, dpavlin, fridolin.somers, jonathan.druart, lucas, martin.renvoize, pedro.amorim, tomascohen
Version: unspecifiedKeywords: rel_23_05_candidate, rel_23_11_candidate
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18016
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31143
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.06,23.11.11
Circulation function:
Bug Depends on:    
Bug Blocks: 38526    
Attachments: Bug 36822: Shibboleth - Add tests
Bug 36822: Shibboleth - Add discard_changes
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file
Bug 36822: LDAP - Add test
Bug 36822: LDAP - Add discard_changes
Bug 36822: Shibboleth - Add tests
Bug 36822: Shibboleth - Add discard_changes
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file
Bug 36822: LDAP - Add test
Bug 36822: LDAP - Add discard_changes
Bug 36822: Shibboleth - Add tests
Bug 36822: Shibboleth - Add discard_changes
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file
Bug 36822: LDAP - Add test
Bug 36822: LDAP - Add discard_changes
Bug 36822: Shibboleth - Add tests
Bug 36822: Shibboleth - Add discard_changes
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file
Bug 36822: LDAP - Add test
Bug 36822: LDAP - Add discard_changes
Bug 36822: atomicupdate
Bug 36822: Shibboleth - Add tests
Bug 36822: Shibboleth - Add discard_changes
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file
Bug 36822: LDAP - Add test
Bug 36822: LDAP - Add discard_changes
Bug 36822: atomicupdate
Bug 36822: (QA follow-up): Fix now time in tests
Bug 36822: (follow-up) Catch MySQL 8 failure in update
Bug 36822: (follow-up) Catch MySQL 8 failure in update
Bug 36822: Improve datetime comparison in tests
Bug 36822: Improve datetime comparison in tests
Bug 36822: Improve datetime comparison in tests

Description betka 2024-05-09 20:06:08 UTC
Relevant versions:
OS: Ubuntu 22.04
Koha: 23.11.04-4
MySQL: 8.0.36-0ubuntu0.22.04.1

When using Auth_with_ldap, new borrowers that are created via LDAP have an updated_on date of 0000-00-00 00:00:00. 

Guessing this may be related to comments in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18016

May also be a side effect of explicitly declaring a default_value of \"current_timestamp" for updated_on in Koha/Schema/Result/Borrower.pm.

(Guessing no one else is still using LDAP to create accounts)
Comment 1 Pedro Amorim 2024-10-25 12:18:51 UTC
I've been looking into this with Martin since we've been experiencing the same issue but with Shibboleth.
We believe we've got a fix and currently working on it to submit soon.
Comment 2 Martin Renvoize (ashimema) 2024-10-25 12:20:22 UTC
We've narrowed the same issue down in Shibboleth and likely have the fix for this case too... we need to refresh the new patron object after it's creation in auth_with_ldap before passing it back to auth.pm else the internal representation of the data isn't up to date with whats in the database and we try to reset those fields at update later.

What I don't fully understand is why the internal \'current_timestamp' is somehow getting converted to 0000-00-00 for insert later from SCALAR dump.. but I'll leave someone wiser than me to dig there if they're keen to.
Comment 3 Pedro Amorim 2024-10-25 14:48:43 UTC Comment hidden (obsolete)
Comment 4 Pedro Amorim 2024-10-25 14:48:46 UTC Comment hidden (obsolete)
Comment 5 Pedro Amorim 2024-10-25 14:48:50 UTC Comment hidden (obsolete)
Comment 6 Pedro Amorim 2024-10-25 14:48:54 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2024-10-25 14:48:57 UTC Comment hidden (obsolete)
Comment 8 Pedro Amorim 2024-10-25 14:58:32 UTC Comment hidden (obsolete)
Comment 9 Pedro Amorim 2024-10-25 14:58:35 UTC Comment hidden (obsolete)
Comment 10 Pedro Amorim 2024-10-25 14:58:38 UTC Comment hidden (obsolete)
Comment 11 Pedro Amorim 2024-10-25 14:58:42 UTC Comment hidden (obsolete)
Comment 12 Pedro Amorim 2024-10-25 14:58:45 UTC Comment hidden (obsolete)
Comment 13 Pedro Amorim 2024-10-25 15:04:13 UTC
Created attachment 173391 [details] [review]
Bug 36822: Shibboleth - Add tests

prove t/db_dependent/Auth_with_shibboleth.t
Comment 14 Pedro Amorim 2024-10-25 15:04:16 UTC
Created attachment 173392 [details] [review]
Bug 36822: Shibboleth - Add discard_changes

Test plan:
1) Apply only Shibboleth tests patch
2) Run that test file:
$ prove t/db_dependent/Auth_with_shibboleth.t
3) Verify tests fail with the updated_on error message.
4) Apply this patch. Repeat 2) Verify tests now pass.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Pedro Amorim 2024-10-25 15:04:18 UTC
Created attachment 173393 [details] [review]
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file

Without this rearrangement, C4::Context->config('useldapserver') is always 0.
It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up
Comment 16 Pedro Amorim 2024-10-25 15:04:21 UTC
Created attachment 173394 [details] [review]
Bug 36822: LDAP - Add test
Comment 17 Pedro Amorim 2024-10-25 15:04:23 UTC
Created attachment 173395 [details] [review]
Bug 36822: LDAP - Add discard_changes

Test plan:
1) Apply only LDAP rearrangement patch
2) Run that test file (to prove the rearrangement does not break tests):
$ prove t/db_dependent/Auth_with_ldap.t
3) Verify tests pass
4) Apply LDAP test patch on top
5) Run the test file. Notice it fails.
6) Apply this patch. Run LDAP tests again. Verify tests pass.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 David Nind 2024-10-25 18:02:49 UTC
Created attachment 173402 [details] [review]
Bug 36822: Shibboleth - Add tests

prove t/db_dependent/Auth_with_shibboleth.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2024-10-25 18:02:51 UTC
Created attachment 173403 [details] [review]
Bug 36822: Shibboleth - Add discard_changes

Test plan:
1) Apply only Shibboleth tests patch
2) Run that test file:
$ prove t/db_dependent/Auth_with_shibboleth.t
3) Verify tests fail with the updated_on error message.
4) Apply this patch. Repeat 2) Verify tests now pass.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2024-10-25 18:02:54 UTC
Created attachment 173404 [details] [review]
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file

Without this rearrangement, C4::Context->config('useldapserver') is always 0.
It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2024-10-25 18:02:57 UTC
Created attachment 173405 [details] [review]
Bug 36822: LDAP - Add test

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2024-10-25 18:03:00 UTC
Created attachment 173406 [details] [review]
Bug 36822: LDAP - Add discard_changes

Test plan:
1) Apply only LDAP rearrangement patch
2) Run that test file (to prove the rearrangement does not break tests):
$ prove t/db_dependent/Auth_with_ldap.t
3) Verify tests pass
4) Apply LDAP test patch on top
5) Run the test file. Notice it fails.
6) Apply this patch. Run LDAP tests again. Verify tests pass.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 23 Pedro Amorim 2024-10-29 13:16:47 UTC
Created attachment 173646 [details] [review]
Bug 36822: atomicupdate

Fix any existing updated_on issues
Comment 24 Aleisha Amohia 2024-10-31 04:47:41 UTC
Really keen to pass QA on this but a few QA tool warnings to address first please:


 WARN	C4/Auth.pm
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 387, now: 388)

 OK	C4/Auth_with_ldap.pm

 OK	C4/Auth_with_shibboleth.pm

 OK	installer/data/mysql/atomicupdate/bug_36822.pl

 WARN	t/db_dependent/Auth_with_ldap.t
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 104, now: 105)

 WARN	t/db_dependent/Auth_with_shibboleth.t
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 109, now: 116)


Processing additional checks OK!


	* Was this bug sponsored by anyone?

Running tests (2)
	* Proving /kohadevbox/koha/t/db_dependent/Auth_with_shibboleth.t OK!
	* Proving /kohadevbox/koha/t/db_dependent/Auth_with_ldap.t OK!
Comment 25 Pedro Amorim 2024-11-04 11:34:40 UTC
Created attachment 173903 [details] [review]
Bug 36822: Shibboleth - Add tests

prove t/db_dependent/Auth_with_shibboleth.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Pedro Amorim 2024-11-04 11:34:43 UTC
Created attachment 173904 [details] [review]
Bug 36822: Shibboleth - Add discard_changes

Test plan:
1) Apply only Shibboleth tests patch
2) Run that test file:
$ prove t/db_dependent/Auth_with_shibboleth.t
3) Verify tests fail with the updated_on error message.
4) Apply this patch. Repeat 2) Verify tests now pass.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 27 Pedro Amorim 2024-11-04 11:34:45 UTC
Created attachment 173905 [details] [review]
Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file

Without this rearrangement, C4::Context->config('useldapserver') is always 0.
It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Pedro Amorim 2024-11-04 11:34:48 UTC
Created attachment 173906 [details] [review]
Bug 36822: LDAP - Add test

Signed-off-by: David Nind <david@davidnind.com>
Comment 29 Pedro Amorim 2024-11-04 11:34:51 UTC
Created attachment 173907 [details] [review]
Bug 36822: LDAP - Add discard_changes

Test plan:
1) Apply only LDAP rearrangement patch
2) Run that test file (to prove the rearrangement does not break tests):
$ prove t/db_dependent/Auth_with_ldap.t
3) Verify tests pass
4) Apply LDAP test patch on top
5) Run the test file. Notice it fails.
6) Apply this patch. Run LDAP tests again. Verify tests pass.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 30 Pedro Amorim 2024-11-04 11:34:54 UTC
Created attachment 173908 [details] [review]
Bug 36822: atomicupdate

Fix any existing updated_on issues
Comment 31 Pedro Amorim 2024-11-04 11:34:57 UTC
Created attachment 173909 [details] [review]
Bug 36822: (QA follow-up): Fix now time in tests

This is to prevent the scenario of tests failing if they take long to run, e.g.:

    #   Failed test 'updated_on correctly saved on newly created user'
    #   at t/db_dependent/Auth_with_shibboleth.t line 317.
    #     Structures begin differing at:
    #          ->[0] = '2024-11-04 11:20:13'
    #     ->[0] = '2024-11-04 11:20:14
Comment 32 Pedro Amorim 2024-11-04 11:38:22 UTC
(In reply to Aleisha Amohia from comment #24)
> Really keen to pass QA on this but a few QA tool warnings to address first
> please:

Thank you Aleisha, I've squashed perltidy tidyness to the tests files patches to address the warnings of Auth_with_ldap.t and Auth_with_shibboleth.t
I've also submitted a follow-up to prevent tests from failing if enough time has gone by between user being created and the test running (stumbled upon this while revisiting).

I've spent an unreasonable amount of time trying fix the tidy warning of 1 untidy line in C4/Auth.pm and I can't fix it barring tidying the whole file.
I'm happy to have someone else fix this tidy warning for me on C4/Auth.pm but I also don't think this should be reason enough to block these patches.
Comment 33 Martin Renvoize (ashimema) 2024-11-04 14:26:17 UTC
Let's not hold this one up just for the Tidy challenges..
Comment 34 Katrin Fischer 2024-11-07 15:32:46 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 35 Aleisha Amohia 2024-11-07 22:38:57 UTC
This affects our libraries as early as 23.05 so have added the relevant keywords
Comment 36 Katrin Fischer 2024-11-14 12:40:40 UTC
This causes an issue with MySQL 8 that doesn't even allow comparisons to an invalid date:


11:19:21 koha_1       | Upgrade to 24.06.00.056  [10:19:04]: Bug 36822 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36822) - Sanitize borrowers.updated_on
11:19:21 koha_1       | ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::st execute failed: Incorrect TIMESTAMP value: '0000-00-00 00:00:00' at /kohadevbox/koha/C4/Installer.pm line 742

We have discussed several solutions. I guess we can assume a database that strict doesn't have invalid dates, so a try-catch was what we thought would work best.
Comment 37 Martin Renvoize (ashimema) 2024-11-14 12:54:51 UTC
Created attachment 174523 [details] [review]
Bug 36822: (follow-up) Catch MySQL 8 failure in update

We add a try/catch around the call to fix 0000-00-00 dates in the
database. MySQL 8.0 throws an error on the WHERE clause for such a
search.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize (ashimema) 2024-11-14 13:10:33 UTC
Created attachment 174525 [details] [review]
Bug 36822: (follow-up) Catch MySQL 8 failure in update

We add a CAST to the fetch of 0000-00-00 dates in the database. This
prevents an error in MySQL 8.0 throws htat aborts the update.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Katrin Fischer 2024-11-14 13:16:32 UTC
Follow-up "Catch MySQL 8 failur in update" picked for main.
Comment 40 Katrin Fischer 2024-11-14 14:32:59 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 41 Lucas Gass (lukeg) 2024-11-15 16:13:40 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 42 Pedro Amorim 2024-11-22 13:29:43 UTC
Created attachment 174926 [details] [review]
Bug 36822: Improve datetime comparison in tests

This removes the seconds part of the datetime comparison because when running tests this may mismatch causing tests to become flaky.
The previous patch 'Fix now time in tests' was not enough to prevent this.

prove t/db_dependent/Auth_with_ldap.t
prove t/db_dependent/Auth_with_shibboleth.t
Comment 43 Pedro Amorim 2024-11-22 13:44:31 UTC
Created attachment 174927 [details] [review]
Bug 36822: Improve datetime comparison in tests

This removes the seconds part of the datetime comparison because when running tests this may mismatch causing tests to become flaky.
The previous patch 'Fix now time in tests' was not enough to prevent this.

prove t/db_dependent/Auth_with_ldap.t
prove t/db_dependent/Auth_with_shibboleth.t
Comment 44 Jonathan Druart 2024-11-22 20:05:11 UTC
Pedro, you need to use t::lib::Dates::compare
Comment 45 Jonathan Druart 2024-11-25 10:58:08 UTC
Created attachment 174965 [details] [review]
Bug 36822: Improve datetime comparison in tests
Comment 46 Jonathan Druart 2024-11-25 10:59:08 UTC
(In reply to Jonathan Druart from comment #45)
> Created attachment 174965 [details] [review] [review]
> Bug 36822: Improve datetime comparison in tests

Moved to bug 38526
Comment 47 Fridolin Somers 2024-11-27 09:11:26 UTC
Pushed to 23.11.x for 23.11.11