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

Require password for all graphs #1596

Closed
wants to merge 1 commit into from
Closed

Require password for all graphs #1596

wants to merge 1 commit into from

Conversation

203lir
Copy link

@203lir 203lir commented Oct 2, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

This PR aims to make less information available to anyone who has not signed in, and also makes the page more consistent, because all graphs are treated the same now.

How does this PR accomplish the above?:

The section for the graph of total queries vs time was moved into the section where authorization is required.

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/disable-dashboard-if-not-logged-in/10719/18

@rcdailey
Copy link

What will it take to get this reviewed by the dev team?

@dschaper
Copy link
Member

A feature request with votes for it.

@rcdailey
Copy link

A feature request with votes for it.

To click a merge button? The work has been done for you.

@dschaper
Copy link
Member

dschaper commented Oct 28, 2020

I have no reason to "click a merge button" for a feature that will drastically change the user experience without any reasons behind it or requests from the community for it.

@rcdailey
Copy link

I have no reason to "click a merge button"

I mean, most of the time open source devs are asking people to contribute work instead of just putting in feature requests. I find it odd that in this case the work has been done but it still can't be accepted, not even reviewed. It would take a matter of minutes to spend time giving some feedback: How you feel about the change, what issues you feel it has, what can be done to improve it and increase the chances of it being accepted. I can think of a lot of ways you can make it constructive. I do not find "A feature request with votes for it" to be constructive, really I'd personally feel like you were blowing me off if I was the author.

for a feature that will drastically change the user experience

Drastically? That's arguable and not likely. In the grander scheme of things, this is not a big change IMHO.

without any reasons behind it

This change has plenty of merit, especially given the security implications. The fact that Pihole exposes data other than a login form when a password is set is a major issue, regardless of the demand for it.

...requests from the community for it.

Your bot has linked to a discussion thread that has plenty of requests for it. It may not have the 1000 upvotes it seems like you want, but there's demand and plenty of merit for it.

@dschaper
Copy link
Member

You linked to a discussion thread that has my response.

@rcdailey
Copy link

This?

Don't. Don't. Don't.

That's not much. Are you saying always use a VPN? Or saying don't access it publicly at all? Where's the substance in this response?

I currently use a reverse proxy to allow access to Pihole over port 443. No issue with that if Pihole properly hid the dashboard without authenticating first.

@dschaper
Copy link
Member

Don't expose the web interface to the public internet, only expose it to trusted networks.

@rcdailey
Copy link

Don't expose the web interface to the public internet, only expose it to trusted networks.

Why not?

@dschaper
Copy link
Member

If you need someone to explain that to you then you shouldn't be running Pi-hole.

@rcdailey
Copy link

If you need someone to explain that to you then you shouldn't be running Pi-hole.

I never said I needed it explained to me. I'm asking you to share your perspective. Honestly I don't see anything wrong with exposing it on public networks. The fact that the software lacks any sort of proper authentication mechanism isn't a reason to avoid it, it's a reason to improve/correct the software. There are other ways I'm working around the issue right now.

And we're back to the whole reason this PR exists and why there's discussion about it. I don't feel like we're getting anywhere with you at the helm. It feels like you're very defensive and unopen to change. Your responses are short, unhelpful, and unconstructive. It's very frustrating discussing this with you.

Hopefully you're not the only contributor in the project and we can get someone else here to help move things forward in a helpful way.

@dschaper
Copy link
Member

Honestly I don't see anything wrong with exposing it on public networks.

An openly accessible interface is highly indicative of a openly accessible resolver. I'm not doing anything that will encourage that behavior.

There are other ways I'm working around the issue right now.

Then you're taking on the risk, which is your right and also your responsibility.

And we're back to the whole reason this PR exists and why there's discussion about it.

If it's to practice security through obscurity then my answer is an even more emphatic 'no'.

I don't feel like we're getting anywhere with you at the helm.

Because I don't agree with you? Great, I don't.

Hopefully you're not the only contributor in the project

I'm not, but the other two haven't done anything with this PR either, so that's a big hint there.

@rcdailey
Copy link

Really, we're mixing two different issues here. For the sake of making some progress let's try to separate them out.

  1. Public access to pihole. Completely irrelevant here. The requested feature was most likely motivated by it, but unrelated to it.
  2. Hiding information behind the already present password prompt, regardless of network infrastructure.

#2 has merit even on LAN networks. What if I simply do not want the few users on my LAN to see the dashboard? I think it is simple enough to merge this PR to enable me to have that. Out of the roughly 20 web applications I host on my network, the Pihole is the ONLY one that doesn't immediately take me to a login prompt when I visit it. It's also the only one that shows me more than a login prompt. It's a fair comparison of common UX design.

Does this feature have anything to do with solving the bigger issues you have with a pihole being accessible to a WAN network? No. And I don't think it should. You can I could go on all day about how we don't agree with each other. But I think that needs to happen elsewhere, and I also don't think it should influence the merits of this feature.

@dschaper
Copy link
Member

What if I simply do not want the few users on my LAN to see the dashboard? I think it is simple enough to merge this PR to enable me to have that

That's your choice. Currently we don't do that. Changing that means every other user that is content with the current design is now required to abide by what you want. We have to support every user that depends on the interface to not suddenly change. This is a major, breaking change and would only happen with a major release.

@rcdailey
Copy link

That's why options/settings exist. Make it opt in. And your argument about "sudden changes" is immediately nullified after all the stuff that broke with the latest UI rework of Pihole and how different block lists are.

We're not getting anywhere so this is going to be the end of it from me. I'm very disappointed that you're a member on this project. You've shown no enthusiasm to work with people or listen to ideas. I can only hope the author's work gets a little more respect and visibility with the other developers.

Over and out.

@dschaper
Copy link
Member

dschaper commented Oct 28, 2020

immediately nullified after all the stuff that broke with the latest UI rework of Pihole and how different block lists are.

What are you talking about?

You've shown no enthusiasm to work with people or listen to ideas

I'm not agreeing with you and that's making you mad.

@PromoFaux
Copy link
Member

@203lir thanks for the submission. However I'm not sure this is the best way to go about things.. IF we were to move in this direction. Moving the auth check up above the currently visible graphs only has the effect of making the landing page of pi.hole/admin blank, until the user navigates manually to the login page.

A better check would be "if user is not authed, then redirect immediately to login page" in the header.

However, I'm still unsure of the argument for removing this information in the first place. None of the data visible is sensitive - and simply shows, at a glance, that the Pi-hole device is working and accepting queries.

There is an argument to be said that we could, instead, add in a setting for making these graphs not visible to an unauthorised user, but that would again just make the default non-authed page look like this:

image

I'd rather see a brief (anonymised) overview, personally.

So, to wrap up - my opinion on this is that IF we are to do something along these lines, it must be a complete redirect to login page when browsing to /admin, and it must be a toggle option in the settings, defaulted to being just how it is currently.

To respond to the initial comment... We're all on limited time. Reviews will be got to when they are got to. Impatience and entitlement will be met with more of the same. (this sentence does not need a reply - and is not a point for discussion)

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

I disagree with this change as new standard behavior. It may be added as a new setting (opt-in), then I'm fine with it. I agree with @PromoFaux's comment that an entirely empty page looks really odd as well. I guess a better behavior in this case would be an immediate redirect to the login page, as there is nothing to be seen without logging into the system.

@DL6ER
Copy link
Member

DL6ER commented Dec 10, 2020

Rejected after discussion

@DL6ER DL6ER closed this Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants