Mkdir as user #421
Mkdir as user #421
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/app.tsx
Outdated
| @@ -47,11 +47,15 @@ export interface FolderFileInfo extends FileInfo { | |||
| to: string | null, | |||
| } | |||
|
|
|||
| interface FilesContextType { | |||
| addAlert: (title: string, variant: AlertVariant, key: string) => void, | |||
| rootInfo: FileInfo | null, | |||
There was a problem hiding this comment.
"root" sounds a bit confusing here -- this is the file information for the current directory, no? I.e. cwdInfo or currentDir, or currentDirInfo?
src/app.tsx
Outdated
| addAlert: (title: string, variant: AlertVariant, key: string) => console.warn("FilesContext not initialized") | ||
| }); | ||
| addAlert: () => console.warn("FilesContext not initialized"), |
There was a problem hiding this comment.
Hmm, the old signature made more sense to me as documentation. Perhaps at least add a comment how it is meant to be called?
There was a problem hiding this comment.
Sure, but I got a bit tired of the copy pasting of typing.
src/app.tsx
Outdated
| } | ||
|
|
||
| export const FilesContext = React.createContext({ | ||
| addAlert: () => console.warn("FilesContext not initialized"), | ||
| rootInfo: null, | ||
| currentPath: "/", |
There was a problem hiding this comment.
This is never meant to be a file. currentDir perhaps, as an indication?
src/app.tsx
Outdated
| if (!currentPath.endsWith('/')) { | ||
| currentPath = `${currentPath}/`; |
There was a problem hiding this comment.
If the given ?path= argument is not a directory, this gives something invalid. I suppose ENOENT is handled somehow, but what if it's a file, link, etc.?
src/fileActions.jsx
Outdated
| const options = { err: "message" }; | ||
|
|
||
| // Current user does not "own" the current directory so we require superuser | ||
| if (user.name !== rootInfo.user || user.name !== rootInfo.group) { |
There was a problem hiding this comment.
Why the group comparison here? This feels brittle.
|
|
||
| // Current user does not "own" the current directory so we require superuser | ||
| if (user.name !== rootInfo.user || user.name !== rootInfo.group) { | ||
| options.superuser = "try"; |
There was a problem hiding this comment.
I'm not a fan of this approach, as it still creates files/dirs as root if you are in e.g. /home/someotheruser/ or /var/lib/postgresql/. IMHO a better default is to always create new files/dirs with the same user+group as the current directory. That requires some runuser wrapping with cockpit.spawn commands, but that feels not too hard.
There was a problem hiding this comment.
I'm not sure I like this — it kinda flies in the face of how UNIX is supposed to work. That's what the u+s and g+s bits on directories are for (and we should definitely respect them if they're set).
I feel like the correct behaviour is something like "do it as the unprivileged user if it works, otherwise do it as root" with no extra chmod/chown/etc. to make sure things happen in the most "natural" way possible.
There was a problem hiding this comment.
We won't be using chown/chmod either way but run it as the "intended" user ie:
runuser -u postgresql -- mkdir foo
Doing some try and error research, you need to own the directory to be able to make a sub directory or create a file. So checking the group is wrong and not needed.
So that leaves a decision to be made about what ownership the created file ends up if the user is not the owner of the directory.
There was a problem hiding this comment.
Nack on creating dirs/files as root in non-root owned directories. Doing that in someone's home directory is weird and wrong. If you don't like runuser, then I suppose the compromise is to not allow creating files or dirs in directories which aren't owned by cockpit.user or root? Can you live with that?
There was a problem hiding this comment.
Let's make a bunch of test cases which we should support and then reason about that so:
- Folder with
root:adminbut lacksg+wr - Folder with
root:userswithg+wruser should be able to write - User can create files because of ACL's
Last two should work as normal user, but what happens as superuser.
There was a problem hiding this comment.
I've revised the whole patch, dropped all the state shenanigans and simplified it.
I come to the conclusion that we have 3 (or more) options:
- always create as root and chown to parent
- try to mkdir, on failure use root and chown
- run test -w and then sudo / chown if needed
As runuser is not a solution if we expect a created directory in /var/lib/mock (root:mock) to be created as root:mock then we can't use runuser. Along with other cases where mkdir might fail as normal user but are possible as root like a shared dir:
drwxr-xr-x. 1 root users 0 May 16 09:28 shared
When logged in as administrator we would default to creating a directory as "root" even if it was in your own home directory. So instead use superuser only when required and always mkdir / chown the correct permissions when administrative access is enabled. Related: cockpit-project#390
|
@allisonkarlitskaya @martinpitt let's discuss this next Tuesday the 21st. |
| } catch (err) { | ||
| setErrorMessage(err.message); | ||
| return; |
There was a problem hiding this comment.
These 3 added lines are not executed by any test.
@allisonkarlitskaya @martinpitt I would love to discuss the expected behavior in #390 then I'll write some tests. The interesting part here is this commit 15c9b4f