cockpit: support setting owner/group in fsreplace1 #21128

jelly · 2024-10-16T16:07:51Z

Cockpit-files wants to support uploading or creating a file owned by the current directory which might be different from the logged in user.

For example as superuser uploading a database into /var/lib/postgresql which would be owned by postgres and the database file should receive the same permissions.

pkg/lib/cockpit.d.ts

pkg/playground/test.js

jelly · 2024-11-21T15:36:17Z

Only triggered Fedora for now, lets review one round and then run all tests.

martinpitt

Thanks! Nice unit tests with the mocking!

doc/protocol.md

martinpitt · 2024-11-22T09:10:21Z

doc/protocol.md

+ * `uid`: an integer, the uid of the file owner (`st_uid`)
+ * `owner`: a string, or an integer, the uid of the file owner (`st_uid`)


These conflict. Please document which one wins, or that specifying both is an error. Same for group.

Is there some deeper reason to allow both? AFAIUI, owner can take an uid, so this feels redundant.

Well @allisonkarlitskaya argued that this should take the fsinfo fields so those are uid/gid/owner/group

fwiw, fsinfo calls this user, not owner.

We also discussed this in a call: we should just implement user and group (supporting either strings or ints), not uid and gid.

pkg/playground/test.js

martinpitt · 2024-11-22T09:14:48Z

pkg/playground/test.js

+        })
+                .catch(exc => {
+                    console.log(exc);
+                    fsreplace_error.textContent = exc;


Hm, doesn't this need a .toString() Otherwise you'll just get some useless "[Object]".

For one instance it worked, but yeah safer to toString()

Still no .toString(), reopening.

pkg/playground/test.js

src/cockpit/channels/filesystem.py

allisonkarlitskaya · 2024-11-26T15:42:05Z

pkg/lib/cockpit.d.ts

@@ -257,9 +257,16 @@ declare module 'cockpit' {
        remove(): void;
    }

+    interface ReplaceAttrs {


At least mode should also get added while you're at it. I think. I'm not 100% sure what we should do with this and its interaction with umask...

size is also in the fsinfo blob. This would be been an interesting alternative to the size-hint attribute we added at the top level. I think maybe the ship has sailed on that one already?

Yeah but many fields are already not possible from fsinfo so hmm tagging that along seems arbitrary.

Mode does not make sense but I don't want to add it now.

BTW, we want to use this in files soon (tm) but we ofcourse have no hint that we support this. As passing attrs={} just silently ignores it. But maybe we can read the tag, which I hope is returned by await file.replace("").

martinpitt

Thanks!

martinpitt · 2024-11-27T09:27:34Z

doc/protocol.md

+ - `owner`: a string, or an integer, the uid of the file owner (`st_uid`)
+ - `group`: a string, or an integer, the gid of the file group (`st_gid`)


This renders wrong: https://github.com/jelly/cockpit/blob/fsreplace1-chown/doc/protocol.md#payload-fsreplace1

You may have to indent these two a bit more. Please also document that you (currently) have to set both of these.

Also see the outstanding thread to rename owner → user for consistency (I agree to this).

martinpitt · 2024-11-27T09:31:27Z

pkg/playground/test.js

+        })
+                .catch(exc => {
+                    console.log(exc);
+                    fsreplace_error.textContent = exc;


Still no .toString(), reopening.

src/cockpit/channels/filesystem.py

martinpitt · 2024-11-27T09:37:52Z

src/cockpit/channels/filesystem.py

+        if owner is None or group is None:
+            raise ChannelError('protocol-error', message='"owner" or "group" attribute is empty')


So that means as soon as you specify any attrs in fsreplace1, you always have to specify user and group. This is fine right now, but we'll have to relax this in the future once we start supporting other attributes. We only need to enforce "neither or both", not "always both".

Yes, but the logic here would have be if you specify owner you have to specify group and vice-versa

Right, but right now it's not "if", it's "you always have to specify user and group". E.g. you couldn't call file.replace({ mode: 0755 }) (in a future where we support more attributes)

Alright, I've added this new logic which also gives a better error message.

Cockpit-files wants to support uploading or creating a file owned by the current directory which might be different from the logged in user. For example as superuser uploading a database into `/var/lib/postgresql` which would be owned by `postgres` and the database file should receive the same permissions.

cockpituous · 2024-11-27T16:19:25Z

pkg/playground/test.js

+                .catch(exc => {
+                    fsreplace_error.textContent = exc.toString();


These 2 added lines are not executed by any test.

jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 16, 2024

jelly force-pushed the fsreplace1-chown branch 2 times, most recently from 2602089 to 489d3f5 Compare November 1, 2024 14:48

jelly marked this pull request as ready for review November 14, 2024 15:50

jelly marked this pull request as draft November 14, 2024 15:50

jelly force-pushed the fsreplace1-chown branch 6 times, most recently from 20ea543 to 8554761 Compare November 21, 2024 13:52

jelly commented Nov 21, 2024

View reviewed changes

pkg/lib/cockpit.d.ts Outdated Show resolved Hide resolved

jelly commented Nov 21, 2024

View reviewed changes

pkg/playground/test.js Outdated Show resolved Hide resolved

jelly requested review from mvollmer, allisonkarlitskaya and martinpitt and removed request for mvollmer November 21, 2024 15:35

jelly marked this pull request as ready for review November 21, 2024 15:35

jelly closed this Nov 21, 2024

jelly reopened this Nov 21, 2024

jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 21, 2024

martinpitt requested changes Nov 22, 2024

View reviewed changes

allisonkarlitskaya reviewed Nov 26, 2024

View reviewed changes

jelly force-pushed the fsreplace1-chown branch 2 times, most recently from 96db00e to 2764d3e Compare November 26, 2024 16:39

jelly requested a review from martinpitt November 27, 2024 09:14

martinpitt requested changes Nov 27, 2024

View reviewed changes

jelly force-pushed the fsreplace1-chown branch from 2764d3e to 3bedec1 Compare November 27, 2024 15:09

jelly force-pushed the fsreplace1-chown branch from 3bedec1 to 98c455f Compare November 27, 2024 15:26

cockpituous reviewed Nov 27, 2024

View reviewed changes

jelly mentioned this pull request Nov 28, 2024

fsreplace1 support mode as attribute #21350

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cockpit: support setting owner/group in fsreplace1 #21128

cockpit: support setting owner/group in fsreplace1 #21128

jelly commented Oct 16, 2024 •

edited

Loading

jelly commented Nov 21, 2024

martinpitt left a comment

martinpitt Nov 22, 2024

jelly Nov 22, 2024

allisonkarlitskaya Nov 26, 2024

martinpitt Nov 22, 2024

jelly Nov 22, 2024

martinpitt Nov 27, 2024

allisonkarlitskaya Nov 26, 2024

jelly Nov 26, 2024

jelly Nov 26, 2024

martinpitt left a comment

martinpitt Nov 27, 2024

martinpitt Nov 27, 2024

martinpitt Nov 27, 2024

jelly Nov 27, 2024

martinpitt Nov 27, 2024

jelly Nov 27, 2024

cockpituous Nov 27, 2024

		* `uid`: an integer, the uid of the file owner (`st_uid`)
		* `owner`: a string, or an integer, the uid of the file owner (`st_uid`)

		- `owner`: a string, or an integer, the uid of the file owner (`st_uid`)
		- `group`: a string, or an integer, the gid of the file group (`st_gid`)

		if owner is None or group is None:
		raise ChannelError('protocol-error', message='"owner" or "group" attribute is empty')

		.catch(exc => {
		fsreplace_error.textContent = exc.toString();

cockpit: support setting owner/group in fsreplace1 #21128

Are you sure you want to change the base?

cockpit: support setting owner/group in fsreplace1 #21128

Conversation

jelly commented Oct 16, 2024 • edited Loading

jelly commented Nov 21, 2024

martinpitt left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a 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 commented Oct 16, 2024 •

edited

Loading