Fix #1023 anonymous users don't get deleted (#1214)

fix(backend): remove temporary user on shutdown or remove

* test(backend): temporary user get removed
* test(auth): check when upgrade
* refactor(frontend): allow ws anonyous on some channels
* refactor(frontend): removed legacy ping backend
* test(volatile): check users are removed

closes issue #1023

lib/Ravada.pm

@@ -161,7 +161,7 @@ sub _init_user_daemon {
 sub _update_user_grants {
     my $self = shift;
     $self->_init_user_daemon();
-    my $sth = $CONNECTOR->dbh->prepare("SELECT id FROM users");
+    my $sth = $CONNECTOR->dbh->prepare("SELECT id FROM users WHERE is_temporary=0");
     my $id;
     $sth->execute;
     $sth->bind_columns(\$id);
@@ -169,7 +169,13 @@ sub _update_user_grants {
         my $user = Ravada::Auth::SQL->search_by_id($id);
         next if $user->name() eq $USER_DAEMON_NAME;
 
-        next if $user->grants();
+        my %grants = $user->grants();
+
+        for my $key (keys %grants) {
+            delete $grants{$key} if !defined $grants{$key};
+        }
+        next if keys %grants;
+
         $USER_DAEMON->grant_user_permissions($user);
         $USER_DAEMON->grant_admin_permissions($user)    if $user->is_admin;
     }
@@ -991,7 +997,7 @@ sub _null_grants($self) {
     $sth->execute;
     my ($count) = $sth->fetchrow;
 
-    exit if !$count && $self->{_null}++;
+    warn "No null grants found" if !$count && $self->{_null_grants}++;
     return $count;
 }
 
@@ -1234,6 +1240,7 @@ sub _upgrade_tables {
         $sth->execute;
     }
     $self->_upgrade_table('users','external_auth','char(32) DEFAULT NULL');
+    $self->_upgrade_table('users','date_created','timestamp DEFAULT CURRENT_TIMESTAMP');
 
     $self->_upgrade_table('networks','requires_password','int(11)');
     $self->_upgrade_table('networks','n_order','int(11) not null default 0');
@@ -3431,15 +3438,19 @@ sub _remove_unnecessary_downs($self, $domain) {
 
 sub _refresh_volatile_domains($self) {
    my $sth = $CONNECTOR->dbh->prepare(
-        "SELECT id, name, id_vm FROM domains WHERE is_volatile=1"
+        "SELECT id, name, id_vm, id_owner FROM domains WHERE is_volatile=1"
     );
     $sth->execute();
-    while ( my ($id_domain, $name, $id_vm) = $sth->fetchrow ) {
+    while ( my ($id_domain, $name, $id_vm, $id_owner) = $sth->fetchrow ) {
         my $domain = Ravada::Domain->open(id => $id_domain, _force => 1);
         if ( !$domain || $domain->status eq 'down' || !$domain->is_active) {
             if ($domain) {
                 $domain->_post_shutdown(user => $USER_DAEMON);
                 $domain->remove($USER_DAEMON);
+            } else {
+                my $sth= $CONNECTOR->dbh->prepare("DELETE FROM users WHERE id=?");
+                $sth->execute($id_owner);
+                $sth->finish;
             }
             my $sth_del = $CONNECTOR->dbh->prepare("DELETE FROM domains WHERE id=?");
             $sth_del->execute($id_domain);
@@ -3487,6 +3498,7 @@ sub _cmd_set_base_vm {
 
 sub _cmd_cleanup($self, $request) {
     $self->_clean_volatile_machines( request => $request);
+    $self->_clean_temporary_users( );
     $self->_clean_requests('cleanup', $request);
     $self->_clean_requests('cleanup', $request,'done');
     $self->_clean_requests('enforce_limits', $request,'done');
@@ -3720,6 +3732,26 @@ sub _enforce_limits_active($self, $request) {
     }
 }
 
+sub _clean_temporary_users($self) {
+    my $sth_users = $CONNECTOR->dbh->prepare(
+        "SELECT u.id, d.id, u.date_created"
+        ." FROM users u LEFT JOIN domains d "
+        ." ON u.id = d.id_owner "
+        ." WHERE u.is_temporary = 1 AND u.date_created < ?"
+    );
+
+    my $sth_del = $CONNECTOR->dbh->prepare(
+        "DELETE FROM users "
+        ." WHERE is_temporary = 1 AND id=?"
+    );
+    my $one_day = _date_now(-24 * 60 * 60);
+    $sth_users->execute( $one_day );
+    while ( my ( $id_user, $id_domain, $date_created ) = $sth_users->fetchrow ) {
+        next if $id_domain;
+        $sth_del->execute($id_user);
+    }
+}
+
 sub _clean_volatile_machines($self, %args) {
     my $request = delete $args{request};
 
@@ -3734,8 +3766,13 @@ sub _clean_volatile_machines($self, %args) {
         );
         if ($domain_real) {
             next if $domain_real->domain && $domain_real->is_active;
-            $domain_real->_post_shutdown();
-            $domain_real->remove($USER_DAEMON);
+            eval { $domain_real->_post_shutdown() };
+            warn $@ if $@;
+            eval { $domain_real->remove($USER_DAEMON) };
+            warn $@ if $@;
+        } elsif ($domain->{id_owner}) {
+            my $sth = $CONNECTOR->dbh->prepare("DELETE FROM users where id=?");
+            $sth->execute($domain->{id_owner});
         }
 
         $sth_remove->execute($domain->{id});

lib/Ravada/Domain.pm

@@ -1450,6 +1450,9 @@ sub _pre_remove_domain($self, $user, @) {
     $self->_remove_iptables()   if $self->is_known();
     $self->shutdown_now($user)  if $self->is_active;
 
+    my $owner;
+    $owner= Ravada::Auth::SQL->search_by_id($self->id_owner)    if $self->is_known();
+    $owner->remove() if $owner && $owner->is_temporary();
 }
 
 # check the node is active
@@ -1492,6 +1495,7 @@ sub _after_remove_domain {
     $self->_remove_volumes_db();
     $self->_remove_bases_vm_db();
     $self->_remove_domain_db();
+
 }
 
 sub _remove_all_volumes($self) {
@@ -2625,34 +2629,20 @@ sub _test_iptables_jump {
 }
 
 
-sub _remove_temporary_machine {
-    my $self = shift;
+sub _remove_temporary_machine($self) {
 
     return if !$self->is_volatile;
 
-    my %args = @_;
-
-    return if !$self->is_known();
-    return if !$self->is_volatile();
-
-    my $user;
-    eval { $user = Ravada::Auth::SQL->search_by_id($self->id_owner) };
-    return if !$user;
-
-    my $req= $args{request};
-        $req->status(
-            "removing"
-            ,"Removing volatile machine ".$self->name)
-                if $req;
+    my $owner;
+    $owner= Ravada::Auth::SQL->search_by_id($self->id_owner)    if $self->is_known();
 
         if ($self->is_removed) {
             $self->remove_disks();
             $self->_after_remove_domain();
-        } else {
-            $self->remove($user)    if $user->is_temporary;
         }
-    $self->remove($user);
+    $self->remove(Ravada::Utils::user_daemon);
 
+    $owner->remove() if $owner && $owner->is_temporary();
 }
 
 sub _post_resume {

lib/Ravada/VM/KVM.pm

@@ -945,7 +945,7 @@ sub _domain_create_from_base {
     confess "argument id_base or base required ".Dumper(\%args)
         if !$args{id_base} && !$args{base};
 
-    die "Domain $args{name} already exists"
+    confess "Domain $args{name} already exists"
         if $self->search_domain($args{name});
 
     my $base = $args{base};

rvd_front.pl

@@ -6,7 +6,7 @@ use locale ':not_characters';
 #####
 use lib 'lib';
 
-use Carp qw(confess);
+use Carp qw(confess cluck);
 use Data::Dumper;
 use Digest::SHA qw(sha256_hex);
 use Hash::Util qw(lock_hash);
@@ -117,6 +117,8 @@ our $SESSION_TIMEOUT = ($CONFIG_FRONT->{session_timeout} or 5 * 60);
 our $SESSION_TIMEOUT_ADMIN = ($CONFIG_FRONT->{session_timeout_admin} or 15 * 60);
 
 my $WS = Ravada::WebSocket->new(ravada => $RAVADA);
+my %ALLOWED_ANONYMOUS_WS = map { $_ => 1 } qw(list_bases_anonymous list_alerts);
+
 init();
 ############################################################################3
 
@@ -154,6 +156,7 @@ hook before_routes => sub {
     # anonymous URLs
     if (($url =~ m{^/machine/(clone|display|info|view)/}
         || $url =~ m{^/(list_bases_anonymous|request/)}i
+        || $url =~ m{^/ws/subscribe}
         ) && !_logged_in($c)) {
         $USER = _anonymous_user($c);
         return if $USER->is_temporary;
@@ -1214,6 +1217,13 @@ websocket '/ws/subscribe' => sub {
     $c->inactivity_timeout( $expiration );
     $c->on(message => sub {
             my ($ws, $channel ) = @_;
+            if (!$USER) {
+                cluck "Warning: USER unknown";
+                return;
+            }
+            return access_denied($c)
+              if !$ALLOWED_ANONYMOUS_WS{$channel} && $USER->is_temporary;
+
             $WS->subscribe( ws => $ws
                 , channel => $channel
                 , login => $USER->name
@@ -2347,10 +2357,9 @@ sub _random_name {
 sub _new_anonymous_user {
     my $c = shift;
 
-    my $name_mojo = reverse($c->signed_cookie('mojolicious'));
-
     my $length = 32;
-    $name_mojo = _random_name($length)    if !$name_mojo;
+    my $cookie = ($c->signed_cookie('mojolicious') or _random_name($length));
+    my $name_mojo = reverse($cookie);
 
     $name_mojo =~ tr/[^a-z][^A-Z][^0-9]/___/c;
 

sql/mysql/users.sql

@@ -7,6 +7,7 @@ CREATE TABLE `users` (
   `is_temporary` integer DEFAULT 0,
   `is_external` integer DEFAULT 0,
   `language` char(3) DEFAULT NULL,
+  `date_created` timestamp DEFAULT CURRENT_TIMESTAMP
   PRIMARY KEY (`id`),
   UNIQUE KEY `name` (`name`)
 )

sql/sqlite/users.sql

@@ -7,5 +7,6 @@ CREATE TABLE `users` (
 ,  `is_temporary` integer DEFAULT 0
 ,  `is_external` integer DEFAULT 0
 ,  `language` char(3) DEFAULT NULL
+,   `date_created` timestamp default CURRENT_TIMESTAMP
 ,  UNIQUE (`name`)
 );

t/vm/v10_volatile.t

@@ -9,6 +9,9 @@ use Data::Dumper;
 use POSIX qw(WNOHANG);
 use Test::More;
 
+no warnings "experimental::signatures";
+use feature qw(signatures);
+
 use_ok('Ravada');
 use_ok('Ravada::Network');
 
@@ -66,65 +69,121 @@ sub allow_anonymous {
     $sth->finish;
 }
 
+sub _cleanup_info($user_id) {
+    my $req = Ravada::Request->cleanup( );
+    wait_request(debug => 0);
+    my $sth = connector->dbh->prepare("SELECT name,date_created FROM users where id=?");
+    $sth->execute($user_id);
+
+    return $sth->fetchrow;
+}
+
+sub test_volatile_cleanup ($base) {
+    return if $base->type ne 'KVM';
+    my $user = Ravada::Auth::SQL::add_user(name => "user_".new_domain_name(), is_temporary => 1);
+    my $user_id = $user->id;
+
+    my ($name, $date_created) = _cleanup_info($user_id);
+    ok($name);
+
+    my $sth_update = connector->dbh->prepare("UPDATE users set date_created=? WHERE id=?");
+    my $date = _date();
+    $sth_update->execute($date,$user_id);
+
+    ($name, $date_created) = _cleanup_info($user_id);
+    ok($name, "date $date_created");
+
+    $sth_update->execute(_date(time - 25 * 3600),$user_id);
+    my $clone = $base->clone(
+        user => $user
+        , name => new_domain_name
+    );
+    $clone->start($user)                if !$clone->is_active;
+
+    ($name, $date_created) = _cleanup_info($user_id);
+    ok($name, "date $date_created");
+
+    my $sth_deldom = connector->dbh->prepare("DELETE FROM domains WHERE id=?");
+    $sth_deldom->execute($clone->id);
+
+    ($name, $date_created) = _cleanup_info($user_id);
+    ok(!$name);
+
+    shutdown_domain_internal($clone);
+}
+
+sub _date($time = time) {
+    my @now = localtime($time);
+    $now[5]+=1900;
+    $now[4]++;
+    for ( 0 .. 4 ) {
+        $now[$_] = "0$now[$_]" if length($now[$_]) < 2;
+    }
+    return "$now[5]-$now[4]-$now[3] $now[2]:$now[1]:$now[0]";
+}
+
 sub test_volatile {
     my ($vm_name, $base) = @_;
 
     my $vm = rvd_back->search_vm($vm_name);
     my $name = new_domain_name();
 
-    {
     my $user_name = "user_".new_domain_name();
-    my $user = Ravada::Auth::SQL::add_user(name => $user_name, is_temporary => 1);
+    my $user_id;
+    {
+        my $user = Ravada::Auth::SQL::add_user(name => $user_name, is_temporary => 1);
+        is($user->is_temporary,1);
+        $user_id = $user->id;
 
-    my $clone = $base->clone(
-          user => $user
-        , name => $name
-    );
-    $clone->start($user)                if !$clone->is_active;
-    is($clone->is_active,1,"[$vm_name] Expecting clone active");
+        my $clone = $base->clone(
+            user => $user
+            , name => $name
+        );
+        $clone->start($user)                if !$clone->is_active;
+        is($clone->is_active,1,"[$vm_name] Expecting clone active");
 
-    like($clone->spice_password,qr{..+},"[$vm_name] ".$clone->name)
+        like($clone->spice_password,qr{..+},"[$vm_name] ".$clone->name)
         if $vm_name eq 'KVM';
 
-    is($clone->is_volatile,1,"[$vm_name] Expecting is_volatile");
+        is($clone->is_volatile,1,"[$vm_name] Expecting is_volatile");
 
-    my $clone2 = rvd_back->search_domain($name);
-    is($clone2->is_volatile,1,"[$vm_name] Expecting is_volatile");
+        my $clone2 = rvd_back->search_domain($name);
+        is($clone2->is_volatile,1,"[$vm_name] Expecting is_volatile");
 
-    my $clone3 = $vm->search_domain($name);
-    is($clone3->is_volatile,1,"[$vm_name] Expecting is_volatile");
+        my $clone3 = $vm->search_domain($name);
+        is($clone3->is_volatile,1,"[$vm_name] Expecting is_volatile");
 
-    my @volumes = $clone->list_volumes();
+        my @volumes = $clone->list_volumes();
 
-    is($clone->is_active, 1);
-    eval { $clone->shutdown_now(user_admin)    if $clone->is_active};
-    is(''.$@,'',"[$vm_name] Expecting no error after shutdown");
+        is($clone->is_active, 1);
+        eval { $clone->shutdown_now(user_admin)    if $clone->is_active};
+        is(''.$@,'',"[$vm_name] Expecting no error after shutdown");
 
-    is($clone->is_active, 0);
-    # test out of the DB
-    my $sth = connector->dbh->prepare("SELECT id,name FROM domains WHERE name=?");
-    $sth->execute($name);
-    my $row = $sth->fetchrow_hashref;
-    ok(!$row,"Expecting no domain info in the DB, found ".Dumper($row))    or exit;
+        is($clone->is_active, 0);
+        # test out of the DB
+        my $sth = connector->dbh->prepare("SELECT id,name FROM domains WHERE name=?");
+        $sth->execute($name);
+        my $row = $sth->fetchrow_hashref;
+        ok(!$row,"Expecting no domain info in the DB, found ".Dumper($row))    or exit;
 
-    # search for the removed domain
-    my $domain2 = $vm->search_domain($name);
-    ok(!$domain2,"[$vm_name] Expecting domain $name removed after shutdown\n"
-        .Dumper($domain2)) or exit;
+        # search for the removed domain
+        my $domain2 = $vm->search_domain($name);
+        ok(!$domain2,"[$vm_name] Expecting domain $name removed after shutdown\n"
+            .Dumper($domain2)) or exit;
 
-    is(rvd_front->domain_exists($name),0,"[$vm_name] Expecting domain removed after shutdown")
-        or exit;
+        is(rvd_front->domain_exists($name),0,"[$vm_name] Expecting domain removed after shutdown")
+            or exit;
 
-    my $user2 = Ravada::Auth::SQL->new(name => $user_name);
-    # TODO
-    # ok(!$user2->id,"Expecting user '$user_name' removed");
-    my $domain_b = rvd_back->search_domain($name);
-    ok(!$domain_b,"[$vm_name] Expecting domain removed after shutdown");
+        my $user2 = Ravada::Auth::SQL->new(name => $user_name);
+        ok(!$user2->id,"Expecting user '$user_name' removed");
 
-    my $domains_f = rvd_front->list_domains();
-    ok(!grep({ $_->{name} eq $name } @$domains_f),"[$vm_name] Expecting $name not listed");
+        my $domain_b = rvd_back->search_domain($name);
+        ok(!$domain_b,"[$vm_name] Expecting domain removed after shutdown");
 
-    $name = undef;
+        my $domains_f = rvd_front->list_domains();
+        ok(!grep({ $_->{name} eq $name } @$domains_f),"[$vm_name] Expecting $name not listed");
+
+        $name = undef;
 
         $vm->refresh_storage_pools();
         $vm->refresh_storage();
@@ -157,6 +216,13 @@ sub test_volatile {
     ok(grep({ $_->{name} eq $name2 } @$domains_nf),"[$vm_name] Expecting $name2 listed");
 
     $clone_normal->remove(user_admin);
+
+    my $clone_removed = rvd_back->search_domain($name);
+    is($clone_removed,undef);
+
+    my $user_removed = Ravada::Auth::SQL->search_by_id($user_id);
+    is($user_removed,undef,"User ".$user_id." should be removed") or exit;
+
 }
 
 # KVM volatiles get auto-removed
@@ -236,6 +302,7 @@ sub test_volatile_auto_kvm {
         ok(! -e $file,"[$vm_name] Expecting volume $file removed");
     }
 
+    $user = Ravada::Auth::SQL::add_user(name => "$user_name.2", is_temporary => 1);
     my $clone2;
     eval {
         $clone2 = $base->clone(
@@ -263,10 +330,29 @@ sub test_volatile_auto_kvm {
     is(keys(%$row),0);
 }
 
+sub test_upgrade {
+    my $user_name = "user_".new_domain_name();
+    my $user = Ravada::Auth::SQL::add_user(name => $user_name, is_temporary => 1);
+
+
+    my $sth = connector->dbh->prepare("DELETE FROM grants_user WHERE id_user=?");
+    $sth->execute($user->id);
+
+    rvd_back->_update_data();
+
+    $sth = connector->dbh->prepare("SELECT id from grants_user WHERE id_user=?");
+    $sth->execute($user->id);
+    my ($found) = $sth->fetchrow;
+
+    is($found,undef);
+
+    $user->remove();
+}
 ################################################################################
 
 clean();
 
+test_upgrade();
 
 for my $vm_name ('Void', 'KVM') {
     my $vm = rvd_back->search_vm($vm_name);
@@ -292,6 +378,7 @@ for my $vm_name ('Void', 'KVM') {
         $base->is_public(1);
         allow_anonymous($base);
 
+        test_volatile_cleanup($base);
         test_volatile($vm_name, $base);
         test_volatile_auto_kvm($vm_name, $base) if $vm_name eq'KVM';