feature: custom domains using Fairings
#596
已關閉
igalic 請求將 48 次程式碼提交從 igalic/feat/custom-fairing-domains
合併至 master
拉取自: igalic/feat/custom-fairing-domains
合併到: Plume:master
Plume:paginate-search-init
Plume:main
Plume:s3
Plume:fix-delete-user
Plume:timeline-cli
Plume:blog-title
Plume:signature
Plume:remove-dup-images
Plume:ldap-non-anon
Plume:drone-ci
Plume:DearRude/force-lang
Plume:igalic/go/async-all-mut
Plume:go/async
Plume:floreal/translations-update
Plume:missing-docs
Plume:RAOF/fix-arm64-build
Plume:epsilon-phase/authorized-fetch
Plume:upgrade
Plume:improve-the-editor-once-again
Plume:feature/ldap
Plume:test/dotenv_error
Plume:fix-mobile-margin
審核者
請求審核
沒有審核者
標籤
清除已選取標籤
Related to the REST API
Code running on the server
Stuff related to Federation
Related to the front-end
Translations, and related code
More about project management or code than the project itself
The building, or installation process of Plume
Something isn't working
We need to talk
New feature or request
This is a new feature
Compatibility with different browsers, readers and OS
Related to an external package that Plume uses
UI/UX related issues and PRs
Good for newcomers
Extra attention is needed
Issues affecting only mobile UX
How elements're rendered out for the end user
Something else needs to be fixed first
This issue or pull request already exists
This PR is not complete yet
Issues concern a limited number of instances
This doesn't seem right
Need to be discussed by the community (on Loomio)
This PR is ready to be reviewed
Proposed ideas worth considering
This is issue has been created after a vote on Loomio
This will not be worked on
套用標籤
A: API
Related to the REST API
A: Backend
Code running on the server
A: Federation
Stuff related to Federation
A: Front-End
Related to the front-end
A: I18N
Translations, and related code
A: Meta
More about project management or code than the project itself
A: Security
Build
The building, or installation process of Plume
C: Bug
Something isn't working
C: Discussion
We need to talk
C: Enhancement
New feature or request
C: Feature
This is a new feature
Compatibility
Compatibility with different browsers, readers and OS
Dependency
Related to an external package that Plume uses
Design
UI/UX related issues and PRs
Documentation
Good first issue
Good for newcomers
Help welcome
Extra attention is needed
Mobile
Issues affecting only mobile UX
Rendering
How elements're rendered out for the end user
S: Blocked
Something else needs to be fixed first
S: Duplicate
This issue or pull request already exists
S: Incomplete
This PR is not complete yet
S: Instance specific
Issues concern a limited number of instances
S: Invalid
This doesn't seem right
S: Needs Voting/Discussion
Need to be discussed by the community (on Loomio)
S: Ready for review
This PR is ready to be reviewed
Suggestion
Proposed ideas worth considering
S: Voted on Loomio
This is issue has been created after a vote on Loomio
S: Wontfix
This will not be worked on
未選擇標籤
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
里程碑
設定里程碑
清除已選取里程碑
沒有項目
未選擇里程碑
負責人
指派負責人
清除負責人
沒有負責人
2 參與者
通知
截止日期
截止日期無效或超出範圍,請使用「yyyy-mm-dd」的格式。
未設定截止日期。
先決條件
未設定先決條件。
參考: Plume/Plume#596
新增問題並參考
尚未有任何內容
刪除分支「igalic/feat/custom-fairing-domains」
刪除分支是永久的。 此動作不可還原,是否繼續?
否
是
This Pull Request addresses #94; replaces #447;
We start by adding
custom_domain
to theblogs
table; and a custom type Host(String)A blog with
custom_domain
has special routes with higher priority, but administrative routes will remain on the instance host.We'll use
Fairings
to extract theHost
header, 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/new
and/blog/edit
to 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
guard
already put us herelimit(1)
also doesn't make sense given thatcustom_domain
isUNIQUE
agreed. 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,
rustc
is 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 ErrorPage
impl.or
map_err(Error::from)
?seemed like a good deal to me to directly map a
diesel::result::Error
to anErrorPage
otoh, 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_url
andoutbox_url
on 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 blogs
CREATE 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 blogs
CREATE 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 blogs
CREATE 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.fqn
I 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/?2
when 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: &Blog
above) 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_domain
is 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.0
and 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_since
instead, 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>
fortheme
inEditForm
but i have to useString
here?第一步:
在您的儲存庫中切換到新分支並測試變更。第二步:
合併變更並更新到 Forgejo。