Feature/new status page #2337

whitx · 2017-05-19T13:56:17Z

Description

Status page and device-registration are merged together, and improvements to device-registration

Require

https://github.com/fingerbank/perl-client/pull/29 to be merged

Impacts

Status page
Device Registration page

Delete branch after merge

YES

NEWS file entries

New Features

Re-factor of the status page, to offer more options and be configurable.

extrafu · 2017-05-19T15:24:02Z

conf/documentation.conf

+[device_registration.oses]
+type=fingerbank_select
+description=<<EOT
+Lists of OSES where the MAC vendor will be allowed to be registered via the device registration page.


Shouldn't we just say "List of allowed devices to be registered via the device registration page." ? If we use Fingerbank's device, we don't need to tie this with OS/MAC vendors.

extrafu · 2017-05-19T15:25:30Z

conf/pf.conf.defaults

+# device_registration.oses
+#
+# Lists of OSES where the MAC vendor will be allowed to be registered via the device registration page.
+oses= 


I would prefer "operating_systems" or "operatingsystems" or I as said before, maybe just "allowed_devices"

+1 for allowed_devices

julsemaan · 2017-05-23T14:27:00Z

conf/violations.conf.defaults

+enabled=Y
+template=banned_devices
+auto_enable=N
+user_mail_message=Your devices %mac as been declared as lost or stolen. Please contact your system administrator to be able to use this device again on the network.


*Your device

julsemaan · 2017-05-23T14:27:38Z

html/captive-portal/lib/captiveportal/PacketFence/Controller/DeviceRegistration.pm

@@ -178,7 +178,7 @@ sub registerNode : Private {
            reevaluate_access($mac, 'manage_register');
        }
    } else {
-        $self->showError($c,"Please verify the provided MAC address.");
+        $self->showError($c,"The provided MAC address is not allowed to be register.");


*to be registered using this self-service page.

julsemaan · 2017-05-23T14:28:00Z

html/captive-portal/lib/captiveportal/PacketFence/Controller/LostStolen.pm

+    my $node = node_view($mac);
+    my $owner = lc($node->{pid});
+    my $username = lc($c->user_session->{username});
+    my $vid = "1300005";


Put that in a constant in the violations constants

julsemaan · 2017-05-23T14:29:48Z

html/captive-portal/lib/captiveportal/PacketFence/Controller/LostStolen.pm

+    my ( $self, $c, $mac ) = @_;
+    my $node = node_view($mac);
+    my $owner = lc($node->{pid});
+    my $username = lc($c->user_session->{username});


We'll need to see how we handle the case where the device was registered using a realm

Ex:
bobby@inverse.ca is the owner of the device
But when bobby uses the self-service portal, he logs in as bobby

I think the easiest way would be to strip $owner and $username and ignore the realm

julsemaan · 2017-05-23T14:30:50Z

html/captive-portal/lib/captiveportal/PacketFence/Controller/LostStolen.pm

+
+        if ($trigger) {
+            $c->stash(
+                mac => $mac,


stash mac + template outside of your if and then just stash the status in the if{}else{}

Also stash a $TRUE/$FALSE versus a string and handle it accordingly in your templates/rendering

Just re-read the code and it seems you indeed need a string status since its not only boolean, so forget about my second comment

julsemaan · 2017-05-23T18:51:22Z

lib/pf/web/device_registration.pm

+        my $endpoint = fingerbank::Model::Endpoint->new(name => $device_name, version => undef, score => undef);
+
+        for my $id (@oses) {
+            $logger->debug("The devices type ".$device_name." is authorized to be registered via the device-registration module");


That logging statement is only true if the condition on the line below is true so that statement is false...

Wrap your return + this logging in the if block

julsemaan · 2017-05-23T18:52:07Z

conf/pf.conf.defaults

+# device_registration.oses
+#
+# Lists of OSES where the MAC vendor will be allowed to be registered via the device registration page.
+oses= 


+1 for allowed_devices

julsemaan · 2017-05-23T18:52:39Z

conf/violations.conf.defaults

@@ -251,6 +251,16 @@ template=bandwidth_expiration
 auto_enable=N
 enabled=N

+[1300005]
+priority=1
+actions=email_user,email_admin,log,role


You need the reevaluate action on this instead of setting the role

julsemaan · 2017-05-23T18:53:03Z

conf/violations.conf.defaults

+template=banned_devices
+auto_enable=N
+user_mail_message=Your devices %mac as been declared as lost or stolen. Please contact your system administrator to be able to use this device again on the network.
+target_category=isolation


And you can remove this when you use the reevaluate_access action

julsemaan · 2017-05-23T18:54:25Z

lib/pf/web/device_registration.pm

 use pf::web;
 use pf::web::custom;    # called last to allow redefinitions

 use pf::authentication;
 use pf::Authentication::constants;
 use List::MoreUtils qw(any);

-Readonly our @DEVICE_OUI => _load_file_into_array($allowed_device_oui_file);
-Readonly our @DEVICE_TYPES => _load_file_into_array($allowed_device_types_file);
-
 =head1 SUBROUTINES


A 💅 comment:
We're trying to move away from the lib/pf/web/ directory and remove things that are in it.
I think your code for the device validation would be better placed in the DeviceRegistration controller

whitx · 2017-05-31T17:43:42Z

@cgx might have some CSS improvements to add

dwlfrth · 2017-06-14T14:15:30Z

@whitx rebase

julsemaan · 2017-06-14T16:42:18Z

html/captive-portal/lib/captiveportal/PacketFence/Controller/DeviceRegistration.pm

+    my @oses = @{$Config{'device_registration'}{'allowed_devices'}};
+
+    # If no oses are defined then it will not match any oses
+    return $FALSE if @oses == 0;


Shouldn't it be allowed if no OS is defined in the configuration ?

julsemaan · 2017-06-14T16:46:19Z