Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PAC support to proxy #2018

Merged
merged 23 commits into from
Jul 6, 2018
Merged

Add PAC support to proxy #2018

merged 23 commits into from
Jul 6, 2018

Conversation

fabianfrz
Copy link
Member

@fabianfrz fabianfrz commented Dec 25, 2017

Add PAC file support to the UI to allow Proxy autoconfiguration of some clients that support that standard

@fichtner fichtner self-assigned this Dec 26, 2017
@@ -32,7 +32,6 @@
<field>
<id>blacklist.filter</id>
<label>categories (if available)</label>
<type>text</type>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should break this out and merge it immediately?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not important at the moment - it is overwritten in the next line but it should be removed in the future as it might cause problems in the future (if for some reasons the other element is used).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge glitch

@fichtner fichtner added the feature Adding new functionality label Dec 26, 2017
@fabianfrz
Copy link
Member Author

@fichtner the only thing still missing is a template reload button on all three tabs.

@@ -466,7 +466,6 @@
<td></td>
<td>
<button data-action="add" type="button" class="btn btn-xs btn-default"><span class="fa fa-plus"></span></button>
<button data-action="deleteSelected" type="button" class="btn btn-xs btn-default"><span class="fa fa-trash-o"></span></button>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fichtner this button did nothing so I removed it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok if @AdSchellevis doesn’t want to keep it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabianfrz @fichtner it can go, I guess someone copied it in there a long time ago....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdSchellevis It did probably come from IDS because it is the only view with a list containing checkable checkboxes.

@fabianfrz
Copy link
Member Author

@fichtner the model needs some improvements regarding to validation but the rest seems good now.

@fabianfrz fabianfrz mentioned this pull request Dec 26, 2017
@@ -230,7 +447,7 @@
<tr>
<td colspan="2">
<div id="remoteACLchangeMessage" class="alert alert-info" style="display: none" role="alert">
{{ lang._('Note: after changing categories, please remember to download the ACL again to apply your new settings') }}
{{ lang._('Note: After changing categories, please remember to download the ACL again to apply your new settings') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to ditch „Note: „ prefix if this is changed.

@@ -8,3 +8,4 @@ post-auth.conf:/usr/local/etc/squid/post-auth/dummy.conf
pre-auth.conf:/usr/local/etc/squid/pre-auth/dummy.conf
rc.conf.d:/etc/rc.conf.d/squid/squid
squid.conf:/usr/local/etc/squid/squid.conf
wpad.dat:/usr/local/www/wpad.dat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s a bit strange to drop files into www but I know what you are after

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a better idea? The URL does not really match the pattern of standard OPNsense rules. Otherwise we would have to heavily rewrite routes in lighttpd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I’m only raising awareness. In any case this will not merge before stable/18.1 is branched so we have time to think about this. For now this is good enough for -devel

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only thing I could think of is adding a custom rewrite to a controller without authentication.

@fabianfrz
Copy link
Member Author

@fichtner it's time for a test run ;)

@fabianfrz fabianfrz changed the title WIP: Add PAC support to proxy Add PAC support to proxy Dec 26, 2017
@fabianfrz
Copy link
Member Author

@fichtner the generated JS is valid and it looks good. It may be published in devel.

@fichtner
Copy link
Member

Not yet, or else this lands in 18.1-RC1...

@fabianfrz
Copy link
Member Author

fixed conflict

@fabianfrz
Copy link
Member Author

@fichtner I found a small GUI issue - maybe you have an idea: Some fields will be hidden when the modal shows up: https://github.com/opnsense/core/pull/2018/files#diff-23d36dcc2f04f176686a881d17c288bdR190

so the event will be emitted here for example where the data should already be mapped: https://github.com/opnsense/core/blob/master/src/opnsense/www/js/opnsense_bootgrid_plugin.js#L217
However it seems $.val() may return a wrong value as the form may show an incorrect dependent field (Some kind of a race condition?)

@fabianfrz
Copy link
Member Author

@fichtner the issue was fixed a week ago

@fabianfrz
Copy link
Member Author

ping

Additional PRs:

@fichtner
Copy link
Member

Let me finish 18.1.6 for new images, then we pull this in. Is everything ready from your end?

@fichtner
Copy link
Member

(Your PR summary says otherwise)

@fabianfrz
Copy link
Member Author

@fichtner It was for a long time as I can remember. This pull request only adds a template to Squid to allow to create a PAC file as OPNSENSE-INTERFACE-HOST/wpad.dat which can be used to automatically configure the proxy in Firefox, Windows, Atom and so on. The Other Pull requests are more simple - they only add the WPAD DNS-Records and DHCP Options. I will give this branch a try again.

@fabianfrz
Copy link
Member Author

fabianfrz commented Mar 31, 2018

found a bug and fixed it but the rest looks good.

@fabianfrz
Copy link
Member Author

fixed conflict of "+Targets" via GitHub online editor.

@fichtner
Copy link
Member

@fabianfrz will you rebase / fix conflict? thanks

@fabianfrz
Copy link
Member Author

I hope the github merge did what it was intended to do.

@fichtner fichtner merged commit 7386a5f into opnsense:master Jul 6, 2018
@fichtner
Copy link
Member

fichtner commented Jul 6, 2018

Merged, thanks!

@fabianfrz fabianfrz deleted the pac branch July 9, 2018 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding new functionality
Development

Successfully merging this pull request may close these issues.

3 participants