From f819b178c5c1cb8adda0182c610e5c52fad8bea7 Mon Sep 17 00:00:00 2001
From: Michael Orlitzky <michael@orlitzky.com>
Date: Sun, 1 Nov 2015 21:35:57 -0500
Subject: [PATCH] Overhaul everything to get consistent error reports.

---
 bin/mailshears                    | 44 +++++++++++++++++----
 doc/TODO                          | 41 ++++++--------------
 lib/common/agendav_plugin.rb      | 17 +++------
 lib/common/davical_plugin.rb      |  7 ++--
 lib/common/domain.rb              | 23 +++++++++++
 lib/common/dovecot_plugin.rb      | 55 +++++++++------------------
 lib/common/errors.rb              | 10 ++++-
 lib/common/plugin.rb              | 32 ++++++++++++----
 lib/common/postfixadmin_plugin.rb | 14 ++++---
 lib/common/roundcube_plugin.rb    |  7 ++--
 lib/common/user.rb                | 63 +++++++++++++++++++++++++++++++
 lib/mv/mv_dummy_runner.rb         | 10 +++--
 lib/mv/mv_plugin.rb               |  2 +-
 lib/mv/mv_runner.rb               | 58 +++++++++++++++++-----------
 lib/mv/plugins/agendav.rb         | 13 ++++---
 lib/mv/plugins/davical.rb         | 18 +++++----
 lib/mv/plugins/dovecot.rb         | 57 ++++++++++++++++------------
 lib/mv/plugins/postfixadmin.rb    | 29 +++++++-------
 lib/mv/plugins/roundcube.rb       | 14 ++++---
 lib/prune/prune_dummy_runner.rb   | 18 +++------
 lib/prune/prune_runner.rb         | 21 +++--------
 lib/rm/plugins/agendav.rb         |  4 +-
 lib/rm/plugins/davical.rb         |  4 +-
 lib/rm/plugins/dovecot.rb         | 12 +++++-
 lib/rm/plugins/postfixadmin.rb    | 10 ++---
 lib/rm/plugins/roundcube.rb       |  2 +-
 lib/rm/rm_dummy_runner.rb         | 17 ++++++---
 lib/rm/rm_plugin.rb               | 18 +++++++--
 lib/rm/rm_runner.rb               | 51 +++++++++++--------------
 29 files changed, 403 insertions(+), 268 deletions(-)
 create mode 100644 lib/common/domain.rb
 create mode 100644 lib/common/user.rb

diff --git a/bin/mailshears b/bin/mailshears
index 4d69f8f..2c91f17 100755
--- a/bin/mailshears
+++ b/bin/mailshears
@@ -30,16 +30,27 @@ end
 require 'mailshears'
 
 # Since we removed both the executable name and the mode name (if it
-# existed) from ARGV, what remains should be the required
-# arguments.
-if (mode == :prune and (ARGV.length() != 0)) or
-    (mode == :rm and (ARGV.length() < 1))   or
-    (mode == :mv and (ARGV.length() != 2))   then
-  puts "ERROR: missing (or extra) command-line arguments."
+# existed) from ARGV, what remains should be the required arguments.
+# Figure out if we have the wrong number of arguments, and store the
+# associated error message in args_error_message if we do.
+args_error_message = nil
+if mode == :prune and ARGV.length() != 0 then
+  args_error_message = "ERROR: prune mode takes no additional arguments."
+elsif mode == :rm and ARGV.length() < 1 then
+  args_error_message = "ERROR: rm mode takes two or more user arguments."
+elsif mode == :mv and ARGV.length() != 2 then
+  args_error_message = "ERROR: mv mode takes exactly two user arguments."
+end
+
+# If we got the wrong number of arguments, we'll have some error
+# message. Report it and quit.
+if not args_error_message.nil? then
+  STDERR.puts args_error_message
   puts "Usage: #{usage}"
   Kernel.exit(ExitCodes::BAD_COMMAND_LINE)
 end
 
+
 cfg = Configuration.new()
 
 # Load each of the plugins that we'll need.
@@ -84,15 +95,32 @@ else
   plugin_module = PrunePlugin
 end
 
-
 # Buffer the output so that we can avoid printing the informational
 # header when no plugins produce output.
 require 'stringio'
 output_buffer = StringIO.new()
 $stdout = output_buffer
 
+# Parse the remaining arguments as User/Domain objects. If we get some
+# other argument that isn't one of those, it's an error.
+parsed_args = []
+ARGV.each do |arg|
+  begin
+    u = User.new(arg)
+    parsed_args << u
+  rescue InvalidUserError
+    begin
+      d = Domain.new(arg)
+      parsed_args << d
+    rescue InvalidDomainError
+      STDERR.puts "ERROR: invalid user/domain argument #{arg}"
+      Kernel.exit(ExitCodes::BAD_COMMAND_LINE)
+    end
+  end
+end
+
 begin
-  plugin_module.run(cfg, *ARGV)
+  plugin_module.run(cfg, *parsed_args)
 ensure
   # Now restore stdout, and print the header plus whatever the plugins
   # produced if they produced anything. If they didn't produce any
diff --git a/doc/TODO b/doc/TODO
index 8482260..3a2ebfe 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -1,29 +1,12 @@
-* Error reporting sucks, and when a domain or user doesn't exist we
-  should be able to say so. The describe_domain/user functions
-  should also work better. For plugins that don't implement domains,
-  we can return a (count of?) list of users, or fall back to the
-  user deletion descriptions.
-
-* Error reporting is inconsistent. We know what goes wrong, and then
-  errors bubble up, but where do they get reported? Are they fatal or
-  informational? Do we want later plugins to run of earlier ones
-  failed?
-
-* Implement "mv".
-
-  Design:
-
-  This should only work from a domain that exists in postfixadmin to a
-  domain that already exists in postfixadmin. If the target domain
-  does not exist, we should error out as soon as possible.
-
-  Once we're sure that the target domain does exist, we can hand off
-  the "mv" operation to the plugins. Some of them might have nothing
-  to do -- that's fine.
-
-  MAKE SURE WE DON'T OVERWRITE AN EXISTING USER!
-
-* Add convenience methods (e.g. user_exists, domain_exists) that we
-  can use in the tests to check results.
-
-* Potentially add OpenDKIM support.
+* There is essentially no error handling. We report errors, but we
+  don't fail when we see one. The main reason for this is that we
+  don't know when each plugin will be run. If the first plugin
+  encounters an error, we could quit right there. But what if the
+  third one fails after the first two succeed? We would need some kind
+  of rollback mechanism.
+
+  For "mv", a rollback is conceivable. But with "rm", there's no going
+  back. Maybe relying on the user to interpret the output and go
+  fix stuff himself is the best we can do?
+
+* Add OpenDKIM support.
diff --git a/lib/common/agendav_plugin.rb b/lib/common/agendav_plugin.rb
index ae9b11c..fc19d3d 100644
--- a/lib/common/agendav_plugin.rb
+++ b/lib/common/agendav_plugin.rb
@@ -1,4 +1,5 @@
 require 'common/plugin'
+require 'common/user'
 
 module AgendavPlugin
   # Code that all Agendav plugins (Prune, Rm, Mv...) will
@@ -18,17 +19,11 @@ module AgendavPlugin
 
 
   def describe_domain(domain)
-    # AgenDAV doesn't have a concept of domains.
-    return domain
+    return domain.to_s()
   end
 
-
   def describe_user(user)
-    if self.user_exists(user)
-      return "Username: #{user}"
-    else
-      return 'User not found'
-    end
+    return user.to_s()
   end
 
 
@@ -37,7 +32,7 @@ module AgendavPlugin
     # Produce a list of AgenDAV users. This is public because it's
     # useful in testing.
     #
-    usernames = []
+    users = []
 
     # Just assume PostgreSQL for now.
     begin
@@ -54,7 +49,7 @@ module AgendavPlugin
       sql_query += '(SELECT user_from FROM shared);'
 
       connection.query(sql_query) do |result|
-        usernames = result.field_values('username')
+        users = result.field_values('username')
       end
 
       connection.close()
@@ -63,7 +58,7 @@ module AgendavPlugin
       raise DatabaseError.new(e)
     end
 
-    return usernames
+    return users.map{ |u| User.new(u) }
   end
 
 end
diff --git a/lib/common/davical_plugin.rb b/lib/common/davical_plugin.rb
index 8c9646f..557c1dd 100644
--- a/lib/common/davical_plugin.rb
+++ b/lib/common/davical_plugin.rb
@@ -1,4 +1,5 @@
 require 'common/plugin'
+require 'common/user'
 
 module DavicalPlugin
   # Code that all Davical plugins (Prune, Rm, Mv...) will share.  That
@@ -18,7 +19,7 @@ module DavicalPlugin
 
   def describe_domain(domain)
     # DAViCal doesn't have a concept of domains.
-    return domain
+    return domain.to_s()
   end
 
 
@@ -62,7 +63,7 @@ module DavicalPlugin
       raise DatabaseError.new(e)
     end
 
-    return usernames
+    return usernames.map{ |u| User.new(u) }
   end
 
 
@@ -85,7 +86,7 @@ module DavicalPlugin
       sql_query += "      ON principal.user_no = usr.user_no) "
       sql_query += "WHERE usr.username = $1;"
 
-      connection.query(sql_query, [user]) do |result|
+      connection.query(sql_query, [user.to_s()]) do |result|
         if result.num_tuples > 0
           principal_id = result[0]['principal_id']
         end
diff --git a/lib/common/domain.rb b/lib/common/domain.rb
new file mode 100644
index 0000000..5fb345b
--- /dev/null
+++ b/lib/common/domain.rb
@@ -0,0 +1,23 @@
+require 'common/errors'
+
+class Domain
+
+  @domain = nil
+
+  def initialize(domain)
+    if domain.empty? then
+      msg = "domain cannot be empty"
+      raise InvalidDomainError.new(msg)
+    end
+
+    @domain = domain
+  end
+
+  def to_s()
+    return @domain
+  end
+
+  def ==(other)
+    return self.to_s() == other.to_s()
+  end
+end
diff --git a/lib/common/dovecot_plugin.rb b/lib/common/dovecot_plugin.rb
index dfd8749..4718483 100644
--- a/lib/common/dovecot_plugin.rb
+++ b/lib/common/dovecot_plugin.rb
@@ -1,5 +1,8 @@
-require 'common/plugin'
+require 'common/domain'
 require 'common/filesystem'
+require 'common/plugin'
+require 'common/user'
+
 
 module DovecotPlugin
   # Code that all Dovecot plugins (Prune, Rm, Mv...) will
@@ -29,55 +32,31 @@ module DovecotPlugin
   end
 
 
+  def domain_exists(domain)
+    domains = list_domains()
+    return domains.include?(domain)
+  end
+
+
   protected;
 
   def get_domain_path(domain)
     # Return the filesystem path for the given domain.
     # That is, the directory where its mail is stored.
     # Only works if the domain directory exists!
-    domain_path = File.join(@domain_root, domain)
-
-    if File.directory?(domain_path)
-      return domain_path
-    else
-      raise NonexistentDomainError.new(domain)
-    end
+    return File.join(@domain_root, domain.to_s())
   end
 
 
-  def get_user_path(user, existence_check = true)
-    # Return the filesystem path of this user's mailbox. With
-    # existence_check = true, it only works if the user exists!
-    # We always require that the domain exists.
-    if not user.include?('@')
-      msg = "#{user}: Users must contain an '@' symbol."
-      raise InvalidUserError.new(msg)
-    end
-
-    user_parts = user.split('@')
-    local_part = user_parts[0]
-    domain_part = user_parts[1]
-
-    begin
-      domain_path = get_domain_path(domain_part)
-    rescue NonexistentDomainError
-      raise NonexistentUserError.new(user)
-    end
-
-    user_path = File.join(domain_path, local_part)
-
-    return user_path if not existence_check
-
-    if File.directory?(user_path)
-      return user_path
-    else
-      raise NonexistentUserError.new(user)
-    end
+  def get_user_path(user)
+    # Return the filesystem path of this user's mailbox.
+    domain_path = get_domain_path(user.domain())
+    return File.join(domain_path, user.localpart())
   end
 
 
   def list_domains()
-    return Filesystem.get_subdirs(@domain_root)
+    return Filesystem.get_subdirs(@domain_root).map{ |d| Domain.new(d) }
   end
 
   def list_domains_users(domains)
@@ -92,7 +71,7 @@ module DovecotPlugin
         usernames = Filesystem.get_subdirs(domain_path)
 
         usernames.each do |username|
-          users << "#{username}@#{domain}"
+          users << User.new("#{username}@#{domain}")
         end
       rescue NonexistentDomainError
         # Party hard.
diff --git a/lib/common/errors.rb b/lib/common/errors.rb
index bcae691..7a2fbba 100644
--- a/lib/common/errors.rb
+++ b/lib/common/errors.rb
@@ -8,12 +8,20 @@ end
 class InvalidUserError < StandardError
 end
 
+# Domain is syntactically invalid.
+class InvalidDomainError < StandardError
+end
+
 
 # Used to indicate that an user does not exist.
 class NonexistentUserError < StandardError
 end
 
-
 # Used to indicate that a domain does not exist.
 class NonexistentDomainError < StandardError
 end
+
+
+# When you try to rename a user on top of one that already exists.
+class UserAlreadyExistsError < StandardError
+end
diff --git a/lib/common/plugin.rb b/lib/common/plugin.rb
index 272f0df..c6f750b 100644
--- a/lib/common/plugin.rb
+++ b/lib/common/plugin.rb
@@ -1,3 +1,6 @@
+require 'common/domain'
+require 'common/user'
+
 # All plugins should include this module. It defines the basic
 # operations that all plugins are supposed to support.
 module Plugin
@@ -26,7 +29,7 @@ module Plugin
     end
 
     def dummy_runner()
-      # The RummyRunner associated with this plugin.
+      # The DummyRunner associated with this plugin.
       raise NotImplementedError
     end
 
@@ -48,6 +51,18 @@ module Plugin
     end
   end
 
+  def describe(target)
+    # A generic version of describe_user/describe_domain that
+    # dispatches base on the class of the target.
+    if target.is_a?(User)
+      return describe_user(target)
+    elsif target.is_a?(Domain)
+      return describe_domain(target)
+    else
+      raise NotImplementedError
+    end
+  end
+
   def describe_domain(domain)
     # Provide a "description" of the domain. This is output along
     # with the domain name and can be anything of use to the system
@@ -67,27 +82,28 @@ module Plugin
     raise NotImplementedError
   end
 
-  def user_exists(username)
+  def user_exists(user)
     # Does the given username exist for this plugin? We use a naive
     # implementation here based on list_users() which is required to
     # exist above. Plugins can override this with something fast.
     users = list_users()
-    return users.include?(username)
+    return users.include?(user)
   end
 
   def list_domains_users(domains)
-    # Get all usernames belonging to the given domains. If a username
-    # ends in @example.com, it belongs to the domain example.com
+    # Get all users belonging to the given domains. If a user has
+    # domainpart "example.com" then it belongs to the domain
+    # "example.com".
     #
     # This uses a naive loop, but relies only on the existence of a
     # list_users() method which is guaranteed above. More efficient
     # implementations can usually be made within the plugin.
     domains_users = []
 
-    usernames = list_users();
+    users = list_users();
     domains.each do |d|
-      matches = usernames.select do |username|
-        username =~ /@#{d}$/
+      matches = users.select do |user|
+        user.domainpart() == d.to_s()
       end
 
       domains_users += matches
diff --git a/lib/common/postfixadmin_plugin.rb b/lib/common/postfixadmin_plugin.rb
index 97d59f2..60880d9 100644
--- a/lib/common/postfixadmin_plugin.rb
+++ b/lib/common/postfixadmin_plugin.rb
@@ -1,4 +1,6 @@
+require 'common/domain'
 require 'common/plugin'
+require 'common/user'
 require 'pg'
 
 module PostfixadminPlugin
@@ -19,13 +21,13 @@ module PostfixadminPlugin
 
   def describe_user(user)
     # There's no other unique identifier in PostfixAdmin
-    return user
+    return user.to_s()
   end
 
 
   def describe_domain(domain)
     # There's no other unique identifier in PostfixAdmin
-    return domain
+    return domain.to_s()
   end
 
 
@@ -53,7 +55,7 @@ module PostfixadminPlugin
       raise DatabaseError.new(e)
     end
 
-    return domains
+    return domains.map{ |d| Domain.new(d) }
   end
 
 
@@ -89,7 +91,7 @@ module PostfixadminPlugin
       raise DatabaseError.new(e)
     end
 
-    return users
+    return users.map{ |u| User.new(u) }
   end
 
 
@@ -108,7 +110,7 @@ module PostfixadminPlugin
 
       sql_query  = 'SELECT username FROM mailbox WHERE domain IN $1;'
 
-      connection.query(sql_query, [domains]) do |result|
+      connection.query(sql_query, domains.map{|d| d.to_s()}) do |result|
         usernames = result.field_values('username')
       end
 
@@ -118,7 +120,7 @@ module PostfixadminPlugin
       raise DatabaseError.new(e)
     end
 
-    return usernames
+    return usernames.map{ |u| User.new(u) }
   end
 
 
diff --git a/lib/common/roundcube_plugin.rb b/lib/common/roundcube_plugin.rb
index 1f9805a..b1c74a6 100644
--- a/lib/common/roundcube_plugin.rb
+++ b/lib/common/roundcube_plugin.rb
@@ -1,4 +1,5 @@
 require 'common/plugin'
+require 'common/user'
 
 module RoundcubePlugin
   # Code that all Roundcube plugins (Prune, Rm, Mv...) will share.
@@ -18,7 +19,7 @@ module RoundcubePlugin
 
   def describe_domain(domain)
     # Roundcube doesn't have a concept of domains.
-    return domain
+    return domain.to_s()
   end
 
   def describe_user(user)
@@ -58,7 +59,7 @@ module RoundcubePlugin
       raise DatabaseError.new(e)
     end
 
-    return usernames
+    return usernames.map{ |u| User.new(u) }
   end
 
   protected;
@@ -77,7 +78,7 @@ module RoundcubePlugin
 
       sql_query = "SELECT user_id FROM users WHERE username = $1;"
 
-      connection.query(sql_query, [user]) do |result|
+      connection.query(sql_query, [user.to_s()]) do |result|
         if result.num_tuples > 0
           user_id = result[0]['user_id']
         end
diff --git a/lib/common/user.rb b/lib/common/user.rb
new file mode 100644
index 0000000..bb239d1
--- /dev/null
+++ b/lib/common/user.rb
@@ -0,0 +1,63 @@
+require 'common/domain'
+require 'common/errors'
+
+class User
+  # A class representing a syntactically valid user; that is, an email
+  # address. Once constructed, you can be sure that it's valid.
+
+  @localpart = nil
+  @domain = nil
+
+  def domain()
+    return @domain
+  end
+
+  def domainpart()
+    return @domain.to_s()
+  end
+
+  def initialize(username)
+    # Initialize this user object, but throw an error if either the
+    # localpart or domainpart are invalid. The one argument is an
+    # email address string.
+    if not username.is_a?(String)
+      msg =  'username must be a String '
+      msg += "but a #{username.class.to_s()} was given"
+      raise InvalidUserError.new(msg)
+    end
+
+    parts = username.split('@')
+
+    if parts.length() < 2 then
+      msg = "the username #{username} does not contain an '@' symbol"
+      raise InvalidUserError.new(msg)
+    end
+
+    localpart = parts[0]
+
+    if localpart.length() > 64 then
+      msg = "the local part of #{username} cannot have more than 64 characters"
+      raise InvalidUserError(msg)
+    end
+
+    if localpart.empty? then
+      msg = "the local part of #{username} cannot be empty"
+      raise InvalidUserError.new(msg)
+    end
+
+    @localpart = localpart
+    @domain = Domain.new(parts[1])
+  end
+
+  def localpart()
+    return @localpart
+  end
+
+  def to_s()
+    return @localpart + '@' + @domain.to_s()
+  end
+
+  def ==(other)
+    return self.to_s() == other.to_s()
+  end
+end
diff --git a/lib/mv/mv_dummy_runner.rb b/lib/mv/mv_dummy_runner.rb
index 11f2ff5..662abff 100644
--- a/lib/mv/mv_dummy_runner.rb
+++ b/lib/mv/mv_dummy_runner.rb
@@ -4,11 +4,13 @@ class MvDummyRunner
   include Runner
 
   def run(plugin, src, dst)
-    if src.include?('@') then
-      puts "Would move user: #{src} to #{dst}"
-    else
-      raise NotImplementedError.new('Only users can be moved.')
+
+    if src.is_a?(Domain) or dst.is_a?(Domain) then
+      msg = 'only users can be moved'
+      raise NotImplementedError.new(msg)
     end
+
+    puts "Would move user #{src.to_s()} to #{dst.to_s()}."
   end
 
 end
diff --git a/lib/mv/mv_plugin.rb b/lib/mv/mv_plugin.rb
index 29f697c..089e6c7 100644
--- a/lib/mv/mv_plugin.rb
+++ b/lib/mv/mv_plugin.rb
@@ -13,7 +13,7 @@ module MvPlugin
     return MvDummyRunner
   end
 
-  def mv_user(from, to)
+  def mv_user(src, dst)
     # Rename the given user.
     raise NotImplementedError
   end
diff --git a/lib/mv/mv_runner.rb b/lib/mv/mv_runner.rb
index ee762e1..6da6f6d 100644
--- a/lib/mv/mv_runner.rb
+++ b/lib/mv/mv_runner.rb
@@ -1,3 +1,4 @@
+require 'common/domain'
 require 'common/errors'
 require 'common/runner'
 
@@ -5,35 +6,50 @@ class MvRunner
   include Runner
 
   def run(plugin, src, dst)
-    # Why think too hard? An user has an @, a domain doesn't.
-    if not src.include?('@') then
-      # We only support moving users right now, and the destination
-      # domain must already exist.
-      raise NotImplementedError.new('Only users can be moved.')
-    end
 
-    # Handle this once so we don't have to do it in every plugin.
-    if not dst.include?('@') then
-      msg = "the destination user #{dst} is not valid"
-      raise InvalidUserError.new(msg)
+    if src.is_a?(Domain) or dst.is_a?(Domain) then
+      msg = 'only users can be moved'
+      raise NotImplementedError.new(msg)
     end
 
     begin
-      src_description = plugin.describe_user(src)
+      src_description = plugin.describe(src)
       plugin.mv_user(src, dst)
-      msg  = "Moved user #{src} (#{src_description}) "
-      msg += "to #{dst}."
+      dst_description = plugin.describe(dst)
+
+      msg  = "Moved user #{src} "
+
+      # Only append the extra description if it's useful.
+      if not src_description.nil? and
+         not src_description.empty? and
+         not src_description == src.to_s() then
+        msg += "(#{src_description}) "
+      end
+
+      msg += "to #{dst}"
+
+      # Only append the extra description if it's useful.
+      if not dst_description.nil? and
+         not dst_description.empty? and
+         not dst_description == dst.to_s() then
+        msg += " (#{dst_description})"
+      end
+
+      msg += "."
       report(plugin, msg)
-    rescue InvalidUserError => e
-      report(plugin, e.message)
+
     rescue NonexistentUserError => e
-      report(plugin, e.message)
+      # This means that the SOURCE user didn't exist, since a
+      # nonexistent destination user is perfectly expected.
+      report(plugin, "Source user #{src.to_s()} not found.")
     rescue NonexistentDomainError => e
-      report(plugin, e.message)
-    rescue StandardError => e
-      msg = "There was an error (#{e.to_s}) moving the user: #{e.message}"
-      report(plugin, msg)
-      Kernel.exit(ExitCodes::DATABASE_ERROR)
+      # This could mean that the source domain doesn't exist, but in
+      # that case, we just report that the source user doesn't
+      # exist. So a nonexistent domain refers to a nonexistent
+      # DESTINATION domain.
+      report(plugin, "Destination domain #{dst.domainpart()} not found.")
+    rescue UserAlreadyExistsError => e
+      report(plugin, "Destination user #{dst.to_s()} already exists.")
     end
   end
 
diff --git a/lib/mv/plugins/agendav.rb b/lib/mv/plugins/agendav.rb
index 9c508c9..0324e6d 100644
--- a/lib/mv/plugins/agendav.rb
+++ b/lib/mv/plugins/agendav.rb
@@ -8,12 +8,15 @@ class AgendavMv
   include AgendavPlugin
   include MvPlugin
 
+  def mv_user(src, dst)
+    # It's obviously an error if the source user does not exist. It
+    # would also be an error if the destination domain didn't exist;
+    # however, Agendav doesn't know about domains, so we let that slide.
+    raise NonexistentUserError.new(src.to_s()) if not user_exists(src)
 
-  def mv_domain(from, to)
-    # AgenDAV doesn't have a concept of domains.
-  end
+    # And it's an error if the destination user exists already.
+    raise UserAlreadyExistsError.new(dst.to_s()) if user_exists(dst)
 
-  def mv_user(from, to)
     sql_queries = ['UPDATE prefs SET username = $1 WHERE username = $2;']
     sql_queries << 'UPDATE shared SET user_from = $1 WHERE user_from = $2;'
     sql_queries << 'UPDATE shared SET user_which = $1 WHERE user_which = $2;'
@@ -28,7 +31,7 @@ class AgendavMv
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [to, from])
+        connection.query(sql_query, [dst.to_s(), src.to_s()])
       end
 
       connection.close()
diff --git a/lib/mv/plugins/davical.rb b/lib/mv/plugins/davical.rb
index a7cfe80..bd66df8 100644
--- a/lib/mv/plugins/davical.rb
+++ b/lib/mv/plugins/davical.rb
@@ -12,15 +12,19 @@ class DavicalMv
   include MvPlugin
 
 
-  def mv_domain(from, to)
-    # DAViCal doesn't have a concept of domains.
-  end
-
-
-  def mv_user(from, to)
+  def mv_user(src, dst)
     # Switch the given usernames. DAViCal uses foreign keys properly
     # and only supports postgres, so we let the ON UPDATE CASCADE
     # trigger handle most of the work.
+
+    # It's obviously an error if the source user does not exist. It
+    # would also be an error if the destination domain didn't exist;
+    # however, DAViCal doesn't know about domains, so we let that slide.
+    raise NonexistentUserError.new(src.to_s()) if not user_exists(src)
+
+    # And it's an error if the destination user exists already.
+    raise UserAlreadyExistsError.new(dst.to_s()) if user_exists(dst)
+
     sql_queries = ['UPDATE usr SET username = $1 WHERE username = $2']
 
     begin
@@ -33,7 +37,7 @@ class DavicalMv
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [to, from])
+        connection.query(sql_query, [dst.to_s(), src.to_s()])
       end
 
       connection.close()
diff --git a/lib/mv/plugins/dovecot.rb b/lib/mv/plugins/dovecot.rb
index 9cdfc36..bd3084e 100644
--- a/lib/mv/plugins/dovecot.rb
+++ b/lib/mv/plugins/dovecot.rb
@@ -9,36 +9,43 @@ class DovecotMv
   include DovecotPlugin
   include MvPlugin
 
-  def mv_user(user_from, user_to)
-    # Move the user's mail directory from one domain (that should
-    # exist) to another domain (that should exist).
+  def mv_user(src, dst)
+    # It's obviously an error if the source user does not exist.
+    raise NonexistentUserError.new(src.to_s()) if not user_exists(src)
+
+    # And it's an error if the destination user exists already.
+    raise UserAlreadyExistsError.new(dst.to_s()) if user_exists(dst)
+
+    # But is it an error if the target domain does not exist? That's a
+    # bit subtle... The domain may exist in the database, but if it
+    # has not received any mail yet, then its directory won't exist
+    # on-disk.
+    #
+    # There are two possible "oops" scenarios resulting from the fact
+    # that we may run either the Postfixadmin move first or the
+    # Dovecot move first. If we move the user in the database, we
+    # definitely want to move him on disk (that is, we should create
+    # the directory here). But if we move him on disk first, then we
+    # don't know if the database move will fail! We don't want to move
+    # his mail files if he won't get moved in the database.
     #
-    # This is actually a little subtle, because none of the
-    # directories in question have to exist. If I create two new
-    # domains in postfixadmin and one mailbox but never deliver any
-    # mail, it should be possible to move the mailbox to the other
-    # domain. No directories will exist until mail is delivered,
-    # though.
-
-    # get_user_path() will fail if there's no mailbox to move. It will
-    # also fail on an invalid user -- oh well!
-    begin
-      from_path = self.get_user_path(user_from)
-    rescue NonexistentUserError
-      # Do nothing, there's no source mailbox...
-      return
+    # Faced with two equally-bad (but easy-to-fix) options, we do the
+    # simplest thing and fail if the destination domain directory
+    # doesn't exist. If nothing else, this is at least consistent.
+    if not self.domain_exists(dst.domain()) then
+      raise NonexistentDomainError.new(dst.domainpart())
     end
 
     # We may need to create the target domain directory, even if the
     # domain exists in the database.
-    domain_to = user_to.split('@')[1]
-    domain_dir = self.get_domain_path(domain_to)
-    FileUtils.mkdir_p(domain_dir)
-
-    # The from_path exists or else we would have returned earlier. The
-    # parent of to_path exists because we just created it.
-    to_path = self.get_user_path(user_to, false)
-    FileUtils.mv(from_path, to_path)
+    FileUtils.mkdir_p(self.get_domain_path(dst.domain()))
+
+    # The parent of dst_path exists because we just created it.The
+    # source path should exist too, because the "source user" does,
+    # and, well, how did we determine that?
+    src_path = self.get_user_path(src)
+    dst_path = self.get_user_path(dst)
+    FileUtils.mv(src_path, dst_path)
   end
 
 end
diff --git a/lib/mv/plugins/postfixadmin.rb b/lib/mv/plugins/postfixadmin.rb
index 3b6d188..338be05 100644
--- a/lib/mv/plugins/postfixadmin.rb
+++ b/lib/mv/plugins/postfixadmin.rb
@@ -8,20 +8,19 @@ class PostfixadminMv
   include PostfixadminPlugin
   include MvPlugin
 
-  def mv_user(user_from, user_to)
-    if not user_exists(user_from)
-      raise NonexistentUserError.new("source user #{user_from} does not exist")
-    end
-
-    user_to_parts = user_to.split('@')
-    localpart_to = user_to_parts[0]
-    domain_to = user_to_parts[1]
+  def mv_user(src, dst)
+    # It's obviously an error if the source user does not exist.
+    raise NonexistentUserError.new(src.to_s()) if not user_exists(src)
 
-    if not domain_exists(domain_to)
-      msg = "destination domain #{domain_to} does not exist"
-      raise NonexistentDomainError.new(msg)
+    # And if the destination domain does not exist.
+    if not domain_exists(dst.domain())
+      raise NonexistentDomainError.new(dst.domain.to_s())
     end
 
+    # But the destination *user* should *not* exist.
+    # And it's an error if the destination user exists already.
+    raise UserAlreadyExistsError.new(dst.to_s()) if user_exists(dst)
+
     mailbox_query  = 'UPDATE mailbox SET '
     mailbox_query += '  username=$1,'
     mailbox_query += '  domain=$2,'
@@ -51,10 +50,10 @@ class PostfixadminMv
 
       sql_queries.each do |sql_query|
           varchar = 1043 # from pg_type.h
-          params = [{:value => user_to, :type => varchar},
-                    {:value => domain_to, :type => varchar},
-                    {:value => localpart_to, :type => varchar},
-                    {:value => user_from, :type => varchar}]
+          params = [{:value => dst.to_s(), :type => varchar},
+                    {:value => dst.domainpart(), :type => varchar},
+                    {:value => dst.localpart(), :type => varchar},
+                    {:value => src.to_s(), :type => varchar}]
         connection.query(sql_query, params)
       end
 
diff --git a/lib/mv/plugins/roundcube.rb b/lib/mv/plugins/roundcube.rb
index bac8344..4b3ebcb 100644
--- a/lib/mv/plugins/roundcube.rb
+++ b/lib/mv/plugins/roundcube.rb
@@ -9,11 +9,15 @@ class RoundcubeMv
   include MvPlugin
 
 
-  def mv_domain(from, to)
-    # Roundcube doesn't have a concept of domains.
-  end
+  def mv_user(src, dst)
+    # It's obviously an error if the source user does not exist. It
+    # would also be an error if the destination domain didn't exist;
+    # however, Roundcube doesn't know about domains, so we let that slide.
+    raise NonexistentUserError.new(src.to_s()) if not user_exists(src)
+
+    # And it's an error if the destination user exists already.
+    raise UserAlreadyExistsError.new(dst.to_s()) if user_exists(dst)
 
-  def mv_user(from, to)
     sql_queries = ['UPDATE users SET username = $1 WHERE username = $2;']
 
     begin
@@ -26,7 +30,7 @@ class RoundcubeMv
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [to, from])
+        connection.query(sql_query, [dst.to_s(), src.to_s()])
       end
 
       connection.close()
diff --git a/lib/prune/prune_dummy_runner.rb b/lib/prune/prune_dummy_runner.rb
index 6cb789f..64c2606 100644
--- a/lib/prune/prune_dummy_runner.rb
+++ b/lib/prune/prune_dummy_runner.rb
@@ -1,7 +1,6 @@
 require 'common/runner'
-
-# This is always needed, regardless of which plugin is running.
 require 'prune/plugins/postfixadmin'
+require 'rm/rm_dummy_runner'
 
 class PruneDummyRunner
   include Runner
@@ -16,18 +15,11 @@ class PruneDummyRunner
     db_users = pfa.list_users()
     db_domains = pfa.list_domains()
 
-    leftover_users = plugin.get_leftover_users(db_users)
-    leftover_domains = plugin.get_leftover_domains(db_domains)
-
-    leftover_users.each do |user|
-      user_description = plugin.describe_user(user)
-      report(plugin, "Would remove user: #{user} (#{user_description})")
-    end
+    leftovers  = plugin.get_leftover_users(db_users)
+    leftovers += plugin.get_leftover_domains(db_domains)
 
