Nexmo / nexmo-laravel

Add Vonage functionality such as SMS and voice calling to your Laravel app with this Laravel Service Provider.
MIT License
317 stars 79 forks source link

Support private key as environment variable #38

Closed dwightwatson closed 4 years ago

dwightwatson commented 4 years ago

If the environment variable NEXMO_PRIVATE_KEY is the private key, rather than the path to a file that is the private key it should just be used as-is. I'm not sure if there is a better way to determine if a string is a private key but using Str::startsWith() is a pretty simple check.

Supporting this will make it much simpler to use private keys, but also makes the configuration much more in line with Laravel conventions.

dragonmantank commented 4 years ago

Thanks for the PR!

I like the idea of this, though for this to work will take some work on the developer end converting a private key into a single-line string and converting newlines to \n, so I'm wondering how much of a use case there is for this. On one hand I do see making it easy to inject a private key as an environment variables, since that is a deploy-time config, but on the other hand this can be solved by just pushing the key as a file into the environment.

Basically I'm not shooting down the idea, just need to mull on it a bit :P

Can you also add some tests around this?

dwightwatson commented 4 years ago

I'm not sure that's entirely correct - I am currently loading the multi-line private key through both my .env file and through environment variables in production without issue. I set it up in my own service provider, but now that Laravel pulls this package in with the Nexmo channel it would be great to have it work out of the box.

$credentials = new Keypair(
    config('services.nexmo.private_key'), config('services.nexmo.application_id')
);

return new NexmoClient($credentials);

Also - I don't even know how I'd "push the key as a file into the environment". How would I achieve that using Laravel Forge/Envoyer, or what about Heroku? That's just additional complexity, and I could imagine people just copying their private key into the repo as an easy solution - which isn't ideal at all. Supporting it as an environment variable just makes it that little bit easier to get up and running - hope that's a good sell!


I did just have a go at adding tests... but this hard-coded line in loadPrivateKey means I can't really test the functionality (and that the current file-loading functionality isn't tested either).

if (app()->runningUnitTests()) {
    return '===FAKE-KEY===';
}
dwightwatson commented 4 years ago

Any additional thoughts here - or anything else I can do to help get this through?

dragonmantank commented 4 years ago

This works well. I just tested with various linebreaks and such and was able to send phone messages, so this is all good.

Thank you!