refactor: casl factory - proposals#2779
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ProposalsControllerthe/proposals/:pid/datasetsendpoint now usesproposalAccess(user)but checks permissions onDatasetClass(e.g.ability.can(Action.AccessAny, DatasetClass)andAction.DatasetRead), whichProposalAbilitydoes not define; this will effectively always deny or mis-evaluate dataset permissions and should be switched back to a dataset-specific ability (e.g.datasetInstanceAccess) or extended with dataset rules. - The changes in
DatasetsAccessService.getRelationViewAccessandaddRelationViewRestrictionsmean unauthenticated users getcanView*mapped fromProposalReadbut the restriction logic now short-circuits whencurrentUseris falsy, resulting in no$matchpipeline and potentially exposing non-public proposals via dataset lookups; consider restoring a public-only match (e.g.isPublished: true) for unauthenticated users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ProposalsController` the `/proposals/:pid/datasets` endpoint now uses `proposalAccess(user)` but checks permissions on `DatasetClass` (e.g. `ability.can(Action.AccessAny, DatasetClass)` and `Action.DatasetRead`), which `ProposalAbility` does not define; this will effectively always deny or mis-evaluate dataset permissions and should be switched back to a dataset-specific ability (e.g. `datasetInstanceAccess`) or extended with dataset rules.
- The changes in `DatasetsAccessService.getRelationViewAccess` and `addRelationViewRestrictions` mean unauthenticated users get `canView*` mapped from `ProposalRead` but the restriction logic now short-circuits when `currentUser` is falsy, resulting in no `$match` pipeline and potentially exposing non-public proposals via dataset lookups; consider restoring a public-only match (e.g. `isPublished: true`) for unauthenticated users.
## Individual Comments
### Comment 1
<location path="src/datasets/datasets-access.service.ts" line_range="147-150" />
<code_context>
if (!canViewAny) {
let pipeline: PipelineStage.Lookup["$lookup"]["pipeline"];
- if (canViewAccess) {
+ if (currentUser && canViewAccess) {
pipeline = [
{
</code_context>
<issue_to_address>
**🚨 issue (security):** Guarding the lookup pipeline on `currentUser` can leave unauthenticated users with an unrestricted proposal lookup.
Because the `if` branches now require `currentUser`, when `currentUser` is falsy but `canViewAccess`/`canViewOwner` is true, `pipeline` stays `undefined` while `!canViewAny`. With `getRelationViewAccess` deriving these flags from `ProposalRead` (including the `isPublished` rule), unauthenticated users can end up with `canViewAccess === true` and `currentUser === undefined`, so the `$lookup` runs without any `isPublished` or group constraints.
That can either prevent unauthenticated users from seeing public proposals or, depending on the lookup shape, expose non‑public proposals. We should either:
- Keep and use a `canViewPublic` flag to always enforce `{ isPublished: true }` for unauthenticated users, or
- Add an explicit unauthenticated branch that builds a public‑only pipeline instead of gating on `currentUser`.
Given this directly affects data exposure, the unauthenticated case needs a more explicit, constrained pipeline.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This is true, but mirrors the current logic in |
Description
Subsection of PR #2748 for proposals. PR depends on #2759 to be merged first.
This unifies the proposalsEndpointAccess and proposalsInstanceAccess functions in CaslAbilityFactory, removes instance-level Action elements and adjusts the affected controllers to accommodate the change. The proposal-specific code is extracted into a separate module.
Changes:
Tests included
Documentation
Summary by Sourcery
Unify and centralize proposal authorization logic in a new CASL ability, replacing legacy proposal-specific actions and access methods across controllers and services.
Enhancements:
Tests: