IP address management for svc_acct, #19567

12 files changed, 530 insertions, 208 deletions

diff --git a/FS/FS/Conf.pm b/FS/FS/Conf.pm
index c9f30fe..e74c19f 100644
--- a/FS/FS/Conf.pm
+++ b/FS/FS/Conf.pm
@@ -1136,6 +1136,13 @@ sub reason_type_options {
   },
 
   {
+    'key'         => 'svc_acct-ip_addr',
+    'section'     => '',
+    'description' => 'Enable IP address management on login services like for broadband services.',
+    'type'        => 'checkbox',
+  },
+
+  {
     'key'         => 'exclude_ip_addr',
     'section'     => '',
     'description' => 'Exclude these from the list of available broadband service IP addresses. (One per line)',
diff --git a/FS/FS/IP_Mixin.pm b/FS/FS/IP_Mixin.pm
new file mode 100644
index 0000000..fdeb51d
--- /dev/null
+++ b/FS/FS/IP_Mixin.pm
@@ -0,0 +1,305 @@
+package FS::IP_Mixin;
+
+use strict;
+use NetAddr::IP;
+use FS::addr_block;
+use FS::router;
+use FS::Record qw(qsearch);
+use FS::Conf;
+# careful about importing anything here--it will end up in a LOT of 
+# namespaces
+
+use vars qw(@subclasses $DEBUG $conf);
+
+$DEBUG = 0;
+
+# any subclass that can have IP addresses needs to be added here
+@subclasses = (qw(FS::svc_broadband FS::svc_acct));
+
+sub conf {
+  $conf ||= FS::Conf->new;
+}
+
+=head1 NAME
+
+FS::IP_Mixin - Mixin class for objects that have IP addresses assigned.
+
+=head1 INTERFACE
+
+The inheritor may provide the following methods:
+
+=over 4
+
+=item ip_addr [ ADDRESS ]
+
+Get/set the IP address, as a string.  If the inheritor is also an
+L<FS::Record> subclass and has an 'ip_addr' field, that field will be 
+used.  Otherwise an C<ip_addr> method must be defined.
+
+=item addr_block [ BLOCK ]
+
+Get/set the address block, as an L<FS::addr_block> object.  By default,
+the 'blocknum' field will be used.
+
+=item router [ ROUTER ]
+
+Get/set the router, as an L<FS::router> object.  By default, the 
+'routernum' field will be used.  This is strictly optional; if present
+the IP address can be assigned from all those available on a router, 
+rather than in a specific block.
+
+=item _used_addresses [ BLOCK ]
+
+Return a list of all addresses in use (within BLOCK, if it's specified).
+The inheritor should cache this if possible.
+
+=item _is_used ADDRESS
+
+Test a specific address for availability.  Should return an empty string
+if it's free, or else a description of who or what is using it.
+
+=back
+
+=head1 METHODS
+
+=over 4
+
+=item ip_check
+
+The method that should be called from check() in the subclass.  This does 
+the following:
+
+- In an C<auto_router> situation, sets the router and block to match the 
+  object's IP address.
+- Otherwise, if the router and IP address are both set, validate the 
+  choice of router and set the block correctly.
+- Otherwise, if the router is set, assign an address (in the selected
+  block if there is one).
+- Check the IP address for availability.
+
+Returns an error if this fails for some reason (an address can't be 
+assigned from the requested router/block, or the requested address is
+unavailable, or doesn't seem to be an IP address).
+
+If router and IP address are both empty, this will do nothing.  The 
+object's check() method should decide whether to allow a null IP address.
+
+=cut
+
+sub ip_check {
+  my $self = shift;
+
+  if ( $self->ip_addr eq '0.0.0.0' ) { #ipv6?
+    $self->ip_addr('');
+  }
+
+  if ( $self->ip_addr
+       and !$self->router
+       and $self->conf->exists('auto_router') ) {
+    # assign a router that matches this IP address
+    return $self->check_ip_addr || $self->assign_router;
+  }
+  if ( my $router = $self->router ) {
+    if ( $router->manual_addr ) {
+      # Router is set, and it's set to manual addressing, so 
+      # clear blocknum and don't tamper with ip_addr.
+      $self->addr_block(undef);
+    } else {
+      my $block = $self->addr_block;
+      if ( !$block or !$block->manual_flag ) {
+        my $error = $self->assign_ip_addr;
+        return $error if $error;
+      }
+      # otherwise block is set to manual addressing
+    }
+  }
+  return $self->check_ip_addr;
+}
+
+=item assign_ip_addr
+
+Set the IP address to a free address in the selected block (C<addr_block>)
+or router (C<router>) for this object.  A block or router MUST be selected.
+If the object already has an IP address and it is in that block/router's 
+address space, it won't be changed.
+
+=cut
+
+sub assign_ip_addr {
+  my $self = shift;
+  my %opt = @_;
+
+  my @blocks;
+  my $na = $self->NetAddr;
+
+  if ( $self->addr_block ) {
+    # choose an address in a specific block.
+    @blocks = ( $self->addr_block );
+  } elsif ( $self->router ) {
+    # choose an address from any block on a specific router.
+    @blocks = $self->router->auto_addr_block;
+  } else {
+    # what else should we do, search ALL blocks? that's crazy.
+    die "no block or router specified for assign_ip_addr\n";
+  }
+
+  my $new_addr;
+  my $new_block;
+  foreach my $block (@blocks) {
+    if ( $self->ip_addr and $block->NetAddr->contains($na) ) {
+      return '';
+    }
+    # don't exit early on assigning a free address--check the rest of 
+    # the blocks to see if the current address is in one of them.
+    if (!$new_addr) {
+      $new_addr = $block->next_free_addr->addr;
+      $new_block = $block;
+    }
+  }
+ 
+  return 'No IP address available on this router' unless $new_addr;
+
+  $self->ip_addr($new_addr);
+  $self->addr_block($new_block);
+  '';
+}
+
+=item assign_router
+
+If the IP address is set, set the router and block accordingly.  If there
+is no block containing that address, returns an error.
+
+=cut
+
+sub assign_router {
+  my $self = shift;
+  return '' unless $self->ip_addr;
+  my $na = $self->NetAddr;
+  foreach my $router (qsearch('router', {})) {
+    foreach my $block ($router->addr_block) {
+      if ( $block->NetAddr->contains($na) ) {
+        $self->addr_block($block);
+        $self->router($router);
+        return '';
+      }
+    }
+  }
+  return $self->ip_addr . ' is not in an allowed block.';
+}
+
+=item check_ip_addr
+
+Validate the IP address.  Returns an empty string if it's correct and 
+available (or null), otherwise an error message.
+
+=cut
+
+sub check_ip_addr {
+  my $self = shift;
+  my $addr = $self->ip_addr;
+  return '' if $addr eq '';
+  my $na = $self->NetAddr
+    or return "Can't parse address '$addr'";
+  if ( my $block = $self->addr_block ) {
+    if ( !$block->NetAddr->contains($na) ) {
+      return "Address $addr not in block ".$block->cidr;
+    }
+  }
+  # this returns '' if the address is in use by $self.
+  if ( my $dup = $self->is_used($self->ip_addr) ) {
+    return "Address $addr in use by $dup";
+  }
+  '';
+}
+
+# sensible defaults
+sub addr_block {
+  my $self = shift;
+  if ( @_ ) {
+    my $new = shift;
+    if ( defined $new ) {
+      die "addr_block() must take an address block"
+        unless $new->isa('FS::addr_block');
+      $self->blocknum($new->blocknum);
+      return $new;
+    } else {
+      #$new is undef
+      $self->blocknum('');
+      return undef;
+    }
+  }
+  # could cache this...
+  FS::addr_block->by_key($self->blocknum);
+}
+
+sub router {
+  my $self = shift;
+  if ( @_ ) {
+    my $new = shift;
+    if ( defined $new ) {
+      die "router() must take a router"
+        unless $new->isa('FS::router');
+      $self->routernum($new->routernum);
+      return $new;
+    } else {
+      #$new is undef
+      $self->routernum('');
+      return undef;
+    }
+  }
+  FS::router->by_key($self->routernum);
+}
+
+=item used_addresses [ BLOCK ]
+
+Returns a list of all addresses (in BLOCK, or in all blocks)
+that are in use.  If called as an instance method, excludes 
+that instance from the search.
+
+=cut
+
+sub used_addresses {
+  my $self = shift;
+  my $block = shift;
+  return ( map { $_->_used_addresses($block, $self) } @subclasses );
+}
+
+sub _used_addresses {
+  my $class = shift;
+  die "$class->_used_addresses not implemented";
+}
+
+=item is_used ADDRESS
+
+Returns a string describing what object is using ADDRESS, or 
+an empty string if it's not in use.
+
+=cut
+
+sub is_used {
+  my $self = shift;
+  my $addr = shift;
+  for (@subclasses) {
+    my $used = $_->_is_used($addr, $self);
+    return $used if $used;
+  }
+  '';
+}
+
+sub _is_used {
+  my $class = shift;
+  die "$class->_is_used not implemented";
+}
+
+=back
+
+=head1 BUGS
+
+We can't reliably check for duplicate addresses across tables.  A 
+more robust implementation would be to put all assigned IP addresses
+in a single table with a unique index.  We do a best-effort check 
+anyway, but it has a race condition.
+
+=cut
+
+1; 
diff --git a/FS/FS/Schema.pm b/FS/FS/Schema.pm
index 01250e5..912f3e2 100644
--- a/FS/FS/Schema.pm
+++ b/FS/FS/Schema.pm
@@ -2184,6 +2184,9 @@ sub tables_hashref {
         'shell',     'varchar',   'NULL',   $char_d, '', '', 
         'quota',     'varchar',   'NULL',   $char_d, '', '', 
         'slipip',    'varchar',   'NULL',   15, '', '', #four TINYINTs, bah.
+        # IP address mgmt
+        'routernum', 'int', 'NULL',      '', '', '',
+        'blocknum',  'int', 'NULL',      '', '', '', 
         'seconds',   'int', 'NULL',   '', '', '', #uhhhh
         'seconds_threshold',   'int', 'NULL',   '', '', '',
         'upbytes',   'bigint', 'NULL',   '', '', '', 
diff --git a/FS/FS/addr_block.pm b/FS/FS/addr_block.pm
index 686bdbd..6a62777 100755
--- a/FS/FS/addr_block.pm
+++ b/FS/FS/addr_block.pm
@@ -6,6 +6,7 @@ use FS::Record qw( qsearchs qsearch dbh );
 use FS::router;
 use FS::svc_broadband;
 use FS::Conf;
+use FS::IP_Mixin;
 use NetAddr::IP;
 use Carp qw( carp );
 use List::Util qw( first );
@@ -238,7 +239,7 @@ sub next_free_addr {
   my $self = shift;
   my $selfaddr = $self->NetAddr;
 
-  return if $self->manual_flag;
+  return () if $self->manual_flag;
 
   my $conf = new FS::Conf;
   my @excludeaddr = $conf->config('exclude_ip_addr');
@@ -249,9 +250,7 @@ sub next_free_addr {
     $selfaddr->addr,
     $selfaddr->network->addr,
     $selfaddr->broadcast->addr,
-    (map { $_->NetAddr->addr }
-       qsearch('svc_broadband', { blocknum => $self->blocknum })
-    ), @excludeaddr
+    FS::IP_Mixin->used_addresses($self)
   );
 
   # just do a linear search of the block
diff --git a/FS/FS/svc_IP_Mixin.pm b/FS/FS/svc_IP_Mixin.pm
new file mode 100644
index 0000000..7026205
--- /dev/null
+++ b/FS/FS/svc_IP_Mixin.pm
@@ -0,0 +1,123 @@
+package FS::svc_IP_Mixin;
+
+use strict;
+use base 'FS::IP_Mixin';
+use FS::Record qw(qsearchs qsearch);
+
+=item addr_block
+
+Returns the address block assigned to this service.
+
+=item router
+
+Returns the router assigned to this service, if there is one.
+
+=cut
+
+#addr_block and router methods provided by FS::IP_Mixin
+
+=item NetAddr
+
+Returns the address as a L<NetAddr::IP> object.  Use C<$svc->NetAddr->addr>
+to put it into canonical string form.
+
+=cut
+
+sub NetAddr {
+  my $self = shift;
+  NetAddr::IP->new($self->ip_addr);
+}
+
+=item ip_addr
+
+Wrapper for set/get on the IP address field.
+
+=cut
+
+sub ip_addr {
+  my $self = shift;
+  my $ip_field = $self->table_info->{'ip_field'}
+    or return '';
+  if ( @_ ) {
+    $self->set($ip_field, @_);
+  } else {
+    $self->get($ip_field);
+  }
+}
+
+=item allowed_routers
+
+Returns a list of L<FS::router> objects allowed on this service.
+
+=cut
+
+sub allowed_routers {
+  my $self = shift;
+  my $svcpart = $self->svcnum ? $self->cust_svc->svcpart : $self->svcpart;
+  my @r = map { $_->router } 
+    qsearch('part_svc_router', { svcpart => $svcpart });
+
+  if ( $self->cust_main ) {
+    my $agentnum = $self->cust_main->agentnum;
+    return grep { !$_->agentnum or $_->agentnum == $agentnum } @r;
+  } else {
+    return @r;
+  }
+}
+
+=item svc_ip_check
+
+Wrapper for C<ip_check> which also checks the validity of the router.
+
+=cut
+
+sub svc_ip_check {
+  my $self = shift;
+  my $error = $self->ip_check;
+  return $error if $error;
+  if ( my $router = $self->router ) {
+    if ( grep { $_->routernum eq $router->routernum } $self->allowed_routers ) {
+      return '';
+    } else {
+      return 'Router '.$router->routername.' not available for this service';
+    }
+  }
+  '';
+}
+
+sub _used_addresses {
+  my ($class, $block, $exclude) = @_;
+  my $ip_field = $class->table_info->{'ip_field'}
+    or return ();
+  # if the service doesn't have an ip_field, then it has no IP addresses 
+  # in use, yes? 
+
+  my %hash = ( $ip_field => { op => '!=', value => '' } );
+  $hash{'blocknum'} = $block->blocknum if $block;
+  $hash{'svcnum'} = { op => '!=', value => $exclude->svcnum } if ref $exclude;
+  map { $_->NetAddr->addr } qsearch($class->table, \%hash);
+}
+
+sub _is_used {
+  my ($class, $addr, $exclude) = @_;
+  my $ip_field = $class->table_info->{'ip_field'}
+    or return '';
+
+  my $svc = qsearchs($class->table, { $ip_field => $addr })
+    or return '';
+
+  return '' if ( ref $exclude and $exclude->svcnum == $svc->svcnum );
+
+  my $cust_svc = $svc->cust_svc;
+  if ( $cust_svc ) {
+    my @label = $cust_svc->label;
+    # "svc_foo 1234 (Service Desc)"
+    # this should be enough to identify it without leaking customer
+    # names across agents
+    "$label[2] $label[3] ($label[0])";
+  } else {
+    join(' ', $class->table, $svc->svcnum, '(unlinked service)');
+  }
+}
+
+1;
diff --git a/FS/FS/svc_acct.pm b/FS/FS/svc_acct.pm
index 7ce79ae..8e71d82 100644
--- a/FS/FS/svc_acct.pm
+++ b/FS/FS/svc_acct.pm
@@ -6,6 +6,7 @@ use base qw( FS::svc_Domain_Mixin
              FS::svc_CGPRule_Mixin
              FS::svc_Radius_Mixin
              FS::svc_Tower_Mixin
+             FS::svc_IP_Mixin
              FS::svc_Common );
 use vars qw( $DEBUG $me $conf $skip_fuzzyfiles
              $dir_prefix @shells $usernamemin
@@ -1126,6 +1127,8 @@ sub check {
               || $self->ut_foreign_key( 'domsvc', 'svc_domain', 'svcnum' )
               || $self->ut_foreign_keyn('pbxsvc', 'svc_pbx',    'svcnum' )
               || $self->ut_foreign_keyn('sectornum','tower_sector','sectornum')
+              || $self->ut_foreign_keyn('routernum','router','routernum')
+              || $self->ut_foreign_keyn('blocknum','addr_block','blocknum')
               || $self->ut_textn('sec_phrase')
               || $self->ut_snumbern('seconds')
               || $self->ut_snumbern('upbytes')
@@ -1161,6 +1164,15 @@ sub check {
   ;
   return $error if $error;
 
+  # assign IP address, etc.
+  if ( $conf->exists('svc_acct-ip_addr') ) {
+    my $error = $self->svc_ip_check;
+    return $error if $error;
+  } else { # I think this is correct
+    $self->routernum('');
+    $self->blocknum('');
+  }
+
   my $cust_pkg;
   local $username_letter = $username_letter;
   local $username_uppercase = $username_uppercase;
@@ -1314,7 +1326,7 @@ sub check {
 
   unless ( $part_svc->part_svc_column('slipip')->columnflag eq 'F' ) {
     if ( $recref->{slipip} eq '' ) {
-      $recref->{slipip} = '';
+      $recref->{slipip} = ''; # eh?
     } elsif ( $recref->{slipip} eq '0e0' ) {
       $recref->{slipip} = '0e0';
     } else {
@@ -1322,7 +1334,6 @@ sub check {
         or return "Illegal slipip: ". $self->slipip;
       $recref->{slipip} = $1;
     }
-
   }
 
   #arbitrary RADIUS stuff; allow ut_textn for now
@@ -1384,6 +1395,7 @@ sub check {
   else {
     return "invalid password encoding ('".$recref->{_password_encoding}."'";
   }
+
   $self->SUPER::check;
 
 }
diff --git a/FS/FS/svc_broadband.pm b/FS/FS/svc_broadband.pm
index 26659d5..af81353 100755
--- a/FS/FS/svc_broadband.pm
+++ b/FS/FS/svc_broadband.pm
@@ -1,5 +1,10 @@
 package FS::svc_broadband;
-use base qw(FS::svc_Radius_Mixin FS::svc_Tower_Mixin FS::svc_Common);
+use base qw(
+  FS::svc_Radius_Mixin
+  FS::svc_Tower_Mixin
+  FS::svc_IP_Mixin 
+  FS::svc_Common
+  );
 
 use strict;
 use vars qw($conf);
@@ -412,38 +417,13 @@ sub check {
   }
   my $agentnum = $cust_pkg->cust_main->agentnum if $cust_pkg;
 
-  if ( $conf->exists('auto_router') and $self->ip_addr and !$self->routernum ) {
-    # assign_router is guaranteed to provide a router that's legal
-    # for this agent and svcpart
-    my $error = $self->_check_ip_addr || $self->assign_router;
-    return $error if $error;
+  # assign IP address / router / block
+  $error = $self->svc_ip_check;
+  return $error if $error;
+  if ( !$self->ip_addr 
+       and !$conf->exists('svc_broadband-allow_null_ip_addr') ) {
+    return 'IP address is required';
   }
-  elsif ($self->routernum) {
-    return "Router ".$self->routernum." does not provide this service"
-      unless qsearchs('part_svc_router', { 
-        svcpart => $svcpart,
-        routernum => $self->routernum
-    });
-  
-    my $router = $self->router;
-    return "Router ".$self->routernum." does not serve this customer"
-      if $router->agentnum and $agentnum and $router->agentnum != $agentnum;
-
-    if ( $router->manual_addr ) {
-      $self->blocknum('');
-    }
-    else {
-      my $addr_block = $self->addr_block;
-      if ( $self->ip_addr eq '' 
-           and not ( $addr_block and $addr_block->manual_flag ) ) {
-        my $error = $self->assign_ip_addr;
-        return $error if $error;
-      }
-    }
- 
-    my $error = $self->_check_ip_addr;
-    return $error if $error;
-  } # if $self->routernum
 
   if ( $cust_pkg && ! $self->latitude && ! $self->longitude ) {
     my $l = $cust_pkg->cust_location_or_main;
@@ -459,104 +439,12 @@ sub check {
   $self->SUPER::check;
 }
 
-=item assign_ip_addr
-
-Assign an IP address matching the selected router, and the selected block
-if there is one.
-
-=cut
-
-sub assign_ip_addr {
-  my $self = shift;
-  my @blocks;
-  my $ip_addr;
-
-  if ( $self->addr_block and $self->addr_block->routernum == $self->routernum ) {
-    # simple case: user chose a block, find an address in that block
-    # (this overrides an existing IP address if it's not in the block)
-    @blocks = ($self->addr_block);
-  }
-  elsif ( $self->routernum ) {
-    @blocks = $self->router->auto_addr_block;
-  }
-  else { 
-    return '';
-  }
-#warn "assigning ip address in blocks\n".join("\n",map{$_->cidr} @blocks)."\n";
-
-  foreach my $block ( @blocks ) {
-    if ( $self->ip_addr and $block->NetAddr->contains($self->NetAddr) ) {
-      # don't change anything
-      return '';
-    }
-    $ip_addr = $block->next_free_addr;
-    if ( $ip_addr ) {
-      $self->set(ip_addr => $ip_addr->addr);
-      $self->set(blocknum => $block->blocknum);
-      return '';
-    }
-  }
-  return 'No IP address available on this router';
-}
-
-=item assign_router
-
-Assign an address block and router matching the selected IP address.
-Does nothing if IP address is null.
-
-=cut
-
-sub assign_router {
-  my $self = shift;
-  return '' if !$self->ip_addr;
-  #warn "assigning router/block for ".$self->ip_addr."\n";
-  foreach my $router ($self->allowed_routers) {
-    foreach my $block ($router->addr_block) {
-      if ( $block->NetAddr->contains($self->NetAddr) ) {
-        $self->blocknum($block->blocknum);
-        $self->routernum($block->routernum);
-        return '';
-      }
-    }
-  }
-  return $self->ip_addr.' is not in an allowed block.';
-}
-
-sub _check_ip_addr {
-  my $self = shift;
-
-  if (not($self->ip_addr) or $self->ip_addr eq '0.0.0.0') {
-    return '' if $conf->exists('svc_broadband-allow_null_ip_addr'); 
-    return 'IP address required';
-  }
-  else {
-    return 'Cannot parse address: '.$self->ip_addr unless $self->NetAddr;
-  }
-
-  if ( $self->addr_block 
-      and not $self->addr_block->NetAddr->contains($self->NetAddr) ) {
-    return 'Address '.$self->ip_addr.' not in block '.$self->addr_block->cidr;
-  }
-
-#  if (my $dup = qsearchs('svc_broadband', {
-#        ip_addr => $self->ip_addr,
-#        svcnum  => {op=>'!=', value => $self->svcnum}
-#      }) ) {
-#    return 'IP address conflicts with svcnum '.$dup->svcnum;
-#  }
-  '';
-}
-
 sub _check_duplicate {
   my $self = shift;
   # Not a reliable check because the table isn't locked, but 
   # that's why we have a unique index.  This is just to give a
   # friendlier error message.
   my @dup;
-  @dup = $self->find_duplicates('global', 'ip_addr');
-  if ( @dup ) {
-    return "IP address in use (svcnum ".$dup[0]->svcnum.")";
-  }
   @dup = $self->find_duplicates('global', 'mac_addr');
   if ( @dup ) {
     return "MAC address in use (svcnum ".$dup[0]->svcnum.")";
@@ -565,64 +453,6 @@ sub _check_duplicate {
   '';
 }
 
-
-=item NetAddr
-
-Returns a NetAddr::IP object containing the IP address of this service.  The netmask 
-is /32.
-
-=cut
-
-sub NetAddr {
-  my $self = shift;
-  new NetAddr::IP ($self->ip_addr);
-}
-
-=item addr_block
-
-Returns the FS::addr_block record (i.e. the address block) for this broadband service.
-
-=cut
-
-sub addr_block {
-  my $self = shift;
-  qsearchs('addr_block', { blocknum => $self->blocknum });
-}
-
-=item router
-
-Returns the FS::router record for this service.
-
-=cut
-
-sub router {
-  my $self = shift;
-  qsearchs('router', { routernum => $self->routernum });
-}
-
-=item allowed_routers
-
-Returns a list of allowed FS::router objects.
-
-=cut
-
-sub allowed_routers {
-  my $self = shift;
-  my $svcpart = $self->svcnum ? $self->cust_svc->svcpart : $self->svcpart;
-  my @r = map { $_->router } qsearch('part_svc_router', 
-    { svcpart => $svcpart });
-  if ( $self->cust_main ) {
-    my $agentnum = $self->cust_main->agentnum;
-    return grep { !$_->agentnum or $_->agentnum == $agentnum } @r;
-  }
-  else {
-    return @r;
-  }
-}
-
-=back
-
-
 =item mac_addr_formatted CASE DELIMITER
 
 Format the MAC address (for use by exports).  If CASE starts with "l"
diff --git a/httemplate/edit/process/svc_acct.cgi b/httemplate/edit/process/svc_acct.cgi
index 41aca65..d4bcd35 100755
--- a/httemplate/edit/process/svc_acct.cgi
+++ b/httemplate/edit/process/svc_acct.cgi
@@ -31,6 +31,11 @@ foreach (map { $_,$_."_threshold" } qw( upbytes downbytes totalbytes )) {
   $cgi->param($_, FS::UI::bytecount::parse_bytecount($cgi->param($_)) );
 }
 
+#for slipip, convert '(automatic)' to null
+my $ip_addr = $cgi->param('slipip');
+$ip_addr =~ s/[^\d\.]//g;
+$cgi->param('slipip', $ip_addr);
+
 #unmunge cgp_accessmodes (falze laziness-ish w/part_svc.pm::process &svc_domain)
 unless ( $cgi->param('cgp_accessmodes') ) {
   $cgi->param('cgp_accessmodes', 
diff --git a/httemplate/edit/router.cgi b/httemplate/edit/router.cgi
index fdcd7b3..0df9b45 100755
--- a/httemplate/edit/router.cgi
+++ b/httemplate/edit/router.cgi
@@ -29,8 +29,15 @@ die "access denied"
   unless $curuser->access_right('Broadband configuration')
     || $curuser->access_right('Broadband global configuration');
 
+my @svc_x = 'svc_broadband';
+if ( FS::Conf->new->exists('svc_acct-ip_addr') ) {
+  push @svc_x, 'svc_acct';
+}
+
 my $callback = sub {
   my ($cgi, $object, $fields) = (shift, shift, shift);
+
+  my $extra_sql = ' AND svcdb IN(' . join(',', map { "'$_'" } @svc_x) . ')';
   unless ($object->svcnum) {
     push @{$fields},
       { 'type'          => 'tablebreak-tr-title',
@@ -41,7 +48,8 @@ my $callback = sub {
         'target_table'  => 'part_svc',
         'link_table'    => 'part_svc_router',
         'name_col'      => 'svc',
-        'hashref'       => { 'svcdb' => 'svc_broadband', 'disabled' => '' },
+        'hashref'       => { 'disabled' => '' },
+        'extra_sql'     => $extra_sql,
       };
   }
 };
diff --git a/httemplate/edit/svc_acct.cgi b/httemplate/edit/svc_acct.cgi
index 142c111..c1f7455 100755
--- a/httemplate/edit/svc_acct.cgi
+++ b/httemplate/edit/svc_acct.cgi
@@ -276,14 +276,26 @@ function randomPass() {
              'communigate' => $communigate,
 &>
 
-% if ( $part_svc->part_svc_column('slipip')->columnflag =~ /^[FA]$/ ) { 
-  <INPUT TYPE="hidden" NAME="slipip" VALUE="<% $svc_acct->slipip %>">
-% } else { 
-  <TR>
-    <TD ALIGN="right"><% mt('IP') |h %></TD>
-    <TD><INPUT TYPE="text" NAME="slipip" VALUE="<% $svc_acct->slipip %>"></TD>
-  </TR>
-% } 
+% if ( $conf->exists('svc_acct-ip_addr') ) {
+%   # router/block selection UI
+%   # (should we show this if slipip is fixed?)
+<& /elements/tr-select-router_block_ip.html, 
+  'object' => $svc_acct,
+  'ip_field' => 'slipip'
+&>
+% } else {
+%   # don't expose these to the user--they're only useful in the other case
+  <INPUT TYPE="hidden" NAME="routernum" VALUE="<% $svc_acct->routernum %>">
+  <INPUT TYPE="hidden" NAME="blocknum"  VALUE="<% $svc_acct->blocknum  %>">
+%   if ( $part_svc->part_svc_column('slipip')->columnflag =~ /^[FA]$/ ) { 
+    <INPUT TYPE="hidden" NAME="slipip" VALUE="<% $svc_acct->slipip %>">
+%   } else { 
+    <TR>
+      <TD ALIGN="right"><% mt('IP') |h %></TD>
+      <TD><INPUT TYPE="text" NAME="slipip" VALUE="<% $svc_acct->slipip %>"></TD>
+    </TR>
+%   }
+% }
 
 % my %label = ( seconds => 'Time',
 %               upbytes => 'Upload bytes',
diff --git a/httemplate/elements/tr-select-router_block_ip.html b/httemplate/elements/tr-select-router_block_ip.html
index 95d1787..11f7c48 100644
--- a/httemplate/elements/tr-select-router_block_ip.html
+++ b/httemplate/elements/tr-select-router_block_ip.html
@@ -58,12 +58,13 @@ function clearhint_ip_addr (what) {
 </td></tr>
 <& /elements/tr-td-label.html, label => 'IP address' &>
 <td>
-% if ( $fixed{'ip_addr'} ) {
-  <input type="hidden" id="input_ip_addr" name="ip_addr" 
+% warn Dumper \%fixed;
+% if ( exists $fixed{$ip_field} ) {
+  <input type="hidden" id="input_ip_addr" name="<% $ip_field %>" 
     value="<% $opt{'ip_addr'} |h%>"><% $opt{'ip_addr'} || '' %>
 % }
 % else {
-  <input type="text" id="input_ip_addr" name="ip_addr" 
+  <input type="text" id="input_ip_addr" name="<% $ip_field %>" 
   value="<% $opt{'ip_addr'} |h%>" onfocus="clearhint_ip_addr(this)">
 % }
 </td> </tr>
@@ -78,6 +79,7 @@ my $conf = FS::Conf->new;
 
 my $svc_x = $opt{'object'};
 if ( $svc_x ) {
+  # $svc_x->ip_addr does work, even for non-svc_broadband.
   $opt{$_} = $svc_x->$_
     foreach qw(routernum blocknum ip_addr svcpart);
   if ( $svc_x->svcnum ) {
@@ -86,6 +88,8 @@ if ( $svc_x ) {
 }
 
 my $svcpart = $opt{'svcpart'} || '';
+my $ip_field = $opt{'ip_field'} || 'ip_addr';
+
 my %fixed; #  which fields are fixed
 $svcpart =~ /^\d*$/ or die "invalid svcpart '$svcpart'";
 if ( $svcpart ) {
@@ -93,13 +97,13 @@ if ( $svcpart ) {
   # Traditionally, columnflag 'F' on IP address means that it MUST 
   # be auto-assigned (or, if null IP addresses are allowed, that 
   # it must be null).
-  foreach (qw(routernum blocknum ip_addr)) {
+  foreach (qw(routernum blocknum), $ip_field) {
     my $psc = $part_svc->part_svc_column($_);
     if ( $psc and $psc->columnflag eq 'F' ) {
       $fixed{$_} = $psc->columnvalue;
     }
   }
-  if ( $fixed{'routernum'} ) {
+  if ( exists $fixed{'routernum'} ) {
     @routers = (FS::router->by_key($fixed{'routernum'}))
   }
   else {
diff --git a/httemplate/view/svc_acct/basics.html b/httemplate/view/svc_acct/basics.html
index 1cdf776..2d9953f 100644
--- a/httemplate/view/svc_acct/basics.html
+++ b/httemplate/view/svc_acct/basics.html
@@ -91,15 +91,29 @@
 
 % }
 
+<%perl>
+# minor false laziness w/ view/svc_broadband.cgi
+sub slipip {
+  my $svc_acct = shift;
+  my $out = $svc_acct->slipip or return '';
+  if ( $out eq '0.0.0.0' or $out eq '0e0' ) {
+    return '<I>('.mt('Dynamic').'</I>';
+  }
+  $out .= ' ('.
+          include('/elements/popup_link-ping.html', ip => $svc_acct->slipip).
+          ')';
+  if ( my $addr_block = $svc_acct->addr_block ) {
+    $out .= '<br>Netmask: ' . $addr_block->NetAddr->mask .
+            '<br>Gateway: ' . $addr_block->ip_gateway;
+  }
+  $out;
+}
+</%perl>
+
 % if ($svc_acct->slipip) { 
   <& /view/elements/tr.html,
        label=>mt('IP address'),
-       value=> ( $svc_acct->slipip eq "0.0.0.0" || $svc_acct->slipip eq '0e0' )
-                 ? "<I>(".mt('Dynamic').")</I>"
-                 : $svc_acct->slipip. ' '.
-                   include('/elements/popup_link-ping.html',
-                             'ip'=>$svc_acct->slipip,
-                          )
+       value=> slipip($svc_acct)
   &>
 % }