This commit introduces a new config variable, files.maximum-file-size.
It affects newly uploaded files, preventing them from being uploaded if
they exceed the specified file size.
Partially resolves T15972
Details
Details
- Reviewers
- None
- Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15972: Add config option for maximum file size
- Set files.maximum-file-size to zero or a negative value
- Upload any file via /file/upload (should work)
- Upload any file via /T1 -> the "Upload File" button when submitting a comment (should work)
- Set files.maximum-file-size to 524288
- Upload a file <= 512 KB via /file/upload (should work)
- Upload a file <= 512 KB via /T1 -> the "Upload File" button when submitting a comment (should work)
- Upload a file > 512 KB via /file/upload (should not work)
- Upload a file > 512 KB via /T1 -> the "Upload File" button when submitting a comment (should not work)
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- master
- Lint
Lint Passed - Unit
Tests Skipped - Build Status
Buildable 1661 Build 1661: arc lint + arc unit
Event Timeline
| src/applications/files/storage/PhabricatorFile.php | ||
|---|---|---|
| 330 | I outlined this function for future expansion, in case if anyone (either us/upstream, or a downstream local fork) wants to add a extra functionality such as exemptions based on the author's joined projects or a policy object. Therefore, I had to call PhabricatorEnv#getEnvConfig() twice. (Well, technically, I could pass it from buildFromFileData(), but that felt a bit too hacky for me) | |
| src/applications/files/storage/__tests__/PhabricatorFileTestCase.php | ||
| 607 | Best case scenario is probably to initialise the engine only once for this test case, but honestly I didn't know how to do that (aside from a class variable, which felt a bit much, or through a closure, which I'm not sure if it's okay) | |
| src/applications/files/storage/PhabricatorFile.php | ||
|---|---|---|
| 330 | If you are bold enough, you can convert this into an "assert" approach like self::assertNotExceedingSizeLimit($size) And have that method throwing in case. So the exception message is recycled too. (P.S. normally we set non-static method but in this case it's maybe OK, since in the future other things may want to call that from outside) | |
| src/applications/files/storage/__tests__/PhabricatorFileTestCase.php | ||
|---|---|---|
| 607 | I guess that's OK. Just like other examples | |
Comment Actions
Move assertion to a different function
To reduce duplicate calls for PhabricatorEnv#getEnvConfig, the code now
outsources throwing the exception to a new function called
"assertNotExceedingSizeLimit", which also fetches the config value of
files.maximum-file-size for the file size comparison.
| src/applications/files/storage/PhabricatorFile.php | ||
|---|---|---|
| 330 | That's a good idea actually, thanks! | |
| src/applications/files/storage/__tests__/PhabricatorFileTestCase.php | ||
|---|---|---|
| 607 | Maybe private method | |
| src/applications/files/storage/__tests__/PhabricatorFileTestCase.php | ||
|---|---|---|
| 607 | Assertion failed, expected values to be equal (at PhutilTestCase.php:414): Test case '512 KB' was expected to succeed, but it raised an exception of class RuntimeException with message: call_user_func() expects parameter 1 to be a valid callback, cannot access private method PhabricatorFileTestCase::newRandomFile() | |
| src/applications/files/storage/__tests__/PhabricatorFileTestCase.php | ||
|---|---|---|
| 607 | LOL ok sorry <3 | |
Comment Actions
Ouch we should maybe already skip this limit if the user has $user->isOmnipotent(), otherwise some daemons may crash.
That means "Uff", extra logics to actually receive an User object 🐇 🐇 🐇 and I don't know how to do that lol
Comment Actions
Hoping to help: maybe maybe, we can be prepared to receive that User $actor but: null as default \o/ and we can just skip this additional limitation if it's null, assuming that null = SomebodyImportant™
With this extra care we should be able to introduce zero regressions but extra limitation applied for lusers
Comment Actions
$params has an optional authorPHID key, whose value is... well, the PHID of the author (if applicable). I suppose we can reuse that here?
...but it seems like uploading a profile picture does not set it. It depends on what the purpose of this is for I suppose.
Comment Actions
I wonder if we can expand that to provide an authorUser too, in many places as possible 🤔
...but it seems like uploading a profile picture does not set it. It depends on what the purpose of this is for I suppose.
\o/ You've probably also discovered the root problem of T15407: People: profile picture should be editable by their author (not by "No one")
Turns out that this doesn't work if the file is uploaded via chunking
So maybe that's a good news. Maybe it means we should move the logic inside PhabricatorFileUploadSource() that is more high-level (?) but we still need to add some setAuthor() methods
We are inside a rabbit hole my friend lol
Comment Actions
I wonder if we can expand that to provide an authorUser too, in many places as possible 🤔
but i'm lazyyyy :(
\o/ You've probably also discovered the root problem of T15407: People: profile picture should be editable by their author (not by "No one")
Oh no
So maybe that's a good news. Maybe it means we should move the logic inside PhabricatorFileUploadSource() that is more high-level (?) but we still need to add some setAuthor() methods
Tried that, chunking still bypasses it lol
We are inside a rabbit hole my friend lol
Yes, yes we are.