refactor: casl factory - datasets#2760
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In several places you now rely on
ability.can(Action.DatasetRead, DatasetClass)(or other dataset actions) with the class as subject, but the newdatasetAccessrules are all condition-based; CASL ignores conditions when checking against a type, so these checks effectively become unconditional and may grant broader access than intended—consider switching those checks to use an instance or reintroducing explicit owner/access/published predicates where you need them. - The new unauthenticated behaviour in
DatasetsAccessService.addDatasetAccess/addDatasetAccessToPipelineappears to no longer enforceisPublished: truefor lookups (becausecanViewnow becomes true due to the genericDatasetReadrule on the class), which could expose non‑public datasets via lookups; it would be safer to explicitly add theisPublishedfilter for unauthenticated users instead of relying on the generic ability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In several places you now rely on `ability.can(Action.DatasetRead, DatasetClass)` (or other dataset actions) with the class as subject, but the new `datasetAccess` rules are all condition-based; CASL ignores conditions when checking against a type, so these checks effectively become unconditional and may grant broader access than intended—consider switching those checks to use an instance or reintroducing explicit owner/access/published predicates where you need them.
- The new unauthenticated behaviour in `DatasetsAccessService.addDatasetAccess` / `addDatasetAccessToPipeline` appears to no longer enforce `isPublished: true` for lookups (because `canView` now becomes true due to the generic `DatasetRead` rule on the class), which could expose non‑public datasets via lookups; it would be safer to explicitly add the `isPublished` filter for unauthenticated users instead of relying on the generic ability.
## Individual Comments
### Comment 1
<location path="src/datasets/datasets-access.service.ts" line_range="121-126" />
<code_context>
- Action.DatasetReadManyAccess,
- DatasetClass,
- );
+ const ability = this.caslAbilityFactory.datasetAccess(currentUser);
+ const canViewAny = ability.can(Action.AccessAny, DatasetClass);
+ const canView = ability.can(Action.DatasetRead, DatasetClass);
if (!canViewAny) {
- if (canViewAccess) {
+ if (canView) {
fieldValue.$lookup.pipeline?.unshift({
$match: {
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential runtime error in `addDatasetAccess` when `currentUser` is undefined.
In `addDatasetAccess`, `currentUser` is still dereferenced when it may be `undefined`:
```ts
const currentUser = this.request.user as JWTUser;
const ability = this.caslAbilityFactory.datasetAccess(currentUser);
const canViewAny = ability.can(Action.AccessAny, DatasetClass);
const canView = ability.can(Action.DatasetRead, DatasetClass);
if (!canViewAny) {
if (canView) {
fieldValue.$lookup.pipeline?.unshift({
$match: {
$or: [
{ ownerGroup: { $in: currentUser.currentGroups } },
{ accessGroups: { $in: currentUser.currentGroups } },
{ sharedWith: { $in: [currentUser.email] } },
{ isPublished: true },
],
},
});
}
}
```
With the new `datasetAccess` behavior, unauthenticated users can have `canView === true` for `DatasetRead` on `DatasetClass`, so this branch will run with `currentUser === undefined`, causing a runtime error. Previously, the branch depended on `canViewAccess`, which was `false` for unauthenticated users.
Please gate this branch on the presence of a user (e.g. `if (currentUser && canView)`) or handle the unauthenticated case separately, similar to `getAccessDetailsForLookup`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
HayenNico
force-pushed
the
refactor-casl-factory-datasets
branch
from
6fa3dd6 to
cc8fc38
Compare
|
Overall review: Code Review: refactor-casl-factory-datasets BranchAssessmentThe branch refactors CASL authorization for datasets by:
Improvement needed
VerdictNecessary with caveats. The refactoring improves maintainability by reducing duplication and separating concerns. However, it trades fine-grained permissions for simplicity. Verify that:
If these are confirmed, the changes are a net improvement. If not, revert or extend the action model to preserve necessary granularity. Generated with Mistral AI |
Code Review: refactor-casl-factory-datasets Branch
1. Logic Correctness: VERIFIED ✅The refactoring maintains equivalent permission logic through a coarse-grained action model:
CASL behavior: Multiple can(Action.DatasetRead, DatasetClass, ifOwner);
can(Action.DatasetRead, DatasetClass, ifAccess);
can(Action.DatasetRead, DatasetClass, ifPublished);This is functionally equivalent to the old model's separate actions. 2. Edge Cases: ALL HANDLED ✅
3. What the Changed Code DoesExtracted Components:
Simplified Action Model:
Controller Changes:Replaced verbose permission checks: // OLD
ability.can(Action.DatasetReadAny, DatasetClass) ||
ability.can(Action.DatasetReadOneOwner, datasetInstance) ||
ability.can(Action.DatasetReadOneAccess, datasetInstance) ||
ability.can(Action.DatasetReadOnePublic, datasetInstance)
// NEW
ability.can(Action.DatasetRead, datasetInstance)
Access Service Changes:
4. Do the Changes Make Sense?YES, with architectural benefits:
Trade-offs:
Verdict: The trade-off is worthwhile. The old model's granularity was not being fully utilized (controllers already combined multiple checks with OR logic), and the new model achieves the same runtime behavior with significantly cleaner code. 5. Unreachable Code: NONE FOUND ✅Analyzed files:
Potential issue identified:
Final Assessment
Recommendation: MERGE — The refactoring is well-executed, maintains correctness, and significantly improves code maintainability. Generated with Mistral AI |
nitrosx
left a comment
nitrosx
left a comment
There was a problem hiding this comment.
Great work.
Small code refactoring in dataset abilities regarding the order of the permissions.
I do not understand the need of the need for AccessAny. Could you please elaborate on it?
I do apologize for all the comments about AccessAny on all the files.
There was a problem hiding this comment.
Even if the logic is correct and authorization is correct, I would suggest to order the if statements in the opposite order and only the branch that applies should be executed
- admin
- createDatasetPrivileged
- createDatasetWithPid
- createDataset
Delete and updateDatasetLifecycle should not be modified
There was a problem hiding this comment.
I decided to sort by ascending privileges since the early return for unauthenticated users needs to be first. In general, I believe leaving the current format and accumulating rights is better than making things mutually exclusive. Exclusive if-else branches can lead to problems if the different special groups can not be ordered in one transitive sequence.
Example from jobs: In the current casl, CREATE_JOBS_PRIVILEGED_GROUPS and UPDATE_JOBS_PRIVILEGED_GROUPS are mutually exclusive - which leads to the unintended side-effect that if a group is added to both in configuration, it silently loses the update permission
There was a problem hiding this comment.
The order of the different special groups can of course be changed if there's a strong preference either way
| const canViewAny = ability.can(Action.AccessAny, DatasetClass); | ||
| const canView = ability.can(Action.DatasetRead, DatasetClass); |
There was a problem hiding this comment.
I am not sure I understand 2why we introduced AccessAny
There was a problem hiding this comment.
In basically all controllers, when access-based filters are added to search queries there is always three cases:
- Unauthenticated users: Only public resources
- Authenticated users: Access-based filters (ownerGroup/accessGroups/isPublished)
- Admin access: No additional access filters are applied for admins, this requires a special permission action that is only for admins. I used AccessAny for that. In the dataset case, this is what Action.DatasetReadAny used to do.
So canViewAny is only true for admins (or groups with admin-level access, I think under the current model CreateDatasetPrivilegedGroups also get AccessAny). The canView check is always true with current permissions, but in general is needed since this is a cross-resource endpoint and during endpoint auth it was only checked that the user may read samples, but not datasets
| const canViewAny = ability.can(Action.AccessAny, DatasetClass); | ||
| const canView = ability.can(Action.DatasetRead, DatasetClass); |
There was a problem hiding this comment.
I'm not sure I understand AccessAny
| const canViewAny = ability.can(Action.AccessAny, DatasetClass); | ||
| const canView = ability.can(Action.DatasetRead, DatasetClass); |
There was a problem hiding this comment.
Not sure I understand the reason behind AccessAny
| const canViewAny = ability.can(Action.AccessAny, DatasetClass); | ||
| const canView = ability.can(Action.DatasetRead, DatasetClass); |
There was a problem hiding this comment.
what is the difference between AccessAny and DatasetRead?
There was a problem hiding this comment.
What is the purpose of AccessAny?
Security Review: refactor-casl-factory-datasets Branch
1. Injection Vulnerabilities: ❌ NONEAnalysis:
Example of safe usage: // datasets.ability.ts
const ifOwner = { ownerGroup: { $in: user?.currentGroups } };
can(Action.DatasetRead, DatasetClass, ifOwner);→ 2. Sensitive User Data Exposure: ❌ NONEAnalysis:
Verdict: The refactoring does not expose any additional user data. 3. Insecure API Usage: ❌ NONEAnalysis:
Verdict: No insecure API usage patterns introduced. 4. Authentication Bypass: ❌ NONEDeep Analysis:
Critical Check: CASL Semantics
V4 Controller Note: if (!user) {
// In API v4 unauthorized users must use the public endpoints
throw new ForbiddenException("Unauthorized access");
}This is intentional (per comment) and represents a security improvement, not a bypass. Final Verdict: ALL SECURE ✅
Conclusion: The refactoring maintains the same security posture while improving code clarity. No security vulnerabilities introduced. Generated with Mistral Ai |
2 participants
Description
Subsection of PR #2748 for datasets. PR depends on #2759 to be merged first.
This unifies the datasetEndpointAccess and datasetInstanceAccess functions in CaslAbilityFactory, removes instance-level Action elements and adjusts all affected controllers to accommodate the change. The dataset-specific code is extracted into a separate module
Fixes
Changes:
Tests included
Documentation
official documentation info
Summary by Sourcery
Consolidate dataset authorization into a single CASL ability and align controllers and access services with the simplified permission model.
Bug Fixes:
Enhancements:
Tests: