Repository navigation
Conversation
There was a problem hiding this comment.
Sorry @HayenNico, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
Sorry @HayenNico, your pull request is larger than the review limit of 150000 diff characters
|
@HayenNico amazing work. I am in the process of reviewing it. |
|
...also I wonder if we need to review the authorization tests to update the existing test cases and add new ones |
|
@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. |
|
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 |
|
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 |
## 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.
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
Changes:
Possible extensions
DatasetOrigdatablockRead) via controller refactoringTests included
Documentation