View Issue Details

IDProjectCategoryView StatusLast Update
0001544Main CAcert Websiteaccount administrationpublic2023-01-11 14:15
Reporterbdmc Assigned Tobdmc  
PriorityimmediateSeverityblockReproducibilityalways
Status ready to deployResolutionopen 
PlatformDefaultOSanyOS Versionany
Summary0001544: Unable to delete users in support console due to date calculation
DescriptionSupport is unable to delete user records due to a miss-calculation of dates.
Steps To ReproduceThis bug does only occur on MariaDB, not on MySQL, so it can currently only be provoked on test3.cacert.org:

- Find an account which has at least one certificate attached, but all attached certificates are expired more than 90 days ago. Since it looks like accounts can be "deleted" multiple times, try searching for "a20221021.1.1%".
- Using a user with support access, search for the account in "System Admin -> Find User"
- Click on the "Delete Account" link
- Enter a (syntactically correct) new user name. If you did search for a20221021.1.1, use the same name.
- Klick "OK" ==> the system reports "The CCA retention time for at least one certificate is not over. Can't continue."

Note: This procedure won't provoke the bug if the calculation "NOW()-90*86400" accidentially results in a "convertable" integer. I don't think that this is possible, but have not verified this...
Tagssupport
Reviewed byTed
Test Instructions

Relationships

related to 0001545 newbdmc bugs.cacert.org Date Calculation issues throughout system 

Activities

Ted

2022-10-19 14:22

administrator   ~0006138

Is there a bit more info?
What is the error message, what is the mis-calculation?

bdmc

2022-10-19 15:46

developer   ~0006139

As per conversation with Dirk, modified notary.inc.php to use AddDate function instead of manual date calculation.


See Pull Request https://github.com/CAcertOrg/cacert-devel/pull/37

bdmc

2022-10-19 15:48

developer   ~0006140

Ted, as Dirk reported to me, in the current database, the date calculation fails and users can not be removed because that date calculation, in the current database, results in comparing a date with a number, rather than a date with a date.

bdmc

2022-10-19 15:50

developer   ~0006141

I see other places in the code where the "now()" function is used in various ways, so could have similar issues, but this fix was to address this specific issue.

Ted

2022-10-19 18:59

administrator   ~0006142

Can I get a bit more information about this bug, to have some documentation? What exactly did happen?

Was this problem found in a code review or did it happen on the support console? If the latter, what was the error message? Or what was the situation when it was decided that it was a bug? Was it possible to provoke the situation on the test system?

I see your proposed code changes and I have some guesses, but I'd really like to get a more detailed description to verify that I'm on the right track!

egal

2022-10-19 19:34

administrator   ~0006143

i got the information from support/ales, that members can't be deleted despite all certificates had been expired (or revoked) more than 100 days ago.
while tracking it down i found a date-calculation in notary.inc.php
on a local machine (using mysql-version we have on test-server) i created a table with a field containing a date in past and used the calculation we're using in the php-script
on my own servers (mariadb) i tried the same, but got different results compared to mysql

unfortunately the testserver we have are based on mysql using debian8 instead of mariadb on debian11

