feature: custom domains using Fairings
#596
Fermé
igalic veut fusionner 48 révision(s) depuis igalic/feat/custom-fairing-domains vers master
Aucune évaluation
Pas d'étiquette
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
Aucun jalon
Pas d'assignataires
2 participants
Notifications
Échéance
Aucune échéance n'a été définie.
Dépendances
No dependencies set.
Reference: Plume/Plume#596
Chargement…
Référencer dans un nouveau ticket
Il n'existe pas encore de contenu.
Supprimer la branche 'igalic/feat/custom-fairing-domains'
Supprimer une branche est permanent. Cela NE PEUVENT être annulées. Continuer ?
This Pull Request addresses #94; replaces #447;
We start by adding
custom_domainto theblogstable; and a custom type Host(String)A blog with
custom_domainhas special routes with higher priority, but administrative routes will remain on the instance host.We'll use
Fairingsto extract theHostheader, to then specially route them.Domain administration is left to the user (and/or admin)
(but we might be able to provide a check on whether it's already correctly configured)
TODO
Host)Blog(andNewBlog)/blog/newand/blog/editto add custom domain field/custom_domains/mount-point which handles these domainsCodecov Report
Although it's very useful for debugging and fine for now, I'm gonna point this just to make sure we don't forget it when time for merging come
you might wanna check ok_or
(this is actually false, load can return a success with zero elements (type
select * from blogs where custom_domain = 'none';in an sql shell if you need to convince yourself, no error, but nothing matched))I know this not used everywhere else, but using first would be more meaning full
i understand, but this function is only called after our
guardalready put us herelimit(1)also doesn't make sense given thatcustom_domainisUNIQUEagreed. If I'm correct however, first returns a single entry, not a Vec where you have to try to take the first by yourself
results in
i don't know if to laugh or cry.
I meant diesel first, in place of load. Not Vec first (actually it's slice's, but Vec deref to slice)
but if i use
QueryResult<Blog>, then i can't use?for error handling later 🙍removed comment.
actually,
rustcis saying exactly what i need to do, so i did that.What about adding a
map_err(|_| Error::NotFound)here, to keepResult<Blog>as the return type (with the advantages it has, like having the possibility to "throw" it to get an error page)?And also it would allow you to remove the
From<diesel::result::Error> for ErrorPageimpl.or
map_err(Error::from)?seemed like a good deal to me to directly map a
diesel::result::Errorto anErrorPageotoh, i can also see how this would lead to the kind of error handling that PHP + MySQL are famous for
Codecov Report
so: I'm a bit at an impasse or crossroads with this patch, and it all kinda depends on how far i wanna push it.
For now, i've added a 404 handler, which redirects to the main-site, which means, actually, everything works now 😹 (by redirecting to the main site)
We could present the complete illusion that this custom domain, is the real domain, and all there is is this one blog.
To do this, I would have to adapt the URLs
ap_url,inbox_urlandoutbox_urlon blog creation (or update) and i would have to modify the templates to use the correct URL scheme.Alternatively we create an URL builder which we resolves the correct URLs.
Currently, there are a lot of places in which URLs are built ad-hoc.
In some cases, this is okay, because it's just pointing to a rocket route, but in many cases it feels painful, because we're doing manual labour that a function could remove.
the
compute_box()function for instance, feels quite pointless. There is no computation happening at all, just concatenation!compute_box()itself callsap_url()which only prependshttps://I would like to propose a Universal URL builder… or rather, a plumey-versal URL builder, which can be used from any part of plume's code to construct a correct plume URL.
I think it would be best to still handle most of (~all) ap stuff on the main domain, having an id (as requested in #296) for the activity that goes to the main domain, with a redirect to the correct path when queried (so either the main domain with better name than the id, or custom one when applicable). Also with some webfinger trick it should be possible to have more than one handle for a blog, so there could be the one from the main domain, that shouldn't change because it would break a lot of federation stuff, and an alias based on the custom domain. All this would be in the spirit of not breaking things if the custom domain for a blog changes
that would be a good idea for everything that need to integrate the domain in addition to a path. It could be a small macro that call the rocket one, and prepand the domain name, either from local_instance if none is given, or the one given (which would be a custom domain)
okay, so, at this point, i'm about as done as i can be, without touching the templates or refactoring the URL building.
@ -0,0 +1,57 @@-- add custom_domain to blogsCREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (Just wondering why copying the table is required, instead of just adding the column?
@ -0,0 +1,57 @@-- add custom_domain to blogsCREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (because SQLite supports a limited subset of ALTER TABLE
@ -0,0 +1,57 @@-- add custom_domain to blogsCREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (Damn, I missed it was SQLite. Thanks.
I gave it a try, and it seems to work quite well so far! I left a few comment about the code itself.
It should be
self.fqnI think, because title is the "name" of the blog, not its slug.@ -26,4 +33,3 @@let blog = Blog::find_by_fqn(&rockets, &name)?;let posts = Post::blog_page(conn, &blog, page.limits())?;let articles_count = Post::count_for_blog(conn, &blog)?;let authors = &blog.list_authors(conn)?;I think it will not properly format the page: you will get something like
https://doma.in/?2when you probably want?page=2.@ -40,2 +38,3 @@) -> Result<Ructe, ErrorPage> {rockets: &PlumeRocket,) -> Result<RespondOrRedirect, ErrorPage> {let conn = &*rockets.conn;Does that (and the
blog: &Blogabove) work?@ -40,2 +38,3 @@) -> Result<Ructe, ErrorPage> {rockets: &PlumeRocket,) -> Result<RespondOrRedirect, ErrorPage> {let conn = &*rockets.conn;nvm, I thought it was a route, but it is just a regular function.
I think this URL (and the one after) should include the article's slug.
You can use the handy
unreachable!("<msg>")instead of a plain panic, it has the same effect but is probably cleareroh gosh yes that was a copy paste!
@ -60,0 +151,4 @@#[get("/<custom_domain>", rank = 1)]pub fn activity_details(custom_domain: String,clippy says
@ -60,0 +151,4 @@#[get("/<custom_domain>", rank = 1)]pub fn activity_details(custom_domain: String,I can't accept your proposal via Github (apparently I don't have the right to push to this repository 🙃), but I made it manually, thanks.
validation?
we can add a validate function here, but it seems rather limited what we can return
for instance: DNS doesn't resolve? we can't say anything at this point, but should still tell the person that it doesn't resolve (yet)
another test would then be to try to connect and see if it's connecting to us, but the question here is: how??
would https://github.com/Plume-org/Plume/issues/296 be a solution to this issue?
I think we should just make a DNS request to check that there is a CNAME record pointing to the main domain? And if it doesn't resolve, we can probably show a warning like: "Your custom domain is not properly configured yet, please re-try later in your blog settings." (or something like that, I trust you to find a better wording).
And I don't think #296 is a solution to this specific issue, even if it may help somewhere else in this branch…
In some complex setup, checking only a CNAME can be not enough, for instance at home I have
subdomain.my_domain=>my_domain, butmy_domainis also a CNAME to a domain provided by my isp. Suppose Plume run on subdomain.my_domain, pointing tomy_domain, or A/AAAA to my public ips would work, but would also be rejected. I think having an endpoint which we can query with Reqwest is more solid to complex/unexpected setup@ -86,0 +205,4 @@random_id.clone(),Instant::now().checked_add(Duration::new(60, 0)).unwrap(),);And it avoids having to do some DNS queries directly, so even better 😄
what would we be looking for when making such a Reqwest?
how do we identify our own instance?
We could query
/nodeinfo/2.0and check if metadata.nodeName matches local value. It does not protect against voluntary "mistake" (actually, nothing can), but will catch configuration erroris there a cryptographically secure way in ActivityPub or OcapPub to do this?
I don't know OcapPub, AP has no cryptography (non normative security sections everywhere), but in general, no cryptographic protection has any effect against a proxy attack, someone could just temporary proxy traffic to Plume, then do nothing else (that's a big problem on smart-cards btw, they tend to use timing info to fight against, but it's not very efficient). The errors we can catch here are mistakes, not clever enough attacks
@ -49,3 +90,4 @@activity_detail_guts(blog, rockets, _ap)}#[get("/blogs/new")]the name mismatch the one in main.rs, and Result take 2 type arguments (probably both String I think)
@ -49,3 +90,4 @@activity_detail_guts(blog, rockets, _ap)}#[get("/blogs/new")]thanks / sorry for the noise 🙇♀️
the two files were edited with about a week or two in between 😅
@ -49,3 +90,4 @@activity_detail_guts(blog, rockets, _ap)}#[get("/blogs/new")]np 😉
👀
@ -178,0 +193,4 @@{let rewrite_uri = format!("/custom_domains/{}/{}", host.unwrap(), req.uri());let uri = Origin::parse_owned(rewrite_uri).unwrap();let uri = uri.to_normalized().into_owned();I'd be in favor of using the newtype design pattern for this, so that it make more sense what it is, even at the type level.
I don't know where are the &str coming from, but you might want to use String instead, or you'll have some difficulties with lifetime
you should use
duration_sinceinstead, as you're unwrapping it it's doing exactly the same thing, and it's stable(this will only return the string 410, not the error code)
(this will only return the string 404, not the error code)
@ -178,0 +193,4 @@{let rewrite_uri = format!("/custom_domains/{}/{}", host.unwrap(), req.uri());let uri = Origin::parse_owned(rewrite_uri).unwrap();let uri = uri.to_normalized().into_owned();aye. i haven't gotten that far yet, because i haven't implemented the other side of this
👀
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {))}I believe there are consts that already contains standards http status in the module
Same as above
@ -86,0 +213,4 @@Ok(resp) => resp.status().is_success(),Err(_) => false,}}You should remove the feature, as it's no longer used
I don't get returning a constant value, I don't see any early return?
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {))}@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {))}Have you tried simply
?
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {))}No, cuz I wanted to give a specific response.
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {))}imported from rocket::response::status::Custom
Your version is sending a standard error code with a non standard text, some implementations don't handle this well. This return the corresponding "error" text, and set the body of the result to the text you provide
It is a quite personal opinion, but I think
blog.custom_domain.map(ToString::to_string).unwrap_or_default()is more explicit (but, yes, it is only my opinion, maybe it is not).another question might be: why were you able to use
Option<String>forthemeinEditFormbut i have to useStringhere?Étape 1:
Depuis le dépôt de votre projet, sélectionnez une nouvelle branche et testez les modifications.Étape 2:
Fusionner les modifications et mettre à jour sur Forgejo.