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

Mulitple "My Folder" folders after upgrade #3046

Closed
leigh-pointer opened this issue Jul 19, 2023 · 5 comments
Closed

Mulitple "My Folder" folders after upgrade #3046

leigh-pointer opened this issue Jul 19, 2023 · 5 comments

Comments

@leigh-pointer
Copy link
Contributor

leigh-pointer commented Jul 19, 2023

image

It appears the all the "My Folder" for each Registered User is being displayed.

image

@sbwalker
Copy link
Member

@leigh-pointer are you logged in as a host user - I want to make sure I understand the context.

@leigh-pointer
Copy link
Contributor Author

Yes logged in as host

@sbwalker
Copy link
Member

sbwalker commented Jul 19, 2023

This was caused by this PR #2989 which was caused by changes to the FolderController related to the FileManager. Basically a Folder has 3 types of permissions:

View - is allowed to open/view files which exist in a folder
Edit - is allowed to upload/edit/delete files which exist in a folder
Browse - is allowed to view the list of files which exist in a folder

In #2989 the Folder GET methods were modified to filter based upon View permissions rather than Browse permissions. The rationale for this modification was that Browse permission is related to obtaining a list of Files - not Folders. If someone is requesting a Folder it should be based on View permissions.

The scenario which prompted this change was related to a configuration of the FileManager where you are simply uploading files to a folder but have no requirement to see a list of files (ie. you have set DisplayFiles = false in the FileMananager - note that the FileManager is multi-purpose - it is used as a file picker, a file uploader, or both). For example you may have a Folder in your site where you want to allow users to upload files but not obtain a listing of all of the other files in that folder (ie. a shared upload folder). In this scenario the permissions would be:

View - everyone
Edit - registered users
Browse - administrators

Under the previous API logic, if a registered user called GetFolders it would exclude this "shared" folder because it requires Browse permissions - which was not correct. So the logic was changed to check for View permissions rather than Browse permissions.

However.... a side effect is that User Folders are created with the following permissions:

View - everyone
Edit - only the user who owns the folder
Browse - only the user who owns the folder

So based on the FolderController GET changes in #2989, the User folders are now visible in the Folder list (note that if you choose a User folder other than your own, you will not be able to view the list of Files or upload/delete files - so this is NOT a security vulnerability).

I believe the FolderController API change was valid. However it appears that an additional change is required in the FileManager based on its specific cpnfiguration in each instance. If the FileManager is configured with DisplayFiles = true then it should security trim the list of Folders displayed in the UI based on Browse permissions (as it is being used as a filer picker in this scenario).

@sbwalker
Copy link
Member

sbwalker commented Jul 19, 2023

@leigh-pointer so I am having a bit of a dilemma on this item...

The 2 standard GET methods which return a single Folder object MUST rely on View permissions in order to preserve other functionality. So they cannot be rolled back to the previous behavior - they need to retain the new behavior.

The GET method which returns a collection of Folder objects...

  1. Could be rolled back to rely on Browse permissions. This would resolve every scenario where a list of Folders is displayed, as the User Folders would be filtered out. This change would also preserve legacy behavior so that third parties who used this API method would not be affected. The downside is that this method would depend on a Permission which is inconsistent with the other GET methods.

  2. Could remain as it is and rely on View permissions. This affects a number of areas in the framework where logic would need to be added to filter folders based on Browse permissions to remove the User Folders. The benefit is that this method would depend on the same Permission (View) as the other GET methods.

I am leaning more towards # 1 - what are your thoughts?

@leigh-pointer
Copy link
Contributor Author

leigh-pointer commented Jul 19, 2023

@sbwalker I think also # 1 is the best solution for now. This choice makes sense if we prioritize preserving legacy behavior and ensuring that third-party users of the API method are not negatively impacted.

sbwalker added a commit that referenced this issue Jul 19, 2023
fix #3046 - user folders displayed in folder lists
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

No branches or pull requests

2 participants