Fix key files permission on Windows #1447
Conversation
| @@ -29,7 +29,7 @@ jobs: | |||
| uses: shivammathur/setup-php@v2 | |||
| with: | |||
| php-version: ${{ matrix.php }} | |||
| extensions: dom, curl, libxml, mbstring, zip | |||
| extensions: dom, curl, libxml, mbstring, sodium, zip | |||
There was a problem hiding this comment.
sodium is not enabled on Windows by default, it is required by lcobucci/jwt package.
| self::assertSame($permission, decoct(fileperms(__DIR__ . '/Stubs/public.key') & 0777)); | ||
| self::assertSame($permission, decoct(fileperms(__DIR__ . '/Stubs/private.key.crlf') & 0777)); | ||
| } | ||
|
|
There was a problem hiding this comment.
You may delete this test, I added this to prove the difference on Windows only!
| @@ -72,7 +72,7 @@ public function __construct(string $keyPath, protected ?string $passPhrase = nul | |||
| throw new LogicException('Invalid key supplied'); | |||
| } | |||
|
|
|||
| if ($keyPermissionsCheck === true) { | |||
| if ($keyPermissionsCheck === true && PHP_OS_FAMILY !== 'Windows') { | |||
There was a problem hiding this comment.
Ignore key permissions check on Windows, as it always will be '666' or '444'.
|
We've had a similar PR for this submitted in the past. It was rejected on the basis that the implementer can ignore these checks if need be by setting the appropriate construct argument to false. This change does mean that regardless of this flag, on windows we will ignore it. This is potentially not obvious to the end user and while nothing bad will come of it, it doesn't feel right. Why can't this option be exposed in Laravel so folks can turn it off if need be? |
The ignored check is not working on Windows anyway. The tests are always failing on Windows without this change.
On Passport, we had to turn this off on Windows, but it doesn't feel right either! What if additional checks added in future, or the code changed in a way that works on Windows? Feel free to close this PR if it was not convincing. We will continue to ignore this for Windows on Passport for now, so it's not a blocking pull request. |
|
Thanks @hafezdivandari - I hadn't really considered testing against Windows so this does make sense. Will merge on that basis. Cheers! |
Closes #901
Related to laravel/passport#1789
This PR adds windows tests and fixes the permission check on Windows. The possible file permissions on Windows are '666' and '444'.