-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[Web] Disable login UI on autoprotocol domains #6867
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
[Web] Disable login UI on autoprotocol domains #6867
Conversation
|
Thank you for the PR. |
That would indeed simplify the implementation quite a bit; just leaving a few of the lines in I also didn't want to make this into a breaking change which some users might - for some reason - currently be relying on. Those are always a pain to figure out when you upgrade a system and something you've grown to using (whether advisable or not) just stops working without a warning. |
|
Oh... furthermore... there are no guarantees that this change actually solves anything in regards to issue 6747 - disabling the DNS-entries for the domains remedied the situation for me for now ... but since Google's detection is an actual black box, saying anything definite is quite impossible. |
I like this change and I fully agree. No need for making it customizable. I'd also suggest in replacing: if (preg_match('/^(autodiscover|autoconfig)\./i', $server_name)) {with something more simple like: (not tested!) if (substr($server_name, 0, 12) === "autodiscover" || substr($server_name, 0, 10) === "autoconfig") {Regex tends to be more costly performance-wise. And in the end, both should do the same? |
|
I could squeeze a bit more juice by using Only question that remains is; do I just add another commit and keep the existing history, or should I rebase & rewrite history of this PR and force-push a new version? Or is there a third way things are preferred in this project? |
|
I'd say rebase & force-push - to keep the discussion in this PR too. Should do the trick. |
6c174ca to
b18c5b7
Compare
Autodiscover and autoconfig domains (autodiscover.*, autoconfig.*) are intended solely for client autoconfiguration endpoints and should not display the mailcow login page. This change check the hostname and disables unauthenticated users from seeing the login page on those domains; HTTP 404 response is returned when necessary.
b18c5b7 to
95e0608
Compare
|
I've removed all the unnecessary changes that enabled making this feature optional (UI, localisations, backend storage) and left the essentials. At one point I noticed diving deep into professional paranoia rabbithole trying to handle various imaginary edge cases that are very unlikely to ever be seen in real life and decided I was overthinking and should take a step back :D The current version of the code makes one memory allocation for the lowercase hostname, which could be avoided at the cost of increased code complexity - but I think this implementation with a much more straight-forward approach is a good compromise between performance and future maintainability. During my latest testing I did notice that the current server implementation actually responds to both protocol requests on both domains (i.e. you can request autoconfig-endpoints using autodiscovery-domain and vice versa) but that issue is way beyond the scope of this PR ... and probably of little consequence one way or the other. |
|
I understand this PR has only been open a few weeks (sometimes I've waited months for review etc. - often enough to completely forget what I was thinking about when submitting one:) but I wanted to ask if there is still something you folks would want changed / addressed in this pull request context? @FreddleSpl0it @patschi |
|
@DiscoNova Looks good to me. I don’t have time for development right now, but I’ll likely get back to your PR in late December. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
While it looks like Google has come to its senses about marking the sites "unsafe"/"dangerous", I believe this PR would still make a good addition to the codebase - simply to avoid user ambiguity regarding the way the autoprotocol domains are meant to be used. But this is admittedly purely my personal opinion. |
Contribution Guidelines
What does this PR include?
Short Description
This PR adds a new configuration option to disable the login page on autodiscover and autoconfig domains (autodiscover.*, autoconfig.*). When enabled, these domains will return a 404 error instead of displaying the login interface for endpoints that haven't been defined elsewhere. This is useful because autodiscover/autoconfig domains are intended solely for automatic client configuration and should not expose the login interface.
For me, disabling the login-interface on the mentioned domains solved the problem I was facing with issue #6747 ... at least for now.
The implementation includes:
n.b. While I am a native Finnish/Swedish speaker myself with English as third language, and have studied French and German decades ago, my “Dutch” is actually learnt from Flemish speakers in Belgium, so some TLC review of the translations would be highly appreciated to avoid unintended idiosyncrasies and achieving technical consistency.
Affected Containers
Did you run tests?
I tested the feature manually, yes.
What did you tested?
Following components were tested:
What were the final results? (Awaited, got)
Expected behavior:
Actual results:
All tests passed and the feature works as expected: