Skip to content

Conversation

@DavideIadeluca
Copy link
Member

@DavideIadeluca DavideIadeluca commented Nov 10, 2024

Fixes #393

Changes proposed in this pull request:

  • Implement extra permissions for better transparency what which permissions exactly do
  • Consume those new permissions in the Model Policy
  • Refactor The DeleteFileHandler to consume the Model Policy rather then readding the permission logic
  • Add migration for the permissions to lessen chance of breaking existing integrations

Reviewers should focus on:
No security vulnerabilities are introduced in DeleteFileHandler & FilePolicy. Here is the intended migration logic:

If a group had fof-upload.deleteUserUploads (previously meaning meaning “delete others”):

  • Group needs fof-upload.deleteOtherUsersUploads
  • Group needs fof-upload.hideOtherUsersUploads

If a group had fof-upload.upload-shared-files:

  • Groups needs fof-upload.deleteSharedUploads
  • Group needs fof-upload.hideSharedUploads

To allow that users can hide their own files by default, I've added a separate migration (2025_11_07_000000_grant_hide_own_to_members.php) which grants members this permission

Screenshot
Before and after the permission migration:

Before:
Screenshot 2025-11-07 at 12 52 08

After:
Screenshot 2025-11-07 at 12 53 01

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

@DavideIadeluca DavideIadeluca self-assigned this Nov 10, 2024
@DavideIadeluca DavideIadeluca changed the title fix: use more fine grained permission checks fix: implement more fine grained permission checks Nov 10, 2024
@jaspervriends
Copy link
Contributor

This PR would resolve the following bug as well: it's currently not possible to delete files when you're a moderator (even if you added the "Delete user uploads" permission, since there's a admin only check on it currently). This PR seems to resolve that.

Is there anything that is holding back merging it? So far it looks good to me.

@DavideIadeluca
Copy link
Member Author

Thanks @jaspervriends for bringing this up again. I'll try to make progress here next week

@DavideIadeluca DavideIadeluca marked this pull request as ready for review November 7, 2025 11:56
@DavideIadeluca DavideIadeluca requested a review from a team as a code owner November 7, 2025 11:56
@DavideIadeluca
Copy link
Member Author

@imorland I've now made the changes as discussed and am happy with them at the moment, namely:

  • Refactoring the Model Policy to be more readable
  • Consume the model policy in the DeleteFileHandler
  • Adjusting tests
  • Adding migration path

Do you have time to review this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This right here is more of a POC of how we can handle cases where other permissions are prerequisites for another permission, similiar like Tags does it for example (permission to create discussion in a tag can only be granted to a new group when the viewForum permissions has that group already.

Open for feedback if this should be extended to cover more cases in fof/upload or if I should abandon this. See https://github.com/flarum/framework/blob/097b3c5baa025b46b1fe96a3191ce25fb9a062e7/extensions/tags/js/src/admin/addTagsPermissionScope.tsx#L34-L46 for a similar implementation in the framework

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Members cannot delete their own files

4 participants