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

add check for specific user inherited permission #556

Conversation

andrewandante
Copy link
Contributor

@andrewandante andrewandante commented Jun 15, 2023

Connected to silverstripe/silverstripe-framework#10819

Allows file/folder access to be restricted on a user-by-user basis, rather than having to use groups

Issue

src/File.php Outdated Show resolved Hide resolved
src/File.php Outdated Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

One thing that's a little confusing is if you have previously set a group, and then you decide to swap to setting explicit users, the selected group is still present.

This is solved for pages by hiding the field, but as I mentioned in one of these PRs that may be out of scope for this PR... the other way to solve it would be to clear the groups if the permission type is set to something else (and same for users).... can you foresee any problems if we implemented that?

@andrewandante andrewandante force-pushed the FEAT_add_individual_users_permissions_check branch from fa8975c to d1a7f95 Compare July 6, 2023 04:53
@andrewandante
Copy link
Contributor Author

can you foresee any problems if we implemented that?

Hmm. It makes "changing back" pretty tricky, which is a bit annoying. I'd prefer to go with the "hide what is irrelevant" approach - though I could also see a "Reset to defaults"-style action which pushes everything back to Inherit and clears all the relationships. I agree it looks a bit.. mediocre 😅

@andrewandante
Copy link
Contributor Author

@GuySartorelli presumably this needs an updated silverstripe/framework: ^5.1 constraint too?

@GuySartorelli
Copy link
Member

Ahh, yes please! I thought I checked it for that but apparently I didn't 😅

@andrewandante andrewandante force-pushed the FEAT_add_individual_users_permissions_check branch from d1a7f95 to 47fa6c6 Compare July 6, 2023 21:32
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit 7605cda into silverstripe:2 Jul 6, 2023
8 checks passed
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