Skip to content

Share permission lists across core roles#7847

Merged
labkey-adam merged 1 commit into
developfrom
fb_share_perms
Jul 15, 2026
Merged

Share permission lists across core roles#7847
labkey-adam merged 1 commit into
developfrom
fb_share_perms

Conversation

@labkey-adam

@labkey-adam labkey-adam commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Rationale

Core roles (RestrictedReaderRole -> ReaderRole -> AuthorRole -> EditorWithoutDeleteRole -> EditorRole), share common permissions but their actual permission lists are duplicated in each class. This can easily lead to mistakes. A better approach is for the roles to share permission lists, each role adding just the permissions it needs to add. This is similar to the way we build the permission lists for admin roles.

Tasks

  • Dev
  • Claude Code
  • Essentially no functional change and existing automated testing should be sufficient to verify correctness

@labkey-adam labkey-adam requested a review from a team July 15, 2026 00:53
}

@Override
public boolean isApplicable(SecurableResource resource)

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.

Moving this method is a small functional change but presumably intentional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inverted the hierarchy so EditorRole now inherits from EditorWithoutDeleteRole. So a couple methods had to move to the latter.

Claude isn't thrilled with this change (instanceof semantics will change), but it makes more sense to me.

@labkey-adam labkey-adam merged commit 3687d43 into develop Jul 15, 2026
11 checks passed
@labkey-adam labkey-adam deleted the fb_share_perms branch July 15, 2026 17:27
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