Plume
/
Plume
8
16
Derivar 22
Código Questões 174 Pedidos de integração 8 Lançamentos 8 Wiki Trabalho

Rótulos Etapas

feature: custom domains using Fairings #596

fechada(s)
igalic quer integrar 48 cometimento(s) do ramo igalic/feat/custom-fairing-domains no ramo master
puxar de: igalic/feat/custom-fairing-domains
integrar em: Plume:master
Diálogo 5 Cometimentos 48 Ficheiros modificados 23
igalic comentou há 5 anos (Migrado de 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] comentou há 5 anos (Migrado de 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 reviu há 5 anos
src/main.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 reviu há 5 anos
plume-models/src/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a) 
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 reviu há 5 anos
plume-models/src/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 comentou há 5 anos
Proprietário(a)

(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 comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
plume-models/src/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos 
    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 reviu há 5 anos
plume-models/src/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos 
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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

removed comment.

removed comment.
igalic (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
elegaanz (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
elegaanz (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
plume-models/src/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
plume-models/src/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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] comentou há 5 anos (Migrado de 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 comentou há 5 anos (Migrado de 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 comentou há 5 anos
Proprietário(a)

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 comentou há 5 anos (Migrado de 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 (Migrado de github.com) reviu há 5 anos
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 (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
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 (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
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 (Migrado de github.com) comentou há 5 anos

Damn, I missed it was SQLite. Thanks.

Damn, I missed it was SQLite. Thanks.
elegaanz (Migrado de github.com) reviu há 5 anos
elegaanz (Migrado de github.com) deixou um comentário

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 (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
src/routes/posts.rs
@ -40,2 +38,3 @@
) -> Result<Ructe, ErrorPage> {
rockets: &PlumeRocket,
) -> Result<RespondOrRedirect, ErrorPage> {
let conn = &*rockets.conn;
elegaanz (Migrado de github.com) comentou há 5 anos

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

Does that (and the `blog: &Blog` above) work?
elegaanz (Migrado de github.com) reviu há 5 anos
src/routes/posts.rs
@ -40,2 +38,3 @@
) -> Result<Ructe, ErrorPage> {
rockets: &PlumeRocket,
) -> Result<RespondOrRedirect, ErrorPage> {
let conn = &*rockets.conn;
elegaanz (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
src/routes/posts.rs
elegaanz (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
src/routes/posts.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
src/routes/posts.rs
igalic (Migrado de github.com) comentou há 5 anos

oh gosh yes that was a copy paste!

oh gosh yes that was a copy paste!
trinity-1686a reviu há 5 anos
src/routes/blogs.rs
@ -60,0 +151,4 @@
#[get("/<custom_domain>", rank = 1)]
pub fn activity_details(
custom_domain: String,
trinity-1686a comentou há 5 anos
Proprietário(a)

clippy says

    let custom_domain = if form.custom_domain.is_empty() {
clippy says ```suggestion let custom_domain = if form.custom_domain.is_empty() { ```
elegaanz (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
@ -60,0 +151,4 @@
#[get("/<custom_domain>", rank = 1)]
pub fn activity_details(
custom_domain: String,
elegaanz (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
igalic (Migrado de github.com) deixou um comentário

validation?

validation?
src/routes/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
elegaanz (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
src/routes/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 reviu há 5 anos
src/routes/blogs.rs
@ -86,0 +205,4 @@
random_id.clone(),
Instant::now().checked_add(Duration::new(60, 0)).unwrap(),
);
trinity-1686a comentou há 5 anos
Proprietário(a) 
        Flash::warning(
```suggestion Flash::warning( ```
elegaanz (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
elegaanz (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
src/routes/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
src/routes/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 reviu há 5 anos
src/routes/blogs.rs
@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}
#[get("/blogs/new")]
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}
#[get("/blogs/new")]
igalic (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
src/routes/blogs.rs
@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}
#[get("/blogs/new")]
trinity-1686a comentou há 5 anos
Proprietário(a)

np 😉

np :wink:
trinity-1686a reviu há 5 anos
trinity-1686a deixou um comentário
Proprietário(a)

👀

: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 comentou há 5 anos
Proprietário(a)

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 comentou há 5 anos
Proprietário(a)

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 comentou há 5 anos
Proprietário(a)

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

(this will only return the string 410, not the error code)
trinity-1686a comentou há 5 anos
Proprietário(a)

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

(this will only return the string 404, not the error code)
igalic (Migrado de github.com) reviu há 5 anos
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 (Migrado de github.com) comentou há 5 anos

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 reviu há 5 anos
trinity-1686a deixou um comentário
Proprietário(a)

👀

:eyes:
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a comentou há 5 anos
Proprietário(a)

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 comentou há 5 anos
Proprietário(a)

Same as above

Same as above
src/routes/blogs.rs
@ -86,0 +213,4 @@
Ok(resp) => resp.status().is_success(),
Err(_) => false,
}
}
trinity-1686a comentou há 5 anos
Proprietário(a)

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

You should remove the feature, as it's no longer used
trinity-1686a reviu há 5 anos
src/routes/blogs.rs
trinity-1686a comentou há 5 anos
Proprietário(a)

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
igalic (Migrado de github.com) comentou há 5 anos 
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 reviu há 5 anos
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a comentou há 5 anos
Proprietário(a)

Have you tried simply

        return Status::Gone;

?

Have you tried simply ```suggestion return Status::Gone; ``` ?
igalic (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
igalic (Migrado de github.com) comentou há 5 anos

No, cuz I wanted to give a specific response.

No, cuz I wanted to give a specific response.
trinity-1686a reviu há 5 anos
src/routes/blogs.rs
@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a comentou há 5 anos
Proprietário(a) 
        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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
elegaanz (Migrado de github.com) comentou há 5 anos

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 (Migrado de github.com) reviu há 5 anos
src/routes/blogs.rs
igalic (Migrado de github.com) comentou há 5 anos

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 fechou este pedido de integração há 3 anos
Este pedido de integração não pode ser reaberto porque o ramo foi eliminado.
Também pode ver as instruções para a linha de comandos.

Passo 1:

No seu repositório, crie um novo ramo e teste as modificações.
git checkout -b igalic/feat/custom-fairing-domains master
git pull origin igalic/feat/custom-fairing-domains

Passo 2:

Integre as modificações e envie para o Forgejo.
git checkout master
git merge --no-ff igalic/feat/custom-fairing-domains
git push origin master
Inicie a sessão para participar neste diálogo.
Revisores
Solicitar revisão
Sem revisores
Rótulos
Aplicar rótulos
Retirar rótulos   
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
Sem rótulo
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
Etapa
Definir etapa
Limpar etapa
Sem itens
Sem etapa
Encarregados
Definir encarregados
Retirar todos os encarregados
Sem encarregados
2 Participantes
Notificações
Data de vencimento
A data de vencimento é inválida ou está fora do intervalo permitido. Por favor, use o formato 'aaaa-mm-dd'.

Sem data de vencimento definida.

Dependências

Não estão definidas dependências.

Referência: Plume/Plume#596
Escrever Pré-visualizar
Carregando…
Cancelar
Guardar
Ainda não há conteúdo.
Eliminar o ramo 'igalic/feat/custom-fairing-domains'

Eliminar um ramo é algo permanente. NÃO PODERÁ ser revertido. Quer continuar?

Não
Sim