Skip to content

SAK-51375 Content fix Edit Folder Permissions in sub-folder #13631

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ottenhoff
Copy link
Contributor

No description provided.

@ottenhoff ottenhoff requested a review from adrianfish April 21, 2025 20:06
@@ -121,7 +122,7 @@ public Map<String, Object> getPermissions(@PathVariable String siteId, @PathVari

on.put(role.getId(), filteredFunctions);

if (overrideRole != null) {
if (overrideRole != null && !"content".equalsIgnoreCase(tool)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Adrian we reviewed this in the core call and didn't quite understand the authz override, could you help clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep. The authz override specifies and authzgroup with permissions which lock any "child" authzgroup permissions where the overriding group has that permission set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what no one could understand on the core call: what is the use case for locking the permissions so that instructor cannot modify them? does this PR look correct to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because top level folder permissions are supposed to dominate. I did it on purpose base on Sean Horner's description of the use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry this isn't making sense to me. The use case that we've supported for a dozen years is creating a sub-folder inside Resources that has increased permissions (e.g., students can add/edit files). That seems to no longer be possible.

I see Sean advocating for disabling checkboxes in https://sakaiproject.atlassian.net/browse/SAK-50984, but I think this hasn't been fully thought through.

The root of the problem seems to be that the sub-folder realm permissions are additive on top of the parent folder permissions? E.g., if top parent has content.delete for student role than it's impossible to remove that permission from a sub-folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sakai 23 has changed a lot after the permissions helper was removed and permissions web component replaced it.

Here is the way Sakai 22 works today:

  1. The root Resources tool permissions are the base-level permissions.
  2. sub-folder permissions inherit from the parent folder permissions and from the tool permissions.
  3. You cannot remove permissions from a folder that are defined in (1) or (2). these are locked/disabled
  4. if a permission is granted on a parent folder, there is no way to remove it on a sub-folder.

IDK, this isn't what i assumed. do you want more people to weight in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to codify how this is supposed/expected to work so it probably makes sense for others to chime in. I'm worried that if I change things again somebody else will complain that I've broken it. You know how it goes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It currently inherits from the top level folder alone I think, so the hierarchy is flattened under that folder. That seems wrong anyway - a folders permissions, as in 22, should inherit from its parent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it had gone all too well when I switched content to the web component ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm fine with implementing this exactly as it behaves on Sakai 22. but i think it's ultimately all wrong ;) but at least it would be consistent

@wilmahodges
Copy link

In my opinion, the expected behavior for Resources permissions is as follows: 1) When a new folder is created, it inherits the permissions of the parent folder by default. 2) However, if you go to the child folder and select Actions > Edit Folder Permissions, you should be able to remove or add permissions which override the inherited default for that folder and any subfolders within it. Each subfolder can likewise restrict/add permissions as needed if set individually.

This sort of behavior is consistent with what users expect in modern file management/sharing applications. For example, in Google Drive, you can share folders with different people at any level of the folder hierarchy. Subfolders inherit the sharing rules from the parent folder, but the sharing can be modified for any subfolder individually. (You get a message about it being easier to remove sharing on the parent folder, but it still lets you remove permissions from just the subfolder if you choose.)

That being said, in order to facilitate scenarios such as allowing folders to only be seen by certain groups, or hiding a folder from students so they don't see answer keys or test images, a better way to manage these use cases is by going to Actions > Edit Details for the resources folder (or file) and changing the Availability and Access settings to control whether or not students can view an item. Since Availability and Access handles the removal of permissions for most use cases, really the main reason to go to Edit Folder Permissions is to add permissions, such as allowing students to edit or delete files within a folder. I suspect this is where the idea of locking permissions at the parent folder came about. However, it is a bit confusing to have two different places where an instructor goes to manage folder and file permissions and access. Perhaps people are going to the wrong place to set up the sort of access restriction they want?

I've posted the above comment to the sakai-user list to see if others want to weigh in.

@adrianfish
Copy link
Contributor

Boom! And Wilma has arrived on the scene. Finally, one of us knows what the heck's going on :)

@hornersa
Copy link
Contributor

After 'verifying' the other related jira, I'm just now catching up to this. Will read more and come up to speed tomorrow. (Fwiw, my assertions in the test plan from the other jira are based on past behavior with the assumption that past behavior = "should". I'm open to a revised sense of "should".)

@hornersa
Copy link
Contributor

To contextualize why and where I filed jiras related to this issue, I'll write those in the comments section of the jira corresponding with this PR. (I need to refer to other jiras and don't want those issue IDs muddying up github to jira conventions during merge, etc.)

@adrianfish
Copy link
Contributor

@hornersa I'd just like to thank you for your attention to detail on issues such as this. You regularly seem to get involved in some complex corners of Sakai and your insight is appreciated.

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.

5 participants