Skip to content

Don't ask for username/password if none are defined #403

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

Merged
merged 1 commit into from
May 27, 2020

Conversation

Indemnity83
Copy link
Contributor

@Indemnity83 Indemnity83 commented May 9, 2020

This commit optimizes the Nginx config implementation of an access list, preventing an auth-check if no users are actually defined.

This should still mean that creating an empty access rule results in an inaccessible site because of the default deny all rule (secure by default). But prevents Nginx from even processing Basic Auth, or asking for a username and password if there are no users to check against.

This PR also fixes a bug that was preventing Auth rules from being applied to a site after enabling a previously disabled site. (moved to #407)

@andyjhall
Copy link

andyjhall commented May 12, 2020

If you're only passing in the item list in order to ensure its greater than zero would it make more sense to just pass a bool in instead? (e.g. hasItems)

@Indemnity83
Copy link
Contributor Author

There are actually a couple places where I end up counting the number of items, I can pull that common logic into the access-list model easily enough. Since the template is the only place I end up checking if it's greater than 0 I don't think it makes sense to pull that up.

@Indemnity83
Copy link
Contributor Author

Actually, now that I look at it there is only one use in the backend, and one use on the front-end. Pulling the counting logic into the back-end model and presenting that data to the front-end would require changes to the API to add information that is easily inferred and I don't think it makes any significant improvement to the readability or maintainability of the code.

@jc21
Copy link
Member

jc21 commented May 20, 2020

These changes seem to conflict with #407 ?

@jc21
Copy link
Member

jc21 commented May 20, 2020

Docker Image for build 3 is available on DockerHub as jc21/nginx-proxy-manager:github-pr-403

@Nedlinin
Copy link

👏 Looking forward to this making it in. Appears to fix #419

@andyjhall
Copy link

Looks to be working here 👍🏻

@Indemnity83
Copy link
Contributor Author

These changes seem to conflict with #407 ?

Not intentionally; I tried to pull out the conflicting bits into #407. I'm happy to rebase and de-conflict either PR after one of them is merged. I wanted to keep the PRs self-contained.

@jc21 jc21 merged commit 84d8fb0 into NginxProxyManager:develop May 27, 2020
@geertmeersman
Copy link

Hi, I just ran the last version (docker v2.3.0) and now the access rules (IP list) is not included anymore... Am I missing something?

@Subv
Copy link
Contributor

Subv commented May 30, 2020

@geertmeersman Hi! Could you please try #435?

@geertmeersman
Copy link

@geertmeersman Hi! Could you please try #435?

Hi @Subv , I just tested the docker image jc21/nginx-proxy-manager:github-pr-435 and confirm it is working as expected! Thx

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