Current implementation of recursive chmod considered harmful

[allisonkarlitskaya]

We currently have a button for "Change Permissions for Enclosed Files" which calls chmod -R with a numeric mode. That's a problem because you usually don't want files and directories to have the same mode. Either all your files end up executable or all of your directories end up broken.

In GNOME this button is labelled "Change Permissions for Enclosed Files..." and opens a second dialog:

If we're going to have recursive mode changing at all, we need something like this. Until that point, I'd argue that we should remove the existing button.

[garrett]

I agree.

[allisonkarlitskaya]

chmod also has a way to say +X which means "add executable permission if any other user has executable permissions". That usually does the "right thing" for both files and directories (and also includes files that are executable files).

If we dumb down the dialog a bit and make it about read/write permissions only, and use +X in the same places we allow read permissions, it might make even more sense than having the full-blown GNOME approach (which breaks existing executable files).

[martinpitt]

Agreed. I also added that to the MVP milestone.

[allisonkarlitskaya]

So here's a very concrete proposal for what I think should happen:

Files and directories should be treated exactly the same, always. To state it explicitly: we should forget about the idea of having "list" (+r) and "access" (+x) treated separately for directories. That's just "read", as far as we're concerned.

In any UI to deal with this, we'll have three categories: user, group, other. Each of those will have a dropdown containing the options "Read and write", "Read-only", "No access":

Access permissions

   User     [  Read and write  | v ]

   Group    [  Read-only       | v ]

   Other    [  No access       | v ]

with each of the dropdowns looking like:

   User     [  Read and write      ]
            |  Read-only           |
            |  No access           |

From this dialog, we construct a string via concatenation, using the following approach:

  "User" = "u"
    "Read and write" = "+rwX"
    "Read-only" = "+rX-w"
    "No access" = "-rwx"
","
  "Group" = "g"
    ... as for the options for user ...
","
  "Other" = "o"
    ... as for the options for user ...

So given the example above, the resulting string would be u+rwX,g+rX-w,o-rwx. This is suitable for passing to chmod or chmod -R for any file or directory.

The only thing that's missing here is support for setting the executable bit on a script. I think that this option does not make sense to be applied recursively, but might make sense on a single file or multiple-selection containing only files (not directories). We might add a separate checkbox for it in this case, in which case we'd see something like (for the single file case):

Access permissions

   User     [  Read and write  | v ]

   Group    [  Read-only       | v ]

   Other    [  No access       | v ]

[x] This file is executable.

This box would be checked if any executable bit is set on the file when the dialog is open. In the case that this explicit UI element is visible we could modify our mode string construction behaviour:

• This file is executable:
  • Read and write: +rwx
  • Read-only: +rx-w
  • No access: -rwx
• This file is executable:
  • Read and write: +rw-x
  • Read-only: +r-wx
  • No access: -rwx

This leaves only the case about what we do when we select multiple files (not directories) and modify their attributes all at once and the selected set has inconsistent executable bits set on it from the beginning.

We could either refuse to handle this case (not show the check) or handle it by showing a checkbox in an inconsistent state (does Patternfly have that?). If the user leaves the check in the inconsistent state then we proceed as per the case where the option wasn't shown.

[allisonkarlitskaya]

a checkbox in an inconsistent state (does Patternfly have that?)

https://www.patternfly.org/components/forms/checkbox/

isChecked
boolean | null
false
Flag to show if the checkbox is checked. If null, the checkbox will be indeterminate (partially checked).

[jelly]

a checkbox in an inconsistent state (does Patternfly have that?)

https://www.patternfly.org/components/forms/checkbox/

isChecked
boolean | null
false
Flag to show if the checkbox is checked. If null, the checkbox will be indeterminate (partially checked).

No, use a Switch https://www.patternfly.org/components/switch/#checked-with-label :)

[martinpitt]

@jelly says that this functionality doesn't exist, so it doesn't need to block the first release.

[garrett]

This is similar to the mockups I already made in PF style and shared a while back. Here's the latest version:

BTW: It's a checkbox instead of a switch as switches are instant-apply and checkboxes are queued. See https://www.patternfly.org/components/switch/design-guidelines#when-to-use-switches-versus-checkboxes for details.

(Edit: Previous version, nearly the same, is at #471 (comment) — this one adds the checkbox for files and makes the title of the file and dir more obvious. It does not, notably, add SELinux contexts, but @jelly and I talked and agreed that it would be good to do the same thing as GNOME and add it in as a read-only blob of text.)

[garrett]

FWIW, that screenshot at the top of the page is the ages old GNOME settings. For an updated reference, here's what it looks like today:

And then you click permissions and get this:

(However, I think there's a bug, as this is a file and it still has a button that says " Change permissions for enclosed files...".)

[garrett]

Mockup from above for files, with a SELinux security context shoved in.

[jelly]

@garrett I have one question about the implementation of the executable option.

In GNOME Files this is implemented there is some logic to only show this when it is logical to make a file excutable by looking at the mimetype:

const char *mime_type = nautilus_file_get_mime_type (get_file (self));
return g_content_type_can_be_executable (mime_type);

Furthermore GNOME only sees something as "executable" when all bits are set executable so owner, group and other.

Do we replicate this behavior? FYI: this makes sense, as this happens for normal system binaries.

-rwxr-xr-x 1 root root 35K Aug 30 13:57 /usr/bin/uname*

This also agrees with what @allisonkarlitskaya said before I think:

chmod also has a way to say +X which means "add executable permission if any other user has executable permissions". That usually does the "right thing" for both files and directories (and also includes files that are executable files).

If we dumb down the dialog a bit and make it about read/write permissions only, and use +X in the same places we allow read permissions, it might make even more sense than having the full-blown GNOME approach (which breaks existing executable files).

[garrett]

@jelly: Sure, that all sounds reasonable. Show it when the filetype can be executable (we should use the same logic), then use +X.

(I thought we talked about this in a call a few months ago and agreed on it, but it's good to have it clearly agreed upon and written down here regardless. Thanks for asking and formalizing it!)

I'm guessing binary files and shell scripts can be executable, as well as things like AppImages (which are mainly outside the scope of Cockpit, except if an admin is browsing a home dir, I suppose). I suppose it uses magic sniffing for mimetypes with an accept list.