Define user @mention criterion for work package comments with restricted visibility #18409
Conversation
akabiru
changed the title
akabiru
changed the title
MentionableOnWorkPackageFilter does not depend solely on permissions, rather on project membership whilst RestrictedMentionableOnWorkPackageFilter depends solely on a permission- hence we cannot easily reuse the `User.allowed_members_on_work_package(action, project)` method that would indeed be more concise- it's possible this can be revisitted in the near future
The downside is that if the user has both an edit editor open and one adding a comment, the restriction prevails so they would not be able to mention non restricted users. Unless they close out the restricted editor. This is mainly because we are using a singular function to retrieve the mentions- the could be a better way that I didn't yet think about but my reckoning is this is a rare edge case- and I much prefer the secure by default approach
akabiru
force-pushed
the
implementation/60988-define-mention-criterion-for-work-package-comments-with-restricted-visibility
branch
from
b557b50 to
5d34f9b
Compare
| # See COPYRIGHT and LICENSE files for more details. | ||
| # ++ | ||
|
|
||
| class Queries::Principals::Filters::RestrictedMentionableOnWorkPackageFilter < |
There was a problem hiding this comment.
@ulferts I did not manage to easily extend the current MentionableOnWorkPackageFilter:
d00cda7 MentionableOnWorkPackageFilter does not depend solely on permissions, rather on project membership whilst RestrictedMentionableOnWorkPackageFilter depends primarily on a permission- hence we cannot easily reuse the
User.allowed_members_on_work_package(action, project)method that would indeed be more concise- it's possible this can be revisited in the near future
There was a problem hiding this comment.
AFAICT, we will not be able to use the User.allowed_members_on_work_package(action, project) on the restricted filter too after all in it's current shape, as it excludes Groups- which are valid mentions. The resulting query is also much simpler. This perhaps explains why the MentionableOnWorkPackageFilter did also not use it.
Once more relying on the User.allowed_members_* helper did not fit the bill as we need to also include Groups in mentionables which are direct decendants of Principal- the resulting query is also much simpler and loosely decoupled- so I favour retaining this approach for the time being. Follows: d00cda7
akabiru
force-pushed
the
implementation/60988-define-mention-criterion-for-work-package-comments-with-restricted-visibility
branch
from
941c26c to
cc75dc1
Compare
app/models/queries/principals/filters/restricted_mentionable_on_work_package_filter.rb
Show resolved
Hide resolved
...c/stimulus/controllers/dynamic/work-packages/activities-tab/restricted-comment.controller.ts
Show resolved
Hide resolved
|
Gentle nudge @Kharonus to have another look :) |
Thanks! Still a useful perspective, @brunopagno / @ulferts can top up the review before merge. There's no huge rush on it albeit going stale. |
There was a problem hiding this comment.
The code changes look good, nothing called my attention as potentially problematic. So all good there.
But when I was testing locally I was able to get an error like this (likely caused by the state of my local environment):
It happened because I got a POST 500 on /work_packages/41/activities/sanitize_restricted_mentions.
I think we need to make sure the errors are not exposed to the client.
Really nice catch! It would seem the try, catch does not work as intended- will add a test case for this 👍🏾 |
| try { | ||
| const response = await fetch(sanitizePath, { | ||
| method: 'POST', | ||
| body: JSON.stringify({ journal: { notes: editorData } }), | ||
| headers: { | ||
| 'X-CSRF-Token': csrfToken, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| const sanitizedNotes = await response.text(); | ||
| this.ckEditorInstance.setData(sanitizedNotes); | ||
| } catch (error) { | ||
| console.error(`Failed to sanitize restricted mentions: ${error}`); |
There was a problem hiding this comment.
@akabiru now that you pointed out this is indeed wrong. The try catch block will capture exceptions, but not responses with errors. I think we need to explicitly handle with a response.ok check here.
There was a problem hiding this comment.
Thanks @brunopagno - handled in 22cc675 via primer banner. I would have wished to display the error closer to the comment box- and perhaps even transition the ckeditor input into an error state, but it doesn't seem like we have this convention atm (with ckeditor input)- perhaps an enhancement to discuss with UX and apply in the future.
The screenshot is from manual testing by raising an StandardError
akabiru
deleted the
implementation/60988-define-mention-criterion-for-work-package-comments-with-restricted-visibility
branch
Ticket
https://community.openproject.org/wp/60988
What are you trying to accomplish?
Screenshots
restricted-mentions.mp4
What approach did you choose and why?
Introduce a sanitization endpoint on the server side that converts users mentions that do not have restricted visibility permissions- the functionality is further applied in create/update of comments.
Merge checklist