Add ownership options for "Create directory" #478

allisonkarlitskaya · 2024-05-27T12:48:54Z

If we have superuser capabilities, then add a drop-down list to the "Create directory" dialog with possible ownerships for the create directory.

By default we try to create the new directory with the same owner as the parent, but also have options for root:root or the user:group of the logged in user (if it is different from the parent directory).

The logic for determining the potential owners of newly created items is generic and might also be applied to things like upload, so keep it in its own file.

Also, move the dialog into a new file: src/dialogs/mkdirt.tsx. fileActions has become a bit too much of a catch-all.

Todo:

better UI
pkg/lib: port form-helper component to typescript cockpit#20516 and sync
tests for creating in various directories and ensuring "reasonable default"

src/fileActions.jsx

allisonkarlitskaya · 2024-05-27T13:38:26Z

# src/dialogs/mkdir.tsx(107,26): error TS2739: Type '{ fieldId: string; helperTextInvalid: string | null; }' is missing the following properties from type '{ helperText: any; helperTextInvalid: any; variant: any; icon: any; fieldId: any; }': helperText, variant, icon

This is a pretty annoying issue. This comes from cockpit repo, and it's written in js. js assumes that some of these fields might be missing (and deals with them if they're undefined). It would be nice if we could figure out a way to work around issues like this with something better than "port the cockpit component to typescript" every time...

src/ownership.tsx

jelly · 2024-05-27T13:53:30Z

src/ownership.tsx

+    }
+
+    const id = Number(match[1]);
+    const name = match[2] === "unbound" ? null : match[2];


When is match[2] ever unbound? Is that something id prints?

I tested running id as a user without a /etc/passwd entry and it printed "unbound" for the username.

I also noticed that ls -l does this now as well, instead of printing the numeric ID, which is a bit annoying.

I wonder if it's some nss module responsible for that, or a recent change in coreutils. I tried to look into it a bit but I didn't get too far.

Ok, unbound is also a fairly popular daemon so that's "interesting"

Sounds like it might be nss https://github.com/search?q=repo%3Acoreutils%2Fcoreutils%20unbound&type=code

This unbound check still feels a bit suspect to me.

src/dialogs/mkdir.tsx

jelly · 2024-05-27T13:57:38Z

src/dialogs/mkdir.tsx

+                                  key={owner}
+                                  value={owner}
+                                  /* Translators: the $0 is a string like root:root or admin:staff */
+                                  label={cockpit.format(_("Create directory owned by '$0'"), owner)}


I don't think the Select should include this very verbose information but only root:root.

Agreed, but I didn't know how to do the thing with the Label :)

Okay. I did something here. I don't know if it's good enough.

src/ownership.tsx

allisonkarlitskaya · 2024-05-29T11:45:45Z

Screenshots:

jelly · 2024-05-29T14:00:08Z

test/check-application

+            b.wait_visible("[data-item='newdir']")
+            self.assert_owner('/root/newdir', 'root:root')
+        finally:
+            m.execute(['rm', '-rf', '/root/newdir'])


This should be `self.addCleanup(m.execute, "rm -rf /root/newdir")

True.

I first tried to do .restore_dir('/root') but this had problems since it would need to unmount the overlayed root homedir while running a command executed over ssh... as root. So I did this. But you're right — addCleanup is definitely more idiomatic.

jelly · 2024-05-29T14:03:14Z

test/check-application

+            b.wait_visible("[data-item='newdir']")
+            self.assert_owner('/tmp/newdir', 'root:root')
+        finally:
+            m.execute(['rm', '-rf', '/tmp/newdir'])


Same as above.

test/check-application

allisonkarlitskaya · 2024-06-04T09:51:59Z

The rawhide issue is a straight-up race. The tests are fast enough to go through the process of creating the directory before sudo gets done with what it's doing. We do superuser: try for the mkdir and that ends up waiting until sudo is done, which will create the directory as root, even though we expected that we were doing that in a non-authenticated state.

This stats a file on the VM with the given format string.

src/ownership.tsx

jelly · 2024-06-04T15:18:59Z

src/dialogs/mkdir.tsx

+                  <Button
+                    variant="primary"
+                    onClick={createDirectory}
+                    isDisabled={errorMessage !== undefined || nameError !== undefined}


This needs to be disabled when superuser.allowed && !id && !cwdInfo

- isDisabled={errorMessage !== undefined || nameError !== undefined} + isDisabled={errorMessage !== undefined || nameError !== undefined || (superuser.allowed && (!id && !cwdInfo))}

I think you meant (!id || !cwdInfo) or !(id && cwdInfo), yes?

jelly · 2024-06-04T16:32:30Z

src/ownership.tsx

+}
+
+export function useId(): IdInfo | null {
+    /* XXX This should be part of the cockpit UserInfo */


And it is, so linking cockpit-project/cockpit#20517 would be nice

jelly · 2024-06-04T16:33:04Z

src/ownership.tsx

+
+    const [id, setId] = React.useState<IdInfo | null>(null);
+
+    React.useEffect(() => {


useInit is like useEffect(, [])

jelly

If we add isDisabled then the change is fine to me

allisonkarlitskaya · 2024-06-05T11:48:24Z

If we add isDisabled then the change is fine to me

Done.

jelly · 2024-06-05T13:33:19Z

Two trivial lint issues left.

If we have superuser capabilities, then add a drop-down list to the "Create directory" dialog with possible ownerships for the create directory. By default we try to create the new directory with the same owner as the parent, but also have options for root:root or the user:group of the logged in user (if it is different from the parent directory). The logic for determining the potential owners of newly created items is generic and might also be applied to things like upload, so keep it in its own file. Also, move the dialog into a new file: src/dialogs/mkdirt.tsx. fileActions has become a bit too much of a catch-all. Add some test cases to make sure that we create files with "reasonable default" ownership in various different locations.

jelly · 2024-06-05T13:58:53Z

download tests can be flaky, retrying.

cockpituous · 2024-06-05T14:04:51Z

src/dialogs/mkdir.tsx

+        await cockpit.spawn(["mkdir", path], opts);
+        await cockpit.spawn(["chown", owner, path], opts);
+    } else {
+        await cockpit.spawn(["mkdir", path], { err: "message" });


This added line is not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/dialogs/mkdir.tsx

+                  isHorizontal onSubmit={e => {
+                      createDirectory();
+                      e.preventDefault();
+                      return false;


These 4 added lines are not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/dialogs/mkdir.tsx

+                            <FormSelect
+                              id='create-directory-owner'
+                              value={owner}
+                              onChange={(_ev, val) => setOwner(val)}


This added line is not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/ownership.tsx

+    if (!match) {
+        throw new Error(`invalid id ${content}`);


These 2 added lines are not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/ownership.tsx

+    }
+
+    const id = Number(match[1]);
+    const name = match[2] === "unbound" ? null : match[2];


This added line is not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/ownership.tsx

+            try {
+                const uid = parse_id(fields.uid);
+                const gid = parse_id(fields.gid);
+                const groups = fields.groups?.split(',')?.map(parse_id) || [];


This added line is not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/ownership.tsx

+            } catch (exc) {
+                console.log(`Failed to parse output of "id" command: ${stdout}: ${exc}`);


These 2 added lines are not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/ownership.tsx

+    // create as, mirroring the usual POSIX behaviour.  There are other cases
+    // where the "BSD group semantics" come into play (like mount options) but
+    // we don't currently support those.  We might in the future, though...
+    const setgid = (info.group !== undefined && (info.mode || 0) & 0o2000) ? `${info.group}` : null;


This added line is not executed by any test.

cockpituous · 2024-06-05T14:04:51Z

src/ownership.tsx

+    // The last option is always available: create as the normal user.  In case
+    // of something inside of the user's home directory, this was probably the
+    // first option as well...
+    candidates.add(`${id.uid.name || id.uid.id}:${setgid || id.gid.name || id.gid.id}`);


This added line is not executed by any test.

jelly

Merging with the promise that we kill useId with Cockpit 318 :)

allisonkarlitskaya requested a review from jelly May 27, 2024 12:48

github-advanced-security bot found potential problems May 27, 2024

View reviewed changes

src/fileActions.jsx Fixed Show fixed Hide fixed

jelly reviewed May 27, 2024

View reviewed changes

allisonkarlitskaya force-pushed the mkdir branch 3 times, most recently from 3c77a94 to 579964d Compare May 27, 2024 15:46

jelly reviewed May 28, 2024

View reviewed changes

src/ownership.tsx Show resolved Hide resolved

allisonkarlitskaya force-pushed the mkdir branch from 579964d to 4a7660f Compare May 29, 2024 10:03

jelly requested changes May 29, 2024

View reviewed changes

allisonkarlitskaya force-pushed the mkdir branch from 4a7660f to 6b08cfa Compare June 4, 2024 09:15

test: add a new self.stat() helper

d7d7fd9

This stats a file on the VM with the given format string.

allisonkarlitskaya force-pushed the mkdir branch from 6b08cfa to 3455d5b Compare June 4, 2024 12:38

jelly marked this pull request as ready for review June 4, 2024 12:38

jelly reviewed Jun 4, 2024

View reviewed changes

allisonkarlitskaya force-pushed the mkdir branch from 3455d5b to 26f25f5 Compare June 4, 2024 15:28

jelly reviewed Jun 4, 2024

View reviewed changes

src/ownership.tsx

const [id, setId] = React.useState<IdInfo | null>(null);

React.useEffect(() => {

Copy link

Member

jelly Jun 4, 2024

Choose a reason for hiding this comment

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

useInit is like useEffect(, [])

jelly reviewed Jun 4, 2024

View reviewed changes

jelly mentioned this pull request Jun 5, 2024

Mkdir as user #421

Closed

3 tasks

allisonkarlitskaya force-pushed the mkdir branch from 26f25f5 to 0ed39d3 Compare June 5, 2024 11:47

allisonkarlitskaya requested a review from jelly June 5, 2024 11:48

allisonkarlitskaya force-pushed the mkdir branch from 0ed39d3 to 3951826 Compare June 5, 2024 13:42

cockpituous reviewed Jun 5, 2024

View reviewed changes

jelly approved these changes Jun 5, 2024

View reviewed changes

jelly merged commit 75cf854 into cockpit-project:main Jun 5, 2024
14 checks passed

allisonkarlitskaya mentioned this pull request Jun 5, 2024

unconditionally creates dirs/files/links as root #390

Closed

allisonkarlitskaya deleted the mkdir branch June 19, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ownership options for "Create directory" #478

Add ownership options for "Create directory" #478

allisonkarlitskaya commented May 27, 2024 •

edited

Loading

allisonkarlitskaya commented May 27, 2024

jelly May 27, 2024

allisonkarlitskaya May 27, 2024

jelly May 27, 2024

jelly May 27, 2024

jelly Jun 4, 2024

jelly May 27, 2024

allisonkarlitskaya May 27, 2024

allisonkarlitskaya May 27, 2024

allisonkarlitskaya commented May 29, 2024

jelly May 29, 2024

allisonkarlitskaya May 29, 2024

jelly May 29, 2024

allisonkarlitskaya commented Jun 4, 2024

jelly Jun 4, 2024

jelly Jun 4, 2024

allisonkarlitskaya Jun 5, 2024

jelly Jun 4, 2024 •

edited

Loading

jelly Jun 4, 2024

jelly left a comment

allisonkarlitskaya commented Jun 5, 2024

jelly commented Jun 5, 2024

jelly commented Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

cockpituous Jun 5, 2024

jelly left a comment


		const [id, setId] = React.useState<IdInfo \| null>(null);

		React.useEffect(() => {

		} catch (exc) {
		console.log(`Failed to parse output of "id" command: ${stdout}: ${exc}`);

Add ownership options for "Create directory" #478

Add ownership options for "Create directory" #478

Conversation

allisonkarlitskaya commented May 27, 2024 • edited Loading

allisonkarlitskaya commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jun 5, 2024

jelly commented Jun 5, 2024

jelly commented Jun 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

allisonkarlitskaya commented May 27, 2024 •

edited

Loading

jelly Jun 4, 2024 •

edited

Loading