-    leftover_domains.each do |domain|
-      domain_description = plugin.describe_domain(domain)
-      report(plugin, "Would remove domain: #{domain} (#{domain_description})")
-    end
+    rm_dummy_runner = RmDummyRunner.new()
+    rm_dummy_runner.run(plugin, *leftovers)
   end
 
 end
diff --git a/lib/prune/prune_runner.rb b/lib/prune/prune_runner.rb
index 12a02d7..8ae8ad2 100644
--- a/lib/prune/prune_runner.rb
+++ b/lib/prune/prune_runner.rb
@@ -1,7 +1,6 @@
 require 'common/runner'
-
-# This is always needed, regardless of which plugin is running.
 require 'prune/plugins/postfixadmin'
+require 'rm/rm_runner'
 
 class PruneRunner
   include Runner
@@ -16,21 +15,11 @@ class PruneRunner
     db_users = pfa.list_users()
     db_domains = pfa.list_domains()
 
-    leftover_users = plugin.get_leftover_users(db_users)
-    leftover_domains = plugin.get_leftover_domains(db_domains)
-
-    leftover_users.each do |user|
-      user_description = plugin.describe_user(user)
-      plugin.delete_user(user)
-      report(plugin, "Removed user: #{user} (#{user_description})")
-    end
-
-    leftover_domains.each do |domain|
-      domain_description = plugin.describe_domain(domain)
-      plugin.delete_domain(domain)
-      report(plugin, "Removed domain: #{domain} (#{domain_description})")
-    end
+    leftovers  = plugin.get_leftover_users(db_users)
+    leftovers += plugin.get_leftover_domains(db_domains)
 
+    rm_runner = RmRunner.new()
+    rm_runner.run(plugin, *leftovers)
   end
 
 end
diff --git a/lib/rm/plugins/agendav.rb b/lib/rm/plugins/agendav.rb
index 0f4e790..bebab61 100644
--- a/lib/rm/plugins/agendav.rb
+++ b/lib/rm/plugins/agendav.rb
@@ -12,7 +12,7 @@ class AgendavRm
   def delete_user(user)
     # Delete the given username and any records in other tables
     # belonging to it.
-    raise NonexistentUserError.new(user) if not user_exists(user)
+    raise NonexistentUserError.new(user.to_s()) if not user_exists(user)
 
     sql_queries = ['DELETE FROM prefs WHERE username = $1;']
     sql_queries << 'DELETE FROM shared WHERE user_from = $1;'
@@ -27,7 +27,7 @@ class AgendavRm
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [user])
+        connection.query(sql_query, [user.to_s()])
       end
 
       connection.close()
diff --git a/lib/rm/plugins/davical.rb b/lib/rm/plugins/davical.rb
index f7d116e..3526dcf 100644
--- a/lib/rm/plugins/davical.rb
+++ b/lib/rm/plugins/davical.rb
@@ -16,7 +16,7 @@ class DavicalRm
     # Delete the given username. DAViCal uses foreign keys properly
     # and only supports postgres, so we let the ON DELETE CASCADE
     # trigger handle most of the work.
-    raise NonexistentUserError.new(user) if not user_exists(user)
+    raise NonexistentUserError.new(user.to_s()) if not user_exists(user)
 
     sql_queries = ['DELETE FROM usr WHERE username = $1']
 
@@ -30,7 +30,7 @@ class DavicalRm
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [user])
+        connection.query(sql_query, [user.to_s()])
       end
 
       connection.close()
diff --git a/lib/rm/plugins/dovecot.rb b/lib/rm/plugins/dovecot.rb
index 837a7fd..0ced5be 100644
--- a/lib/rm/plugins/dovecot.rb
+++ b/lib/rm/plugins/dovecot.rb
@@ -11,14 +11,22 @@ class DovecotRm
 
 
   def delete_domain(domain)
-    # Will raise an exception if the path doesn't exist.
     domain_path = self.get_domain_path(domain)
+
+    if not File.directory?(domain_path)
+      raise NonexistentDomainError.new(domain.to_s())
+    end
+
     FileUtils.rm_r(domain_path)
   end
 
   def delete_user(user)
-    # Will raise an exception if the path doesn't exist.
     user_path = self.get_user_path(user)
+
+    if not File.directory?(user_path)
+      raise NonexistentUserError.new(user.to_s())
+    end
+
     FileUtils.rm_r(user_path)
   end
 
diff --git a/lib/rm/plugins/postfixadmin.rb b/lib/rm/plugins/postfixadmin.rb
index 94526bc..eddf939 100644
--- a/lib/rm/plugins/postfixadmin.rb
+++ b/lib/rm/plugins/postfixadmin.rb
@@ -10,7 +10,7 @@ class PostfixadminRm
 
 
   def delete_user(user)
-    raise NonexistentUserError.new(user) if not user_exists(user)
+    raise NonexistentUserError.new(user.to_s()) if not user_exists(user)
 
     sql_queries = ['DELETE FROM alias WHERE address = $1;']
     # Wipe out any aliases pointed at our user.
@@ -33,7 +33,7 @@ class PostfixadminRm
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [user])
+        connection.query(sql_query, [user.to_s()])
       end
 
       # The earlier alias update query will leave things like
@@ -53,7 +53,7 @@ class PostfixadminRm
 
 
   def delete_domain(domain)
-    raise NonexistentDomainError.new(domain) if not domain_exists(domain)
+    raise NonexistentDomainError.new(domain.to_s()) if not domain_exists(domain)
 
     sql_queries = ['DELETE FROM domain_admins WHERE domain = $1;']
     sql_queries << 'DELETE FROM alias WHERE domain = $1;'
