yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.23k stars 6.91k forks source link

Widgets as action providers? #623

Closed cebe closed 11 years ago

cebe commented 11 years ago

I've been using this feature a lot in Yii 1.1 and I am wondering if this is already implemented in a way in yii2 or is yet to be implemented. Haven't found the code for it so far...

creocoder commented 11 years ago

Why not stay separate widget and action? As i see current framework flow is to be easier and simplier and this is right.

creocoder commented 11 years ago

@cebe But may be there is some usecases that i do not see. Can you show some examples from this

a lot

? :)

slavcodev commented 11 years ago

No, thanks.

cebe commented 11 years ago

No, thanks.

@slavcodev what do you mean?

@creocoder Will provide use cases and examples later.

cebe commented 11 years ago

This discussion is related as it tries to achive similar thing via behaviors: http://www.yiiframework.com/forum/index.php/topic/10652-actions-by-behavioring/page__st__20

slavcodev commented 11 years ago

Because I guess is unnecessary feature. Importing actions from behavior so simple

class PostController extends Controller {
  public function actions() {
    $actions = parent::actions();

    if ($this->asa('someBehavior')) {
         $actions = array_merge($actions, $this->someBehavior->actions());
    }

    return $actions;
  }
}
creocoder commented 11 years ago

This discussion is related as it tries to achive similar thing via behaviors: http://www.yiiframework.com/forum/index.php/topic/10652-actions-by-behavioring/page__st__20

I dont like such ideas at all. I'm trying to avoid behaviors if there is any alternative solution. Along action classes is more reusable. I do not see any reason to provide actions by behavior.

creocoder commented 11 years ago

@cebe As for me widgets as action provider violate SOLID principles.

cebe commented 11 years ago

Another con argument: http://www.yiiframework.com/wiki/146/how-to-use-a-widget-as-an-action-provider/#c13950

Ragazzo commented 11 years ago

very custom feature, also it is very simple to implement, dont think that must be in the core. users can do it by themselves, core code must be clean and explicit.

qiangxue commented 11 years ago

Yes, the concept of action provider is dropped in Yii 2 in favor of more straightforward code. The situation is a bit like the Captcha widget which uses a separate action class to achieve the goal.

bwoester commented 11 years ago

I'm not really convinced by the current solution. While I appreciate the idea of separating widget actions into their own classes, I don't like the idea of having to modify my controllers (to import these actions) only because I decided to use another widget in the view. In a more advanced use case, an application could be themed. Theoretically, the themed views could use a whole other set of widgets than the original views. This shouldn't affect my controller as long as those different widgets can manage to work with the same data the controller already passed to the initial views.

So isn't there a way to execute actions without a controller? This way widget actions wouldn't be needed to be imported. Widgets were truly self-contained.

While I understand that a possible solution shouldn't be too different from the normal request workflow, the constraint that an action must always be run in a controller seems to cause problems in this use case. Maybe some sort of a generic WidgetController could help. Something that lazily attaches matching actions to itself before being executed. Sounds somewhat exotic though. Maybe someone can come up with a better idea? :)

qiangxue commented 11 years ago

The problem is that before a widget runs, how to register itself to respond to the routing process?

bwoester commented 11 years ago

Not sure if I understand you. Did you mean how to register the widget's actions? Or do widgets themselves register somewhere?

I implemented a simple ActionController that acts as a dummy controller for arbitrary actions. It allows you to run an action by requesting an URL like index.php?r=action/{actionClassName}. I tested it with the captcha widget and it seems to work (no more need to import the captchaAction into SiteController). However, I'm not sure if this approach is a good idea...

But anyway, here's the snippet: (Captcha and CaptchaValidator need to use action/yii\web\CaptchaAction as captchaAction to make it work without specifying the action in SiteController)

// file ActionController.php
<?php

namespace app\controllers;

use Yii;
use yii\web\Controller;

/**
 * Dummy container for actions to be run without being attached to any controller.
 * 
 * To run an action without attaching it to one of your controllers, you can
 * use the ActionController. The controller part of the route is "action", the
 * action part of the route must be the classname of the action to run.
 * 
 * Example - run captcha action:
 * index.php?r=action/yii\web\CaptchaAction
 */
class ActionController extends Controller
{

    /**
     * Creates an action based on the given class name.
     * 
     * @param string $class the class name of the action
     * @return Action the newly created action instance. Null if the class name couldn't be resolved.
     */
    public function createAction($class)
    {
        if (class_exists($class) && is_subclass_of($class,'yii\base\Action')) {
            return Yii::createObject( $class, $class, $this );
        }

        return null;
    }

}
qiangxue commented 11 years ago

Yes, you understood me correctly.

Your snippet is interesting, but it has a security hole: imagine your app allows users to upload files, and I uploaded a malicious PHP file, and then attempt to call the action that would resolve to this file.

I think basically the requirements are:

  1. A widget needs to have a way to automatically register one or multiple actions somewhere so that the routing system can dispatch requests to them;
  2. The actions being registered may need to be customized/configured (e.g. CaptchaAction);
  3. We should not allow executing/including arbitrary PHP files for security purpose.
samdark commented 11 years ago

Current actions method fits perfrectly except automatics. I don't think 1 and 2,3 are possible at the same time.

bwoester commented 11 years ago

How about signing routes to the ActionController when they are created, and verifying the data in createAction? This way, it wouldn't be possible any longer to execute arbitrary actions by simply pointing to a certain URL. But when the link was created from within the app, it contains the signature and thus can execute the action.

I think such a mechanism would also be necessary for requirement 2. If you want self contained widgets, you don't import their actions explicitly. This also means, you have no way to explicitly configure those actions. The only thing you instantiate is the widget, where you could configure something. If you want to configure one of the widget's actions, you had to pass this configuration to the action when you call it. At this point, you need to ensure nobody tampers with the configuration. Maybe, it also contains sensitive data, so another requirement might be to encrypt the config.

qiangxue commented 11 years ago

The idea of signing routes is interesting. How secure do you think this would be? We possibly need additional security measures to prevent including arbitrary PHP files.

I think there's no need to pass detailed configuration via URL. Instead, we can use a configuration hash ID and save the corresponding configuration somewhere on the server, such as runtime. When the action is requested, it can convert the config ID into detailed configuration.

qiangxue commented 11 years ago

A new idea:

  1. When a widget runs, it will save an action configuration array (including action class) in a file under runtime/widgets. The file name is the hash value of the configuration array. If a file with the same name already exists, it will do nothing (for performance reason).
  2. The widget uses a special route to reference the action, and the hash value is appended as a parameter to the URL.
  3. Yii provides an internal controller to handle the special route. The controller will use the hash value to find the corresponding action configuration array and use that to handle the request.

In the above, user may also explicit set the name of the action configuration file, instead of using the hash code. If this is the case, the widget should check if the configuration changes or not to decide whether it needs to overwrite the existing configuration file.

Let's use the captcha widget as an example.

What do you think about this proposal?

samdark commented 11 years ago

If a file with the same name already exists, it will do nothing (for performance reason).

It will be very confusing during development and after production updates if one will forget to clean it up.

The widget Captcha will support configuration options for CaptchaAction. So users will configure the captcha in a single place.

What if I have 5 pages with captcha and want to configure all these at once (colors, characters etc.)?

What do you think about this proposal?

I do not understand why it's better than current approach with separate actions.

andersonamuller commented 11 years ago

Why don't add the actions in the WidgetFactory (or something alike) config, defining 'actionID' => [ 'WidgetClass', 'widgetMethod' ], routes can the point to this action IDs

qiangxue commented 11 years ago

It will be very confusing during development and after production updates if one will forget to clean it up.

What confusion will it cause? The configuration array is always up-to-date. I was just describing the strategy to make sure the performance does not degrade.

What if I have 5 pages with captcha and want to configure all these at once (colors, characters etc.)?

This requirement is the same as other widgets for which you want to use the same set of property values when they are used in different places. This can be done via Yii::$objectConfig.

I do not understand why it's better than current approach with separate actions.

The goal is to separate the concerns and simplify the usage of widgets that need the cooperation of some actions. Without this feature, you would have to

samdark commented 11 years ago

OK. Overall it makes sense.

What confusion will it cause?

Widget code is updated while action routes aren't.

klimov-paul commented 11 years ago

When a widget runs, it will save an action configuration array (including action class) in a file under runtime/widgets

Using files in “runtime” directory will fail once project is deployed at any Cloud Hosting such as Amazon. Files saved at one web instance will be not accessible at the other one.

The goal is to separate the concerns and simplify the usage of widgets that need the cooperation of some actions.

I am afraid this is not enough. Check the CAPTCHA. It affects not only widget and controller, it also affects the model via CaptchaValidator.

klimov-paul commented 11 years ago

Also do not forget about Access Control. Widget actions should provide the ability to setup access level.

qiangxue commented 11 years ago

Widget code is updated while action routes aren't.

Are you referring to the hash code? Note that the route is fixed, e.g. always _action. The hash code is only related with the configuration content. It has nothing to do with the widget code.

Using files in “runtime” directory will fail once project is deployed at any Cloud Hosting such as Amazon. Files saved at one web instance will be not accessible at the other one.

Yes, this is the biggest concern, I suppose. But we can have alternative storage solutions, such as APC.

I am afraid this is not enough. Check the CAPTCHA. It affects not only widget and controller, it also affects the model via CaptchaValidator.

Yes, I have explained how this will work with captcha. Instead of using route to link CaptchaValidator with CaptchAction, we use the hash code of the configuration. Unless you want to use different fonts/background etc. in the same application, there's no extra configuration requirement for CaptchaValidator.

qiangxue commented 11 years ago

Also do not forget about Access Control. Widget actions should provide the ability to setup access level.

This can be achieved by configuring the internal controller handling these widget actions.

bwoester commented 11 years ago

The idea of signing routes is interesting. How secure do you think this would be?

It's pretty much a standard operation when you look at messaging, so I don't think the approach has flaws. Our message would be an URL. It might come down to choosing the correct algorithm. With this, we could be sure the ActionController only does any work when the issued link has been generated by the application and hasn't been modified. However, if the application could be hacked in a way that allows the attacker to generate and sign arbitrary links, we're back at zero.


About the new suggestion: It's a bit like persisting one big controller that gets new actions attached whenever a widget runs with a new action configuration, right?

Problem with the config hashes is, you can't reference the actions any longer if you don't know the hash itself or the action configuration to recalculate the hash. And using named actions means, you can only persist one configuration for each action.

The problem with the runtime folder and cloud hostings is a global one, nothing specific to this issue. Maybe there's need for a general persistence layer?

The fact that CaptchaValidator requires an action to do its work makes me wonder if it isn't a design issue. Maybe there should be a CaptchaModel implementing the logic, which could be used by both the action and the validator.

Access control could be a problem imo. At least I can't think of a solution that covers multiple action configurations right now. It could be part of the widget configuration (where you configure the action). In the view. Not sure about that...

qiangxue commented 11 years ago

It's a bit like persisting one big controller that gets new actions attached whenever a widget runs with a new action configuration, right?

Yes.

And using named actions means, you can only persist one configuration for each action.

You can have multiple configurations for the same action, provided you name them differently (e.g. you use Captcha in different pages with different configurations). In most cases, you don't need to use named actions, however. Captcha needs this only because CaptchaValidator needs such a reference

Access control could be a problem imo...

The internal controller can be implemented as an application component. You can customize how action configurations are stored. You can use it to retrieve the reference of a configuration. You can also configure access control of the actions here. You may refer to an action using the explicit name if you want to control a single instance, or its class name if you want to control all its instances.

IMO, the main problem is whether this feature is too automagical. On one hand, I feel this feature will make using widgets that require an action much easier, as explained here. On the other hand, because the widget is so easy to use now, it also means hiding some details and could cause unexpected headaches for new users.

bwoester commented 11 years ago

Talking about automagical behavior: I guess often, a widget's actions should inherit the access control rules from the action that caused the widget to render. Wouldn't make much sense to place a widget in a view that I can access, only to see it not working, because it defines different rules. Duplicating them isn't nice either.

There might be exceptions though. Imagine a file manager widget. It contains actions to browse the file system and actions to upload new files. While viewing the page with the widget and browsing might be allowed to a larger user base, uploading might be restricted to admins only. So inheriting access rules alone won't be enough. There needs to be mechanism to extend or overwrite inherited rules.

creocoder commented 11 years ago

@bwoester Why not simple take this widget into:

<?php if (Yii::$app->user->checkAccess(...)): ?>
    // widget call here
<?php endif; ?>
bwoester commented 11 years ago

Consider you allow uploading only from a certain IP range (from within your company network). Access control rules not only support RBAC. Also, you really want to render the widget, only with different features depending on the current visitor. So you might have something like:

// however you decide this. Could as well come from controller.
$allowWrite = Yii::$app->user->checkAccess(...);

// render, but configure the feature set
$this->widget(FileSystemWidget::className(), array(
  'allowWrite' => $allowWrite,
));

Now, this will register... lets say a ListFolderContentsAction. So the user is not allowed to write, only to read. The action id will be some hash as suggested by qiangxue. Maybe 123abc. So the action can be executed by requesting index.php?r=_action&hash=123abc&folder=/foo. The widget knows this link, because it calculated it depending on its configuration. But the user (he is in some privileged group or currently in the company network) can also learn that link simply by looking at his network console. If he gives the link away (email to a friend), his friend could also use it to list the contents of the folder. Or the user could keep using the link from outside the company network. Or after he has been removed from the privileged group. That's why the action really must support access control rules by itself and must not rely on the setup of the page that rendered the widget.

qiangxue commented 11 years ago

Access control could be done at controller level (coarser level) or at action level (finer level). The latter is not a problem for this new proposal. The former could be solved by using explicit action ID and configuring it with the internal controller.

The main issue about access control is that the developer may think the action naturally inherits the security filter applied on the widget while it is actually not. Without this feature, the developer would have to explicitly declare the action and would thus notice the necessity of access control of the action. This is one of the side effects of introducing this feature.

bwoester commented 11 years ago

I wonder if it is feasible to define access control rules for a widget's actions. The main idea was, that when using a widget, you shouldn't care about what actions it uses to accomplish its tasks. They're part of the widgets internals. Instead, it might be better to define access control rules on a per feature base.

Taking the file manager widget from above as an example, it should be sufficient to define access control rules for 'read-only mode' and for 'read-write mode'. The widget could then care about assigning those rules to the appropriate actions, even if a new version of the widget introduces new actions for its internals. Maybe version 1.1 of the FileManagerWidget introduces a PreviewFileAction, which could then reuse the rules associated with 'read-only mode'.

qiangxue commented 11 years ago

For the file manager example, I don't see problem with access control because it can be done at the action level. The action should be aware of the current user and the file/folder that the user is trying to access. And it can determine the access permission from some persistent storage.

bwoester commented 11 years ago

Maybe the file manager is just a bad example. What do you think about new widget versions introducing new features/ re-factoring their actions? If you look at them as internals of the widget, you really shouldn't care about configuring them separately.

qiangxue commented 11 years ago

@bwoester I don't quite get you. Could you explain a bit more?

bwoester commented 11 years ago

@qiangxue

Could you explain a bit more?

Sure. I initially wrote I'm not totally happy with the current implementation, because it requires me to modify my controllers when all I want to do is changing views. We talked about how we could eliminate the need to modify controllers by introducing something that wouldn't require a widget's actions to be imported manually.

With such an approach, widgets would become more standalone. Actions they might require would become an implementation detail. You wouldn't need to care about what actions a widget uses or if it does use any at all. The widget would become more of a black box.

But when @klimov-paul pointed to access control, we started to talk about how configuring a widget's actions could also be done with the new, generic, widget action executing controller. I think this is problematic, because basically, this re-introduces the need to know about a widgets internals, its actions.

This need of prior knowledge about a widget's internal workings leads us back to a situation where you can't substitute one widget for another by only modifying the widget's host view. Example use cases for substituting widgets that came to my mind are

  1. application themes (theme uses different set of widgets) and
  2. new widget versions (new version uses different set of actions, because of re-factoring)

So maybe, configuring widget actions should be done differently.

  1. If you place a widget in a view, you intend it to work. This means, the controller action that executes a widget should be a good source of default values for the action values of the widget (including, but not necessarily limited to access control configuration). Duplicating configuration like access control will introduce a source of errors, because now, you need to keep the configurations in sync.
  2. To ease widget development (allow future re-factoring), required actions could be grouped. So a user of the widget wouldn't configure each single widget action, but maybe widget operation modes (maybe call them scenarios as we already use that term with AR). The exemplary FileManagerWidget I mentioned earlier could support read-only and read-write scenarios. Internally, the widget could use those configurations for actions associated with the respective scenario.
qiangxue commented 11 years ago

I think this is problematic, because basically, this re-introduces the need to know about a widgets internals, its actions.

You're right.

... This means, the controller action that executes a widget should be a good source of default values for the action values of the widget (including, but not necessarily limited to access control configuration). ...

This is only possible if we let the controller action executes the widget action. Even so, there are cases where the controller action and the widget action need to be configured differently.

Now I think perhaps we should give up this idea.

qiangxue commented 11 years ago

Close it for now based on the above discussion.

StagnantIce commented 8 years ago

What about this issue? https://github.com/StagnantIce/yii2-widget-controller