[WIP] OPNblock, first dev version #808

alectrocute · 2018-08-27T16:04:44Z

It has some problems and I'd love for critique/assistance getting it up to par. Thanks so much! :)

fabianfrz · 2018-08-27T16:43:44Z

dns/opnblock/+POST_INSTALL

@@ -0,0 +1,3 @@
+if /usr/local/etc/rc.d/configd status > /dev/null; then
+        /usr/local/etc/rc.d/configd restart
+fi


shouldn't it do this automatically?

Yeah, you're right. I'll change.

fabianfrz

add review comments

fabianfrz · 2018-08-27T16:45:51Z

dns/opnblock/src/opnsense/mvc/app/models/OPNsense/opnblock/opnblock.php

+<?php
+namespace OPNsense\opnblock;
+use OPNsense\Base\BaseModel;
+class opnblock extends BaseModel


classes must be camel case - in your case probably OpnBlock

fabianfrz · 2018-08-27T16:46:58Z

dns/opnblock/src/opnsense/mvc/app/models/OPNsense/opnblock/opnblock.xml

+            <Enabled type="BooleanField">
+                <default>0</default>
+                <Required>Y</Required>
+            </Enabled>


keep all naming consistent - downcase to enabled so it matches the rest of your model

fabianfrz · 2018-08-27T16:48:16Z

dns/opnblock/src/opnsense/mvc/app/views/OPNsense/opnblock/index.volt

+            <li>whatever else you suggest?</li>
+        </ul>
+    </p>
+    <br> <span style="font-family: Courier; font-size: 13px; border-radius: 5px 5px 5px 5px; background-color: #fff; color: #000; padding: 5px; margin: 5px;">You're using OPNblock version: 0.1-dev</span>


all strings should be translatable - this may be an exception because it looks like it should be removed some day.

Definitely, I'd like to rewrite that whole section on index.volt soon; however at this current moment it's the last thing I'll do; after everything is functional! :)

You can find a complete list here:
https://github.com/opnsense/core/tree/master/src/opnsense/mvc/app/models/OPNsense/Base/FieldTypes

Sorry, the comment should be an answer to another comment

fabianfrz · 2018-08-27T16:48:35Z

dns/opnblock/src/opnsense/mvc/app/views/OPNsense/opnblock/index.volt

+    </p>
+    <br> <span style="font-family: Courier; font-size: 13px; border-radius: 5px 5px 5px 5px; background-color: #fff; color: #000; padding: 5px; margin: 5px;">You're using OPNblock version: 0.1-dev</span>
+    <br>
+    <br> </div>


useless breaks

fabianfrz · 2018-08-27T16:50:21Z

dns/opnblock/src/opnsense/scripts/OPNsense/opnblock/opnblock.sh

+# opnblock, opnblock.sh
+
+# prep temp storage and conf file
+rm -r '/tmp/hosts.working' '/var/unbound/opnblock.conf'


why -r - it is a file. It stands for recursive to delete directories and their contents.

Typo! Good catch!

fabianfrz · 2018-08-27T16:51:18Z

dns/opnblock/src/opnsense/mvc/app/views/OPNsense/opnblock/index.volt

