Skip to content
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

Disabling rights verification on Windows #1202

Conversation

eugene-borovov
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

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.

@Sephster Sephster closed this Apr 18, 2021
@eugene-borovov
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants