Plume
/
Plume
8
16
Forka 22
Codice Problemi 174 Pull Requests 8 Rilasci 8 Wiki Attività

Etichette Traguardi

feature: custom domains using Fairings #596

Chiuso
igalic vorrebbe unire 48 commit da igalic/feat/custom-fairing-domains a master
esegui un pull da: igalic/feat/custom-fairing-domains
unisci a: Plume:master
Conversazione 5 Commit 48 File modificati 23
igalic 5 anni fa ha commentato (Migrato da github.com)

This Pull Request addresses #94; replaces #447;
We start by adding custom_domain to the blogs 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 the Host 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

  • PostgreSQL Migrations
  • SQLite Migrations
  • Custom time to represent domain (Host)
  • Expand Blog (and NewBlog)
  • Update /blog/new and /blog/edit to add custom domain field
  • Cache custom domains on startup and on insert/update.
  • /custom_domains/ mount-point which handles these domains
  • implement routes for
    • blog details
    • post details
    • blog activity_details
    • search
    • routes necessary for federation??
  • implement redirect for the "legacy" routes for the above
    • blog details
    • post details
    • blog activity_details ??
    • search
  • Update URLs in templates
  • Update URLs in base template for search
  • Check correct domain setup
  • Test: Federation
  • Some explanations (in the UI, next to the setting and in the docs)
This Pull Request addresses #94; replaces #447; We start by adding `custom_domain` to the `blogs` 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 the `Host` 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 - [x] PostgreSQL Migrations - [x] SQLite Migrations - [x] Custom time to represent domain (`Host`) - [x] Expand `Blog` (and `NewBlog`) - [x] Update `/blog/new` and `/blog/edit` to add custom domain field - [x] Cache custom domains on startup and on insert/update. - [x] `/custom_domains/` mount-point which handles these domains - [ ] implement routes for - [x] blog details - [x] post details - [x] blog activity_details - [ ] search - [ ] routes necessary for federation?? - [ ] implement redirect for the "legacy" routes for the above - [x] blog details - [x] post details - [ ] blog activity_details ?? - [ ] search - [x] Update URLs in templates - [ ] Update URLs in base template for search - [ ] Check correct domain setup - [ ] Test: Federation - [ ] Some explanations (in the UI, next to the setting and in the docs)
👍 2
codecov[bot] 5 anni fa ha commentato (Migrato da github.com)

Codecov Report

Merging #596 into master will increase coverage by 3.54%.
The diff coverage is 32.14%.

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
+ Coverage   30.96%   34.51%   +3.54%     
==========================================
  Files          66       67       +1     
  Lines        7822     7916      +94     
  Branches     1881     1894      +13     
==========================================
+ Hits         2422     2732     +310     
+ Misses       4700     4421     -279     
- Partials      700      763      +63
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/596?src=pr&el=h1) Report > Merging [#596](https://codecov.io/gh/Plume-org/Plume/pull/596?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/8c59c822b63b33cbf53af2bd96f3f071d2183c73?src=pr&el=desc) will **increase** coverage by `3.54%`. > The diff coverage is `32.14%`. ```diff @@ Coverage Diff @@ ## master #596 +/- ## ========================================== + Coverage 30.96% 34.51% +3.54% ========================================== Files 66 67 +1 Lines 7822 7916 +94 Branches 1881 1894 +13 ========================================== + Hits 2422 2732 +310 + Misses 4700 4421 -279 - Partials 700 763 +63 ```
trinity-1686a revisionato 5 anni fa
src/main.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

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

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
trinity-1686a revisionato 5 anni fa
plume-models/src/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario 
error: redundant closure found
   --> plume-models/src/blogs.rs:321:44
    |
321 |             .map(|res| res.into_iter().map(|s| s.unwrap()).collect::<Vec<_>>())
    |                                            ^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::unwrap`
    |
    = note: `-D clippy::redundant-closure` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
``` error: redundant closure found --> plume-models/src/blogs.rs:321:44 | 321 | .map(|res| res.into_iter().map(|s| s.unwrap()).collect::<Vec<_>>()) | ^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::unwrap` | = note: `-D clippy::redundant-closure` implied by `-D warnings` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure ```
trinity-1686a revisionato 5 anni fa
plume-models/src/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

you might wanna check ok_or

you might wanna check [ok_or](https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.ok_or)
trinity-1686a 5 anni fa ha commentato
Proprietario

(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))

(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))
trinity-1686a 5 anni fa ha commentato
Proprietario

I know this not used everywhere else, but using first would be more meaning full

I know this not used everywhere else, but using [first](https://docs.diesel.rs/diesel/query_dsl/trait.RunQueryDsl.html#method.first) would be more meaning full
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

i understand, but this function is only called after our guard already put us here

i understand, but this function is only called after our `guard` already put us here
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

limit(1) also doesn't make sense given that custom_domain is UNIQUE

`limit(1)` also doesn't make sense given that `custom_domain` is `UNIQUE`
trinity-1686a revisionato 5 anni fa
plume-models/src/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

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

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
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato 
    pub fn find_by_host(c: &PlumeRocket, host: Host) -> Result<Blog> {
        let from_db = blogs::table
            .filter(blogs::custom_domain.eq(host))
            .load::<Blog>(&*c.conn)?
            .first();
        from_db.ok_or(Error::NotFound)
    }

results in

error[E0308]: mismatched types
   --> plume-models/src/blogs.rs:203:9
    |
198 |     pub fn find_by_host(c: &PlumeRocket, host: Host) -> Result<Blog> {
    |                                                         ------------ expected `std::result::Result<blogs::Blog, Error>` because of return type
...
203 |         from_db.ok_or(Error::NotFound)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `blogs::Blog`, found reference
    |
    = note: expected type `std::result::Result<blogs::Blog, _>`
               found type `std::result::Result<&blogs::Blog, _>`

error: aborting due to previous error

i don't know if to laugh or cry.

```rust pub fn find_by_host(c: &PlumeRocket, host: Host) -> Result<Blog> { let from_db = blogs::table .filter(blogs::custom_domain.eq(host)) .load::<Blog>(&*c.conn)? .first(); from_db.ok_or(Error::NotFound) } ``` results in ``` error[E0308]: mismatched types --> plume-models/src/blogs.rs:203:9 | 198 | pub fn find_by_host(c: &PlumeRocket, host: Host) -> Result<Blog> { | ------------ expected `std::result::Result<blogs::Blog, Error>` because of return type ... 203 | from_db.ok_or(Error::NotFound) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `blogs::Blog`, found reference | = note: expected type `std::result::Result<blogs::Blog, _>` found type `std::result::Result<&blogs::Blog, _>` error: aborting due to previous error ``` i don't know if to laugh or cry.
trinity-1686a revisionato 5 anni fa
plume-models/src/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

I meant diesel first, in place of load. Not Vec first (actually it's slice's, but Vec deref to slice)

I meant [diesel first](https://docs.diesel.rs/diesel/query_dsl/trait.RunQueryDsl.html#method.first), in place of load. Not [Vec first](https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.first) (actually it's slice's, but Vec deref to slice)
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato 
error[E0308]: mismatched types
   --> plume-models/src/blogs.rs:199:9
    |
198 |       pub fn find_by_host(c: &PlumeRocket, host: Host) -> Result<Blog> {
    |                                                           ------------ expected `std::result::Result<blogs::Blog, Error>` because of return type
199 | /         blogs::table
200 | |             .filter(blogs::custom_domain.eq(host))
201 | |             .first::<Blog>(&*c.conn)
    | |____________________________________^ expected enum `Error`, found enum `diesel::result::Error`
    |
    = note: expected type `std::result::Result<_, Error>`
               found type `std::result::Result<_, diesel::result::Error>`

but if i use QueryResult<Blog>, then i can't use ? for error handling later 🙍

``` error[E0308]: mismatched types --> plume-models/src/blogs.rs:199:9 | 198 | pub fn find_by_host(c: &PlumeRocket, host: Host) -> Result<Blog> { | ------------ expected `std::result::Result<blogs::Blog, Error>` because of return type 199 | / blogs::table 200 | | .filter(blogs::custom_domain.eq(host)) 201 | | .first::<Blog>(&*c.conn) | |____________________________________^ expected enum `Error`, found enum `diesel::result::Error` | = note: expected type `std::result::Result<_, Error>` found type `std::result::Result<_, diesel::result::Error>` ``` but if i use `QueryResult<Blog>`, then i can't use `?` for error handling later 🙍
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

removed comment.

removed comment.
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

actually, rustc is saying exactly what i need to do, so i did that.

actually, `rustc` is saying exactly what i need to do, so i did that.
elegaanz (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

What about adding a map_err(|_| Error::NotFound) here, to keep Result<Blog> as the return type (with the advantages it has, like having the possibility to "throw" it to get an error page)?

What about adding a `map_err(|_| Error::NotFound)` here, to keep `Result<Blog>` as the return type (with the advantages it has, like having the possibility to "throw" it to get an error page)?
elegaanz (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

And also it would allow you to remove the From<diesel::result::Error> for ErrorPage impl.

And also it would allow you to remove the `From<diesel::result::Error> for ErrorPage` impl.
trinity-1686a revisionato 5 anni fa
plume-models/src/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

What about adding a map_err(|_| Error::NotFound) here

or map_err(Error::from) ?

> What about adding a `map_err(|_| Error::NotFound)` here or `map_err(Error::from)` ?
igalic (Migrato da github.com) revisionato 5 anni fa
plume-models/src/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

seemed like a good deal to me to directly map a diesel::result::Error to an ErrorPage
otoh, i can also see how this would lead to the kind of error handling that PHP + MySQL are famous for

seemed like a good deal to me to directly map a `diesel::result::Error` to an `ErrorPage` otoh, i can also see how this would lead to the kind of error handling that PHP + MySQL are famous for
codecov[bot] 5 anni fa ha commentato (Migrato da github.com)

Codecov Report

Merging #596 into master will decrease coverage by 0.47%.
The diff coverage is 16.73%.

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   35.33%   34.86%   -0.48%     
==========================================
  Files          68       68              
  Lines        7915     8149     +234     
  Branches     1894     1946      +52     
==========================================
+ Hits         2797     2841      +44     
- Misses       4341     4529     +188     
- Partials      777      779       +2
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/596?src=pr&el=h1) Report > Merging [#596](https://codecov.io/gh/Plume-org/Plume/pull/596?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/fb60236a54893d690a8ddba34f597e4820431154?src=pr&el=desc) will **decrease** coverage by `0.47%`. > The diff coverage is `16.73%`. ```diff @@ Coverage Diff @@ ## master #596 +/- ## ========================================== - Coverage 35.33% 34.86% -0.48% ========================================== Files 68 68 Lines 7915 8149 +234 Branches 1894 1946 +52 ========================================== + Hits 2797 2841 +44 - Misses 4341 4529 +188 - Partials 777 779 +2 ```
igalic 5 anni fa ha commentato (Migrato da github.com)

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 and outbox_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 calls ap_url() which only prepends https://

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.

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` and `outbox_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 calls `ap_url()` which only prepends `https://` 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.
trinity-1686a 5 anni fa ha commentato
Proprietario

To do this, I would have to adapt the URLs ap_url, inbox_url and outbox_url on blog creation (or update) and i would have to modify the templates to use the correct URL scheme.

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

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.

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)

> To do this, I would have to adapt the URLs ap_url, inbox_url and outbox_url on blog creation (or update) and i would have to modify the templates to use the correct URL scheme. 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 > 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. 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)
👍 1
igalic 5 anni fa ha commentato (Migrato da github.com)

okay, so, at this point, i'm about as done as i can be, without touching the templates or refactoring the URL building.

okay, so, at this point, i'm about as done as i can be, without touching the templates or refactoring the URL building.
rfwatson (Migrato da github.com) revisionato 5 anni fa
migrations/sqlite/2019-05-22-124153_add_custom_domains/up.sql
@ -0,0 +1,57 @@
-- add custom_domain to blogs
CREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (
rfwatson (Migrato da github.com) 5 anni fa ha commentato

Just wondering why copying the table is required, instead of just adding the column?

Just wondering why copying the table is required, instead of just adding the column?
igalic (Migrato da github.com) revisionato 5 anni fa
migrations/sqlite/2019-05-22-124153_add_custom_domains/up.sql
@ -0,0 +1,57 @@
-- add custom_domain to blogs
CREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (
igalic (Migrato da github.com) 5 anni fa ha commentato

because SQLite supports a limited subset of ALTER TABLE

because [SQLite supports a limited subset of ALTER TABLE](https://www.sqlite.org/lang_altertable.html)
rfwatson (Migrato da github.com) revisionato 5 anni fa
migrations/sqlite/2019-05-22-124153_add_custom_domains/up.sql
@ -0,0 +1,57 @@
-- add custom_domain to blogs
CREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (
rfwatson (Migrato da github.com) 5 anni fa ha commentato

Damn, I missed it was SQLite. Thanks.

Damn, I missed it was SQLite. Thanks.
elegaanz (Migrato da github.com) revisionato 5 anni fa
elegaanz (Migrato da github.com) lascia un commento

I gave it a try, and it seems to work quite well so far! I left a few comment about the code itself.

I gave it a try, and it seems to work quite well so far! I left a few comment about the code itself.
plume-models/src/blogs.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

It should be self.fqn I think, because title is the "name" of the blog, not its slug.

It should be `self.fqn` I think, because title is the "name" of the blog, not its slug.
src/routes/blogs.rs
@ -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)?;
elegaanz (Migrato da github.com) 5 anni fa ha commentato

I think it will not properly format the page: you will get something like https://doma.in/?2 when you probably want ?page=2.

I think it will not properly format the page: you will get something like `https://doma.in/?2` when you probably want `?page=2`.
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/posts.rs
@ -40,2 +38,3 @@
) -> Result<Ructe, ErrorPage> {
rockets: &PlumeRocket,
) -> Result<RespondOrRedirect, ErrorPage> {
let conn = &*rockets.conn;
elegaanz (Migrato da github.com) 5 anni fa ha commentato

Does that (and the blog: &Blog above) work?

Does that (and the `blog: &Blog` above) work?
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/posts.rs
@ -40,2 +38,3 @@
) -> Result<Ructe, ErrorPage> {
rockets: &PlumeRocket,
) -> Result<RespondOrRedirect, ErrorPage> {
let conn = &*rockets.conn;
elegaanz (Migrato da github.com) 5 anni fa ha commentato

nvm, I thought it was a route, but it is just a regular function.

nvm, I thought it was a route, but it is just a regular function.
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/posts.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

I think this URL (and the one after) should include the article's slug.

I think this URL (and the one after) should include the article's slug.
trinity-1686a revisionato 5 anni fa
src/routes/posts.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

You can use the handy unreachable!("<msg>") instead of a plain panic, it has the same effect but is probably clearer

You can use the handy `unreachable!("<msg>")` instead of a plain panic, it has the same effect but is probably clearer
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/posts.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

oh gosh yes that was a copy paste!

oh gosh yes that was a copy paste!
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
@ -60,0 +151,4 @@
#[get("/<custom_domain>", rank = 1)]
pub fn activity_details(
custom_domain: String,
trinity-1686a 5 anni fa ha commentato
Proprietario

clippy says

    let custom_domain = if form.custom_domain.is_empty() {
clippy says ```suggestion let custom_domain = if form.custom_domain.is_empty() { ```
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
@ -60,0 +151,4 @@
#[get("/<custom_domain>", rank = 1)]
pub fn activity_details(
custom_domain: String,
elegaanz (Migrato da github.com) 5 anni fa ha commentato

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.

I can't accept your proposal via Github (apparently I don't have the right to push to this repository :upside_down_face:), but I made it manually, thanks.
igalic (Migrato da github.com) revisionato 5 anni fa
igalic (Migrato da github.com) lascia un commento

validation?

validation?
src/routes/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

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??

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??
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

would https://github.com/Plume-org/Plume/issues/296 be a solution to this issue?

would https://github.com/Plume-org/Plume/issues/296 be a solution to this issue?
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

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…

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…
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

In some complex setup, checking only a CNAME can be not enough, for instance at home I have subdomain.my_domain => my_domain, but my_domain is also a CNAME to a domain provided by my isp. Suppose Plume run on subdomain.my_domain, pointing to my_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

In some complex setup, checking only a CNAME can be not enough, for instance at home I have `subdomain.my_domain` => `my_domain`, but `my_domain` is also a CNAME to a domain provided by my isp. Suppose Plume run on subdomain.my_domain, pointing to `my_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
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
@ -86,0 +205,4 @@
random_id.clone(),
Instant::now().checked_add(Duration::new(60, 0)).unwrap(),
);
trinity-1686a 5 anni fa ha commentato
Proprietario 
        Flash::warning(
```suggestion Flash::warning( ```
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

And it avoids having to do some DNS queries directly, so even better 😄

And it avoids having to do some DNS queries directly, so even better :smile:
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

what would we be looking for when making such a Reqwest?
how do we identify our own instance?

what would we be looking for when making such a Reqwest? how do we identify our own instance?
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

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 error

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 error
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

is there a cryptographically secure way in ActivityPub or OcapPub to do this?

is there a cryptographically secure way in ActivityPub or OcapPub to do this?
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

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

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
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}
#[get("/blogs/new")]
trinity-1686a 5 anni fa ha commentato
Proprietario

the name mismatch the one in main.rs, and Result take 2 type arguments (probably both String I think)

the name mismatch the one in main.rs, and Result take 2 type arguments (probably both String I think)
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}
#[get("/blogs/new")]
igalic (Migrato da github.com) 5 anni fa ha commentato

thanks / sorry for the noise 🙇‍♀️

the two files were edited with about a week or two in between 😅

thanks / sorry for the noise 🙇‍♀️ the two files were edited with about a week or two in between 😅
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}
#[get("/blogs/new")]
trinity-1686a 5 anni fa ha commentato
Proprietario

np 😉

np :wink:
trinity-1686a revisionato 5 anni fa
trinity-1686a lascia un commento
Proprietario

👀

:eyes:
src/main.rs
@ -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();
trinity-1686a 5 anni fa ha commentato
Proprietario

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

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
src/routes/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

you should use duration_since instead, as you're unwrapping it it's doing exactly the same thing, and it's stable

you should use `duration_since` instead, as you're unwrapping it it's doing exactly the same thing, and it's stable
trinity-1686a 5 anni fa ha commentato
Proprietario

(this will only return the string 410, not the error code)

(this will only return the string 410, not the error code)
trinity-1686a 5 anni fa ha commentato
Proprietario

(this will only return the string 404, not the error code)

(this will only return the string 404, not the error code)
igalic (Migrato da github.com) revisionato 5 anni fa
src/main.rs
@ -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();
igalic (Migrato da github.com) 5 anni fa ha commentato

aye. i haven't gotten that far yet, because i haven't implemented the other side of this

aye. i haven't gotten that far yet, because i haven't implemented the other side of this
trinity-1686a revisionato 5 anni fa
trinity-1686a lascia un commento
Proprietario

👀

:eyes:
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a 5 anni fa ha commentato
Proprietario

I believe there are consts that already contains standards http status in the module

I believe there are consts that already contains standards http status in the module
trinity-1686a 5 anni fa ha commentato
Proprietario

Same as above

Same as above
src/routes/blogs.rs
@ -86,0 +213,4 @@
Ok(resp) => resp.status().is_success(),
Err(_) => false,
}
}
trinity-1686a 5 anni fa ha commentato
Proprietario

You should remove the feature, as it's no longer used

You should remove the feature, as it's no longer used
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
trinity-1686a 5 anni fa ha commentato
Proprietario

I don't get returning a constant value, I don't see any early return?

I don't get returning a constant value, I don't see any early return?
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
igalic (Migrato da github.com) 5 anni fa ha commentato 
error[E0308]: mismatched types
   --> src/routes/blogs.rs:113:28
    |
113 |         return Status::new(Status::NotFound, "validation id not found");
    |                            ^^^^^^^^^^^^^^^^ expected u16, found struct `rocket::http::Status`
    |
    = note: expected type `u16`
               found type `rocket::http::Status`

error[E0308]: mismatched types
   --> src/routes/blogs.rs:123:28
    |
123 |         return Status::new(Status::Gone, "validation expired");
    |                            ^^^^^^^^^^^^ expected u16, found struct `rocket::http::Status`
    |
    = note: expected type `u16`
               found type `rocket::http::Status`
``` error[E0308]: mismatched types --> src/routes/blogs.rs:113:28 | 113 | return Status::new(Status::NotFound, "validation id not found"); | ^^^^^^^^^^^^^^^^ expected u16, found struct `rocket::http::Status` | = note: expected type `u16` found type `rocket::http::Status` error[E0308]: mismatched types --> src/routes/blogs.rs:123:28 | 123 | return Status::new(Status::Gone, "validation expired"); | ^^^^^^^^^^^^ expected u16, found struct `rocket::http::Status` | = note: expected type `u16` found type `rocket::http::Status` ```
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a 5 anni fa ha commentato
Proprietario

Have you tried simply

        return Status::Gone;

?

Have you tried simply ```suggestion return Status::Gone; ``` ?
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
igalic (Migrato da github.com) 5 anni fa ha commentato

No, cuz I wanted to give a specific response.

No, cuz I wanted to give a specific response.
trinity-1686a revisionato 5 anni fa
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a 5 anni fa ha commentato
Proprietario 
        return Custom(Status::Gone, "validation expired");

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

```suggestion return Custom(Status::Gone, "validation expired"); ``` 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
elegaanz (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
elegaanz (Migrato da github.com) 5 anni fa ha commentato

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).

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).
igalic (Migrato da github.com) revisionato 5 anni fa
src/routes/blogs.rs
igalic (Migrato da github.com) 5 anni fa ha commentato

another question might be: why were you able to use Option<String> for theme in EditForm but i have to use String here?

another question might be: why were you able to use `Option<String>` for `theme` in `EditForm` but i have to use `String` here?
KitaitiMakoto chiusa questa pull request 3 anni fa
Questa pull request non può essere riaperta perché il branch è stato eliminato.
Puoi anche visualizzare le istruzioni da riga di comando.

Passo 1:

Dal repository del tuo progetto, fai il check out di un nuovo branch e verifica le modifiche.
git checkout -b igalic/feat/custom-fairing-domains master
git pull origin igalic/feat/custom-fairing-domains

Passo 2:

Fai il merge delle modifiche e aggiorna su Forgejo.
git checkout master
git merge --no-ff igalic/feat/custom-fairing-domains
git push origin master
Effettua l'accesso per partecipare alla conversazione.
Revisori
Richiedi revisione
Nessun revisore
Etichette
Applica etichette
Pulisci le etichette   
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
Nessuna etichetta
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
Traguardo
Imposta traguardo
Milestone pulita
Nessun elemento
Nessuna milestone
Assegnatari
Assegna utenti
Cancella assegnatari
Nessuna assegnatario
2 Partecipanti
Notifiche
Data di scadenza
La data di scadenza non è valida o fuori intervallo. Si prega di utilizzare il formato 'aaaa-mm-dd'.

Nessuna data di scadenza impostata.

Dipendenze

Nessuna dipendenza impostata.

Riferimento: Plume/Plume#596
Scrivi Anteprima
Caricamento…
Annulla
Salva
Non ci sono ancora contenuti.
Elimina branch 'igalic/feat/custom-fairing-domains'

L'eliminazione di un branch è definitiva. È un'operazione che NON PUÒ essere annullata. Continuare?

No
Sì