search

thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.52k stars 1.12k forks source link

Disabling rights verification on Windows #1202

Closed eugene-borovov closed 3 years ago

eugene-borovov commented 3 years ago

I am a Windows user, so the Linux rights check does not work in development environment and does not allow me to run tests. I also have to configure enabling rights verification in production environment separately. So I added a Windows check.

NikoGrano commented 3 years ago

I don't understand why are you fully disabling it. It's anyways user configurable option and you should configure it by yourself. Forcing something like this in library stage is just wrong approach.

eugene-borovov commented 3 years ago

Because this check doesn't make sense on Windows. Not applicable for in-memory keys. This check has no place in this class at all. If you check the access rights to the key file, then you should consider all options, including working on Windows, isn't it?

Sephster commented 3 years ago

@eugene-borovov sorry but why can't you just set the key permission check to false if it doesn't apply to you like @NikoGrano suggests? I appreciate this check will never work for Windows users and it is likely we will remove this check entirely in future as I think is probably not the responsibility of this package but equally, I don't want to increase the cognitive load of devs when looking at the code unnecessarily.

eugene-borovov commented 3 years ago

@Sephster You think more about the developers than the users of the library. :)

Let's take a look from the user's side. The user read the documentation, configured some application, and got an error. It's scary. The meaning of the error is that the library with the default settings does not work on Windows. Let's not scare the users. If permission checking was disabled by default, then the Windows user who enabled it and received the error message would know how to disable it. But changing the default behavior is fraught with consequences.

So I suggested changing the behavior of the library on Windows. And I'm not the first #779. There is a similar PR #901. But in this PR check is more optimal. Another solution to this "problem" is to generate a different error message for Windows users. IMHO if some functionality is not available in a certain environment, then it should be blocked if it does not affect the operation of the entire system as a whole.

PS: I have to disable permission checking not because I want to, but because it doesn't work on Windows.

Sephster commented 3 years ago

Honestly our Windows userbase is pretty small so in the few years we've had this feature in place, we've got very little feedback about it causing issues bar this and the one ticket you posted way back (if memory serves). There is already a way to turn off this check. We could add in OS based flags but this feels overkill to me for now because, as I've stated previously, we will likely just remove these checks in future.

I'm also not sure if this flags will apply to all other OS'es or not. If they don't, I don't want to go down the route of adding in even more conditionals when we have a way to disable this.

Apologies as I know this isn't the answer you wanted but I think we will leave this PR for the timebeing. Thank you for your submission though and sorry I'm not moving forwards with it this time.

eugene-borovov commented 3 years ago

I guess there is little feedback to you due to the fact that laravel/passport just disables this check.

Just for information:

PHP_OS_FAMILY (string) The operating system family PHP was built for. One of 'Windows', 'BSD', 'Darwin', 'Solaris', 'Linux' or 'Unknown'. Available as of PHP 7.2.0.

Thank you for your time.