+        $("#saveAct").click(function () {
+            saveFormToEndpoint(url = "/api/opnblock/settings/set", formid = 'frm_GeneralSettings', callback_ok = function () {
+                ajaxCall(url = "/api/opnblock/service/refresh", sendData = {}, callback = function (data, status) {
+                    $("#responseMsg").html(data['message']);


.text is more secure (does not support html formatting)

fabianfrz · 2018-08-27T16:52:46Z

dns/opnblock/src/opnsense/mvc/app/controllers/OPNsense/opnblock/IndexController.php

+        $this->view->general = $this->getForm("general");
+        $this->view->pick('OPNsense/opnblock/index');
+
+        $this->view->generalForm = $this->getForm("general");


please don't create redundant variables. You can very likely use the same on all places.

fabianfrz · 2018-08-27T16:54:05Z

dns/opnblock/src/opnsense/mvc/app/controllers/OPNsense/opnblock/Api/SettingsController.php

+use \OPNsense\Base\ApiControllerBase;
+use \OPNsense\opnblock\opnblock;
+use \OPNsense\Core\Config;
+class SettingsController extends ApiControllerBase


use ApiMutableModelControllerBase instead. It will reduce the controller code to two lines.

dns/opnblock/src/opnsense/mvc/app/controllers/OPNsense/opnblock/forms/general.xml

marjohn56 · 2018-08-27T18:38:46Z

@alec - Clean Opnsense installlation. Following your instructions https://github.com/al-ec/OPNblock

package installs fine, but no OPNblock menu item appears. Something changed because I did have a menu option a couple of days ago. :)

fabianfrz · 2018-08-27T18:45:09Z

@marjohn56 try to delete the menu cache under /tmp. If it appears, the problem is solved.

marjohn56 · 2018-08-27T18:55:32Z

@alec - No luck. Tried that on my dev system, still the same. Installed onto my live system, no menu item there either... hang on, found it... It's appearing under Lobby.

Methinks it should not be there. :)

marjohn56 · 2018-08-27T18:59:15Z

Getting this when I click on the menu item now..

Notice: Undefined variable: generalForm in /usr/local/opnsense/mvc/app/cache/_usr_local_opnsense_mvc_app_views_opnsense_opnblock_index.volt.php on line 20

fabianfrz · 2018-08-27T20:07:48Z

@marjohn56 he removed it as I requested today but it seems like he forgot to adjust the view. Give him some time.

alectrocute · 2018-08-27T21:43:04Z

@marjohn56 I corrected the view, give it another go!

And it's in Lobby because for some reason, my dev server was kernel panicking when I'd assign it to the "Services" menu. And it's not really a service, but rather an utility. Where do you guys think it belongs?

marjohn56 · 2018-08-28T06:48:01Z

@AL-EC - will do. As for where it belongs. I would put it firewall. Franco will probably say elsewhere when he returns. :)

@fabianfrz @AdSchellevis - Where do you think it should go?

fichtner · 2018-08-27T17:35:32Z

dns/opnblock/src/opnsense/mvc/app/models/OPNsense/opnblock/Menu/Menu.xml

@@ -0,0 +1,5 @@
+<menu>
+  <Lobby>
+    <OPNblock cssClass="fa fa-hand-paper-o" url="/ui/opnblock" />


Not sure, are we using fa-fw here normally?

I followed FA's guide for the stop sign icon. I'd be happy to change, please advise.

fichtner · 2018-08-27T17:38:52Z

dns/opnblock/+POST_INSTALL

@@ -0,0 +1 @@
+


Delete empty file. Custom scripts are never good style. Did not see what this was used for earlier.

fichtner · 2018-08-27T17:41:36Z

dns/opnblock/Makefile

@@ -0,0 +1,7 @@
+PLUGIN_NAME=		OPNblock


Depending on the scope and clarity this could also be named unbound-adblock, but it’s up to you to use a cooler name. :)

Hey @fichtner,

Is there any way to have this show up under "Unbound DNS" in the menu? Can a plugin literally plug-in to other plugin menu items?

The answer is yes

@AL-EC you can register anywhere in the menu, just use the same path as the existing item, without the additional attributes, something like:

<menu> <Services> <Unbound> <OPNblock cssClass="fa fa-hand-paper-o" url="/ui/opnblock" /> </Unbound> </Services> </menu>

should do the trick.

Yeah, with this we could move it to core under a less flashy “Adblock” menu item, change the MVC namespace to Unbound and work on better Unbound integration in general. But as I said it’s up to you to decide.

Would it be as simple as just changing the namespace to Unbound and replace every mention of "opnblock" with "unbound-adblock"? Or would more go into it? Obviously it'll be a different page under the Unbound menu entry.

fichtner · 2018-08-27T17:42:03Z

dns/opnblock/src/opnsense/mvc/app/controllers/OPNsense/opnblock/Api/ServiceController.php

+                $backend->configdRun('template reload OPNsense/opnblock');
+                // refresh and build lists for unbound (opnblock.conf)
+                $backend->configdRun("opnblock refresh");
+                return array("message" => "OPNblock's lists have been updated! Please restart your Unbound DNS server.");


Not translated using gettext()

Thanks. Would you mind linking an example of this so I can see how it's done?

In that case I would not do that since it is an api call. FYI in php it is just a function call.

fichtner · 2018-08-27T17:42:17Z

dns/opnblock/src/opnsense/mvc/app/controllers/OPNsense/opnblock/Api/ServiceController.php

+                $backend->configdRun("opnblock refresh");
+                return array("message" => "OPNblock's lists have been updated! Please restart your Unbound DNS server.");
+        }
+        return array("message" => "Something went wrong...");


No translation

fichtner · 2018-08-27T17:42:56Z

dns/opnblock/src/opnsense/mvc/app/models/OPNsense/opnblock/ACL/ACL.xml

@@ -0,0 +1,9 @@
+<acl>
+   <page-lobby-opnblock>
+      <name>Lobby: OPNblock</name>


Lobby? Should be services?

See other comment regarding menu items :)

fichtner · 2018-08-28T07:23:07Z

dns/opnblock/src/opnsense/scripts/OPNsense/opnblock/opnblock.sh

+touch '/tmp/hosts.working'
+
+# setup variables for curl'ing
+whitelist=$(awk -F '=' '{if (! ($0 ~ /^;/) && $0 ~ /whitelist/) print $2}'  /usr/local/etc/opnblock/opnblock.conf);


We normally write rc.conf files where the templates write this as shell variables so it’s easily loaded without further processing.

Do you have an example of this? Thanks so much!

you could just write

data.sh (generated)

whitelist="{{ your.opnsense.key.replace(',', ' ') }}"

adblock.sh:

. /path/to/data.sh # do stuff

if this is what @fichtner wants to say.

Yes, that’s what rc scripts do in a nutshell.

fichtner

Thinking a bit further, this has potential for core inclusion if we can make integration a bit less obvious. It would certainly required a bit of unbound core wirk for seamless integration and may help move the MVC rewrite up the priority chain. There are no outside dependencies so it’s more or less similar to the remote ACL feature of the web proxy.

In any case: Thanks for doing this work! 😊

alectrocute · 2018-10-04T16:25:17Z

Hey, @mimugmail! I've been very busy at work and with another personal DNS project (https://heatsink.io). I would absolutely love some help as I'm still pretty new to this plugin creation process. I appreciate it greatly, let me know what I can do to get this moving along.

mimugmail · 2018-10-04T17:12:21Z

@fichtner @fabianfrz can you merge to a new branch? I'd fix some stuff :)

fabianfrz · 2018-10-04T21:16:39Z

@mimugmail yes that would work but the comments will not be kept because they are assigned to this pull request. Would that mean that you are going to fix everything mentioned here and then add you own stuff?

Keep in mind that a squashed commit may kill your authorship from a git perspective.

mimugmail · 2018-10-04T21:28:46Z

I dont mind for authoring this PR. Just move to a separate branch and I'll try to make it compatible :)

fabianfrz · 2018-10-05T04:50:50Z

dns/UnboundBL/src/opnsense/mvc/app/views/OPNsense/UnboundBL/index.volt

+<div class="alert alert-info hidden" role="alert" id="responseMsg"> </div>
+<div class="col-md-12"> {{ partial("layout_partials/base_form",['fields':general,'id':'frm_GeneralSettings']) }}
+    <button style="margin: 30px 0px 30px 0px;" class="btn btn-success" id="saveAct" type="button"> <b>{{ lang._('Save') }}</b><i id="saveAct_progress"></i> </button>
+    <p> <b>{{ lang._('REQUIRED') }}:</b> {{ lang._('You must include') }} <span style="font-family: Courier; font-size: 13px; border-radius: 5px 5px 5px 5px; background-color: #000; color: #fff; padding: 5px; margin: 5px;">include:/var/unbound/UnboundBL.conf</span> {{ lang._('in your') }} <a href="../services_unbound.php">Unbound DNS</a> {{ lang._('advanced settings configuration') }}! </p>


String concatenation issue

alectrocute · 2018-11-28T19:13:55Z

Hey, @mimugmail. Have you had the time to do any work on this project?

mimugmail · 2018-11-28T19:22:43Z

I'm waiting for @fichtner creating a new branch so you can open the PR against this and master.

fabianfrz · 2018-11-28T19:29:30Z

@mimugmail I can do this as well

fichtner · 2018-11-28T19:31:25Z

@fabianfrz it would be great if you could help out with your time indeed, thanks

fabianfrz · 2018-11-28T19:32:50Z

https://github.com/opnsense/plugins/tree/opnblock

mimugmail · 2018-11-28T19:34:56Z

@AL-EC can you close and open against this branch? @fabianfrz will merge and then we can try to move on.

fabianfrz · 2018-11-28T19:35:53Z

@mimugmail you can create the PR as well.

fabianfrz · 2018-11-28T19:39:05Z

@mimugmail done

mimugmail · 2018-11-28T19:45:33Z

Shit, this was fast .. seems there's some extra work waiting :)

ma-karai · 2018-12-28T14:25:47Z

I am really looking forward to this :)

mimugmail · 2019-01-24T11:21:18Z

@alectrocute any progress on this? Lack of time? Need help? Shall I take over?

mimugmail · 2019-01-30T07:57:47Z

@alectrocute ping :)

ma-karai · 2019-01-30T08:43:32Z

It would be nice to have it in the 19.x release. I think it's a kick ass feature.

alectrocute · 2019-01-30T20:09:13Z

@mimugmail @bott0r @fabianfrz

I'm going to get back into this immediately!

beren12 · 2019-02-04T19:58:48Z

Looking forward to it. I was just thinking about making a small vm for pi-hole. I does have the advantage of pretty graphs for usage though :-)

ma-karai · 2019-05-24T05:51:52Z

I am basically checking the opnsense repo/plugin part everyday, any update on this?

beren12 · 2019-06-26T16:25:58Z

Also what are the memory requirements? I added a few lists and then my router locked up, pretty sure it was oom. It wouldn't finish rebooting either, got hung up just after unbound loaded, had to uninstall it.

mimugmail · 2019-06-26T17:21:51Z

Maybe it's better to use the scripting logic of bind or dnscrypt ..

ma-karai · 2019-07-05T06:37:54Z

any news on this ?

mimugmail · 2019-07-06T08:08:43Z

@alectrocute TBH, I'm not sure how to go on here. This is a highly requested feature and will produce lots of feature requests and support issues, which in turn requires lots of your time and activity.

pkejval · 2019-12-18T11:32:55Z

Ping. Any progress in this plugin guys?

mimugmail · 2019-12-18T11:42:17Z

Different approach now:

pkg install os-unbound-plus-devel

Still in development ...

alectrocute added 2 commits August 27, 2018 09:03

Add files via upload

a2f6958

Update index.volt

30f996d

fabianfrz reviewed Aug 27, 2018

View reviewed changes

Update +POST_INSTALL

d895432

fabianfrz reviewed Aug 27, 2018

View reviewed changes

alectrocute added 3 commits August 27, 2018 09:57

camel-case added to class

b9c4ef6

Update opnblock.xml

0b8dfa4

Update SettingsController.php

270b744

fabianfrz reviewed Aug 27, 2018

View reviewed changes

dns/opnblock/src/opnsense/mvc/app/controllers/OPNsense/opnblock/forms/general.xml Outdated Show resolved Hide resolved

alectrocute added 4 commits August 27, 2018 10:00

Update index.volt

f1c4032

Update index.volt

47a09b6

Update opnblock.sh

edd8b4d

Update IndexController.php

e384d05

Update index.volt

98b9725

fichtner reviewed Aug 28, 2018

View reviewed changes

alectrocute added 7 commits August 28, 2018 08:35

Delete +POST_INSTALL

43c5b4d

Update ACL.xml

ad17338

Update Menu.xml

fb34fc0

Update index.volt

ba7db96

Update +TARGETS

a6afc76

Update ServiceController.php

f7edaa6

Update index.volt

f21657b

Update rc.conf

6d313c4

fabianfrz reviewed Oct 5, 2018

View reviewed changes

fabianfrz changed the base branch from master to opnblock November 28, 2018 19:36

fabianfrz merged commit 6a201db into opnsense:opnblock Nov 28, 2018

[WIP] OPNblock, first dev version #808

[WIP] OPNblock, first dev version #808

Conversation

alectrocute commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marjohn56 commented Aug 27, 2018

fabianfrz commented Aug 27, 2018

marjohn56 commented Aug 27, 2018

marjohn56 commented Aug 27, 2018

fabianfrz commented Aug 27, 2018

alectrocute commented Aug 27, 2018

marjohn56 commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner left a comment

Choose a reason for hiding this comment

alectrocute commented Oct 4, 2018

mimugmail commented Oct 4, 2018

fabianfrz commented Oct 4, 2018

mimugmail commented Oct 4, 2018

Choose a reason for hiding this comment

alectrocute commented Nov 28, 2018

mimugmail commented Nov 28, 2018

fabianfrz commented Nov 28, 2018

fichtner commented Nov 28, 2018

fabianfrz commented Nov 28, 2018

mimugmail commented Nov 28, 2018

fabianfrz commented Nov 28, 2018

fabianfrz commented Nov 28, 2018

mimugmail commented Nov 28, 2018

ma-karai commented Dec 28, 2018

mimugmail commented Jan 24, 2019

mimugmail commented Jan 30, 2019

ma-karai commented Jan 30, 2019

alectrocute commented Jan 30, 2019

beren12 commented Feb 4, 2019

ma-karai commented May 24, 2019

beren12 commented Jun 26, 2019

mimugmail commented Jun 26, 2019

ma-karai commented Jul 5, 2019

mimugmail commented Jul 6, 2019

pkejval commented Dec 18, 2019

mimugmail commented Dec 18, 2019