#596 feature: custom domains using Fairings

Open
igalic wants to merge 48 commits from igalic/feat/custom-fairing-domains into master
igalic commented 1 year ago (Migrated from 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

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)
codecov[bot] commented 1 year ago (Migrated from github.com)
Owner

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago
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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 commented 1 year ago

(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 commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago
    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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago
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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

removed comment.

removed comment.
igalic (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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] commented 1 year ago (Migrated from github.com)
Owner

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 commented 1 year ago (Migrated from github.com)
Owner

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 commented 1 year ago
Owner

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)
igalic commented 1 year ago (Migrated from github.com)
Owner

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 (Migrated from github.com) reviewed 1 year ago
@@ -0,0 +1,57 @@
-- add custom_domain to blogs
CREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
@@ -0,0 +1,57 @@
-- add custom_domain to blogs
CREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (
Plume_migration_agent commented 1 year ago
because [SQLite supports a limited subset of ALTER TABLE](https://www.sqlite.org/lang_altertable.html)
rfwatson (Migrated from github.com) reviewed 1 year ago
@@ -0,0 +1,57 @@
-- add custom_domain to blogs
CREATE TABLE IF NOT EXISTS "blogs_add_custom_domain" (
Plume_migration_agent commented 1 year ago

Damn, I missed it was SQLite. Thanks.

Damn, I missed it was SQLite. Thanks.
elegaanz (Migrated from github.com) reviewed 1 year ago
elegaanz (Migrated from github.com) left a comment

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

Plume_migration_agent commented 1 year ago

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.
@@ -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)?;
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
@@ -40,2 +38,3 @@
) -> Result<Ructe, ErrorPage> {
rockets: &PlumeRocket,
) -> Result<RespondOrRedirect, ErrorPage> {
let conn = &*rockets.conn;
Plume_migration_agent commented 1 year ago

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

Does that (and the `blog: &Blog` above) work?
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -40,2 +38,3 @@
) -> Result<Ructe, ErrorPage> {
rockets: &PlumeRocket,
) -> Result<RespondOrRedirect, ErrorPage> {
let conn = &*rockets.conn;
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

oh gosh yes that was a copy paste!

oh gosh yes that was a copy paste!
trinity-1686a reviewed 1 year ago
@@ -60,0 +151,4 @@

#[get("/<custom_domain>", rank = 1)]
pub fn activity_details(
custom_domain: String,
trinity-1686a commented 1 year ago

clippy says

    let custom_domain = if form.custom_domain.is_empty() {
clippy says ```suggestion let custom_domain = if form.custom_domain.is_empty() { ```
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -60,0 +151,4 @@

#[get("/<custom_domain>", rank = 1)]
pub fn activity_details(
custom_domain: String,
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
igalic (Migrated from github.com) left a comment

validation?

Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 reviewed 1 year ago
@@ -86,0 +205,4 @@
random_id.clone(),
Instant::now().checked_add(Duration::new(60, 0)).unwrap(),
);

trinity-1686a commented 1 year ago
        Flash::warning(
```suggestion Flash::warning( ```
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a commented 1 year ago

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 reviewed 1 year ago
@@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}

#[get("/blogs/new")]
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
@@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}

#[get("/blogs/new")]
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
@@ -49,3 +90,4 @@
activity_detail_guts(blog, rockets, _ap)
}

#[get("/blogs/new")]
trinity-1686a commented 1 year ago

np 😉

np :wink:
trinity-1686a reviewed 1 year ago
trinity-1686a left a comment

👀

@@ -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 commented 1 year ago

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
trinity-1686a commented 1 year ago

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 commented 1 year ago

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

(this will only return the string 410, not the error code)
trinity-1686a commented 1 year ago

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

(this will only return the string 404, not the error code)
igalic (Migrated from github.com) reviewed 1 year ago
@@ -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();
Plume_migration_agent commented 1 year ago

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 reviewed 1 year ago
trinity-1686a left a comment

👀

@@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a commented 1 year ago

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 commented 1 year ago

Same as above

Same as above
@@ -86,0 +213,4 @@
Ok(resp) => resp.status().is_success(),
Err(_) => false,
}
}
trinity-1686a commented 1 year ago

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

You should remove the feature, as it's no longer used
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
@@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
Plume_migration_agent commented 1 year ago
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 reviewed 1 year ago
@@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a commented 1 year ago

Have you tried simply

        return Status::Gone;

?

Have you tried simply ```suggestion return Status::Gone; ``` ?
igalic (Migrated from github.com) reviewed 1 year ago
@@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
Plume_migration_agent commented 1 year ago

No, cuz I wanted to give a specific response.

No, cuz I wanted to give a specific response.
trinity-1686a reviewed 1 year ago
@@ -57,6 +99,76 @@ pub fn new(rockets: PlumeRocket, _user: User) -> Ructe {
))
}
trinity-1686a commented 1 year ago
        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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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 (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

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?
This pull request has changes conflicting with the target branch.
Cargo.toml
plume-models/Cargo.toml
plume-models/src/blogs.rs
src/main.rs
src/routes/mod.rs
Cargo.lock
plume-models/src/lib.rs
plume-models/src/schema.rs
src/routes/blogs.rs
src/routes/errors.rs
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.