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

Inmemory cryptokey #1196

Closed eugene-borovov closed 3 years ago

eugene-borovov commented 3 years ago

Added the ability to use cryptographic keys stored in memory.

eugene-borovov commented 3 years ago

This is basically a duplicate of #1180 except that key creation is deferred until used. This is done so that when the JWT configuration is centralized, it would be easier to take the key creation to the factory and leave the CryptoKey as a DTO. Plus, the tests are updated to use a key from a file or from memory.

Sephster commented 3 years ago

Thanks for the PR @eugene-borovov - There is a lot that has changed in this PR such as the file permissions etc. I'm going to try and push through #1180 instead as the PR is a bit more focused. If you believe the changes here warrant inclusion in the repo, I'd welcome a PR after #1180 has been merged in. Thanks for your contributions here and sorry I'm not progressing this at the moment.

eugene-borovov commented 3 years ago

The bulk of the changes are in-memory key usage tests. I can remove these tests if they are redundant or confusing. In any case, this is a temporary solution until the JWT configuration is moved to the dependency container #1154.

Sephster commented 3 years ago

Thank you for the offer but we will try and get #1180 merged in and then add any additions from that point. Thanks for the offer of additional work here though, it is appreciated.

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.

Please consider including Windows verification in the file permissions check code.

Sephster commented 3 years ago

The windows code is something we will likely remove in future because it is problematic and I'm not convinced this library should be checking file permissions.

Happy to review such a change but this PR was supposed to be for adding memory crypto keys. I think the inclusion of the Windows permissions change means this PR is doing too many things and makes it harder to review.

It will be more likely PRs are accepted if they are focussed on succinct small changes because it takes me a long time to review code, to ensure it complies to all the various RFCs this library has to conform to.

Thanks for your understanding in this matter.

eugene-borovov commented 3 years ago

Thank you for advice. I will try to make future PRS small and focused on one goal.