From 307a7d85568a15f5eb0d97c648507484108fcc56 Mon Sep 17 00:00:00 2001
From: mark <mark>
Date: Thu, 10 Nov 2011 21:40:05 +0000
Subject: [PATCH] RADIUS groups for svc_broadband, #14695

---
 FS/FS/Conf.pm                                   |   7 +
 FS/FS/nas.pm                                    |   7 +-
 FS/FS/part_export.pm                            |   7 +-
 FS/FS/part_export/broadband_sqlradius.pm        | 100 +++++++++
 FS/FS/part_export/phone_sqlradius.pm            |   1 -
 FS/FS/part_svc.pm                               |  16 +-
 FS/FS/radius_usergroup.pm                       |  14 +-
 FS/FS/svc_Radius_Mixin.pm                       |  94 ++++++++
 FS/FS/svc_acct.pm                               | 137 +-----------
 FS/FS/svc_broadband.pm                          |  11 +-
 FS/bin/freeside-sqlradius-reset                 |   7 +-
 httemplate/browse/part_svc.cgi                  |   6 +-
 httemplate/edit/process/svc_broadband.cgi       |  14 +-
 httemplate/edit/svc_acct.cgi                    |   3 +-
 httemplate/edit/svc_broadband.cgi               |  26 ++-
 httemplate/elements/select-radius_group.html    |  23 +-
 httemplate/elements/tr-fixed.html               |  12 +-
 httemplate/elements/tr-select-radius_group.html |  11 +
 httemplate/view/elements/svc_Common.html        |  14 +-
 httemplate/view/svc_acct/basics.html            |   2 +-
 httemplate/view/svc_broadband.cgi               | 274 +++++-------------------
 21 files changed, 385 insertions(+), 401 deletions(-)
 create mode 100644 FS/FS/part_export/broadband_sqlradius.pm
 create mode 100644 FS/FS/svc_Radius_Mixin.pm
 create mode 100644 httemplate/elements/tr-select-radius_group.html

diff --git a/FS/FS/Conf.pm b/FS/FS/Conf.pm
index 4104fda9c..8d26e91aa 100644
--- a/FS/FS/Conf.pm
+++ b/FS/FS/Conf.pm
@@ -2297,6 +2297,13 @@ and customer address. Include units.',
   },
 
   {
+    'key'         => 'svc_broadband-radius',
+    'section'     => '',
+    'description' => 'Enable RADIUS groups for broadband services.',
+    'type'        => 'checkbox',
+  },
+
+  {
     'key'         => 'svc_acct-alldomains',
     'section'     => '',
     'description' => 'Allow accounts to select any domain in the database.  Normally accounts can only select from the domain set in the service definition and those purchased by the customer.',
diff --git a/FS/FS/nas.pm b/FS/FS/nas.pm
index 4564a6342..af5a23a24 100644
--- a/FS/FS/nas.pm
+++ b/FS/FS/nas.pm
@@ -87,8 +87,11 @@ sub delete {
   my $dbh = dbh;
 
   my $self = shift;
-  my $error = $self->process_m2m([])
-           || $self->SUPER::delete;
+  my $error = $self->process_m2m(
+    link_table    => 'export_nas',
+    target_table  => 'part_export',
+    params        => []
+  ) || $self->SUPER::delete;
 
   if ( $error ) {
     $dbh->rollback;
diff --git a/FS/FS/part_export.pm b/FS/FS/part_export.pm
index f84f2a096..4b60953cf 100644
--- a/FS/FS/part_export.pm
+++ b/FS/FS/part_export.pm
@@ -128,7 +128,12 @@ sub delete {
   local $FS::UID::AutoCommit = 0;
   my $dbh = dbh;
 
-  my $error = $self->SUPER::delete;
+  # clean up export_nas records
+  my $error = $self->process_m2m(
+    'link_table'    => 'export_nas',
+    'target_table'  => 'nas',
+    'params'        => [],
+  ) || $self->SUPER::delete;
   if ( $error ) {
     $dbh->rollback if $oldAutoCommit;
     return $error;
diff --git a/FS/FS/part_export/broadband_sqlradius.pm b/FS/FS/part_export/broadband_sqlradius.pm
new file mode 100644
index 000000000..ae0876ddf
--- /dev/null
+++ b/FS/FS/part_export/broadband_sqlradius.pm
@@ -0,0 +1,100 @@
+package FS::part_export::broadband_sqlradius;
+
+use strict;
+use vars qw($DEBUG @ISA %options %info $conf);
+use Tie::IxHash;
+use FS::Conf;
+use FS::Record qw( dbh str2time_sql ); #qsearch qsearchs );
+use FS::part_export::sqlradius qw(sqlradius_connect);
+
+FS::UID->install_callback(sub { $conf = new FS::Conf });
+
+@ISA = qw(FS::part_export::sqlradius);
+
+$DEBUG = 0;
+
+tie %options, 'Tie::IxHash',
+  'datasrc'  => { label=>'DBI data source ' },
+  'username' => { label=>'Database username' },
+  'password' => { label=>'Database password' },
+  'usergroup'=> { label   => 'Group table',
+                  type    => 'select',
+                  options => [qw( radusergroup usergroup )],
+                },
+# session report doesn't currently know about this export anyway
+#  'hide_ip' => {
+#    type  => 'checkbox',
+#    label => 'Hide IP address on session reports',
+#  },
+  'mac_as_password' => { 
+    type => 'checkbox',
+    default => '1',
+    label => 'Use MAC address as password',
+  },
+  'radius_password' => { label=>'Fixed password' },
+  'ip_addr_as' => { label => 'Send IP address as',
+                    default => 'Framed-IP-Address' },
+;
+
+%info = (
+  'svc'      => 'svc_broadband',
+  'desc'     => 'Real-time export to SQL-backed RADIUS (such as FreeRadius) for broadband services',
+  'options'  => \%options,
+  'nas'      => 'Y',
+  'notes'    => <<END,
+Real-time export of <b>radcheck</b>, <b>radreply</b>, and <b>usergroup</b> 
+tables to any SQL database for 
+<a href="http://www.freeradius.org/">FreeRADIUS</a>
+or <a href="http://radius.innercite.com/">ICRADIUS</a>.
+<br><br>
+
+This export is for broadband service access control based on MAC address.  
+For a more typical RADIUS export, see sqlradius.
+<br><br>
+
+See the
+<a href="http://search.cpan.org/dist/DBI/DBI.pm#connect">DBI documentation</a>
+and the
+<a href="http://search.cpan.org/search?mode=module&query=DBD%3A%3A">documentation for your DBD</a>
+for the exact syntax of a DBI data source.
+
+END
+);
+
+sub rebless { shift; }
+
+sub export_username {
+  my($self, $svc_broadband) = (shift, shift);
+  $svc_broadband->mac_addr;
+}
+
+sub radius_reply {
+  my($self, $svc_broadband) = (shift, shift);
+  my %reply;
+  if (  length($self->option('ip_addr_as',1)) 
+    and length($svc_broadband->ip_addr) ) {
+    $reply{$self->option('ip_addr_as')} = $svc_broadband->ip_addr;
+  }
+  %reply;
+}
+
+sub radius_check {
+  my($self, $svc_broadband) = (shift, shift);
+  my $password_attrib = $conf->config('radius-password') || 'Password';
+  my %check;
+  if ( $self->option('mac_as_password') ) {
+    $check{$password_attrib} = $svc_broadband->mac_addr; #formatting?
+  }
+  elsif ( length( $self->option('radius_password',1)) ) {
+    $check{$password_attrib} = $self->option('radius_password');
+  }
+  %check;
+}
+
+sub _export_suspend {}
+sub _export_unsuspend {}
+
+sub update_svc {} #do nothing
+
+1;
+
diff --git a/FS/FS/part_export/phone_sqlradius.pm b/FS/FS/part_export/phone_sqlradius.pm
index 0816efab6..6b14bed3c 100644
--- a/FS/FS/part_export/phone_sqlradius.pm
+++ b/FS/FS/part_export/phone_sqlradius.pm
@@ -44,7 +44,6 @@ tie %options, 'Tie::IxHash',
   'options'  => \%options,
   'notes'    => <<END,
 Real-time export of <b>radcheck</b> table
-<!--, <b>radreply</b> and <b>usergroup</b>-- tables>
 to any SQL database for <a href="http://www.freeradius.org/">FreeRADIUS</a>
 or <a href="http://radius.innercite.com/">ICRADIUS</a>.
 <br><br>
diff --git a/FS/FS/part_svc.pm b/FS/FS/part_svc.pm
index 249a2b4b9..cc30dbbd4 100644
--- a/FS/FS/part_svc.pm
+++ b/FS/FS/part_svc.pm
@@ -192,6 +192,8 @@ sub insert {
     }
   }
 
+  # XXX shouldn't this update fixed values?
+
   $dbh->commit or die $dbh->errstr if $oldAutoCommit;
 
   '';
@@ -714,11 +716,6 @@ sub process {
   my $old = qsearchs('part_svc', { 'svcpart' => $param->{'svcpart'} }) 
     if $param->{'svcpart'};
 
-  $param->{'svc_acct__usergroup'} =
-    ref($param->{'svc_acct__usergroup'})
-      ? join(',', @{$param->{'svc_acct__usergroup'}} )
-      : $param->{'svc_acct__usergroup'};
-
   #unmunge cgp_accessmodes (falze laziness-ish w/edit/process/svc_acct.cgi)
   $param->{'svc_acct__cgp_accessmodes'} ||=
     join(' ', sort
@@ -737,14 +734,17 @@ sub process {
     } ( fields('part_svc'),
         map { my $svcdb = $_;
               my @fields = fields($svcdb);
-              push @fields, 'usergroup' if $svcdb eq 'svc_acct'; #kludge
+              push @fields, 'usergroup' if $svcdb eq 'svc_acct'
+                                        or $svcdb eq 'svc_broadband'; #kludge
 
               map {
                     my $f = $svcdb.'__'.$_;
-                    if ( $param->{ $f.'_flag' } =~ /^[MAH]$/ ) {
+                    my $flag = $param->{ $f.'_flag' } || ''; #silence warnings
+                    if ( $flag =~ /^[MAH]$/ ) {
                       $param->{ $f } = delete( $param->{ $f.'_classnum' } );
                     }
-		    if ( $param->{ $f.'_flag' } =~ /^S$/ ) {
+		    if ( $flag =~ /^S$/ 
+                          or $_ eq 'usergroup' ) {
                       $param->{ $f } = ref($param->{ $f })
                                          ? join(',', @{$param->{ $f }} )
                                          : $param->{ $f };
diff --git a/FS/FS/radius_usergroup.pm b/FS/FS/radius_usergroup.pm
index 2de142397..8085fe805 100644
--- a/FS/FS/radius_usergroup.pm
+++ b/FS/FS/radius_usergroup.pm
@@ -96,25 +96,29 @@ and replace methods.
 
 sub check {
   my $self = shift;
-
+  my $svcnum = $self->svcnum;
   die "radius_usergroup.groupname is deprecated" if $self->groupname;
 
   $self->ut_numbern('usergroupnum')
-    || $self->ut_foreign_key('svcnum','svc_acct','svcnum')
+    || ( $self->ut_foreign_key('svcnum','svc_acct','svcnum')
+      && $self->ut_foreign_key('svcnum','svc_broadband','svcnum')
+      && "Can't find radius_usergroup.svcnum $svcnum in svc_acct.svcnum or svc_broadband.svcnum" ) 
     || $self->ut_foreign_key('groupnum','radius_group','groupnum')
     || $self->SUPER::check
   ;
 }
 
-=item svc_acct
+=item svc_x
 
-Returns the account associated with this record (see L<FS::svc_acct>).
+Returns the account associated with this record (see L<FS::svc_acct> and 
+L<FS::svc_broadband>).
 
 =cut
 
 sub svc_acct {
   my $self = shift;
-  qsearchs('svc_acct', { svcnum => $self->svcnum } );
+  qsearchs('svc_acct', { svcnum => $self->svcnum } ) ||
+  qsearchs('svc_broadband', { svcnum => $self->svcnum } )
 }
 
 =item radius_group
diff --git a/FS/FS/svc_Radius_Mixin.pm b/FS/FS/svc_Radius_Mixin.pm
new file mode 100644
index 000000000..3a5ce358d
--- /dev/null
+++ b/FS/FS/svc_Radius_Mixin.pm
@@ -0,0 +1,94 @@
+package FS::svc_Radius_Mixin;
+
+use strict;
+use base qw(FS::m2m_Common FS::svc_Common);
+use FS::Record qw(qsearch);
+use FS::radius_group;
+use FS::radius_usergroup;
+use Carp qw(confess);
+
+=head1 NAME
+
+FS::svc_Radius_Mixin - partial base class for services with RADIUS groups
+
+=cut
+
+
+sub insert {
+  my $self = shift;
+  $self->SUPER::insert(@_)
+  || $self->process_m2m(
+    'link_table' => 'radius_usergroup',
+    'target_table' => 'radius_group',
+    'params' => $self->usergroup,
+  );
+}
+
+sub replace  {
+  my $new = shift;
+  my $old = shift;
+  $old = $new->replace_old if !defined($old);
+
+  $old->usergroup; # make sure this is cached for exports
+  $new->process_m2m(
+    'link_table' => 'radius_usergroup',
+    'target_table' => 'radius_group',
+    'params' => $new->usergroup,
+  ) || $new->SUPER::replace($old, @_);
+}
+
+sub delete {
+  my $self = shift;
+  $self->SUPER::delete(@_)
+  || $self->process_m2m(
+    'link_table' => 'radius_usergroup',
+    'target_table' => 'radius_group',
+    'params' => [],
+  );
+}
+
+sub usergroup {
+  my $self = shift;
+  my $value = shift;
+  if ( defined $value ) {
+    if ( ref $value ) {
+      return $self->set('usergroup', $value);
+    }
+    else {
+      return $self->set('usergroup', [ split(/\s*,\s*/, $value) ]);
+    }
+  }
+  $self->get('usergroup') || 
+    # if no argument is passed and usergroup is not set already, 
+    # fetch this service's group assignments
+  $self->set('usergroup', 
+    [ map { $_->groupnum } 
+        qsearch('radius_usergroup', { svcnum => $self->svcnum }) ]
+  );
+}
+
+sub _fieldhandlers {
+  { 
+    'usergroup' => \&usergroup
+  }
+}
+
+=item radius_groups METHOD
+
+Returns a list of RADIUS groups for this service (see L<FS::radius_usergroup>).
+METHOD is the field to return, and can be any method on L<FS::radius_group>.
+Useful values for METHOD include 'groupnum', 'groupname', and 
+'long_description'.  Defaults to 'groupname' for historical reasons.
+
+=cut
+
+sub radius_groups {
+  my $self = shift;
+  my $method = shift || 'groupname';
+  my $groups = join(',', @{$self->usergroup}) || return ();
+  my @groups = qsearch({'table' => 'radius_group',
+                        'extra_sql' => "where groupnum in ($groups)"});
+  return map {$_->$method} @groups;
+}
+
+1;
diff --git a/FS/FS/svc_acct.pm b/FS/FS/svc_acct.pm
index 37c55d67c..12d3b8ecf 100644
--- a/FS/FS/svc_acct.pm
+++ b/FS/FS/svc_acct.pm
@@ -1,7 +1,10 @@
 package FS::svc_acct;
 
 use strict;
-use base qw( FS::svc_Domain_Mixin FS::svc_CGP_Mixin FS::svc_CGPRule_Mixin
+use base qw( FS::svc_Domain_Mixin
+             FS::svc_CGP_Mixin
+             FS::svc_CGPRule_Mixin
+             FS::svc_Radius_Mixin
              FS::svc_Common );
 use vars qw( $DEBUG $me $conf $skip_fuzzyfiles
              $dir_prefix @shells $usernamemin
@@ -339,6 +342,7 @@ sub table_info {
                          type  => 'select-radius_group.html',
                          disable_inventory => 1,
                          disable_select => 1,
+                         multiple => 1,
                        },
         'seconds'   => { label => 'Seconds',
                          label_sort => 'with Time Remaining',
@@ -531,22 +535,6 @@ sub table { 'svc_acct'; }
 
 sub table_dupcheck_fields { ( 'username', 'domsvc' ); }
 
-sub _fieldhandlers {
-  {
-    #false laziness with edit/svc_acct.cgi
-    'usergroup' => sub { 
-                         my( $self, $groups ) = @_;
-                         if ( ref($groups) eq 'ARRAY' ) {
-                           $groups;
-                         } elsif ( length($groups) ) {
-                           [ split(/\s*,\s*/, $groups) ];
-                         } else {
-                           [];
-                         }
-                       },
-  };
-}
-
 sub last_login {
   shift->_lastlog('in', @_);
 }
@@ -699,7 +687,7 @@ sub insert {
   my $dbh = dbh;
 
   my @jobnums;
-  my $error = $self->SUPER::insert(
+  my $error = $self->SUPER::insert( # usergroup is here
     'jobnums'       => \@jobnums,
     'child_objects' => $self->child_objects,
     %options,
@@ -709,20 +697,6 @@ sub insert {
     return $error;
   }
 
-  if ( $self->usergroup ) {
-    foreach my $groupnum ( @{$self->usergroup} ) {
-      my $radius_usergroup = new FS::radius_usergroup ( {
-        svcnum    => $self->svcnum,
-        groupnum  => $groupnum,
-      } );
-      my $error = $radius_usergroup->insert;
-      if ( $error ) {
-        $dbh->rollback if $oldAutoCommit;
-        return $error;
-      }
-    }
-  }
-
   unless ( $skip_fuzzyfiles ) {
     $error = $self->queue_fuzzyfiles_update;
     if ( $error ) {
@@ -935,22 +909,12 @@ sub delete {
     }
   }
 
-  my $error = $self->SUPER::delete;
+  my $error = $self->SUPER::delete; # usergroup here
   if ( $error ) {
     $dbh->rollback if $oldAutoCommit;
     return $error;
   }
 
-  foreach my $radius_usergroup (
-    qsearch('radius_usergroup', { 'svcnum' => $self->svcnum } )
-  ) {
-    my $error = $radius_usergroup->delete;
-    if ( $error ) {
-      $dbh->rollback if $oldAutoCommit;
-      return $error;
-    }
-  }
-
   $dbh->commit or die $dbh->errstr if $oldAutoCommit;
   '';
 }
@@ -1011,49 +975,7 @@ sub replace {
   local $FS::UID::AutoCommit = 0;
   my $dbh = dbh;
 
-  # redundant, but so $new->usergroup gets set
-  $error = $new->check;
-  return $error if $error;
-
-  $old->usergroup( [ $old->radius_groups('NUMBERS') ] );
-  if ( $DEBUG ) {
-    warn $old->email. " old groups: ". join(' ',@{$old->usergroup}). "\n";
-    warn $new->email. " new groups: ". join(' ',@{$new->usergroup}). "\n";
-  }
-  if ( $new->usergroup ) {
-    #(sorta) false laziness with FS::part_export::sqlradius::_export_replace
-    my @newgroups = @{$new->usergroup};
-    foreach my $oldgroup ( @{$old->usergroup} ) {
-      if ( grep { $oldgroup eq $_ } @newgroups ) {
-        @newgroups = grep { $oldgroup ne $_ } @newgroups;
-        next;
-      }
-      my $radius_usergroup = qsearchs('radius_usergroup', {
-        svcnum    => $old->svcnum,
-        groupnum  => $oldgroup,
-      } );
-      my $error = $radius_usergroup->delete;
-      if ( $error ) {
-        $dbh->rollback if $oldAutoCommit;
-        return "error deleting radius_usergroup $oldgroup: $error";
-      }
-    }
-
-    foreach my $newgroup ( @newgroups ) {
-      my $radius_usergroup = new FS::radius_usergroup ( {
-        svcnum    => $new->svcnum,
-        groupnum => $newgroup,
-      } );
-      my $error = $radius_usergroup->insert;
-      if ( $error ) {
-        $dbh->rollback if $oldAutoCommit;
-        return "error adding radius_usergroup $newgroup: $error";
-      }
-    }
-
-  }
-
-  $error = $new->SUPER::replace($old, @_);
+  $error = $new->SUPER::replace($old, @_); # usergroup here
   if ( $error ) {
     $dbh->rollback if $oldAutoCommit;
     return $error if $error;
@@ -1191,15 +1113,10 @@ sub check {
 
   my($recref) = $self->hashref;
 
-  my $x = $self->setfixed( $self->_fieldhandlers );
+  my $x = $self->setfixed;
   return $x unless ref($x);
   my $part_svc = $x;
 
-  if ( $part_svc->part_svc_column('usergroup')->columnflag eq "F" ) {
-    $self->usergroup(
-      [ split(',', $part_svc->part_svc_column('usergroup')->columnvalue) ] );
-  }
-
   my $error = $self->ut_numbern('svcnum')
               #|| $self->ut_number('domsvc')
               || $self->ut_foreign_key( 'domsvc', 'svc_domain', 'svcnum' )
@@ -2549,41 +2466,7 @@ sub get_cdrs {
 
 }
 
-=item radius_groups
-
-Returns all RADIUS groups for this account (see L<FS::radius_usergroup>).
-
-=cut
-
-sub radius_groups {
-  my $self = shift;
-  if ( $self->usergroup ) {
-    confess "explicitly specified usergroup not an arrayref: ". $self->usergroup
-      unless ref($self->usergroup) eq 'ARRAY';
-    #when provisioning records, export callback runs in svc_Common.pm before
-    #radius_usergroup records can be inserted...
-    my $groups = join(',',@{$self->usergroup});
-    my @groups;
-    return @groups unless length($groups);
-    @groups = qsearch({ 'table'         => 'radius_group',
-                           'extra_sql'     => "where groupnum in ($groups)",
-                        });
-    map { $_->groupname } @groups;
-  } else {
-     my $format = shift || '';
-     my @groups = qsearch({ 'table'         => 'radius_usergroup',
-                            'addl_from'     => 'left join radius_group using (groupnum)',
-                            'select'        => 'radius_group.*',
-                            'hashref'       => { 'svcnum' => $self->svcnum },
-                        });
-
-     # this is to preserve various legacy behaviour / avoid re-writing other code
-     return map { $_->groupnum } @groups if $format eq 'NUMBERS';
-     return map { $_->description . " (" . $_->groupname . ")" } @groups
-        if $format eq 'COMBINED';
-     map { $_->groupname } @groups;
-  }
-}
+# sub radius_groups has moved to svc_Radius_Mixin
 
 =item clone_suspended
 
diff --git a/FS/FS/svc_broadband.pm b/FS/FS/svc_broadband.pm
index 576684c56..ad7dedcdd 100755
--- a/FS/FS/svc_broadband.pm
+++ b/FS/FS/svc_broadband.pm
@@ -9,7 +9,7 @@ use FS::addr_block;
 use FS::part_svc_router;
 use NetAddr::IP;
 
-@ISA = qw( FS::svc_Common );
+@ISA = qw( FS::svc_Radius_Mixin FS::svc_Common );
 
 $FS::UID::callback{'FS::svc_broadband'} = sub { 
   $conf = new FS::Conf;
@@ -115,6 +115,15 @@ sub table_info {
      'longitude'    => 'Longitude',
      'altitude'     => 'Altitude',
      'vlan_profile' => 'VLAN profile',
+     'usergroup'    => { 
+                         label => 'RADIUS groups',
+                         type  => 'select-radius_group.html',
+                         #select_table => 'radius_group',
+                         #select_key   => 'groupnum',
+                         #select_label => 'groupname',
+                         disable_inventory => 1,
+                         multiple => 1,
+                       },
     },
   };
 }
diff --git a/FS/bin/freeside-sqlradius-reset b/FS/bin/freeside-sqlradius-reset
index a77bad64f..c8da60a4e 100755
--- a/FS/bin/freeside-sqlradius-reset
+++ b/FS/bin/freeside-sqlradius-reset
@@ -19,14 +19,14 @@ adminsuidsetup $user;
 my @exports = ();
 if ( @ARGV ) {
   foreach my $exportnum ( @ARGV ) {
-    foreach my $exporttype (qw( sqlradius sqlradius_withdomain phone_sqlradius )) {
+    foreach my $exporttype (qw( sqlradius sqlradius_withdomain ohone_sqlradius broadband_sqlradius )) {
     push @exports, qsearch('part_export', { exportnum  => $exportnum,
                                             exporttype => $exporttype, } );
     }
   }
  } else {
   @exports = qsearch('part_export', { exporttype=>'sqlradius' } );
-  push @exports, qsearch('part_export', { exporttype=>'sqlradius_withdomain' } );
+  push @exports, qsearch('part_export', { exporttype=>'sqlradius_withdomain' } ); 
 }
 
 unless ( $opt_n ) {
@@ -34,7 +34,8 @@ unless ( $opt_n ) {
     my $icradius_dbh = DBI->connect(
       map { $export->option($_) } qw( datasrc username password )
     ) or die $DBI::errstr;
-    for my $table (qw( radcheck radreply usergroup )) {
+    my $usergroup = $export->option('usergroup') || 'usergroup';
+    for my $table (qw( radcheck radreply ), $usergroup) {
       my $sth = $icradius_dbh->prepare("DELETE FROM $table");
       $sth->execute or die "Can't reset $table table: ". $sth->errstr;
     }
diff --git a/httemplate/browse/part_svc.cgi b/httemplate/browse/part_svc.cgi
index 4549e44dd..1cd0943bb 100755
--- a/httemplate/browse/part_svc.cgi
+++ b/httemplate/browse/part_svc.cgi
@@ -60,12 +60,14 @@ function part_export_areyousure(href) {
     <TH COLSPAN=2 CLASS="grid" BGCOLOR="#cccccc">Modifier</TH>
 
   </TR>
-
+% my $conf = FS::Conf->new;
 % foreach my $part_svc ( @part_svc ) {
 %     my $svcdb = $part_svc->svcdb;
 %     my $svc_x = "FS::$svcdb"->new( { svcpart => $part_svc->svcpart } );
 %     my @dfields = $svc_x->fields;
-%     push @dfields, 'usergroup' if $svcdb eq 'svc_acct'; #kludge
+%     push @dfields, 'usergroup' if $svcdb eq 'svc_acct' #double kludge
+%                                or ($svcdb eq 'svc_broadband' 
+%                                    and $conf->exists('svc_broadband-radius'));
 %     my @fields =
 %       grep { my $col = $part_svc->part_svc_column($_);
 %              my $def = FS::part_svc->svc_table_fields($svcdb)->{$_};
diff --git a/httemplate/edit/process/svc_broadband.cgi b/httemplate/edit/process/svc_broadband.cgi
index d5c9820bb..36c64d11a 100644
--- a/httemplate/edit/process/svc_broadband.cgi
+++ b/httemplate/edit/process/svc_broadband.cgi
@@ -1,8 +1,20 @@
-<% include('elements/svc_Common.html', 'table' => 'svc_broadband') %>
+<& elements/svc_Common.html,
+  table       => 'svc_broadband',
+  fields      => [ fields('svc_broadband'), 'usergroup' ],
+  precheck_callback => \&precheck,
+&>
 <%init>
+# for historical reasons, process_m2m for usergroup tables is done 
+# in the svc_x::insert/replace/delete methods, not here
 my $curuser = $FS::CurrentUser::CurrentUser;
 
 die "access denied"
   unless $curuser->access_right('Provision customer service'); #something else more specific?
 
+sub precheck {
+  my $cgi = shift;
+  $cgi->param("usergroup", [ $cgi->param('usergroup') ]);
+  ''
+}
+
 </%init>
diff --git a/httemplate/edit/svc_acct.cgi b/httemplate/edit/svc_acct.cgi
index 61058aeb2..52fbd37af 100755
--- a/httemplate/edit/svc_acct.cgi
+++ b/httemplate/edit/svc_acct.cgi
@@ -312,7 +312,7 @@ function randomPass() {
 % } else { 
 %   my $radius_group_selected = '';
 %   if ( $svc_acct->svcnum ) {
-%      $radius_group_selected = join(',',$svc_acct->radius_groups('NUMBERS'));
+%      $radius_group_selected = join(',',$svc_acct->radius_groups('groupnum'));
 %   }
 %   elsif ( !$svc_acct->svcnum && $part_svc_usergroup->columnflag eq 'D' ) {
 %       $radius_group_selected = $part_svc_usergroup->columnvalue;
@@ -320,6 +320,7 @@ function randomPass() {
     <TD><& /elements/select-radius_group.html, 
                 curr_value => $radius_group_selected,
                 element_name => 'radius_usergroup',
+                multiple => 1,
         &>
     </TD>
 % } 
diff --git a/httemplate/edit/svc_broadband.cgi b/httemplate/edit/svc_broadband.cgi
index a67f6f05a..d4baf35a0 100644
--- a/httemplate/edit/svc_broadband.cgi
+++ b/httemplate/edit/svc_broadband.cgi
@@ -1,4 +1,4 @@
-<% include('elements/svc_Common.html',
+<& elements/svc_Common.html,
      'post_url'             => popurl(1). 'process/svc_broadband.cgi',
      'name'                 => 'broadband service',
      'table'                => 'svc_broadband',
@@ -7,8 +7,7 @@
      'dummy'                => $cgi->query_string,
      'onsubmit'             => 'validate_coords',
      'html_foot'            => $js,
-     )
-%>
+&>
 <%init>
 
 die "access denied"
@@ -100,9 +99,19 @@ END
 my @fields = (
   qw( description ip_addr speed_down speed_up blocknum ),
   { field=>'block_label', type=>'fixed' },
-  qw( mac_addr latitude longitude altitude vlan_profile performance_profile authkey plan_id )
+  qw( mac_addr latitude longitude altitude vlan_profile 
+      performance_profile authkey plan_id ),
 );
 
+if ( $conf->exists('svc_broadband-radius') ) {
+  push @fields,
+  { field     => 'usergroup',
+    type      => 'select-radius_group',
+    multiple  => 1,
+  }
+}
+
+
 my $fixedblock = '';
 
 my $callback = sub {
@@ -116,10 +125,17 @@ my $callback = sub {
 
   my $columndef = $part_svc->part_svc_column($fieldref->{'field'});
   if ($columndef->columnflag eq 'F') {
-    $fieldref->{'type'} = 'fixed';
+    $fieldref->{'type'} = length($columndef->columnvalue)
+                            ? 'fixed'
+                            : 'hidden';
     $fieldref->{'value'} = $columndef->columnvalue;
     $fixedblock = $fieldref->{value}
       if $fieldref->{field} eq 'blocknum';
+
+    if ( $fieldref->{field} eq 'usergroup' ) {
+      $fieldref->{'formatted_value'} = 
+        [ $object->radius_groups('long_description') ];
+    }
   }
 
   if ($object->svcnum) { 
diff --git a/httemplate/elements/select-radius_group.html b/httemplate/elements/select-radius_group.html
index e1e3c5980..06972b522 100644
--- a/httemplate/elements/select-radius_group.html
+++ b/httemplate/elements/select-radius_group.html
@@ -1,17 +1,6 @@
-<SELECT MULTIPLE NAME = "<% $opt{'element_name'} || $opt{'field'} || 'usergroup' %>"
-        <% $opt{'element_etc'} %>
->
-% foreach my $selopt ( keys %groups ) {
-%  my $selected = (grep{ $_ eq $selopt } @sel_groups) ? 'SELECTED' : '';
-    <OPTION VALUE="<%$selopt%>" <% $selected %>><% $groups{$selopt} %></OPTION>
-% }
-</SELECT>
-<%init>
-
-my %opt = @_;
-
-my %groups = map { $_->groupnum => $_->long_description }
-                                                    qsearch('radius_group', {});
-my @sel_groups = split(/,/,$opt{'curr_value'});
-
-</%init>
+<& /elements/select-table.html,
+    table       => 'radius_group',
+    name_col    => 'long_description',
+    order_by    => 'groupname', # better idea?
+    @_ 
+&>
diff --git a/httemplate/elements/tr-fixed.html b/httemplate/elements/tr-fixed.html
index 095e1bce9..f358343dd 100644
--- a/httemplate/elements/tr-fixed.html
+++ b/httemplate/elements/tr-fixed.html
@@ -1,6 +1,6 @@
 <% include('tr-td-label.html', @_ ) %>
 
-  <TD BGCOLOR="#dddddd" <% $style %>><% $opt{'formatted_value'} || $opt{'curr_value'} || $opt{'value'} |h %></TD>
+  <TD BGCOLOR="#dddddd" <% $style %>><% $value %></TD>
 
 </TR>
 
@@ -12,4 +12,14 @@ my %opt = @_;
 
 my $style = $opt{'cell_style'} ? 'STYLE="'. $opt{'cell_style'}. '"' : '';
 
+my $value = $opt{'formatted_value'} || $opt{'curr_value'} || $opt{'value'};
+#compatibility with select-table and friends
+if ( $opt{'multiple'} ) {
+  $value = [ split(/\s*,\s*/, $value) ] if !ref $value;
+  $value = join('<BR>', map {encode_entities($_)} @$value);
+}
+else {
+  $value = encode_entities($value)
+}
+
 </%init>
diff --git a/httemplate/elements/tr-select-radius_group.html b/httemplate/elements/tr-select-radius_group.html
new file mode 100644
index 000000000..299252755
--- /dev/null
+++ b/httemplate/elements/tr-select-radius_group.html
@@ -0,0 +1,11 @@
+<% include('tr-td-label.html', label => emt('RADIUS groups'), %opt ) %>
+  <TD <% $style %>>
+  <% include( '/elements/select-radius_group.html', %opt ) %>
+  </TD>
+</TR>
+<%init>
+
+my( %opt ) = @_;
+my $style = $opt{'cell_style'} ? 'STYLE="'. $opt{'cell_style'}. '"' : '';
+
+</%init>
diff --git a/httemplate/view/elements/svc_Common.html b/httemplate/view/elements/svc_Common.html
index 38fa90eb9..a822412bf 100644
--- a/httemplate/view/elements/svc_Common.html
+++ b/httemplate/view/elements/svc_Common.html
@@ -159,13 +159,13 @@ my($label, $value, $svcdb) = $cust_svc->label;
 
 my $part_svc = $cust_svc->part_svc;
 
-  #false laziness w/edit/svc_Common.html
-  #override default labels with service-definition labels if applicable
-  my $labels = $opt{labels}; #not -> here
-  foreach my $field ( keys %$labels ) {
-    my $col = $part_svc->part_svc_column($field);
-    $labels->{$field} = $col->columnlabel if $col->columnlabel !~ /^\S*$/;
-  }
+#false laziness w/edit/svc_Common.html
+#override default labels with service-definition labels if applicable
+my $labels = $opt{labels}; #not -> here
+foreach my $field ( keys %$labels ) {
+  my $col = $part_svc->part_svc_column($field);
+  $labels->{$field} = $col->columnlabel if $col->columnlabel !~ /^\s*$/;
+}
 
 my $pkgnum = $cust_svc->pkgnum;
 
diff --git a/httemplate/view/svc_acct/basics.html b/httemplate/view/svc_acct/basics.html
index 6a0ed9260..a253e3bf7 100644
--- a/httemplate/view/svc_acct/basics.html
+++ b/httemplate/view/svc_acct/basics.html
@@ -100,7 +100,7 @@
 % } 
 
 <& /view/elements/tr.html, label=>mt('RADIUS groups'),
-                      value=>join('<BR>', $svc_acct->radius_groups('COMBINED')) &>
+    value=>join('<BR>', $svc_acct->radius_groups('long_description')) &>
 
 %# Can this be abstracted further?  Maybe a library function like
 %# widget('HTML', 'view', $svc_acct) ?  It would definitely make UI 
diff --git a/httemplate/view/svc_broadband.cgi b/httemplate/view/svc_broadband.cgi
index dead70b7f..627775992 100644
--- a/httemplate/view/svc_broadband.cgi
+++ b/httemplate/view/svc_broadband.cgi
@@ -1,225 +1,63 @@
-<%include("/elements/header.html",'Broadband Service View', menubar(
-  ( ( $custnum )
-    ? ( "View this customer (#$display_custnum)" => "${p}view/cust_main.cgi?$custnum",
-      )                                                                       
-    : ( "Cancel this (unaudited) website" =>
-          "${p}misc/cancel-unaudited.cgi?$svcnum" )
-  )
-))
-%>
-
-<% include('/elements/init_overlib.html') %>
-
-<% include('/view/elements/svc_edit_link.html', 'svc'=>$svc_broadband) %>
-<BR>
-<%ntable("#cccccc")%>
-  <TR>
-    <TD>
-      <%ntable("#cccccc",2)%>
-        <TR>
-          <TD ALIGN="right">Service number</TD>
-          <TD BGCOLOR="#ffffff"><%$svcnum%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">Description</TD>
-          <TD BGCOLOR="#ffffff"><%$description%></TD>
-        </TR>
-
-%       if ( $router ) {
-          <TR>
-            <TD ALIGN="right">Router</TD>
-            <TD BGCOLOR="#ffffff"><%$router->routernum%>: <%$router->routername%></TD>
-          </TR>
-%       }
-
-        <TR>
-          <TD ALIGN="right">Download Speed</TD>
-          <TD BGCOLOR="#ffffff"><%$speed_down%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">Upload Speed</TD>
-          <TD BGCOLOR="#ffffff"><%$speed_up%></TD>
-        </TR>
-
-%       if ( $ip_addr ) { 
-          <TR>
-            <TD ALIGN="right">IP Address</TD>
-            <TD BGCOLOR="#ffffff">
-              <%$ip_addr%>
-              (<% include('/elements/popup_link-ping.html', 'ip'=>$ip_addr ) %>)
-            </TD>
-          </TR>
-          <TR>
-            <TD ALIGN="right">IP Netmask</TD>
-            <TD BGCOLOR="#ffffff"><%$addr_block->NetAddr->mask%></TD>
-          </TR>
-          <TR>
-            <TD ALIGN="right">IP Gateway</TD>
-            <TD BGCOLOR="#ffffff"><%$addr_block->ip_gateway%></TD>
-          </TR>
-%       }
-
-        <TR>
-          <TD ALIGN="right">MAC Address</TD>
-          <TD BGCOLOR="#ffffff"><%$mac_addr%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">Latitude</TD>
-          <TD BGCOLOR="#ffffff"><%$latitude%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">Longitude</TD>
-          <TD BGCOLOR="#ffffff"><%$longitude%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">Altitude</TD>
-          <TD BGCOLOR="#ffffff"><%$altitude%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">VLAN Profile</TD>
-          <TD BGCOLOR="#ffffff"><%$vlan_profile%></TD>
-        </TR>
-        <TR>
-          <TD ALIGN="right">Authentication Key</TD>
-          <TD BGCOLOR="#ffffff"><%$auth_key%></TD>
-        </TR>
-	<TR>
-          <TD ALIGN="right">Service Plan Id</TD>
-          <TD BGCOLOR="#ffffff"><%$plan_id%></TD>
-        </TR>
-        <TR COLSPAN="2"><TD></TD></TR>
-%
-%foreach (sort { $a cmp $b } $svc_broadband->virtual_fields) {
-%  print $svc_broadband->pvf($_)->widget('HTML', 'view',
-%                                        $svc_broadband->getfield($_)), "\n";
-%}
-%
-%
-
-      </TABLE>
-    </TD>
-  </TR>
-</TABLE>
-
-<BR>
-<%ntable("#cccccc", 2)%>
-%
-%  my $sb_router = qsearchs('router', { svcnum => $svcnum });
-%  if ($sb_router) {
-%  
-
-  <B>Router associated: <%$sb_router->routername%> </B>
-  <A HREF="<%popurl(2)%>edit/router.cgi?<%$sb_router->routernum%>">
-    (details)
-  </A>
-  <BR>
-% my @sb_addr_block;
-%     if (@sb_addr_block = $sb_router->addr_block) {
-%     
-
-  <B>Address space </B>
-  <A HREF="<%popurl(2)%>browse/addr_block.cgi">
-    (edit)
-  </A>
-  <BR>
-%   print ntable("#cccccc", 1);
-%       foreach (@sb_addr_block) { 
-
-    <TR>
-      <TD><%$_->ip_gateway%>/<%$_->ip_netmask%></TD>
-    </TR>
-% } 
-
-  </TABLE>
-% } else { 
-
-  <B>No address space allocated.</B>
-% } 
-
-  <BR>
-%
-%  } else {
-%
-
-
-<FORM METHOD="GET" ACTION="<%popurl(2)%>edit/router.cgi">
-  <INPUT TYPE="hidden" NAME="svcnum" VALUE="<%$svcnum%>">
-Add router named 
-  <INPUT TYPE="text" NAME="routername" SIZE="32" VALUE="Broadband router (<%$svcnum%>)">
-  <INPUT TYPE="submit" VALUE="Add router">
-</FORM>
-%
-%}
-%
-
-
-<BR>
-<%joblisting({'svcnum'=>$svcnum}, 1)%>
-
-<% include('/elements/footer.html') %>
+<& elements/svc_Common.html,
+  table   => 'svc_broadband',
+  labels  => \%labels,
+  fields  => \@fields,
+&>
 <%init>
 
-die "access denied"
-  unless $FS::CurrentUser::CurrentUser->access_right('View customer services');
-
-my($query) = $cgi->keywords;
-$query =~ /^(\d+)$/;
-my $svcnum = $1;
-my $svc_broadband = qsearchs({
-  'select'    => 'svc_broadband.*',
-  'table'     => 'svc_broadband',
-  'addl_from' => ' LEFT JOIN cust_svc  USING ( svcnum  ) '.
-                 ' LEFT JOIN cust_pkg  USING ( pkgnum  ) '.
-                 ' LEFT JOIN cust_main USING ( custnum ) ',
-  'hashref'   => { 'svcnum' => $svcnum },
-  'extra_sql' => ' AND '. $FS::CurrentUser::CurrentUser->agentnums_sql(
-                            'null_right' => 'View/link unlinked services'
-                          ),
-}) or die "svc_broadband: Unknown svcnum $svcnum";
-
-#false laziness w/all svc_*.cgi
-my $cust_svc = qsearchs( 'cust_svc', { 'svcnum' => $svcnum } );
-my $pkgnum = $cust_svc->getfield('pkgnum');
-my($cust_pkg, $custnum, $display_custnum);
-if ($pkgnum) {
-  $cust_pkg = qsearchs( 'cust_pkg', { 'pkgnum' => $pkgnum } );
-  $custnum = $cust_pkg->custnum;
-  $display_custnum = $cust_pkg->cust_main->display_custnum;
-} else {
-  $cust_pkg = '';
-  $custnum = '';
+my $conf = FS::Conf->new;
+my $fields = FS::svc_broadband->table_info->{'fields'};
+my %labels = map { $_ => ( ref($fields->{$_}) 
+                            ? $fields->{$_}{'label'} 
+                            : $fields->{$_}
+                          );
+                 } keys %$fields;
+
+$labels{'router'} = emt('Router');
+$labels{'usergroup'} = emt('RADIUS groups'); #?
+
+my @fields = (
+  'description',
+  { field => 'router', value => \&router },
+  'speed_down',
+  'speed_up',
+  { field => 'ip_addr', value => \&ip_addr },
+  'mac_addr',
+  'latitude',
+  'longitude',
+  'altitude',
+  'vlan_profile',
+  'authkey',
+  'plan_id',
+);
+
+push @fields,
+  { field => 'usergroup', value => \&usergroup }
+  if $conf->exists('svc_broadband-radius');
+
+sub router {
+  my $svc = shift;
+  my $addr_block = $svc->addr_block or return '';
+  my $router = $addr_block->router or return '';
+  $router->routernum . ': ' . $router->routername;
 }
-#eofalse
 
-my $addr_block = $svc_broadband->addr_block;
-my $router = $addr_block->router if $addr_block;
-
-#if (not $router) { die "Could not lookup router for svc_broadband (svcnum $svcnum)" };
+sub ip_addr {
+  my $svc = shift;
+  my $ip_addr = $svc->ip_addr;
+  my $out = $ip_addr . ' (' . 
+    include('/elements/popup_link-ping.html', ip => $ip_addr) . ')';
+  if ( my $addr_block = $svc->addr_block ) {
+    $out .= '<br>Netmask: ' . $addr_block->NetAddr->mask .
+            '<br>Gateway: ' . $addr_block->ip_gateway;
+  }
+  $out;
+}
 
-my (
-     $speed_down,
-     $speed_up,
-     $ip_addr,
-     $mac_addr,
-     $latitude,
-     $longitude,
-     $altitude,
-     $vlan_profile,
-     $auth_key,
-     $description,
-     $plan_id,
-   ) = (
-     $svc_broadband->getfield('speed_down'),
-     $svc_broadband->getfield('speed_up'),
-     $svc_broadband->getfield('ip_addr'),
-     $svc_broadband->mac_addr,
-     $svc_broadband->latitude,
-     $svc_broadband->longitude,
-     $svc_broadband->altitude,
-     $svc_broadband->vlan_profile,
-     $svc_broadband->auth_key,
-     $svc_broadband->description,
-     $svc_broadband->plan_id,
-   );
+sub usergroup {
+  my $svc = shift;
+  my $usergroup = $svc->usergroup;
+  join('<BR>', $svc->radius_groups('long_description'));
+}
 
 </%init>
-- 
2.11.0