Skip to content

Send CryptKey instance to oauth2 with new permission check disabled. #454

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

Merged
merged 1 commit into from
Aug 4, 2017
Merged

Send CryptKey instance to oauth2 with new permission check disabled. #454

merged 1 commit into from
Aug 4, 2017

Conversation

valeryan
Copy link
Contributor

@valeryan valeryan commented Aug 4, 2017

This will fix #453 that is caused by the changes to thephpleague/oauth2-server from here thephpleague/oauth2-server#776.

This disables the new permission check that was added in version 6.0.2 of oauth2-server. I feel like the permission check should be controlled through configuration and not just be statically disabled but passport does not currently have any need for configuration and it would be a pain to add configuration for one setting. So I think some decisions still need to be made on how to handle this change. However, this is a quick solution that restores passport to working order.

@prageeth
Copy link

prageeth commented Aug 4, 2017

I was just about to create a patch too 👍 Hopefully the PR gets merged and released soon.

@cringerjs
Copy link

The permission checks introduced completely break windows development as their files will always return 0666. Even though a PHP E_USER_WARNING is thrown, laravel's error handler is converting the user_notice from oauth2 to an exception.

Hope this is pulled in soon. Having to modify the PassportServiceProvider from my vendor folder just to get past this so I can resume development on my app. 🎩

@adiachenko
Copy link

I'm in favor of this. The new permission check breaks our Docker for Windows setup because chmod doesn't work on bind-mounted volumes.

@alexbilbie
Copy link
Contributor

We'll get this merged and a point release out ASAP

@taylorotwell
Copy link
Member

@alexbilbie so I should merge this?

@valeryan
Copy link
Contributor Author

valeryan commented Aug 4, 2017

@taylorotwell @alexbilbie not that my opinion matters, but I think the better solution might be to figure out how to let the oauth user_notice not be an exception so that we don't have to disable the security check. This is my knee jerk fix to get thing working again for windows folks and it might be an okay interim fix while you guys hash out what to really do to fix this permanently.

@alexbilbie
Copy link
Contributor

The main issues have been the following:

  • Windows server incompatibilities with chmod at both a OS and PHP level
  • Deployment difficulty with modern tooling like Kubernetes Secrets/Docker Secrets
  • The OAuth library trying to automatically fix the incorrect permissions issue which many think is outside the scope of the library.

The OAuth library now no longer tries to automatically fix the issue - only warn the developer that something isn't correct.

If Laravel converts E_USER_NOTICE to exceptions then that is an issue for Passport/Laravel to solve not the parent library.

@arikh
Copy link

arikh commented Nov 23, 2018

I am using laravel 5.3 in a project which is 1 and a half years old and recently started using Laravel Passport.
The passport version which is compatible with laravel 5.3 is "laravel/passport": "~1.0". When I installed it, it gives me the same error mentioned above. Am I doing something wrong? Or could you please tell me which would be the correct version of passport to be used for Laravel 5.3?

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

Successfully merging this pull request may close these issues.

7 participants