Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use doctype setting to set auto-extracted file as private #24828

Merged
merged 7 commits into from Mar 15, 2024

Conversation

marination
Copy link
Collaborator

@marination marination commented Feb 9, 2024

  • Use make_attachments_public to determine the file privacy while auto creating files from the text editor field
    Screenshot 2024-02-09 at 3 04 29 PM

    2024-02-09 3 11 57 PM

  • Currently, all files in the text editor field are automatically public and there is no way to control this behaviour except manually setting the file as private each time.

- Use `make_attachments_public` to determine the file privacy while auto creating files from the text editor field
- Currently, all files in the text editor field are automatically public
@marination marination requested a review from a team as a code owner February 9, 2024 14:08
@marination marination requested review from shariquerik and removed request for a team February 9, 2024 14:08
@ankush
Copy link
Member

ankush commented Feb 9, 2024

TODO:

  • test communication crafted using this
  • test pdf crafted using this

@ankush ankush force-pushed the txt-attachment-privacy branch 2 times, most recently from 8653f47 to ed6613c Compare February 9, 2024 14:38
…c for Communication

- Note: DocType Communication's records are used for emails that have the file link embedded in its content
- fix: The files extracted from a Communication must be public so that they are visible in an email (link in email)
- Test: Check if file is created appropriately from a Comment and a Communication
@marination
Copy link
Collaborator Author

@ankush not sure how to handle test pdf crafted using this, i can't seem to get content out of pdfreader's pages. Would be nice if you could handle this one.

@ankush
Copy link
Member

ankush commented Feb 10, 2024

@marination this doesn't work afaik

We use cookies from current request and pass it to wkhtml so it can read private images. However when it's done from background jobs it's not possible.

frappe/frappe/utils/pdf.py

Lines 179 to 193 in 4e1ed29

def get_cookie_options():
options = {}
if frappe.session and frappe.session.sid and hasattr(frappe.local, "request"):
# Use wkhtmltopdf's cookie-jar feature to set cookies and restrict them to host domain
cookiejar = f"/tmp/{frappe.generate_hash()}.jar"
# Remove port from request.host
# https://werkzeug.palletsprojects.com/en/0.16.x/wrappers/#werkzeug.wrappers.BaseRequest.host
domain = frappe.utils.get_host_name().split(":", 1)[0]
with open(cookiejar, "w") as f:
f.write(f"sid={frappe.session.sid}; Domain={domain};\n")
options["cookie-jar"] = cookiejar
return options


Possible fix that we have discussed before:

  • For communication: send images as inline attachments with some sane max size limit (~5mb)
  • For PDFs - send images as base64 encoded source to wkhtml so private file isn't a problem.

Both are long pending design flaws, will have get it fixed somehow to make this work 😔

@ankush ankush self-assigned this Feb 21, 2024
@ankush
Copy link
Member

ankush commented Feb 21, 2024

update: working on base64-ing private images rn. Once we have this it's easier to port this change without breaking anything.

#24980

@ankush
Copy link
Member

ankush commented Feb 22, 2024

the world of email is 💩

https://sendgrid.com/blog/embedding-images-emails-facts/

@barredterra
Copy link
Collaborator

barredterra commented Mar 5, 2024

How about this approach:

This way we stay close to the current implementation but get fairly good security.

@ankush ankush removed their assignment Mar 15, 2024
@ankush ankush merged commit 7640eba into frappe:develop Mar 15, 2024
19 of 23 checks passed
@marination marination added backport version-15-hotfix Backport the PR to v15 backport version-14-hotfix backport to version 14 labels Mar 27, 2024
ankush added a commit that referenced this pull request Mar 27, 2024
…ort #24828) (#25673)

* fix: Use doctype setting to set auto-extracted file as private

- Use `make_attachments_public` to determine the file privacy while auto creating files from the text editor field
- Currently, all files in the text editor field are automatically public

(cherry picked from commit 7445de9)

* test: use meta for fetching settings