there is the plan to create new test-servers from scratch with a proper setup and deployment-system using the same setup as we have on webdb1 (sun1) now, but unfortunately these servers are not yet set up ... ;-(

egal

2022-10-19 19:42

administrator   ~0006144

checking the code linked in https://bugs.cacert.org/view.php?id=1544#c6139 i spot an issue:

now()-90*86400 will get a date in the past (substraction)
adddate (now(),90) will get a date in future (addition)

as we need the date in the past (as it should not be possible to delete an user of there had been valid certificates within the last 90 days according to our rules) this needs to be fixed

alkas

2022-10-19 19:47

manager   ~0006145

The error message with no number simply says, that the account cannot be deleted, as any user's cert did not reach the time period since expired/ or is not revoked. That time period is not specified in the message, according to the Dirk's Telegram message it should be 90 days. But there are users with last expired cert in 2012 and so.

alkas

2022-10-19 19:50

manager   ~0006146

Accounts of some users, who never issued a cert, can be deleted with no problems.

Ted

2022-10-19 19:54

administrator   ~0006147

@alkas, so you tried to klick on "Delete Account" after searching an account in the System Admin application, correct?

I cannot provoke the problem on the testsystem https://test.cacert.org/account.php, but from everything I read here the analysis of @egal and @bdmc sounds plausible.

alkas

2022-10-19 20:06

manager   ~0006148

1. copy username & ticket #
2. System Admin, search account
3. Enter ticket #, go
4. click Delete Acc, fill the serial # from Wiki page (a20141024.1), click Yes
5. if the user has any cert issued, error message appears.

bdmc

2022-10-19 20:55

developer   ~0006149

Sorry about that. I was trying to be too quick. I will fix and re-submit.

Ted

2022-10-19 20:58

administrator   ~0006150

@bdmc no need for another patch request because I'll have to create the patch manually anyway (or can anyone tell me how to convert the patch request into a new branch bug-1544 in github?)...

bdmc

2022-10-19 21:05

developer   ~0006151

Last edited: 2022-10-19 21:07

Patch Request updated.

I was working in branch bug-1544 in GitHub, if that helps.

Ted

2022-10-19 22:07

administrator   ~0006152

Last edited: 2022-10-19 22:08

Though I still don't know exactly what happened, the old code will surely not do what it should do (find all certificates which expired later then 90 days before now).

So, I reviewed commit 626cb03b65e939db95fa62af1f04906cbfa6f27a of branch bug-1544 versus 05b03d6cb5196f296abde465e3d2ecf1395bc132 od release branch.

The review is PASSED. Though I'm not 100% sure that it fixes the current problem, it does the calculation that is expected of it.

Ted

2022-10-19 22:11

administrator   ~0006153

I want to do a quick test on the testserver, to make sure that no typos were overlooked, but this will have to wait till tomorrow.

Ted

2022-10-20 20:59

administrator   ~0006154

I was now able to verify the problem on test3 with MariaDB 10.1.48.

Addition of an integer to a datetime results in the datetime being cast to an integer. For example '2022-10-19 12:34:56' is converted into 20.221.019.123.456, then the integer is added or substracted.

When comparing a datetime to an integer, mysql seems to convert the datetime column into an integer. Though the result is not correct, the error is not obvious.

MariaDB, on the other hand, seems to convert the integer into a datetime. If the integer cannot be converted, for example because the final 2 digits are bigger than 60, this conversion results in '0000-00-00 00:00:00'.

So now I'm quite sure that the fix proposed by @bdmc will fix the issue.

Ted

2022-10-20 21:05

administrator   ~0006155

Now I was also able to provoke the problem on https://test3.cacert.org:14943/account.php

Ted

2022-10-20 22:16

administrator   ~0006156

I made a very rough patch on test3, and I cannot provoke the problem anymore.

Unless anyone else sees any problem I'll try to prepare the patch tomorrow evening. Or on Saturday...

Ted

2022-10-23 12:51

administrator   ~0006157

Patch request sent to critical admins
bug-1544.patch (2,039 bytes)   
diff --git a/includes/notary.inc.php b/includes/notary.inc.php
index 3b8e736..a47da52 100644
--- a/includes/notary.inc.php
+++ b/includes/notary.inc.php
@@ -1234,7 +1234,7 @@ function get_user_agreements($memid, $type=null, $active=null){
 		if (0==$cca) {
 			$query = "select 1 from `gpg` where `memid`='$uid' and `expire`>NOW()";
 		}else{
-			$query = "select 1 from `gpg` where `memid`='$uid' and `expire`>(NOW()-90*86400)";
+			$query = "select 1 from `gpg` where `memid`='$uid' and `expire`>( SUBDATE( NOW(), 90 ))";
 		}
 		$res = mysql_query($query);
 		return mysql_num_rows($res) > 0;
@@ -1248,8 +1248,8 @@ function get_user_agreements($memid, $type=null, $active=null){
 			$query1 = "select 1 from `emailcerts` where `memid`='$uid' and `expire`>NOW() and `revoked`<`created`";
 			$query2 = "select 1 from `emailcerts` where `memid`='$uid' and `revoked`>NOW()";
 		}else{
-			$query1 = "select 1 from `emailcerts` where `memid`='$uid' and `expire`>(NOW()-90*86400)  and `revoked`<`created`";
-			$query2 = "select 1 from `emailcerts` where `memid`='$uid' and `revoked`>(NOW()-90*86400)";
+			$query1 = "select 1 from `emailcerts` where `memid`='$uid' and `expire`>( SUBDATE( NOW(), 90 ))  and `revoked`<`created`";
+			$query2 = "select 1 from `emailcerts` where `memid`='$uid' and `revoked`>( SUBDATE( NOW(), 90 ))";
 		}
 		$res = mysql_query($query1);
 		$r1 = mysql_num_rows($res)>0;
@@ -1279,13 +1279,13 @@ function get_user_agreements($memid, $type=null, $active=null){
 				select 1 from `domaincerts` join `domains`
 					on `domaincerts`.`domid` = `domains`.`id`
 				where `domains`.`memid` = '$uid'
-					and `expire`>(NOW()-90*86400)
+					and `expire`>( SUBDATE( NOW(), 90 ))
 					and `revoked`<`created`";
 			$query2 = "
 				select 1 from `domaincerts` join `domains`
 					on `domaincerts`.`domid` = `domains`.`id`
 				where `domains`.`memid` = '$uid'
-					and `revoked`>(NOW()-90*86400)";
+					and `revoked`>( SUBDATE( NOW(), 90 ))";
 		}
 		$res = mysql_query($query1);
 		$r1 = mysql_num_rows($res)>0;
bug-1544.patch (2,039 bytes)   

alkas

2022-11-21 15:24

manager   ~0006158

Is the Ted's previously mentioned patch "bug-1544" in place? I tested it yesterday, unfortunately the same error message appeared!

Issue History

Date Modified Username Field Change
2022-10-19 14:17 bdmc New Issue
2022-10-19 14:17 bdmc Assigned To => bdmc
2022-10-19 14:17 bdmc Tag Attached: support
2022-10-19 14:22 Ted Note Added: 0006138
2022-10-19 15:46 bdmc Note Added: 0006139
2022-10-19 15:48 bdmc Note Added: 0006140
2022-10-19 15:50 bdmc Note Added: 0006141
2022-10-19 18:59 Ted Note Added: 0006142
2022-10-19 19:34 egal Note Added: 0006143
2022-10-19 19:42 egal Note Added: 0006144
2022-10-19 19:47 alkas Note Added: 0006145
2022-10-19 19:50 alkas Note Added: 0006146
2022-10-19 19:54 Ted Note Added: 0006147
2022-10-19 20:06 alkas Note Added: 0006148
2022-10-19 20:55 bdmc Note Added: 0006149
2022-10-19 20:58 Ted Note Added: 0006150
2022-10-19 21:05 bdmc Note Added: 0006151
2022-10-19 21:06 bdmc Note Edited: 0006151
2022-10-19 21:07 bdmc Note Edited: 0006151
2022-10-19 22:07 Ted Note Added: 0006152
2022-10-19 22:08 Ted Note Edited: 0006152
2022-10-19 22:10 Ted Status new => needs review & testing
2022-10-19 22:11 Ted Note Added: 0006153
2022-10-20 20:59 Ted Note Added: 0006154
2022-10-20 21:05 Ted Note Added: 0006155
2022-10-20 22:14 Ted Reviewed by => Ted
2022-10-20 22:16 Ted Status needs review & testing => ready to deploy
2022-10-20 22:16 Ted Note Added: 0006156
2022-10-23 12:30 Ted Summary Unable to delete users due to date calculation => Unable to delete users in support console due to date calculation
2022-10-23 12:30 Ted Reviewed by Ted => Ted
2022-10-23 12:46 Ted Steps to Reproduce Updated
2022-10-23 12:46 Ted Reviewed by Ted => Ted
2022-10-23 12:49 Ted Steps to Reproduce Updated
2022-10-23 12:49 Ted Reviewed by Ted => Ted
2022-10-23 12:49 Ted Steps to Reproduce Updated
2022-10-23 12:49 Ted Reviewed by Ted => Ted
2022-10-23 12:50 Ted Steps to Reproduce Updated
2022-10-23 12:50 Ted Reviewed by Ted => Ted
2022-10-23 12:51 Ted Note Added: 0006157
2022-10-23 12:51 Ted File Added: bug-1544.patch
2022-11-21 15:24 alkas Note Added: 0006158
2023-01-11 14:15 bdmc Relationship added related to 0001545