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

Provide a secure route that calls the artisan cron:run command #21

Closed
ameenross opened this issue May 7, 2014 · 8 comments
Closed

Provide a secure route that calls the artisan cron:run command #21

ameenross opened this issue May 7, 2014 · 8 comments
Assignees

Comments

@ameenross
Copy link
Contributor

Follow-up of #20

The package should provide a route that calls Artisan::call('cron:run'). This should of course have some form of protection to prevent anyone from accessing the URL inadvertently, potentially causing issues. So my idea is that there should be an automatically generated security token, which will be saved to a configuration file.

A few things to consider (hey, I don't have all the answers):

  • What would the URI path of the route be, so as to not clash with anything existing?
  • How to reconcile with the general idea that routes should be created by the app developer as opposed to packages? Possibly:
    • Use configuration to determine the URL, with default being disabled completely to prevent clashing?
    • Leave the creation of the route to the app developer anyway, but provide example code that includes the security token validation?
  • How to generate configuration values while the app developer runs php artisan config:publish liebig/cron?
@liebig liebig self-assigned this May 8, 2014
@liebig
Copy link
Owner

liebig commented May 8, 2014

I like your idea and this will make Cron easier to use. It is difficult to generate a token and write it into a config file. You need write access and writing a value to a specific position in a file is no fun with php. If Cron is used with an external service, in most cases you can only define the url you call, not the post values or other things (like headers). So we can only use the url with all get parameters to secure the call. I would prefer to use the route only.

Because a generated long route definition could potentially crash with other definitions, I would recommend to let the Cron user define this route in the Cron config file. Maybe we can offer a command which generates a long route string (but does not write it into the config file).

So the steps to use Cron would be to install it, activate the route and/or command function in the config file and maybe set the route string for Cron. Now you can define the jobs in the start/global.php or start/develop.php if this environment is defined. What do you think about that?

@ameenross
Copy link
Contributor Author

It is difficult to generate a token and write it into a config file. You need write access and writing a value to a specific position in a file is no fun with php.

I was hoping there was some kind of Laravel function to hook into, which takes care of it. I haven't found it if it exists though.

So we can only use the url with all get parameters to secure the call. I would prefer to use the route only.

Yes, Drupal does it with a GET parameter, here's an example from their docs: http://www.example.com/cron.php?cron_key=0MgWtfB33FYbbQ5UAC3L0LL3RC0PT3RNUBZILLA0Nf1Re (heh, I only just noted that this cron_key is not entirely random).

If we were to use a GET parameter (which I prefer), then a configuration file could be provided with a cron_key property, which is empty by default. The route will not work if it has not been set. The documentation should provide a way to generate a cron key, which the app developer should copy to their configuration file. Then you're set I think.

@liebig
Copy link
Owner

liebig commented May 8, 2014

I found some functions which help to edit files: Illuminate/Filesystem/Filesystem.php. But there is no function to edit a config value. So we have to write this ;) Laravel does the same with the application key and I like the idea that you can configure everything with commands.

When Drupal use a GET parameter, we should use it, too. A route like /cron.php?cron_key= sounds good. If the key is not set or is not right, we should return a 404.

To simplify Cron we could append the following to the start/global.php if the route and/or command job execution would be activated.

/*
|--------------------------------------------------------------------------
| Cron job definitions
|--------------------------------------------------------------------------
*/
Event::listen('cron.collectJobs', function($rundate) {
    // TODO add Cron jobs with Cron::add here
});

After that the Cron user needs only to add his jobs and configure crontab (or external service). What do you think?

@ameenross
Copy link
Contributor Author

I found some functions which help to edit files: Illuminate/Filesystem/Filesystem.php. But there is no function to edit a config value. So we have to write this ;)

I was thinking of something more high-level. I think it's doable to create a command that produces a config file with default values and a generated cron key. Not sure if that's really worth the hassle though. The cron_key can just be empty by default (which would make the route return 404 no matter what), and the app developer instructed to do a one-off php artisan cron:keygen and copy the result to their config file (or their ENV, however they like to setup their configuration).

To simplify Cron we could append the following to the start/global.php if the route and/or command job execution would be activated.

I don't think that would be a good idea. App developers should be skilled enough to copy such code from the README and paste it where they need it.

After that the Cron user needs only to add his jobs and configure crontab (or external service).

They can already put this in their crontab:

* * * * * php /path/to/laravel/artisan cron:run

The above also has the advantage of not being limited by a webserver context (think max_execution_time). The route will additionally allow setting up remote services.

@liebig
Copy link
Owner

liebig commented May 9, 2014

Okay, I have to do the following:

  • add cron_key to the config file
  • create keygen command
  • adding route
  • changing the README

Did I forget anything?

@ameenross
Copy link
Contributor Author

I guess that's about it. Of course the cron key in the default config file should be empty, and a code comment with a short explanation of the use of cron:keygen would be helpful.

@liebig
Copy link
Owner

liebig commented May 14, 2014

The features were added. Please test it (the route url is laravel/public/cron.php?key=YOURCONFIGKEY). I will write test cases and will change the README. I think I have to completely rewrite the README file and I want to generate PHPDoc for the API. So please be patient :)

@ameenross
Copy link
Contributor Author

Very nice, it works! Thx for implementing! Hope you can tag a new release soon :)

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

No branches or pull requests

2 participants