search

jorijn / laravel-security-checker

Added Laravel functionality to Enlightn Security Checker. Adds a command to check for, and optionally emails you, vulnerabilities when they affect you.
https://jorijn.com
MIT License
198 stars 26 forks source link

Permission failure when using this package with multiple users on the same server #35

Closed thomasderoo4 closed 3 years ago

thomasderoo4 commented 3 years ago

Hello,

i've encountered an issue on the enlightn/security-checker with multiple users on a single server: https://github.com/enlightn/security-checker/issues/17 . I've since fixed the issue with the package creator. We've implemented a command line option to set the tmp-dir and implemented it into the SecurityChecker constructor.

Would it be possible to implement this tmp-dir option into your package either through a command line option or through a config entry?

Let me know what solution you prefer, and if you prefer to implement it yourself or if your want me to create a pull request.

Best regards, Thomas

jorijn commented 3 years ago

Hey @thomasderoo4

Please help me out with this one. It's a busy period.

As for the two options: I'm a big fan of keeping things simple. This means keeping the number of arguments, toggles, flags, and options to a bare minimum. My preference would be a config entry defaulting to the default value of Enlightn.

Thanks!

thomasderoo4 commented 3 years ago

Hello @Jorijn,

I've created a pull-request with, afaik, all the required changes. Also tested it here with enlightn/security-checker v1.7. Works as expected. Let me know what you think!

Best regards!

thomasderoo4 commented 3 years ago

I dont know why the tests on my pull request fail though... What do these tests do? Seems it has nothing to do with my code

jorijn commented 3 years ago

Hey @thomasderoo4

The SecurityChecker class is loaded through Dependency Injection. The tests fail because the classes now instantiate a new object, instead of using the DI container one.

https://github.com/Jorijn/laravel-security-checker/blob/master/tests/TestCase.php#L34

Please look into using the DI container for configuring the SecurityChecker object so injection through the constructor will work as intended.

Thanks, Jorijn

thomasderoo4 commented 3 years ago

Hey,

Sorry for that, never used dependency injection. This seems like a better solution. My pull request has been updated and all tests seem to pass.

Thanks!

jorijn commented 3 years ago

Looks good to me!

DI is great and will enable you to create cleaner, better testable, code. See https://stackify.com/dependency-injection/

I will draft a new release today.

jorijn commented 3 years ago

v2.2.1 is out now which includes your fix.

Thanks again. :-)

thomasderoo4 commented 3 years ago

Just installed through composer, all seems to work at our end! Thanks for the quick response!