Skip to content

Prevent users from manipulating comments (LEAN-5385) - #373

Open
asouqi wants to merge 15 commits into
masterfrom
LEAN-5385
Open

asouqi wants to merge 15 commits into
masterfrom
LEAN-5385

Conversation

@asouqi

@asouqi asouqi commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings March 9, 2026 05:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces capability-based authorization for applying ProseMirror steps, aiming to prevent unauthorized users from creating/resolving/deleting (and generally manipulating) comment nodes when submitting step updates.

Changes:

  • Added a StepAccessService that validates incoming ProseMirror steps against node-level access policies.
  • Introduced a NodeAccessRegistry with a CommentAccessPolicy, plus new AccessContext/Capabilities models and DocumentService.getCapabilities() to derive capabilities from project roles.
  • Wired step validation into AuthorityService.receiveSteps() (throwing StepAccessError on violation) and updated the V2 document controller to pass the computed access context; added unit tests.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/suites/unit/DomainLayer/V2/StepAccessService.spec.ts Adds unit tests for step access validation around comments.
src/Utilities/NodeAccess/NodeAccessRegistry.ts Registry for mapping node types to access policies (registers comment policy).
src/Utilities/NodeAccess/DefaultNodeAccessPolicy.ts Default “allow all” access policy for node operations.
src/Utilities/NodeAccess/CommentAccessPolicy.ts Capability-driven policy for comment insertion/deletion and selected attribute edits.
src/Models/NodeAccessModels.ts Defines the NodeAccessPolicy interface.
src/Models/CapabilitiesModels.ts Defines the Capabilities shape used for access decisions.
src/Models/AccessContextModels.ts Defines the AccessContext (user + capabilities).
src/InternalErrorCodes.ts Adds StepAccessError internal error code.
src/Errors.ts Adds StepAccessError (403) including the rejected step JSON.
src/DomainServices/StepAccessService.ts Implements step validation for Replace/ReplaceAround/Attr steps.
src/DomainServices/DocumentService.ts Adds getCapabilities() derived from project role.
src/DomainServices/AuthorityService.ts Validates each incoming step before applying; signature updated to accept AccessContext.
src/DIContainer/DIContainer.ts Registers stepAccessService and node access registry in the container.
src/Controller/V2/Document/DocumentController.ts Computes capabilities and passes access context into receiveSteps.
pnpm-lock.yaml Locks prosemirror-model version alignment.
package.json Adds prosemirror-model dependency.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread src/DomainServices/StepAccessService.ts Outdated
Comment thread test/suites/unit/DomainLayer/V2/StepAccessService.spec.ts Outdated
Comment thread src/Errors.ts
Comment thread src/Utilities/NodeAccess/CommentAccessPolicy.ts Outdated
Comment thread src/DomainServices/StepAccessService.ts
Comment thread src/DomainServices/DocumentService.ts Outdated
Comment thread src/Utilities/NodeAccess/CommentAccessPolicy.ts Outdated
Comment thread src/Utilities/NodeAccess/CommentAccessPolicy.ts Outdated
@mbartenev-atypon

Copy link
Copy Markdown
Contributor

It looks like a good solution but my main question is with duplicity of capabilities. The other question is how deep we actually want to go with granularity control because if we will start check separate attributes any schema change will need a corresponding update here. This is not a problem but surely a time consuming commitment that will increase this project maintainability requirements. It would be great if we can keep it high level.

@asouqi

asouqi commented Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

The other question is how deep we actually want to go with granularity control because if we will start check separate attributes any schema change will need a corresponding update here. This is not a problem but surely a time consuming commitment that will increase this project maintainability requirements. It would be great if we can keep it high level.

ohh yeah that's a good catch. I think we have more node types to cover like lock on the body and there is a new ticket about lock at ORCID. that should be declared in transform at node spec

@asouqi
asouqi requested a review from mbartenev-atypon April 13, 2026 01:37
asouqi added 5 commits April 23, 2026 21:28
Comment thread src/DomainServices/StepAccessService.ts
Comment thread src/Errors.ts
Comment thread src/DomainServices/StepAccessService.ts Outdated
Comment thread src/DomainServices/StepAccessService.ts Outdated
@asouqi
asouqi requested a review from mbartenev-atypon June 22, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants