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
Multiple domains support #3870
base: main
Are you sure you want to change the base?
Multiple domains support #3870
Conversation
My first question would be, why change And a dev tip, install and activate |
HostBecause it definitely isn't a host. format!("{protocol}://{host}") This is a base URL, but you could say, that domain might also not be the correct term. My reasoning was, that domain is the term used for the config. Pre-CommitI ran |
This comment was marked as resolved.
This comment was marked as resolved.
It does not store a host. It stores a base url. If it stored a host the extractor would be: let headers = ...;
if let Some(host) = headers.get_one("X-Forwarded-Host") {
host
} else if let Some(host) headers.get_one("Host") {
host
} else {
panic!("invalid http request");
} But it's not, the extractor gets a URL from a combination of protocol, host and path (from config.domain()). It doesn't store |
It definitely isn't a domain, a domain is the last 2 (or 3) parts. Edit: so actually, our config is wrong too 😅 |
I know, but the precedent is set in
This definitely isn't a domain, but that's what the precedent says. We combine a host with a protocol and possibly a path into a URL. |
I understand, but the location where you changed it has nothing to do with config, but with the headers. |
So then the correct term would be BaseURL right?? I renamed it, because in the optimal situation (the domain is configured) the struct Stores the value from the But all I really want to know is if this approach (looking up the domain by host), not implementation, vibes with the project vision. |
But, maybe we need to wait for the whole picture. But because we didn't do it correctly in one place, doesn't mean we shouldn't at others or improve. Else we should just make it more clear using comments for other developers maybe. But i would prefer to use the right naming where possible when we adjust the code. |
There are some issues. Mails can be sent without any HTTP connection, so there must be one main host/domain in the config which will be used in those cases. Using a different host/domain for mails when there is a HTTP connection will become confusing. So, the only location where i think it will be useful for are MFA and attachments/sends. |
That's why I added |
Checks are passing now, but I still have some cleanup to be done. Probably will do it tomorrow, after that it should be ready for review. To do (at least):
Behaviours which may need to change (I think they're fine)
|
From my perspective I would say this is ready to review. |
@BlackDex Sorry for the ping, but is anything gonna happen with this PR? Because if not, it should probably just be closed. |
It has to wait for a check. As i mentioned in a few other pr's I'm busy with other stuff (for this project). But i just don't have much time currently. Just be patient and my comments (or other people's comments) will follow eventually. |
the domain chosen is always the first domain
if host isn't an allowed domain, we default to the main domain.
since it just returns a const
43a7b7f
to
c150818
Compare
Fixes #2690
Very WIP PR, I just want some feedback about my approach for now.
I am planning to go with the allowed domains approach.
Overview:
We create 2 Hashmaps, which map the Host header to either Domain or Origin.Limitations:
Future:
- Change JWT system to create tokens, which work for a single domain.