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

Whitelisting changes #174

Merged
merged 38 commits into from Jan 16, 2016
Merged

Whitelisting changes #174

merged 38 commits into from Jan 16, 2016

Conversation

PromoFaux
Copy link
Member

Note : This will break the "ugly hack" @jacobsalmela put in yesterday...

jacobsalmela and others added 30 commits December 16, 2015 11:23
These commands were left out, resulting in the Web interface not showing ads blocked despite the Pi-hole working.  It is just a permissions error.
- Rob's post on medium.com
- TekThing video

Also posting about plans to partner up with Rob for his new service that can help reduce the flame war surrounding ad blocking
Fix kill command and only run as last step.
Fix kill command to re-read hosts file
An error shows up on the first install that `latentWhitelist.txt` doesn't exist and can't be removed.  Redirecting STDERR should fix this.
IPv4 was mentioned in an IPv6 setting - changed to correctly reference IPv6.
I’m pretty sure this works well.  Maybe someone else can try it out.
Failed to be populated if /tmp/piholeIP does not exist. This may happen under
the following conditions:

- was not generated during install
- gravity.sh being run again after the original install (the file is deleted
  during the installation process)
Fix bug in gravity.sh where IPv6 list was not always populated
IPv6 box was missing the title.
Added some IPv6 echoes.
fixes #142 kill -HUP erroring on gravity_reload if dnsmasq is not running
Enables ad-blocking over IPv6 for an even better Internet experience.
installer - only select first (probably default) interface in list instead of all
A much more elegant way to get the screen size.
- removed most of the video images to reduce clutter
- converged the "coverage" and "video" sections into a bulleted list of links
- added a new project link (pi-hole on/off button)
- moved projects up in the page so it's easier to find and see what people are doing with Pi-hole
- removed custom conf section since it will be suited better in the wiki or FAQs
- added a help section
replaces the non-repository managed web interface for pihole and keep…
Running at midnight, not 11:58 in the afternoon.
@PromoFaux
Copy link
Member Author

(I don't know if I was the only one wondering why there were so many commits :) )

There weren't when I originally set the pull request to master.. but since you suggested I merge with the whitelist branch,,,

echo "Immediately whitelists one or more domains in the hosts file"
echo " "
echo "Usage: whitelist.sh domain1 [domain2 ...]"
echo " "
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight ocd on my part, but blacklist.sh has no empty lines between the help output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, thought i'd sorted that.. I think I might have got a bit confused with the merging earlier! Will sort it out in a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in f9a2ca5

@AzureMarker
Copy link
Contributor

No, it's good that the whitelist branch be updated (I think). We can make sure we haven't broken anything major.

@PromoFaux
Copy link
Member Author

There was some stuff committed into the whitelist.sh with a web hook labelled as a "dirty hack" so I just wrote my changes over it, as it probably would not have worked with my new changes anyway

@AzureMarker
Copy link
Contributor

That was @jacobsalmela trying to make a simple whitelister on the site, which would give the console output. It would redirect the console output to somewhere the script could get it and display on the site. The whitelister will soon be deprecated, if my editor is merged.

@AzureMarker
Copy link
Contributor

Hmm, I guess the whitelist branch already got the latest from master. You had originally forked off of master, so since you switched to whitelist, git is bringing your branch up to date. If nothing is breaking now, it should stay safe. 😄

@dschaper
Copy link
Member

What should we do if a user has a domain in both the blacklist and the whitelist? (Say they blacklist a domain, forget they did and a few months later try to whitelist the domain...) Just a hypothetical, not a comment on the current code. Testing out the code now and it's looking good. Thanks!

@PromoFaux
Copy link
Member Author

Funny you should mention that, I've been giving it some thought this evening.

What I'll probably end up doing is giving both the whitelist.sh and blacklist.sh knowledge of each other's lists.

e.g if you add a domain to the whitelist, it will be removed from the blacklist and vice versa. We should probably decide which one takes priority.

@dschaper
Copy link
Member

I do a great impression of a forgetful user! (There is no truth to the rumor that the above scenario is based on reality in any way!)

I think whitelisting should be the winner, but that's just a biased coin flip, really.

@dschaper
Copy link
Member

And now that I think about it some more, how do we handle removing from blacklist/whitelist. Use the same process and have whitelist remove the blacklist and blacklist remove the whitelist? Just throwing things out there. I've added domains with the blacklist script and for now I have to edit gravity.list by hand or run gravity.sh again after cleaning the blacklist.txt.

I guess with the improved web interface these situations will be uncommon, it will be easy to see what's what...

@PromoFaux
Copy link
Member Author

To remove from the blacklist (and also remove it from gravity.list) run blacklist.sh -d [domain]

same goes for whitelist

@dschaper
Copy link
Member

Ah, thanks! (I'll document that in the Wiki when the PR merges.)

@PromoFaux
Copy link
Member Author

I tested it as best I could earlier, but do let me know if you find any bugs, and I'll sort them out! Once it's merged, i'll start work on making the black and whitelists aware of each other.

@PromoFaux
Copy link
Member Author

Oh also, try passing a non domain argument to either script! Quite proud of that bit..

e.g blacklist.sh kjngkdsfnshkdfhn

@dschaper
Copy link
Member

Nice, lol... I'm still in a love/hate mode with regex.

@AzureMarker
Copy link
Contributor

I think the current gravity.sh has blacklist take priority (runs whitelist first, then blacklist). Are you planning on implementing a smart listing system as described above, or just giving one list priority?

@dschaper
Copy link
Member

When I wrote gravity_pulsar() I didn't take into account any order, it's just happenstance that the order is like that... @jacobsalmela may have a preference though.

@jacobsalmela
Copy link
Contributor

Wow guys, I was busy today but my inbox was blowing up with all the chatter. I'll have to check this stuff and all the comments on the other issues/PRs.

Thanks for your hard work.

@jacobsalmela
Copy link
Contributor

Impressive @PromoFaux!

@Mcat12, should I wait to merge your changes on the Web UI?

jacobsalmela added a commit that referenced this pull request Jan 16, 2016
@jacobsalmela jacobsalmela merged commit 87980c6 into pi-hole:whitelist Jan 16, 2016
This was referenced Jan 16, 2016
@AzureMarker
Copy link
Contributor

Ya, I'm going to reinstall my pihole (was running off the dietpi version using apache) and implement the new whitelist and blacklist features. In the mean time, the web interface needs sudo for white/blacklist.sh, in order to restart the dns. I think this has to be done by giving www-data no password, no tty required sudo permissions on the scripts in the sudoers file.

@jacobsalmela
Copy link
Contributor

Right, I was going to work on that.

I'm going to focus on the bugs first, but go ahead and keep working so it's ready. You are obviously better qualified for Web development then me!

@AzureMarker
Copy link
Contributor

lol, I guess I updated my pi too soon. I'm going to have to update again with all the changes committed.

@jacobsalmela
Copy link
Contributor

Yeah sorry about that.; I was kind of on a spree and couldn't stop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants