Skip to content

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

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

refactor: casl-ability-factory refactor/reduction#2748
HayenNico wants to merge 21 commits into
masterfrom
refactor-casl-factory

Conversation

@HayenNico
Copy link
Copy Markdown
Member

@HayenNico HayenNico commented May 21, 2026

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
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

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
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

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
Copy link
Copy Markdown
Member

nitrosx commented May 25, 2026

@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
Copy link
Copy Markdown
Member

nitrosx commented May 25, 2026

...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

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