[16.0][ADD] project_update_portal: add new module#1599
Conversation
marcelsavegnago
force-pushed
the
16.0-add-project_update_portal
branch
4 times, most recently
from
c0502ea to
717f064
Compare
marcelsavegnago
force-pushed
the
16.0-add-project_update_portal
branch
2 times, most recently
from
bd53c11 to
575873d
Compare
marcelsavegnago
changed the title
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the contribution -- exposing project updates in the portal is a genuinely useful feature. The module structure, tests, and documentation are solid overall. However, there are a few security and correctness issues to address.
Security: auth="public" on detail route is too permissive. The detail route uses auth="public" while the list route uses auth="user". Unauthenticated visitors can probe the detail endpoint, and redirect behavior leaks whether a project ID exists. Both routes should be consistent.
Security: single access_token validates two different records. The same token is used to check access to project.project and project.update. Portal tokens are record-specific -- a token for one won't pass _document_check_access for the other. For portal followers this is bypassed via ir.rule, but for token-based flows both checks will fail.
Correctness: unnecessary @api.depends. _compute_access_url depends on message_partner_ids but only uses project_id and id. Adding/removing followers would trigger needless recomputation.
Accessibility: aria-valuenow in list template. The progress bar has literal string "update.progress" instead of the actual value via t-attf-aria-valuenow.
Happy to re-review once updated.
| ["/my/projects/<int:project_id>/update/<int:update_id>"], | ||
| type="http", | ||
| auth="public", | ||
| website=True, |
There was a problem hiding this comment.
This route uses auth="public" while the list route at line 57 uses auth="user". This inconsistency means unauthenticated users can probe this endpoint. The redirect behavior (/my vs /my/projects/<id>) leaks whether a given project_id exists. Change to auth="user" for consistency.
| """Route to view a specific project update""" | ||
| try: | ||
| # Check project access first | ||
| self._document_check_access("project.project", project_id, access_token) |
There was a problem hiding this comment.
The same access_token is used to validate both the project and the update. Portal access tokens are record-specific -- a token for project.update won't pass _document_check_access for project.project and vice versa.
Consider either removing the project access check here (the update->project_id integrity check is sufficient after validating the update), or checking project access without the token.
| class ProjectUpdate(models.Model): | ||
| _name = "project.update" | ||
| _inherit = ["project.update", "portal.mixin"] | ||
|
|
There was a problem hiding this comment.
message_partner_ids in @api.depends but never used in the method body. Remove to avoid needless recomputation on follower changes:
| @api.depends("project_id") |
| role="progressbar" | ||
| aria-valuenow="update.progress" | ||
| aria-valuemin="0" | ||
| aria-valuemax="100" |
There was a problem hiding this comment.
aria-valuenow is a literal string "update.progress" instead of the actual value. Should be t-attf-aria-valuenow="{{ update.progress }}" to match the detail view template.
CristianoMafraJunior
force-pushed
the
16.0-add-project_update_portal
branch
from
575873d to
f243459
Compare
|
@alexey-pelykh The corrections from your review have been made. Could you please check again? |
|
This PR has the |
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
All four items from the previous review have been addressed:
- Detail route now uses
auth="user", consistent with the list route. - Project access check no longer passes the
access_token-- token is only used for the update record, project access relies on ir.rule. Correct separation. @api.dependsreduced to("project_id")only.aria-valuenowusest-attf-aria-valuenowwith the actual value in both list and detail templates.
Looks good, thank you for the quick turnaround.
|
/ocabot merge nobump |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at aef77c9. Thanks a lot for contributing to OCA. ❤️ |
6 participants
This module extends project updates functionality by allowing portal access for project and update followers.
Main Features
Benefits
Dependencies
This module requires:
project: Odoo's project management moduleportal: Odoo's portal module for customer accesscc @parzewski @kaynnan