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

Request: Bulk route filters with parameters #2078

Closed
MGatner opened this issue Jun 27, 2019 · 10 comments
Closed

Request: Bulk route filters with parameters #2078

MGatner opened this issue Jun 27, 2019 · 10 comments
Labels
new feature PRs for new features

Comments

@MGatner
Copy link
Member

MGatner commented Jun 27, 2019

Currently route filters can be applied in two places:

  1. in app/Config/Filters.php using $globals, $methods, or with URI patterns in $filters
  2. in app/Config/Routes.php as an option to any given route or route group, e.g. ['filter' => 'role:admin']

I would like to be able to use the versatility of URI patterns with filter parameters, but currently these are mutually-exclusive features. Entries in Filters.php cannot take parameters and Routes.php doesn't support filter options for non-specified routes. Using route groups we can apply a filter to any specified routes, e.g.:

$routes->group('manage', ['filter' => 'role:admin'], function($routes)
{
	$routes->add('users', 'App\Controllers\Manage\Users::index');
});

... but this requires that all routes be listed explicitly rather than allowing auto-routing.

I would like to see either (or both):

  1. parameter support added to Filters.php
  2. some way of creating a route group (with options) that will apply the options to autorouted matches in Routes.php
@MGatner
Copy link
Member Author

MGatner commented Jun 27, 2019

I think I was able to accomplish what I wanted with the following:

$routes->group('manage', ['filter'=>'role:admin', 'namespace'=>'App\Controllers\Manage'], function($routes)
{
	$routes->add('(.+)', '$1');
});

... but I'm going to leave this here until a few more eyes have seen it just in case.

UPDATE: This doesn't work. It runs the filters correctly on the route but then fails to match to the expected controller. This line from the User Guide made me think I could use dynamic controller routing:

$routes->add('products/([a-z]+)/(\d+)', '$1::id_$2');

... but maybe it doesn't work like that?

@hlohrenz
Copy link

hlohrenz commented Jul 9, 2019

I'm trying to add filters similar to what you are doing to staff routes. It seems as if the filter doesn't take when it's in the Config/Routes.php file like this:

$routes->group('staff', [
    'namespace' => 'Staff\Controllers',
    'filter' => 'permissions:access_staff'
], function($routes)
{
    // Routes listed here
});

I tried removing the parameters and just left it as permissions and it still doesn't initiate it. This is my Config/Filters.php file:

public $aliases = [
    'csrf'        => \CodeIgniter\Filters\CSRF::class,
    // 'toolbar'     => \CodeIgniter\Filters\DebugToolbar::class,
    'honeypot'    => \CodeIgniter\Filters\Honeypot::class,
    'auth'        => \App\Filters\Auth::class,
    'permissions' => \App\Filters\Permissions::class,
    'roles'       => \App\Filters\Roles::class
];

I added the alias for permissions and inside that filter class, I have a simple var_dump('hi');die; and it doesn't throw it. However, if I add the filter inside the filters array inside the Filters.php file like so:

public $filters = [
    'permissions' => [
        'before' => ['v1/staff/*']
    ]
];

It hits the var_dump line. However, I can't add parameters to it like this:

public $filters = [
    'permissions:access_staff' => [
        'before' => ['v1/staff/*']
    ]
];

It doesn't hit the filter :( In the Controller Filters part of the User Guide, it says we can pass parameters. Is this not the case? This is a pretty much necessary functionality of a Filter. I would hate to create a filter for EACH permission.

@MGatner
Copy link
Member Author

MGatner commented Jul 9, 2019

That's the essence of one resolution for this request, to allow parameters from Filters.php. Currently they are not supported, as you latter test exhibits.

Your first approach looks like it should work fine. Make sure your namespaces and URIs are all matching correctly (e.g. your route group matches on "staff*" whereas your global filter matches "v1/staff/*").

@hlohrenz
Copy link

hlohrenz commented Jul 9, 2019

Ah okay.
And the group inside the Routes file is actually wrapped inside a group for all v1 routes:

$routes->group('{locale}/v1', function($routes) {
  // All routes are wrapped inside this
});

@lonnieezell lonnieezell added the new feature PRs for new features label Oct 17, 2019
@jim-parry
Copy link
Contributor

Rhis should be discussed/resolved on the forum before showing up here.

@sfadschm
Copy link
Contributor

sfadschm commented May 1, 2020

UPDATE: This doesn't work. It runs the filters correctly on the route but then fails to match to the expected controller. This line from the User Guide made me think I could use dynamic controller routing:

$routes->add('products/([a-z]+)/(\d+)', '$1::id_$2');

... but maybe it doesn't work like that?

Dynamic controller routing indeed does not seem to work.
I took a look into this and it seems that in "system/Router/Router.php", line 470 prohibits this:
$val = preg_replace('#^' . $key . '$#', $val, $uri);

Here, $val is the routing target prepended by, e.g. App\Controllers\. If you try dynamically routing the controller, the full target then renders as App\Controllers\$1::index, which leads to the preg-replace function to interpret \$1 as an escaped $ sign and yields App\Controllers$1::index as the replacement result.

I'm not sure if this is intended behavior, however, I simply dodged this issue by inserting
$val = str_replace('\\', '\\\\', $val);
before the line mentioned above. So far, all tests I have conducted seem to work.

@crustamet
Copy link
Contributor

So this works, i tried this before the routes does not exists :(
All time these routes implemented by codeigniter 4 team is just not enough.
Like they only have to manually create routes for some automatically generated modules to work.

@crustamet
Copy link
Contributor

UPDATE: This doesn't work. It runs the filters correctly on the route but then fails to match to the expected controller. This line from the User Guide made me think I could use dynamic controller routing:

$routes->add('products/([a-z]+)/(\d+)', '$1::id_$2');

... but maybe it doesn't work like that?

Dynamic controller routing indeed does not seem to work.
I took a look into this and it seems that in "system/Router/Router.php", line 470 prohibits this:
$val = preg_replace('#^' . $key . '$#', $val, $uri);

Here, $val is the routing target prepended by, e.g. App\Controllers\. If you try dynamically routing the controller, the full target then renders as App\Controllers\$1::index, which leads to the preg-replace function to interpret \$1 as an escaped $ sign and yields App\Controllers$1::index as the replacement result.

I'm not sure if this is intended behavior, however, I simply dodged this issue by inserting
$val = str_replace('\\', '\\\\', $val);
before the line mentioned above. So far, all tests I have conducted seem to work.

Bro you are a genius, i don't know how you did that, it is working, i had to rewrite all my controllers with first letter lowercase because all my urls are with lowercase, this is working amazingly.

@sfadschm
Copy link
Contributor

sfadschm commented May 3, 2020

@crustamet : Please keep in mind that this is a change in the core files, so it will be reverted when updating CI and it also might not be intended behavior. So I would wait for someone official to comment on this.

@MGatner
Copy link
Member Author

MGatner commented May 3, 2020

The current thread is a hijack for a different issue. If there is a bug please open a new issue report. Feature Requests should be opened on the forums for discussion and then linked here if deemed appropriate. Please keep conversations on this topic to route filters.

FWIW dynamic Controller auto-routing was considered a security issue so intentionally is not allowed.

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

No branches or pull requests

6 participants