[1/3] testbot/Users: Fix the deletion of user accounts.
Francois Gouget
fgouget at codeweavers.com
Thu Apr 4 05:14:10 CDT 2013
Users cannot be deleted directly because the Jobs table hold references to them. So mark the accounts as deleted and let the Janitor.pl script delete them when they are not referenced anymore.
To do so, transform the 'Active' boolean field into a 'Status' enumeration that can be 'active' (previously Active=Y), 'disabled' (Active=N) or 'deleted'.
The Sessions table also contains references to the Users, but these should be deleted together with the user account, which will forcefully log out the user as a side effect.
---
*NOTE*:
- This patch requires a database update. See testbot/ddl/update24.sql.
- The WineTestBot server will need to be restarted to deal with the new
VM status.
- The web server may need to be restarted to pick up the updates to the
Jobs & co modules (but then it may also pick up the changes
automatically).
This is the patch 8/10 that I could not send yesterday because my ISP
rejected it as spam!
testbot/README | 4 +-
testbot/bin/Janitor.pl | 63 +++++++++++++++++++++++++------
testbot/ddl/update25.sql | 17 +++++++++
testbot/ddl/winetestbot.sql | 6 +--
testbot/doc/winetestbot-schema.dia | 4 +-
testbot/lib/WineTestBot/CGI/Sessions.pm | 11 +++++-
testbot/lib/WineTestBot/Users.pm | 17 +++++----
testbot/web/Register.pl | 2 +-
testbot/web/admin/UsersList.pl | 4 +-
9 files changed, 98 insertions(+), 30 deletions(-)
create mode 100644 testbot/ddl/update25.sql
diff --git a/testbot/README b/testbot/README
index f15f82a..7ddaba9 100644
--- a/testbot/README
+++ b/testbot/README
@@ -87,8 +87,8 @@ the account and make it usable.
Notes:
-* Existing accounts can be suspended by unchecking the 'active' field. This
- prevents the user from logging in and changing the password.
+* Existing accounts can be disabled by setting their status field to
+ 'disabled'. This prevents the user from logging in and changing the password.
* Only accounts that have the 'wine-devel' role can submit jobs. So
administrators who also want to submit jobs should add it to their account
(see the doc/INSTALL.txt file).
diff --git a/testbot/bin/Janitor.pl b/testbot/bin/Janitor.pl
index 0c8b77f..b195dff 100755
--- a/testbot/bin/Janitor.pl
+++ b/testbot/bin/Janitor.pl
@@ -41,6 +41,8 @@ use WineTestBot::Jobs;
use WineTestBot::Log;
use WineTestBot::Patches;
use WineTestBot::PendingPatchSets;
+use WineTestBot::CGI::Sessions;
+use WineTestBot::Users;
use WineTestBot::VMs;
@@ -143,35 +145,72 @@ if ($JobArchiveDays != 0)
$Jobs = undef;
}
-# Purge deleted VMs if they are not referenced anymore
+# Purge the deleted users and VMs if they are not referenced anymore
my $VMs = CreateVMs();
$VMs->AddFilter("Role", ["deleted"]);
-my %DeleteList;
-map { $DeleteList{$_} = 1 } @{$VMs->GetKeys()};
+my %DeletedVMs;
+map { $DeletedVMs{$_} = 1 } @{$VMs->GetKeys()};
-if (%DeleteList)
+my $Users = CreateUsers();
+$Users->AddFilter("Status", ["deleted"]);
+my %DeletedUsers;
+map { $DeletedUsers{$_} = 1 } @{$Users->GetKeys()};
+
+if (%DeletedUsers or %DeletedVMs)
{
foreach my $Job (@{CreateJobs()->GetItems()})
{
- foreach my $Step (@{$Job->Steps->GetItems()})
+ if (exists $DeletedUsers{$Job->User->Name})
{
- foreach my $Task (@{$Step->Tasks->GetItems()})
+ LogMsg "Keeping the ", $Job->User->Name, " account for job ", $Job->Id, "\n";
+ delete $DeletedUsers{$Job->User->Name};
+ }
+
+ if (%DeletedVMs)
+ {
+ foreach my $Step (@{$Job->Steps->GetItems()})
{
- if (exists $DeleteList{$Task->VM->Name})
+ foreach my $Task (@{$Step->Tasks->GetItems()})
{
- LogMsg "Keeping the ", $Task->VM->Name, " VM for task ", join("/", @{$Task->GetMasterKey()}), "\n";
- delete $DeleteList{$Task->VM->Name};
+ if (exists $DeletedVMs{$Task->VM->Name})
+ {
+ LogMsg "Keeping the ", $Task->VM->Name, " VM for task ", join("/", @{$Task->GetMasterKey()}), "\n";
+ delete $DeletedVMs{$Task->VM->Name};
+ }
}
}
}
}
- foreach my $VMKey (keys %DeleteList)
+
+ if (%DeletedUsers)
+ {
+ foreach my $UserName (keys %DeletedUsers)
+ {
+ my $User = $Users->GetItem($UserName);
+ DeleteSessions($User);
+ my $ErrMessage = $Users->DeleteItem($User);
+ if (defined $ErrMessage)
+ {
+ LogMsg "Unable to delete the $UserName account: $ErrMessage\n";
+ }
+ else
+ {
+ LogMsg "Deleted the $UserName account\n";
+ }
+ }
+ }
+
+ foreach my $VMKey (keys %DeletedVMs)
{
my $VM = $VMs->GetItem($VMKey);
my $ErrMessage = $VMs->DeleteItem($VM);
- if (!defined $ErrMessage)
+ if (defined $ErrMessage)
+ {
+ LogMsg "Unable to delete the $VMKey VM: $ErrMessage\n";
+ }
+ else
{
- LogMsg "Deleted the ", $VM->Name, " VM\n";
+ LogMsg "Deleted the $VMKey VM\n";
}
}
}
diff --git a/testbot/ddl/update25.sql b/testbot/ddl/update25.sql
new file mode 100644
index 0000000..fd608f5
--- /dev/null
+++ b/testbot/ddl/update25.sql
@@ -0,0 +1,17 @@
+USE winetestbot;
+
+ALTER TABLE Users
+ ADD Status ENUM('active', 'disabled', 'deleted') NULL
+ AFTER Active;
+
+UPDATE Users
+ SET Status = 'active'
+ WHERE Active = 'Y';
+
+UPDATE Users
+ SET Status = 'disabled'
+ WHERE Active = 'N';
+
+ALTER TABLE Users
+ MODIFY Status ENUM('active', 'disabled', 'deleted') NOT NULL,
+ DROP Active;
diff --git a/testbot/ddl/winetestbot.sql b/testbot/ddl/winetestbot.sql
index c8820cc..e46d5e0 100644
--- a/testbot/ddl/winetestbot.sql
+++ b/testbot/ddl/winetestbot.sql
@@ -7,7 +7,7 @@ CREATE TABLE Users
Name VARCHAR(40) NOT NULL,
EMail VARCHAR(40) NOT NULL,
Password CHAR(49) NOT NULL,
- Active ENUM('Y', 'N') NOT NULL,
+ Status ENUM('active', 'disabled', 'deleted') NOT NULL,
RealName VARCHAR(40) NULL,
ResetCode CHAR(32) NULL,
PRIMARY KEY(Name)
@@ -161,8 +161,8 @@ ENGINE=InnoDB DEFAULT CHARSET=utf8;
INSERT INTO Roles (Name, IsDefaultRole) VALUES('admin', 'N');
INSERT INTO Roles (Name, IsDefaultRole) VALUES('wine-devel', 'Y');
-INSERT INTO Users (Name, EMail, Password, Active, RealName)
- VALUES('batch', '/dev/null', '*', 'Y', 'Batch user for internal jobs');
+INSERT INTO Users (Name, EMail, Password, Status, RealName)
+ VALUES('batch', '/dev/null', '*', 'disabled', 'Batch user for internal jobs');
INSERT INTO Branches (Name, IsDefault) VALUES('master', 'Y');
diff --git a/testbot/doc/winetestbot-schema.dia b/testbot/doc/winetestbot-schema.dia
index 2418c8b..326b3c8 100644
--- a/testbot/doc/winetestbot-schema.dia
+++ b/testbot/doc/winetestbot-schema.dia
@@ -193,10 +193,10 @@
</dia:composite>
<dia:composite type="table_attribute">
<dia:attribute name="name">
- <dia:string>#Active#</dia:string>
+ <dia:string>#Status#</dia:string>
</dia:attribute>
<dia:attribute name="type">
- <dia:string>#BOOL#</dia:string>
+ <dia:string>#ENUM#</dia:string>
</dia:attribute>
<dia:attribute name="comment">
<dia:string>##</dia:string>
diff --git a/testbot/lib/WineTestBot/CGI/Sessions.pm b/testbot/lib/WineTestBot/CGI/Sessions.pm
index 97e33b5..b782cf4 100644
--- a/testbot/lib/WineTestBot/CGI/Sessions.pm
+++ b/testbot/lib/WineTestBot/CGI/Sessions.pm
@@ -57,7 +57,7 @@ use vars qw(@ISA @EXPORT @PropertyDescriptors);
require Exporter;
@ISA = qw(WineTestBot::WineTestBotCollection Exporter);
- at EXPORT = qw(&CreateSessions &NewSession);
+ at EXPORT = qw(&CreateSessions &DeleteSessions &NewSession);
BEGIN
{
@@ -85,6 +85,15 @@ sub DeleteNonPermanentSessions($)
map { $Sessions->DeleteItem($_); } @{$Sessions->GetItems()};
}
+sub DeleteSessions($)
+{
+ my $User = $_[0];
+
+ my $Sessions = CreateSessions();
+ $Sessions->AddFilter("User", [$User]);
+ map { $Sessions->DeleteItem($_); } @{$Sessions->GetItems()};
+}
+
sub NewSession
{
my $self = shift;
diff --git a/testbot/lib/WineTestBot/Users.pm b/testbot/lib/WineTestBot/Users.pm
index a163a17..a0034ee 100644
--- a/testbot/lib/WineTestBot/Users.pm
+++ b/testbot/lib/WineTestBot/Users.pm
@@ -42,7 +42,7 @@ sub InitializeNew
{
my $self = shift;
- $self->Active(1);
+ $self->Status('active');
$self->Password("*");
}
@@ -89,7 +89,7 @@ sub WaitingForApproval
{
my $self = shift;
- return $self->Active && !$self->Activated() && !$self->ResetCode;
+ return $self->Status eq 'active' && !$self->Activated() && !$self->ResetCode;
}
sub GenerateResetCode
@@ -162,9 +162,9 @@ sub SendResetCode
{
my $self = shift;
- if (! $self->Active)
+ if ($self->Status ne 'active')
{
- return "This account has been suspended";
+ return "This account has been " . $self->Status;
}
if ($self->WaitingForApproval())
{
@@ -243,9 +243,9 @@ sub Authenticate
return ("Unknown username or incorrect password", undef);
}
- if (! $self->Active)
+ if ($self->Status ne 'active')
{
- return ("This account has been suspended", undef);
+ return ("This account has been " . $self->Status, undef);
}
return (undef, $self);
@@ -258,7 +258,7 @@ sub FromLDAP
$self->Name($UserName);
$self->Password("*");
- $self->Active("Y");
+ $self->Status('active');
my $SearchFilter = $LDAPSearchFilter;
$SearchFilter =~ s/%USERNAME%/$UserName/;
@@ -300,6 +300,7 @@ WineTestBot::Users - A collection of WineTestBot::User objects
use Net::LDAP;
use ObjectModel::BasicPropertyDescriptor;
+use ObjectModel::EnumPropertyDescriptor;
use ObjectModel::DetailrefPropertyDescriptor;
use ObjectModel::PropertyDescriptor;
use WineTestBot::Config;
@@ -317,7 +318,7 @@ BEGIN
@PropertyDescriptors = (
CreateBasicPropertyDescriptor("Name", "Username", 1, 1, "A", 40),
CreateBasicPropertyDescriptor("EMail", "EMail", !1, 1, "A", 40),
- CreateBasicPropertyDescriptor("Active", "Active", !1, 1, "B", 1),
+ CreateEnumPropertyDescriptor("Status", "Status", !1, 1, ['active', 'disabled', 'deleted']),
CreateBasicPropertyDescriptor("Password", "Password", !1, 1, "A", 49),
CreateBasicPropertyDescriptor("RealName", "Real name", !1, !1, "A", 40),
CreateBasicPropertyDescriptor("ResetCode", "Password reset code", !1, !1, "A", 32),
diff --git a/testbot/web/Register.pl b/testbot/web/Register.pl
index 766d363..d5eb4cd 100644
--- a/testbot/web/Register.pl
+++ b/testbot/web/Register.pl
@@ -78,7 +78,7 @@ sub GenerateFields
{
my $self = shift;
- print "<div><input type='hidden' name='Active' value='Y'></div>\n";
+ print "<div><input type='hidden' name='Status' value='active'></div>\n";
$self->SUPER::GenerateFields();
print "<div class='DetailProperty'><label>Remarks</label><textarea name='Remarks' cols='40' rows='4'></textarea></div>\n";
$self->GenerateRequiredLegend();
diff --git a/testbot/web/admin/UsersList.pl b/testbot/web/admin/UsersList.pl
index 040fdf4..4971f99 100644
--- a/testbot/web/admin/UsersList.pl
+++ b/testbot/web/admin/UsersList.pl
@@ -1,6 +1,7 @@
# User list page
#
# Copyright 2009 Ge van Geldorp
+# Copyright 2013 Francois Gouget
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
@@ -51,7 +52,7 @@ sub DisplayProperty
my $PropertyName = $PropertyDescriptor->GetName();
return $PropertyName eq "Name" || $PropertyName eq "EMail" ||
- $PropertyName eq "Active" || $PropertyName eq "RealName";
+ $PropertyName eq "Status" || $PropertyName eq "RealName";
}
sub GetActions
@@ -61,6 +62,7 @@ sub GetActions
if (defined($LDAPServer))
{
+ # LDAP accounts cannot be deleted
return [];
}
--
1.7.10.4
More information about the wine-patches
mailing list