LemmyNet
/
lemmy
3
2
Fork 3
Code Pull Requests 1 Releases Wiki Activity

Proxy pictshare requests (fixes #371) #13

Closed
nutomic wants to merge 0 commits from nutomic/lemmy:proxy-pictshare into master
pull from: nutomic/lemmy:proxy-pictshare
merge into: LemmyNet:master
Conversation 5 Commits - Files Changed 279 +7077 -17917
nutomic commented 2020-01-11 18:30:48 +00:00
Owner
Copy Link

Tested and working fine for upload, image and thumbnail viewing. I'm couldnt test videos because it always tells me that I reached the size limit (at a few hundred kb).

TODO:

  • remove/adjust nginx pictshare config
  • check authentication for image upload (please tell me how to do that)
Tested and working fine for upload, image and thumbnail viewing. I'm couldnt test videos because it always tells me that I reached the size limit (at a few hundred kb). TODO: - [ ] remove/adjust nginx pictshare config - [ ] check authentication for image upload (please tell me how to do that)
nutomic added a new dependency 2020-01-11 18:30:58 +00:00
#11 Upgrade actix to 2.0 (fixes #392)
dessalines reviewed 2020-01-11 22:44:53 +00:00
dessalines left a comment
Owner
Copy Link

k, let me know when this is ready to test too.

k, let me know when this is ready to test too.
ansible/templates/nginx.conf Outdated
@ -61,6 +61,7 @@ server {
proxy_set_header Connection "upgrade";
}
# TODO: probably remove this (at least the proxy_pass)
dessalines commented 2020-01-11 22:34:21 +00:00
Owner
Copy Link

We should leave, because I do want pictshare to be embedded by default.

We should leave, because I do want pictshare to be embedded by default.
nutomic commented 2020-01-11 23:31:30 +00:00
Author
Owner
Copy Link

Why exactly? This setup has multiple benefits:

  • prevent image uploading without authentification
  • we can provide access control for images (eg for private communities, or images in direct message)
  • we can add or change headers as needed
  • allows us to keep track of images and delete them when the post is deleted
Why exactly? This setup has multiple benefits: - prevent image uploading without authentification - we can provide access control for images (eg for private communities, or images in direct message) - we can add or change headers as needed - allows us to keep track of images and delete them when the post is deleted
docker/dev/docker-compose.yml
@ -26,4 +29,2 @@
ports:
- "127.0.0.1:8537:80"
volumes:
- lemmy_pictshare:/usr/share/nginx/html/data
dessalines commented 2020-01-11 22:37:38 +00:00
Owner
Copy Link

I'd have to test to make sure changing its name like that isn't gonna break the data volume.

I'd have to test to make sure changing its name like that isn't gonna break the data volume.
nutomic commented 2020-01-11 23:29:07 +00:00
Author
Owner
Copy Link

Forgot to mention, I had to change the name because the Url crate fails if the domain contains a _. Volume name stays the same, so it wont break anything.

Forgot to mention, I had to change the name because the `Url` crate fails if the domain contains a `_`. Volume name stays the same, so it wont break anything.
docker/lemmy.hjson Outdated
@ -24,2 +24,4 @@
# The dir for the front end
front_end_dir: "/app/dist"
# The url where pictshare is available (this should only be exposed to lemmy, not to the outside)
pictshare_url: "http://pictshare:80"
dessalines commented 2020-01-11 22:39:04 +00:00
Owner
Copy Link

Def gotta keep this off port 80, even if it is internal only.

Def gotta keep this off port 80, even if it is internal only.
nutomic commented 2020-01-11 23:33:12 +00:00
Author
Owner
Copy Link

Wasnt sure what to put here, in the local dev setup, there is no pictshare around anyway, and in docker we override it via environment. Maybe option is a good idea, then we just return error 500 for all image related requests.

Wasnt sure what to put here, in the local dev setup, there is no pictshare around anyway, and in docker we override it via environment. Maybe option is a good idea, then we just return error 500 for all image related requests.
server/config/defaults.hjson Outdated
@ -25,2 +25,4 @@
# The dir for the front end
front_end_dir: "../ui/dist"
# The url where pictshare is available (this should only be exposed to lemmy, not to the outside)
pictshare_url: "http://localhost:80"
dessalines commented 2020-01-11 22:44:03 +00:00
Owner
Copy Link

This should be commented out by default, using the host/pictshare as a default.

This should be commented out by default, using the `host/pictshare` as a default.
server/src/routes/images.rs Outdated
@ -0,0 +65,4 @@
// Remove `Connection` as per
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection#Directives
for (header_name, header_value) in res.headers().iter().filter(|(h, _)| *h != "connection") {
client_resp.header(header_name.clone(), header_value.clone());
dessalines commented 2020-01-11 22:41:59 +00:00
Owner
Copy Link

I do like the fact that we might be able to manipulate headers here.

I do like the fact that we might be able to manipulate headers here.
server/src/settings.rs Outdated
@ -18,6 +18,7 @@ pub struct Settings {
pub rate_limit: RateLimitConfig,
pub email: Option<EmailConfig>,
pub federation_enabled: bool,
pub pictshare_url: String,
dessalines commented 2020-01-11 22:43:10 +00:00
Owner
Copy Link

This should be an <Option>, using the host/pictshare as default.

This should be an `<Option>`, using the host/pictshare as default.
dessalines commented 2020-01-11 23:00:51 +00:00
Owner
Copy Link

The only thing I found for that is that this needs to go in the nginx server block for pictshare uploads to work:

    # Upload limit for pictshare
    client_max_body_size 50M;

There might be a way to do this with env vars or something, since I think pictshare already embeds its own nginx.

For the auth, here's the main line everything else uses:

    let claims = match Claims::decode(&data.auth) {
      Ok(claims) => claims.claims,
      Err(_e) => return Err(APIError::err(&self.op, "not_logged_in").into()),
    };

Which means that the front end requests for a pictshare upload will also need to bundle an auth token.

The only thing I found for that is that this needs to go in the nginx server block for pictshare uploads to work: ``` # Upload limit for pictshare client_max_body_size 50M; ``` There might be a way to do this with env vars or something, since I think pictshare already embeds its own nginx. For the auth, here's the main line everything else uses: ``` let claims = match Claims::decode(&data.auth) { Ok(claims) => claims.claims, Err(_e) => return Err(APIError::err(&self.op, "not_logged_in").into()), }; ``` Which means that the front end requests for a pictshare upload will also need to bundle an `auth` token.
dessalines reviewed 2020-01-11 23:02:38 +00:00
server/src/routes/images.rs Outdated
dessalines commented 2020-01-11 23:02:38 +00:00
Owner
Copy Link

So this body should be basically {auth, fileData}. However you want to structure it, I can handle it on the front end.

So this body should be basically {auth, fileData}. However you want to structure it, I can handle it on the front end.
nutomic commented 2020-01-11 23:27:28 +00:00
Author
Owner
Copy Link

The auth token is already sent in the cookie, but I'm not sure how to access that via actix.

The auth token is already sent in the cookie, but I'm not sure how to access that via actix.
nutomic changed title from WIP: Proxy pictshare requests (fixes #371) to Proxy pictshare requests (fixes #371) 2020-01-14 16:42:17 +00:00
nutomic commented 2020-01-14 16:42:55 +00:00
Author
Owner
Copy Link

Still waiting for the Docker compile. Should be ready, other than testing and possibly changing nginx/docker-compose configs.

Still waiting for the Docker compile. Should be ready, other than testing and possibly changing nginx/docker-compose configs.
dessalines commented 2020-01-15 02:20:19 +00:00
Owner
Copy Link

I tested this out, and one thing unfortunately is that the size limit is on actix, not pictshare. It needs to basically forward the request in a multipart stream. Here's an example.

https://github.com/actix/examples/blob/master/multipart/src/main.rs

I tested this out, and one thing unfortunately is that the size limit is on actix, not pictshare. It needs to basically forward the request in a multipart stream. Here's an example. https://github.com/actix/examples/blob/master/multipart/src/main.rs
nutomic commented 2020-01-27 17:23:13 +00:00
Author
Owner
Copy Link

It looks like actix itself has no support for multipart. There is an issue and a PR that reference this, but neither were actually resolved/merged. I tried the two libraries mentioned in the issue, but couldnt get either of them working.

I will try to do the backend requests with reqwest instead, but its annoying that we will probably need a seperate library for that, and store the image to disk before forwarding it.

It looks like actix itself has no support for multipart. There is an [issue](https://github.com/actix/actix-web/issues/657) and a [PR](https://github.com/actix/actix-web/pull/477) that reference this, but neither were actually resolved/merged. I tried the two libraries mentioned in the issue, but couldnt get either of them working. I will try to do the backend requests with `reqwest` instead, but its annoying that we will probably need a seperate library for that, and store the image to disk before forwarding it.
nutomic closed this pull request 2020-03-19 15:24:18 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Depends on
#11 Upgrade actix to 2.0 (fixes #392)
LemmyNet/lemmy
Reference: LemmyNet/lemmy#13
No description provided.
Delete Branch "nutomic/lemmy:proxy-pictshare"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?