diff --git a/spp_registry/README.rst b/spp_registry/README.rst index 4eb0e993a..ffa8a1068 100644 --- a/spp_registry/README.rst +++ b/spp_registry/README.rst @@ -139,6 +139,27 @@ Dependencies Changelog ========= +19.0.2.2.5 +~~~~~~~~~~ + +- fix(registry): remove the ``/mail/message/update_content`` controller + override. It was ported from Odoo 17, where it was already redundant: + stock 17 granted author-or-administrator editing too, and the override + only narrowed "administrator" to ``base.group_system`` and answered + with an access error instead of a 404. On Odoo 19 it no longer worked + at all, breaking **Edit** and **Delete** on every chatter message + across the instance: its signature still expected the Odoo 17 request + shape while the Odoo 19 web client sends ``update_data``, so every + call failed before running, and behind that it also called the removed + ``ir.attachment._check_attachments_access`` and returned through the + removed ``message_format()``. Odoo 19's own route already grants + author-or-administrator editing, so the override is dropped and the + route is stock again. Who may edit another user's chatter message + therefore follows Odoo's own administrator test (superuser or + **Administration: Access Rights**, which **Administration: Settings** + implies) instead of Settings only, and the editor must also be allowed + to post on the record; no OpenSPP role sits in that gap (#419) + 19.0.2.2.4 ~~~~~~~~~~ diff --git a/spp_registry/__manifest__.py b/spp_registry/__manifest__.py index 2ed500e32..dd231ba18 100644 --- a/spp_registry/__manifest__.py +++ b/spp_registry/__manifest__.py @@ -3,7 +3,7 @@ { "name": "OpenSPP Registry", "category": "OpenSPP/Core", - "version": "19.0.2.2.4", + "version": "19.0.2.2.5", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_registry/controllers/mail.py b/spp_registry/controllers/mail.py index 804d62fdc..e009ff502 100644 --- a/spp_registry/controllers/mail.py +++ b/spp_registry/controllers/mail.py @@ -1,6 +1,3 @@ -import logging - -from markupsafe import Markup from werkzeug.exceptions import NotFound from odoo import http @@ -10,11 +7,8 @@ from odoo.tools.translate import _ from odoo.addons.mail.controllers.attachment import AttachmentController -from odoo.addons.mail.controllers.thread import ThreadController from odoo.addons.mail.tools.discuss import add_guest_to_context -logger = logging.getLogger(__name__) - class SPPAttachmentController(AttachmentController): @http.route("/mail/attachment/delete", methods=["POST"], type="jsonrpc", auth="public") @@ -54,27 +48,3 @@ def mail_attachment_delete(self, attachment_id, access_token=None): if attachment_sudo.res_model != "mail.compose.message" or attachment_sudo.res_id != 0: raise NotFound() attachment_sudo._delete_and_notify(message) - - -class SPPThreadController(ThreadController): - @http.route("/mail/message/update_content", methods=["POST"], type="jsonrpc", auth="public") - @add_guest_to_context - def mail_message_update_content(self, message_id, body, attachment_ids, attachment_tokens=None, partner_ids=None): - guest = request.env["mail.guest"]._get_guest_from_context() - guest.env["ir.attachment"].browse(attachment_ids)._check_attachments_access(attachment_tokens) - # nosemgrep: odoo-sudo-without-context - message_sudo = guest.env["mail.message"].browse(message_id).sudo().exists() - - # Check if current user is admin or the creator (user or guest) - is_admin = request.env.user.has_group("base.group_system") - is_author = message_sudo.is_current_user_or_guest_author - if not (is_admin or is_author): - raise AccessError(_("You do not have permission to update this message.")) - - if not message_sudo.model or not message_sudo.res_id: - raise NotFound() - body = Markup(body) if body else body # may contain HTML such as @mentions - guest.env[message_sudo.model].browse([message_sudo.res_id])._message_update_content( - message_sudo, body, attachment_ids=attachment_ids, partner_ids=partner_ids - ) - return message_sudo.message_format()[0] diff --git a/spp_registry/i18n/es.po b/spp_registry/i18n/es.po index 04300400f..da5dc88d1 100644 --- a/spp_registry/i18n/es.po +++ b/spp_registry/i18n/es.po @@ -1530,12 +1530,6 @@ msgstr "No puede seleccionar una fecha de nacimiento posterior a hoy" msgid "You do not have permission to delete this attachment." msgstr "No tiene permiso para eliminar este adjunto." -#. module: spp_registry -#. odoo-python -#: code:addons/spp_registry/controllers/mail.py:0 -msgid "You do not have permission to update this message." -msgstr "No tiene permiso para actualizar este mensaje." - #. module: spp_registry #. odoo-python #: code:addons/spp_registry/models/registrant.py:0 diff --git a/spp_registry/i18n/fr.po b/spp_registry/i18n/fr.po index 098f1760a..5de848642 100644 --- a/spp_registry/i18n/fr.po +++ b/spp_registry/i18n/fr.po @@ -1534,12 +1534,6 @@ msgstr "" msgid "You do not have permission to delete this attachment." msgstr "Vous n’avez pas l’autorisation de supprimer cette pièce jointe." -#. module: spp_registry -#. odoo-python -#: code:addons/spp_registry/controllers/mail.py:0 -msgid "You do not have permission to update this message." -msgstr "Vous n’avez pas l’autorisation de mettre à jour ce message." - #. module: spp_registry #. odoo-python #: code:addons/spp_registry/models/registrant.py:0 diff --git a/spp_registry/i18n/spp_registry.pot b/spp_registry/i18n/spp_registry.pot index 2c70d8880..9401f3252 100644 --- a/spp_registry/i18n/spp_registry.pot +++ b/spp_registry/i18n/spp_registry.pot @@ -1495,12 +1495,6 @@ msgstr "" msgid "You do not have permission to delete this attachment." msgstr "" -#. module: spp_registry -#. odoo-python -#: code:addons/spp_registry/controllers/mail.py:0 -msgid "You do not have permission to update this message." -msgstr "" - #. module: spp_registry #. odoo-python #: code:addons/spp_registry/models/registrant.py:0 diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index d2f80ecda..385ae86c7 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.2.5 + +- fix(registry): remove the `/mail/message/update_content` controller override. It was ported from Odoo 17, where it was already redundant: stock 17 granted author-or-administrator editing too, and the override only narrowed "administrator" to `base.group_system` and answered with an access error instead of a 404. On Odoo 19 it no longer worked at all, breaking **Edit** and **Delete** on every chatter message across the instance: its signature still expected the Odoo 17 request shape while the Odoo 19 web client sends `update_data`, so every call failed before running, and behind that it also called the removed `ir.attachment._check_attachments_access` and returned through the removed `message_format()`. Odoo 19's own route already grants author-or-administrator editing, so the override is dropped and the route is stock again. Who may edit another user's chatter message therefore follows Odoo's own administrator test (superuser or **Administration: Access Rights**, which **Administration: Settings** implies) instead of Settings only, and the editor must also be allowed to post on the record; no OpenSPP role sits in that gap (#419) + ### 19.0.2.2.4 - fix(registry): refuse a date of birth in the future on every write path. `_birthdate_onchange` only runs in the form UI, so ORM `create`/`write`, CSV/Excel import and API writes (XML-RPC, API v2, DCI) all persisted a future `birthdate` — which the non-stored `age` compute then rendered as a negative number in views, exports and API reads. A stored-field constraint now enforces it server-side, comparing against the user's own today so a registrar east of UTC is not refused a birth recorded earlier that local day, and naming the record and the offending value so a bad row in a bulk import can be found. The onchange is kept as the friendlier silent-reset UX in the form (#362) diff --git a/spp_registry/static/description/index.html b/spp_registry/static/description/index.html index 48cfcf7c2..3f2fdc563 100644 --- a/spp_registry/static/description/index.html +++ b/spp_registry/static/description/index.html @@ -518,6 +518,28 @@

Changelog

+

19.0.2.2.5

+ +
+

19.0.2.2.4

-
+

19.0.2.2.3

-
+

19.0.2.2.2

  • fix(registry): let an ID type be used again after its ID was removed. @@ -574,7 +596,7 @@

    19.0.2.2.2

    surfacing a database error (#1136)
-
+

19.0.2.2.1

  • feat(registry): registry configuration is consolidated into one @@ -585,7 +607,7 @@

    19.0.2.2.1

    framework refuses a settings save from anyone else (#1009)
-
+

19.0.2.1.4

  • fix(registry): remove the dead @api.constrains("age") @@ -597,7 +619,7 @@

    19.0.2.1.4

    dropped
-
+

19.0.2.1.3

  • fix(registry): show an ID Status column on the group form @@ -608,7 +630,7 @@

    19.0.2.1.3

    (#1110)
-
+

19.0.2.1.1

  • fix(views): add reusable x2many_no_padding JS widget that @@ -618,7 +640,7 @@

    19.0.2.1.1

    don’t bloat the layout (#943).
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_registry/tests/test_mail_controllers.py b/spp_registry/tests/test_mail_controllers.py index ae92d3ca7..36b98acf5 100644 --- a/spp_registry/tests/test_mail_controllers.py +++ b/spp_registry/tests/test_mail_controllers.py @@ -1,22 +1,30 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. -"""Author/admin authorisation on the overridden mail endpoints. +"""Author/admin authorisation on the mail endpoints spp_registry relies on. -Covers spp_registry/controllers/mail.py: -- ``POST /mail/attachment/delete`` (SPPAttachmentController.mail_attachment_delete) -- ``POST /mail/message/update_content`` (SPPThreadController.mail_message_update_content) +- ``POST /mail/attachment/delete`` — overridden in + spp_registry/controllers/mail.py (SPPAttachmentController.mail_attachment_delete). + Its only application-level guard is:: -Both are ``auth="public"`` JSON-RPC endpoints whose only application-level -guard is:: + is_admin = request.env.user.has_group("base.group_system") + is_author = message.is_current_user_or_guest_author + if not (is_admin or is_author): + raise AccessError(...) - is_admin = request.env.user.has_group("base.group_system") - is_author = message.is_current_user_or_guest_author - if not (is_admin or is_author): - raise AccessError(...) +- ``POST /mail/message/update_content`` — stock Odoo 19 + (``mail.controllers.thread.ThreadController``). spp_registry used to carry a + redundant Odoo 17 port of this route that stopped working on 19; it was + removed in 19.0.2.2.5 (#419). Stock ``_can_edit_message`` is author OR + ``res.users._is_admin()``, after ``_get_message_with_access(mode="create")`` + has checked that the caller may post on the thread. The tests pin the + behaviour spp_registry depends on. -These tests assert: author allowed, admin allowed, third party denied, -unauthenticated denied. They run as ``HttpCase`` so the controller stack -(routing, ``@add_guest_to_context``, JSON-RPC envelope) is exercised end -to end — not just the controller method directly. +For ``/mail/message/update_content`` these tests assert: author allowed, +admin allowed, third party denied, unauthenticated denied — for both the +Edit and the Delete payload. For ``/mail/attachment/delete`` the author case +is still an unimplemented placeholder (see its skip). They run as +``HttpCase`` so the controller stack (routing, ``@add_guest_to_context``, +JSON-RPC envelope) is exercised end to end — not just the controller method +directly. """ import json @@ -141,18 +149,36 @@ def test_missing_attachment_returns_without_error(self): @tagged("post_install", "-at_install") class TestMailMessageUpdateContentController(HttpCase): - """``/mail/message/update_content`` — author/admin gate.""" + """``/mail/message/update_content`` — author/admin gate (stock Odoo 19 route). + + The payloads mirror what ``mail/static/src/core/common/message_model.js`` + sends: Edit is ``{"message_id": ..., "update_data": {"body": ..., "attachment_ids": []}}`` + and Delete is the same route with ``removeParams`` (empty body, no + attachments). A denied call is a ``werkzeug.exceptions.NotFound`` (JSON-RPC + error code 404); the assertions check that name so a controller crash + (``TypeError``, ``AttributeError``) can never pass as a denial. + """ + + # What the Odoo 19 web client sends for "Delete" (``Message.removeParams``). + REMOVE_PARAMS = {"attachment_ids": [], "attachment_tokens": [], "body": "", "subject": "", "partner_ids": []} @classmethod def setUpClass(cls): super().setUpClass() + # Stock Odoo 19 also requires the editor to be allowed to *post* on the + # thread (``_mail_post_access`` = write on the document). Both users are + # Registry Officers — the module's own persona with write on + # ``res.partner`` (``security/ir.model.access.csv``) — so the author can + # edit, and so the bystander is refused by the author gate alone rather + # than by lacking thread access (pinned in the bystander tests). + groups = [(6, 0, [cls.env.ref("base.group_user").id, cls.env.ref("spp_registry.group_registry_officer").id])] cls.author = cls.env["res.users"].create( { "name": "Msg Author", "login": "spp_registry_msg_author", "email": "msg_author@example.test", "password": "author_pw", - "group_ids": [(6, 0, [cls.env.ref("base.group_user").id])], + "group_ids": groups, } ) cls.bystander = cls.env["res.users"].create( @@ -161,7 +187,7 @@ def setUpClass(cls): "login": "spp_registry_msg_bystander", "email": "msg_bystander@example.test", "password": "bystander_pw", - "group_ids": [(6, 0, [cls.env.ref("base.group_user").id])], + "group_ids": groups, } ) @@ -178,61 +204,100 @@ def _post_message(self, owner): } ) - def _call_update(self, message_id, body="

    updated

    "): + def _call_update(self, message_id, body="

    updated

    ", update_data=None): + if update_data is None: + update_data = {"body": body, "attachment_ids": []} return self.url_open( "/mail/message/update_content", - data=json.dumps( - { - "params": { - "message_id": message_id, - "body": body, - "attachment_ids": [], - } - } - ), + data=json.dumps({"params": {"message_id": message_id, "update_data": update_data}}), headers={"Content-Type": "application/json"}, ) + def _assert_bystander_may_post(self, msg): + """Both stock gates raise the same bare ``NotFound``. Pinning that the + bystander clears the thread-post gate means a 404 in the bystander + tests can only have come from the author gate.""" + thread = self.env["res.partner"].browse(msg.res_id) + self.assertTrue( + thread.with_user(self.bystander).has_access("write"), + "bystander must be able to post on the thread, so the 404 proves the author gate fired", + ) + + def _assert_updated(self, resp, msg, body_fragment): + self.assertEqual(resp.status_code, 200) + payload = resp.json() + self.assertNotIn("error", payload, f"expected a successful edit, got {payload!r}") + msg.invalidate_recordset(["body"]) + self.assertIn(body_fragment, msg.body) + self.assertNotIn("original", msg.body) + + def _assert_denied_not_found(self, resp, msg, original_body): + payload = resp.json() + self.assertIn("error", payload, f"expected error envelope, got {payload!r}") + error = payload["error"] + self.assertEqual(error.get("code"), 404, f"expected a 404 denial, got {error!r}") + self.assertEqual(error["data"]["name"], "werkzeug.exceptions.NotFound") + msg.invalidate_recordset(["body"]) + self.assertEqual(msg.body, original_body) + def test_author_can_update_own_message(self): - """FINDING: controller is BROKEN on Odoo 19. - - ``spp_registry/controllers/mail.py::mail_message_update_content`` - calls ``ir.attachment._check_attachments_access(attachment_tokens)``, - which no longer exists on ``ir.attachment`` in Odoo 19. The - method was renamed/removed upstream. Every call to the endpoint - fails with ``AttributeError`` — happens to surface as - ``error`` in the JSON-RPC envelope, so the bystander/unauth - tests below pass for the WRONG reason. - - TODO (fix the impl, not the test): port the controller to use - whatever upstream attachment-access check replaced - ``_check_attachments_access`` in Odoo 19. Once the controller - runs, drop this skip and the second-finding skip below. - """ - self.skipTest("BROKEN: controller calls removed Odoo 18 API — see docstring") + msg = self._post_message(self.author) + self.authenticate("spp_registry_msg_author", "author_pw") + resp = self._call_update(msg.id, body="

    updated by author

    ") + self._assert_updated(resp, msg, "updated by author") def test_admin_can_update_any_message(self): - """Same Odoo 19 incompatibility as above — skip until controller - is fixed.""" - self.skipTest("BROKEN: controller calls removed Odoo 18 API — see test_author_can_update_own_message") + msg = self._post_message(self.author) + self.authenticate("admin", "admin") + resp = self._call_update(msg.id, body="

    updated by admin

    ") + self._assert_updated(resp, msg, "updated by admin") def test_bystander_cannot_update_anothers_message(self): msg = self._post_message(self.author) original_body = msg.body + self._assert_bystander_may_post(msg) self.authenticate("spp_registry_msg_bystander", "bystander_pw") resp = self._call_update(msg.id, body="

    hostile edit

    ") - self.assertIn("error", resp.json()) - msg.invalidate_recordset(["body"]) - self.assertEqual(msg.body, original_body) + self._assert_denied_not_found(resp, msg, original_body) def test_unauthenticated_request_is_denied(self): msg = self._post_message(self.author) + original_body = msg.body + # No authenticate() call — HttpCase starts as the public user. The + # public user is read-only on res.partner, so the 404 comes from the + # thread-post-access gate before the author gate is even consulted. resp = self._call_update(msg.id, body="

    anon edit

    ") - self.assertIn("error", resp.json()) + self._assert_denied_not_found(resp, msg, original_body) - def test_message_without_model_returns_not_found(self): - """If the message has no ``model`` / ``res_id`` the controller raises - ``werkzeug.exceptions.NotFound`` (404 over HTTP).""" - # TODO: create a mail.message with empty model/res_id (requires - # sudo + careful create vals) and assert a 404 / NotFound surface. - self.skipTest("not yet implemented — see TODO") + def test_author_can_delete_own_message(self): + msg = self._post_message(self.author) + self.authenticate("spp_registry_msg_author", "author_pw") + resp = self._call_update(msg.id, update_data=self.REMOVE_PARAMS) + self.assertEqual(resp.status_code, 200) + payload = resp.json() + self.assertNotIn("error", payload, f"expected a successful delete, got {payload!r}") + msg.invalidate_recordset(["body"]) + self.assertNotIn("original", msg.body) + + def test_admin_can_delete_any_message(self): + msg = self._post_message(self.author) + self.authenticate("admin", "admin") + resp = self._call_update(msg.id, update_data=self.REMOVE_PARAMS) + payload = resp.json() + self.assertNotIn("error", payload, f"expected a successful delete, got {payload!r}") + msg.invalidate_recordset(["body"]) + self.assertNotIn("original", msg.body) + + def test_bystander_cannot_delete_anothers_message(self): + msg = self._post_message(self.author) + original_body = msg.body + self._assert_bystander_may_post(msg) + self.authenticate("spp_registry_msg_bystander", "bystander_pw") + resp = self._call_update(msg.id, update_data=self.REMOVE_PARAMS) + self._assert_denied_not_found(resp, msg, original_body) + + def test_unauthenticated_delete_is_denied(self): + msg = self._post_message(self.author) + original_body = msg.body + resp = self._call_update(msg.id, update_data=self.REMOVE_PARAMS) + self._assert_denied_not_found(resp, msg, original_body)