(cherry picked from commit ed6613c)

* test: Comment + Comm.n file extraction & Attachments default to public for Communication

- Note: DocType Communication's records are used for emails that have the file link embedded in its content
- fix: The files extracted from a Communication must be public so that they are visible in an email (link in email)
- Test: Check if file is created appropriately from a Comment and a Communication

(cherry picked from commit 65c8a37)

* fix: Use regex in failing test

(cherry picked from commit 1f3e32a)

---------

Co-authored-by: marination <maricadsouza221197@gmail.com>
Co-authored-by: Ankush Menat <ankushmenat@gmail.com>
akhilnarang added a commit that referenced this pull request Mar 27, 2024
…-24828

fix: Use doctype setting to set auto-extracted file as private (backport #24828)
frappe-pr-bot pushed a commit that referenced this pull request Apr 2, 2024
# [15.20.0](v15.19.1...v15.20.0) (2024-04-02)

### Bug Fixes

* advertise insights to system manager only ([7046320](7046320))
* cint -> avoid precision loss if already integer ([#25735](#25735)) ([#25737](#25737)) ([6c822e0](6c822e0))
* **enqueue:** pass the original method argument here ([#25722](#25722)) ([2c14450](2c14450)), closes [/github.com/frappe/frappe/blob/87d121f47a4afc507442a97bf1854bb3d17f42c6/frappe/email/doctype/email_queue/email_queue.py#L735-L736](https://github.com//github.com/frappe/frappe/blob/87d121f47a4afc507442a97bf1854bb3d17f42c6/frappe/email/doctype/email_queue/email_queue.py/issues/L735-L736)
* **event:** clear message after handling exception ([eb9e88e](eb9e88e))
* fieldname extraction (backport [#24411](#24411)) ([#25670](#25670)) ([ed4e1b3](ed4e1b3)), closes [#22892](#22892)
* incorrect status on data import (backport [#25660](#25660)) ([#25703](#25703)) ([0165c75](0165c75))
* incorrect UI icon for desc sort ([#25687](#25687)) ([#25689](#25689)) ([f7f2849](f7f2849))
* invalid filter on email acccount ([#25674](#25674)) ([#25676](#25676)) ([c15b47a](c15b47a))
* let's colored tags in listview ([#25552](#25552)) ([4e17959](4e17959))
* make ads translatable ([217ef0b](217ef0b))
* make insights ad translatable ([43e6734](43e6734))
* message update in custom app if is_standard ([#25754](#25754)) ([86ad2e6](86ad2e6))
* non-html notifications from files ([a35e9ba](a35e9ba))
* preserve original error message ([#25682](#25682)) ([#25685](#25685)) ([3d364b7](3d364b7))
* reserved keywords as col name ([#25718](#25718)) ([#25726](#25726)) ([fca1c1a](fca1c1a))
* **restore:** check backup directory and bench directory if we can't find the file ([e6e4258](e6e4258))
* translatable web footer ([99bbd94](99bbd94))
* Use CssParser to correctly pass options to wkhtmltopdf ([e9811ea](e9811ea))
* Use doctype setting to set auto-extracted file as private (backport [#24828](#24828)) ([#25673](#25673)) ([14ccbe7](14ccbe7))

### Features

* allow skipping msgprint ([59813db](59813db)), closes [/github.com/frappe/frappe/blob/version-15/frappe/desk/doctype/event/event.py#L398](https://github.com//github.com/frappe/frappe/blob/version-15/frappe/desk/doctype/event/event.py/issues/L398)
* **customize_form:** allow setting `creation` as a default sort field ([#25760](#25760)) ([d540c72](d540c72))
* **notification:** specify message type (html, md, txt) ([e9a8a14](e9a8a14))

### Reverts

* Revert "fix: message update in custom app if is_standard (#25754)" (#25767) ([f5a17f7](f5a17f7)), closes [#25754](#25754) [#25767](#25767)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 backport version-15-hotfix Backport the PR to v15
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants