View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001544 | Main CAcert Website | account administration | public | 2022-10-19 14:17 | 2023-03-01 17:44 |
| Reporter | bdmc | Assigned To | bdmc | ||
| Priority | immediate | Severity | block | Reproducibility | always |
| Status | ready to deploy | Resolution | open | ||
| Platform | Default | OS | any | OS Version | any |
| Summary | 0001544: Unable to delete users in support console due to date calculation | ||||
| Description | Support is unable to delete user records due to a miss-calculation of dates. | ||||
| Steps To Reproduce | This 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... | ||||
| Tags | support | ||||
| Reviewed by | Ted | ||||
| Test Instructions | |||||
|
|
Is there a bit more info? What is the error message, what is the mis-calculation? |
|
|
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 |
|
|
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. |
|
|
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. |
|
|
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! |
|
|
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 ... ;-( |
|
|
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 |
|
|
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. |
|
|
Accounts of some users, who never issued a cert, can be deleted with no problems. |
|
|
@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. |
|
|
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. |
|
|
Sorry about that. I was trying to be too quick. I will fix and re-submit. |
|
|
@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?)... |
|
|
Patch Request updated. I was working in branch bug-1544 in GitHub, if that helps. |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
Now I was also able to provoke the problem on https://test3.cacert.org:14943/account.php |
|
|
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... |
|
|
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;
|
|
|
Is the Ted's previously mentioned patch "bug-1544" in place? I tested it yesterday, unfortunately the same error message appeared! |
| 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 |