@@ -73,7 +73,7 @@ class PostfixadminRm
                                   @db_pass)
 
       sql_queries.each do |sql_query|
-        connection.query(sql_query, [domain])
+        connection.query(sql_query, [domain.to_s()])
       end
 
       connection.close()
@@ -102,7 +102,7 @@ class PostfixadminRm
                                   @db_pass)
 
       sql_query = 'SELECT COUNT(domain) as count FROM domain WHERE domain = $1;'
-      connection.query(sql_query, [domain]) do |result|
+      connection.query(sql_query, [domain.to_s()]) do |result|
         return false if result.ntuples() < 1
         begin
           count = result.getvalue(0,0).to_i()
diff --git a/lib/rm/plugins/roundcube.rb b/lib/rm/plugins/roundcube.rb
index 23782d2..a315e77 100644
--- a/lib/rm/plugins/roundcube.rb
+++ b/lib/rm/plugins/roundcube.rb
@@ -11,7 +11,7 @@ class RoundcubeRm
   def delete_user(user)
     # Delete the given username and any records in other tables
     # belonging to it.
-    raise NonexistentUserError.new(user) if not user_exists(user)
+    raise NonexistentUserError.new(user.to_s()) if not user_exists(user)
 
     user_id = self.get_user_id(user)
 
diff --git a/lib/rm/rm_dummy_runner.rb b/lib/rm/rm_dummy_runner.rb
index 477608f..2ab25e8 100644
--- a/lib/rm/rm_dummy_runner.rb
+++ b/lib/rm/rm_dummy_runner.rb
@@ -5,13 +5,18 @@ class RmDummyRunner
 
   def run(plugin, *targets)
     targets.each do |target|
-      if target.include?('@') then
-        user_description = plugin.describe_user(target)
-        report(plugin, "Would remove user: #{target} (#{user_description})")
-      else
-        domain_description = plugin.describe_domain(target)
-        report(plugin, "Would remove domain: #{target} (#{domain_description})")
+      target_description = plugin.describe(target)
+      msg =  "Would remove #{target.class.to_s().downcase()} #{target}"
+
+      # Only append the extra description if it's useful.
+      if not target_description.nil? and
+         not target_description.empty? and
+         not target_description == target.to_s() then
+        msg += " (#{target_description})"
       end
+      msg += '.'
+
+      report(plugin, msg)
     end
 
   end
diff --git a/lib/rm/rm_plugin.rb b/lib/rm/rm_plugin.rb
index fe938b1..fd0c541 100644
--- a/lib/rm/rm_plugin.rb
+++ b/lib/rm/rm_plugin.rb
@@ -15,15 +15,27 @@ module RmPlugin
     return RmDummyRunner
   end
 
+  def delete(target)
+    # A generic version of delete_user/delete_domain that
+    # dispatches base on the class of the target.
+    if target.is_a?(User)
+      return delete_user(target)
+    elsif target.is_a?(Domain)
+      return delete_domain(target)
+    else
+      raise NotImplementedError
+    end
+  end
+
   def delete_domain(domain)
     # Delete the given domain. Some plugins don't have a concept of
     # domains, so just delete all users with a username that looks
     # like it's in the given domain.
-    usernames = list_domains_users([domain])
+    users = list_domains_users([domain])
 
-    raise NonexistentDomainError.new(domain) if usernames.empty?
+    raise NonexistentDomainError.new(domain.to_s()) if users.empty?
 
-    usernames.each do |u|
+    users.each do |u|
       delete_user(u)
     end
   end
diff --git a/lib/rm/rm_runner.rb b/lib/rm/rm_runner.rb
index 7142154..2df8410 100644
--- a/lib/rm/rm_runner.rb
+++ b/lib/rm/rm_runner.rb
@@ -6,37 +6,32 @@ class RmRunner
 
   def run(plugin, *targets)
     targets.each do |target|
-      # Why think too hard? An user has an @, a domain doesn't.
-      if target.include?('@') then
-        begin
-          user_description = plugin.describe_user(target)
-          plugin.delete_user(target)
-          report(plugin, "Removed user: #{target} (#{user_description})")
+      remove_target(plugin, target)
+    end
+  end
+
+  private;
+
+  def remove_target(plugin, target)
+    # Wrap the "remove this thing" operation so that it can be reused
+    # in the prine plugin.
+    target_description = plugin.describe(target)
 
-        rescue NonexistentUserError => e
-          report(plugin, "User not found: #{e.to_s}")
-        rescue StandardError => e
-          report(plugin, "There was an error removing the user: #{e.to_s}")
-          Kernel.exit(ExitCodes::DATABASE_ERROR)
-        end
-      else
-        begin
-          domain_description = plugin.describe_domain(target)
-          plugin.delete_domain(target)
-          report(plugin, "Removed domain: #{target} (#{domain_description})")
+    begin
+      plugin.delete(target)
+      msg =  "Removed #{target.class.to_s().downcase()} #{target}"
 
-        rescue NonexistentUserError => e
-          # Can happen in the usernames.each... block.
-          report(plugin, "User not found: #{e.to_s}")
-        rescue NonexistentDomainError => e
-          report(plugin, "Domain not found: #{e.to_s}")
-        rescue StandardError => e
-          report(plugin, "There was an error removing the domain: #{e.to_s}")
-          Kernel.exit(ExitCodes::DATABASE_ERROR)
-        end
+      # Only append the extra description if it's useful.
+      if not target_description.nil? and
+         not target_description.empty? and
+         not target_description == target.to_s() then
+        msg += " (#{target_description})"
       end
-    end
+      msg += '.'
 
+      report(plugin, msg)
+    rescue NonexistentDomainError, NonexistentUserError => e
+      report(plugin, "#{target.class.to_s()} #{e.to_s} not found.")
+    end
   end
-
 end
-- 
2.43.2