Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permission Checks now use Wildcard semantics. #2355

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

chrisknoll
Copy link
Collaborator

@chrisknoll chrisknoll commented Feb 27, 2024

Removed role cache from Permission Service.
Changed read/write permission checks to use permissions instead of roles.
Permission checks are using Subject.isPermitted() which honors wildcard semantics.
Altered JwtAuthRealm to filter user permissions to either * or first element of permission to check for speed.
Changed permission index from JsonNode to Map<>. Serializes same way, but map semantics are simpler to navigate.
Altered AuthrizationInfo to contain index of Permissions and store Wildcard perms.

General cleanup of unused imports and removed unused dependencies (ie: Autowired fields were removed if no longer needed).

Fixes #2353.

Changed read/write permission checks to use permissions instead of roles.
Permission checks are using Subject.isPermitted() which honors wildcard semantics.
Altered JwtAuthRealm to filter user permissions to either * or first element of permission to check for speed.
Changed permission index from JsonNode to Map<>.  Serializes same way, but map semantics are simpler to navigate.
Altered AuthrizationInfo to contain index of Permissions and store Wildcard perms.

General cleanup of unused imports and removed unused dependencies (ie: Autowired fields were removed if no longer needed).

Fixes #2353.
@chrisknoll
Copy link
Collaborator Author

Leaving this PR as draft as there may be additional code to be removed from the codebase as a result of this change: things like sql templates and entity graphs that are used to find roles for the user may no longer be necessary (I'm not sure where we would ever use roles to determine a permission, except for 'Admin').

As we code review, feel free to identify blocks of code that may be redundant or no longer necessary. We are always making efforts to simplify the codebase.

@chrisknoll chrisknoll changed the title Removed role cache from Permission Service. Permission Checks now use Wildcard semantics. Mar 4, 2024
@rkboyce
Copy link
Contributor

rkboyce commented Mar 15, 2024

@chrisknoll - I tested this change in my environment where have the read-restricted feature enabled and everything worked as expected. Great job!

Reordered test scoped dependencies in pom.xml.
Refactored shared methods to AbstractDatabaseTest.
Added test to grant * permission to user.
@chrisknoll chrisknoll marked this pull request as ready for review April 16, 2024 14:32
@chrisknoll
Copy link
Collaborator Author

The last commit adds tests.

For people on the call, DB Unit has several modes of inserting described here:
https://www.dbunit.org/components.html

The code I demonstrated did a CLEAN_ALL at first which blows away all the data in a table, which would include 'system roles' so it was rightfully pointed out about that issue.

I attempted a quick demo to show how the test could be changed, and I switched it to DatabaseOperation.INSERT thinking it would insert the data, but from the docs, DatabaseOperation.INSERT will fail if the table exists (ie: INSERT requires a table creation). The correct operation I wanted to use is REFRESH, which will leave existing rows alone and add new records based on the test data set.

@anthonysena this is ready for review.

@chrisknoll chrisknoll requested a review from anthonysena April 16, 2024 14:34
@chrisknoll chrisknoll merged commit 6a43da5 into master Apr 23, 2024
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the issue-2353-wildcard-perms branch April 23, 2024 06:10
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.

has[Write|Read]Access does not use wildcard permissions
3 participants