Skip to content

refactor: casl-ability-factory refactor/reduction - #2748

Closed
HayenNico wants to merge 21 commits into
masterfrom
refactor-casl-factory
Closed

HayenNico wants to merge 21 commits into
masterfrom
refactor-casl-factory

Conversation

@HayenNico

@HayenNico HayenNico commented May 21, 2026 •

Copy link
Copy Markdown
Member

Description

This PR aims to significantly reduce the amount of code used for casl endpoint and instance authorization. Both endpoint and instance authorizations are now handled by one merged casl ability per collection, with requirements for endpoint authorization being implied from instance access rights. This includes a significant reduction in the number of actions defined for casl.

The auth logic in individual controllers has been adapted to accomodate this reduction - the core logic is preserved, but edge cases outside of test coverage may have different behavior for each collection. Hence, this PR might introduce a breaking change in such cases.

Fixes

  • Unit tests (proposal): Fix CaslAbilityFactoryMock behavior to not let auth pass in all cases
  • API tests (dataset v4): Remove an illegal filter in test 2500-0400 when sending a request to the count endpoint (can only have where-filters)

Changes:

  • Merge each casl instance and endpoint ability into one for each collection
  • Reduce the number of actions per collections to four (create, read, update, delete) + extras in cases where needed to preserve functionality
  • Use existing AccessAny generic action for admin-level access where needed
  • Refactor auth logic in each controller to match reduced action set and use the new casl ability
  • Instance authorization code for each collection changed to common format

Possible extensions

  • Further reduction in actions to one generic Create, Read, Update, Delete term across all collections. Would require to first eliminate combined access actions (e.g. DatasetOrigdatablockRead) via controller refactoring
  • Instance auth code in controllers is heavily duplicated across the different collections and within each controller; should be possible to refactor out into a generic function to check instance access rights.

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

@HayenNico HayenNico self-assigned this May 21, 2026

@sourcery-ai sourcery-ai Bot 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.

Sorry @HayenNico, your pull request is larger than the review limit of 150000 diff characters

@HayenNico HayenNico changed the title BREAKING CHANGE: Casl-ability-factory refactor/reduction refactor: casl-ability-factory refactor/reduction May 22, 2026
@HayenNico
HayenNico marked this pull request as ready for review May 22, 2026 01:16
@HayenNico
HayenNico requested a review from a team as a code owner May 22, 2026 01:16

@sourcery-ai sourcery-ai Bot 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.

Sorry @HayenNico, your pull request is larger than the review limit of 150000 diff characters

@nitrosx

nitrosx commented May 25, 2026

Copy link
Copy Markdown
Member

@HayenNico amazing work. I am in the process of reviewing it.
I wonder if we can split the casl abilities in separated files so it is easy to review them individually.
If you want I can create a PR on this Pr with the changes.

@nitrosx

nitrosx commented May 25, 2026

Copy link
Copy Markdown
Member

...also I wonder if we need to review the authorization tests to update the existing test cases and add new ones

@HayenNico

Copy link
Copy Markdown
Member Author

@nitrosx I can split it into multiple files, not a problem. Let's chat at the meeting today what format/structure would be best.

For tests: My main focus for now was that this passes all existing tests with no changes to the testing suite. But there's a decent chance some edge cases outside coverage behave different now.
Since you were planning to readjust the special group roles and rights anyway, maybe the extension of tests should come with that so we don't have to fix all those new tests once the permission model changes.

@HayenNico

Copy link
Copy Markdown
Member Author

We should also discuss a strategy for merging this in general, since pretty much any open PR based on the current master will have merge conflicts with these changes since they affect every subsystem

@HayenNico
HayenNico marked this pull request as draft May 26, 2026 14:44
@HayenNico

Copy link
Copy Markdown
Member Author

Note: It was decided that this PR will be closed and split into separate PRs for each subsystem instead. Leaving this up as draft temporarily in case someone already wants to review individual sections

@HayenNico HayenNico closed this Jun 29, 2026
nitrosx added a commit that referenced this pull request Aug 7, 2026
## 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

* The endpoints /proposals/:id/datasets and /samples/:id/datasets would
not apply correct filters due to a mismatch between the used casl
ability and checked Action elements

## Changes:

* Replace CaslAbilityFactory.datasetsInstanceAccess and
CaslAbilityFactory.datasetsEndpointAccess with one function
CaslAbilityFactory.datasetAccess
* Code for CaslAbilityFactory.datasetAccess is factored out into new
module DatasetAbility
* Remove all instance-level dataset Action elements
* Adjust endpoint and instance auth logic in dataset controllers
* Remove unused readOwner checks, treat readPublic as always true
(datasets-access.service)
* Adjust auth logic in samples, proposals, users and origdatablocks
controllers where dataset abilities are used

## Tests included

- [x] Included for each change/fix?
- [x] Passing? <!-- Merge will not be approved unless tests pass -->

## Documentation
- [x] swagger documentation updated (required for API changes)
- [x] official documentation updated

### official documentation info
<!-- If you have updated the official documentation, please provide PR #
and URL of the updated pages -->

## Summary by Sourcery

Consolidate dataset authorization into a single CASL ability and align
controllers and access services with the simplified permission model.

Bug Fixes:
- Ensure /proposals/:id/datasets, /samples/:id/datasets, and other
dataset-related endpoints apply correct filters based on unified dataset
permissions for authenticated and unauthenticated users.

Enhancements:
- Replace separate dataset endpoint and instance access factories with a
single datasetAccess ability using unified actions and conditions.
- Simplify dataset-related controllers, V4 controllers, and access
services to rely on generic Dataset* actions instead of
owner/access/public-specific variants.
- Remove numerous instance-level dataset Action enum values in favor of
a smaller, clearer action set, adding AccessAny for elevated roles.
- Treat public dataset reads consistently by defaulting unauthenticated
access to published datasets and tightening v4 behavior to require
public endpoints for anonymous users.
- Update Swagger-guarded policies and related controllers (datasets,
samples, proposals, origdatablocks, users) to use the new datasetAccess
checks.

Tests:
- Adjust dataset controller specs to mock the new datasetAccess factory
method and keep authorization tests passing.
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.

2 participants