-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Do not allow invalid users to be created #14652
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
Conversation
|
🤖 beep boop beep 🤖 Here are the logs for the failed build: Status of 16946: failureDB=sqlite, ENABLE_REDIS=false, PHP=7.3Show full logTESTS=integration-federation_features
Show full logTESTS=acceptance, TESTS-ACCEPTANCE=app-files
TESTS=sqlite-php7.1-samba-non-nativeShow full log |
19142fe to
6b56980
Compare
MorrisJobke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good 👍
|
CI fixed in #14640 Status of 16951: killedDB=sqlite, ENABLE_REDIS=false, PHP=7.3Show full logTESTS=integration-federation_features
Show full logTESTS=integration-downloadTESTS=acceptance, TESTS-ACCEPTANCE=app-files
Show full log |
Signed-off-by: Roeland Jago Douma <[email protected]>
6b56980 to
969fc45
Compare
|
Retriggered CI. |
| private function verifyUid(string $uid): bool { | ||
| $appdata = 'appdata_' . $this->config->getSystemValueString('instanceid'); | ||
|
|
||
| if ($uid === '.htaccess' || $uid === 'files_external' || $uid === '.ocdata' || $uid === 'owncloud.log' || $uid === 'nextcloud.log' || $uid === $appdata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reject all UIDs for which there exists a file or folder in the data directory? Also, you forgot index.html, owncloud.db, owncloud.db-shm, owncloud.db-wal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reject all UIDs for which there exists a file or folder in the
datadirectory?
We then need to first check the existing user as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand.. What do you want to check, when?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because otherwise a valid UID is not valid anymore once the user is created and thus has a user folder in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem about having existing users with an invalid UID (which was valid before)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what about those additional files? And Why not reject all UIDs for which there exists a file or folder in the data directory??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what about those additional files? And
Why not reject all UIDs for which there exists a file or folder in the data directory??
Makes sense 👍 We should additionally keep the list here to avoid future collisions for not yet created log files or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, has this been implemented already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - mind to open a ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Just discussed with @nickvergessen:
|
|
/backport to stable15 |
|
/backport to stable14 |
|
backport to stable15 in #15071 |
|
backport to stable14 in #15072 |
Signed-off-by: Roeland Jago Douma [email protected]