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] mail: allow bypassing message attachments check #164894

Conversation

reth-odoo
Copy link
Contributor

@reth-odoo reth-odoo commented May 8, 2024

We only care about data fields when it comes to restricting access to message attachments.
As some modules may use these attachments directly, it may be desirable for them to use
the attachment name or similar metadata fields.

"write" is only restricted if writing on a data field, as it's effectively the same as unlinking
for our purposes. Other fields have the same access rights as prior to [1]

Additional changes:

  • Admins can delete attachments from message in the UI again
  • Check returns True instead of None when it succeeds, to match its parent
  • AccessError is raised with a cause so that we can detect if it was raised because of the message check

1: 4c4e63f

task-3519815

@robodoo
Copy link
Contributor

robodoo commented May 8, 2024

Pull request status dashboard.

@reth-odoo reth-odoo force-pushed the 15.0-documents-fix-mail-attachment-write-access-reth branch from 32223b8 to 3d0da46 Compare May 8, 2024 09:54
@C3POdoo C3POdoo added the RD research & development, internal work label May 8, 2024
Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @reth-odoo

A couple light comments but globally LGTM for the community part.

Thanks for your reactivity on this & cheers!

addons/mail/i18n/mail.pot Show resolved Hide resolved
addons/mail/i18n/mail.pot Outdated Show resolved Hide resolved
addons/mail/models/ir_attachment.py Outdated Show resolved Hide resolved
addons/mail/static/src/models/attachment/attachment.js Outdated Show resolved Hide resolved
@reth-odoo reth-odoo force-pushed the 15.0-documents-fix-mail-attachment-write-access-reth branch 2 times, most recently from ff7202a to 28a768c Compare May 8, 2024 12:46
@reth-odoo reth-odoo marked this pull request as ready for review May 8, 2024 12:46
@C3POdoo C3POdoo requested review from a team May 8, 2024 12:47
@reth-odoo reth-odoo force-pushed the 15.0-documents-fix-mail-attachment-write-access-reth branch from 28a768c to 93ef9d8 Compare May 8, 2024 14:36
Some modules may use attachments from mail messages directly.

In that case it may be desirable to at least be able to write
over the name and other non-critical information even if the
attachment is linked to a document.

The restrictions on writing on message attachments is reduced
to only apply to data fields, as those are the only ones that
we really don't want people to change.

Also return True instead of None in the override of `check`
to match the behavior of the parent.

Also reword the error message to convey writing is also forbidden.

Complementary to 4c4e63f

task-3519815
@reth-odoo reth-odoo force-pushed the 15.0-documents-fix-mail-attachment-write-access-reth branch from 93ef9d8 to ab6bac3 Compare May 8, 2024 14:52
Administrators are technically allowed to delete attachments from any message.

However the UI does not reflect that since [1]

1: 4c4e63f

task-3519815
@reth-odoo reth-odoo force-pushed the 15.0-documents-fix-mail-attachment-write-access-reth branch from ab6bac3 to 8537b4e Compare May 8, 2024 14:57
@awa-odoo
Copy link
Contributor

awa-odoo commented May 8, 2024

@robodoo r+ rebase-ff

@robodoo
Copy link
Contributor

robodoo commented May 8, 2024

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request May 8, 2024
Some modules may use attachments from mail messages directly.

In that case it may be desirable to at least be able to write
over the name and other non-critical information even if the
attachment is linked to a document.

The restrictions on writing on message attachments is reduced
to only apply to data fields, as those are the only ones that
we really don't want people to change.

Also return True instead of None in the override of `check`
to match the behavior of the parent.

Also reword the error message to convey writing is also forbidden.

Complementary to 4c4e63f

task-3519815

Part-of: #164894
robodoo pushed a commit that referenced this pull request May 8, 2024
Administrators are technically allowed to delete attachments from any message.

However the UI does not reflect that since [1]

1: 4c4e63f

task-3519815

closes #164894

Related: odoo/enterprise#62174
Signed-off-by: Warnon Aurélien (awa) <awa@odoo.com>
@robodoo robodoo closed this May 8, 2024
@fw-bot
Copy link
Contributor

fw-bot commented May 12, 2024

@fw-bot
Copy link
Contributor

fw-bot commented May 13, 2024

@reth-odoo @awa-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot fw-bot deleted the 15.0-documents-fix-mail-attachment-write-access-reth branch May 22, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants