mod: convert bizRules into callables, avoiding eval()

[bwoester]

This changes the implementation of bizRules. They are no longer strings containing PHP code to be evaluated. Instead they are now callables to be invoked. Related issues are #24 and #42.

Of course Manager-implementations need to be able to (de)serialize the callables, so the usage is most likely limited to defining callbacks in the form of array($classOrInstance,$methodName). PhpManager already used var_export, which works nicely with such callback definitions. DbManager now uses (de)serialize for bizRules just like it does for data.

Since some people were concerned about loosing flexibility if they couldn't pass php code as bizRules, I included an example of an evaluating bizRule callback in the tests. I still don't think this is a commonly required use case, but if someone really requires users being able to write their own bizRule code, they can use such an approach. So we won't loose anything, only difference is it's now in the responsibility of the developer to provide the bizRule callback that does the evaluation.

[qiangxue]

This is not any safer than eval() as long as you allow end users to directly insert the biz rule (e.g. the user could call dangerous existing PHP functions).

Because bizRule uses eval(), end users should never be allowed to directly insert the biz rule. Instead, a middle layer is needed. For example, you first define a list of allowed function calls, then let the users to select any of them. The underlying code would translate the input into PHP statements as the actual biz rules.

[creocoder]

I do not think we need this PR, since it creates a lot of inconvenience without any benefits except prefomance.

[creocoder]

@qiangxue My suggestion to make $bizRule mixed and change executeBizRule() to check what is it. If it string than we use eval(), if it Closure we use it. So PhpManger can use Closures and strings, where DbManager should use only strings.

[qiangxue]

Closure cannot be serialized.

[creocoder]

@qiangxue Yes, and we will not selialize it.

where DbManager should use only strings.

[creocoder]

@qiangxue With that we can take perfomance benefit for PhpManager from this PR and not loose any for DbManager.

[qiangxue]

How do you store a biz rule which is a Closure?

[creocoder]

@qiangxue When you use DbManager you should use only strings. When use PhpManager you will have 2 ways:

'bizRule' => 'return true',

and

'bizRue' => function() { return true; },

So where is the problem with:

How do you store a biz rule which is a Closure?

?

[creocoder]

@qiangxue Hmm... You about PhpManager::saveToFile(). Maybe we can take closures bodies through Reflection?

[bwoester]

I don't understand why you want to stick with eval. I do understand it's not bad in general, but I also realize that as soon as AuthItem's API provides bizRule as a string that will be evaluated as PHP code, this property will be exposed to end users for sure. Just look at existing extensions for v1.1.

I believe by changing bizRule from string to callable, that issue will be eliminated for the most part. Simply because it won't be as easy any more to just provide a text box for hacking in some code. Of course, accompanying documentation on how to use bizRules when it comes to UI would also be of great value.

I also do not see where this change introduces inconvenience. IMO, it's the other way round: Coding bizRules as strings sucks. You get no help whatsoever from your IDE, refactoring tools won't work, those rules never get tested and are prone to be broken when you modify your app. Implementing them as proper code helps you in all respects.

Also, as far as I remember, I always wrote my bizRules when developing applications. I never needed them to be coded after the app was deployed. And why on earth should I write php code, put it into a string, pass it to an authItem, only so that very same authItem can later evaluated that code? If all it really needed was a hint to where the code is it has to execute? Exactly this can be achieved using callbacks.

If someone finds a way to load and save closures, I'd be happy to see them being used, but to my knowledge, it is not possible.

Using callbacks like it is done in this PR on the other hand works out of the box. For both PhpManager and DbManager, no need to introduce features that only half of the implementations can use. It's an easy concept, with barley any overhead, understandable for every php developer.

I really hope you reconsider this.

[creocoder]

Also, as far as I remember, I always wrote my bizRules when developing applications. I never needed them to be coded after the app was deployed.

And this is a problem. Sometimes we need dynamically create bizRules and store it to db. Anyway your PR did not change something. Only illiminate eval and makes the solution of some RBAC tasks impossible.

[bwoester]

Well, I won't go as far as saying every application can have its bizRules defined at dev stage. But I do believe it is way more common than the requirement to change them after the app has been deployed. For cases where this is required, even if I myself never encountered one, the evaluation can still be done in the callback itself. So the PR does not make such a task impossible. It only requires the dev to willingly use eval() inside his callback. I say it again: I think this is an edge case and I don't think this edge case justifies the use of eval for every single bizRule, because for the vast majority, it is not needed.

[qiangxue]

＠bwoester: Your PR fundamentally is equivalent to the eval() approach. As long as you allow the end user to specify (arbitrary) callbacks, you have the same danger as the eval() approach.

Ultimately I think the solution is to have a list of predefined biz rules and only allow untrusted end users to select from them, and allow trusted users to have the capability of creating arbitrary biz rules.

You're right that proper use of biz rules needs good tutorials. For Yii 2, we plan to develop an official RBAC management module, which should serve for this purpose.

[bwoester]

After sleeping on it, I wonder if I'm probably mistaken. After all, my main assumption is that writing bizRules after an app has been deployed is almost never necessary. If you write them while developing your app, there's no need to use eval(), a callback does a better job. In edge cases, it would be possible to use eval() inside the callback, so we wouldn't loose flexibility.

Now I wonder if that assumption is just plain wrong? Do you guys make use of runtime created bizRules a lot? I personally didn't ever need this, but this is probably not true for others.

[creocoder]

After all, my main assumption is that writing bizRules after an app has been deployed is almost never necessary.

You can have very complex RBAC backend which generate bizRules on the fly.

[bwoester]

In theory? Sure. Any real use cases? I never encountered one. Also, till today nobody spoke up to say "Yes, I need this frequently, because xyz". So I adhere to my opinion that this is - if at all - a very rare use case and should be implemented by the dev if it is needed. The vast majority of uses cases - which is what the framework should focus on - does not need to evaluate code but should use callbacks.

Think of it this way: even if you have very complex scenarios where you need to generate bizRules on the fly, you don't want the user to hack code into the bizRule. Instead, you might trigger a callback, that starts some sort of an configurable evaluation pipeline.

[qiangxue]

@bwoester What's the difference between eval() and callback? If you allow end users to directly enter arbitrary callbacks, it is not any different from allowing them to enter PHP statements.

[creocoder]

@qiangxue I think no difference here instead of with callbacks we get inconveniences. @bwoester You just not like eval() ?

[qiangxue]

The key to prevent security problems here is to restrict the freedom of creating arbitrary biz rules by users who cannot be 100% trusted. There are many ways to implement this. Allowing end users to choose from a list of predefined callbacks is one of the choices. You could also predefine a list of named biz rules, each of which is a PHP statement (instead of a callback). The underlying biz rule evaluation mechanism is still eval(), which should not be exposed to end users directly.

BTW, there is no good way of serializing an anonymous function. You could use reflection to find out the code defining the anonymous function. Then you still need to use eval() to "deserialize" the function. There's also trouble of serializing the variables appearing in the use() statement.

[qiangxue]

I think one possible solution we could take is to store an ID or name as a biz rule. There should be an external map between the biz rule ID and the actual definition of the biz rule. The map should only be maintained by 100% trusted users or developers.

[creocoder]

one possible solution we could take is to store an ID or name as a biz rule

Sounds really good! We also will save space in the database. Especially for auth_assignment table.

[creocoder]

@qiangxue Suggest some additional php file with format like:

$biz_rule_name1 = function() {
    ...
}

$biz_rule_name2 = function() {
    ...
}

This also can be used for PhpManager.

[creocoder]

Another format is:

return array(
    'biz_rule_name1' => function(...) {...},
    'biz_rule_name2' => function(...) {...},
    ...
);

[qiangxue]

How to maintain this file (insert, update, delete)? We should consider DB-based storage too.

[bwoester]

You just not like eval() ?

It is true that I don't like it and that I try to avoid it when it is possible. But this PR is not based on "I just don't like it". I hope I presented enough arguments pointing out why it should not be used for bizRules.

What's the difference between eval() and callback? If you allow end users to directly enter arbitrary callbacks, it is not any different from allowing them to enter PHP statements.

I don't think this holds completely true. The difference is using eval(), the user can execute arbitrary code. Using a callback, the user can only decide to execute one method that is already available. It is not possible for him to define his own function. Also, that callback will be executed with the parameters ($manager,$aParams,$data), which further reduces the scope of callbacks that can be used in a meaningful manner. It would be more problematic if the user could also decide what parameters should be passed to the callback he selects (e.g. allowing him to execute the callback rmdir with the parameter "/var/www").

I think one possible solution we could take is to store an ID or name as a biz rule. There should be an external map between the biz rule ID and the actual definition of the biz rule. The map should only be maintained by 100% trusted users or developers.

I agree, there needs to be something that identifies a bizRule. An ID or name could be used, but IMO a callback is not that different. It identifies a piece of code to be executed. Maybe it would be more straight forward to use an ID or a name. It could also be used to decouple the actual implementation of the bizRule. Maybe a bit like filters or actions, where an ID is used to reference it, but the implementation can be inline or a custom class.

I still question the use of eval() to execute the bizRules, at least if it is the only supported mechanism. Developers can write code, they don't need to write strings being evaluated. I'm not sure about the 100% trusted users. This includes the question of who is considered 100% trusted as well as how common a requirement this scenario is. Also, how would those users maintain the map of bizRules? Using the word maintaining, do you mean only rearranging the mapping of IDs to existing bizRules, or does this include writing own bizRules?

[qiangxue]

@bwoester I'm not saying callback is equivalent to eval(). I mean they are both dangerous if you don't impose any restrictions. Yes, the callback signature will impose some restrictions, but it's not reliable. For example, if Yii introduces some silly function like Yii::removeAll(), which removes all files under a default path, then a security hole is immediately introduced, while we can't tell it by looking at the new function alone.

The idea of the named biz rules is to impose the restriction that users can only use a list of predefined biz rules, instead of arbitrary callbacks.

Maintaining biz rules means creating new rules and modifying or deleting existing rules. Only 100% trusted users can perform these actions.

Examples of 100% trusted users: application developers, site owners and/or administrators.

I don't think we can draw the conclusion that biz rules can be developed once for all. Note that biz rules are part of roles/operations. As long as users are allowed to create new roles/operations, there should be cases that new biz rules are needed. The question is who can create what kind of biz rules.

[bwoester]

I'm not saying callback is equivalent to eval(). I mean they are both dangerous if you don't impose any restrictions. Yes, the callback signature will impose some restrictions, but it's not reliable. For example, if Yii introduces some silly function like Yii::removeAll(), which removes all files under a default path, then a security hole is immediately introduced, while we can't tell it by looking at the new function alone.

Got your point and you're right, thx for elaborating on it. 👍

Also your argument of new bizRules for new operations makes sense to me.

So basically what remains are two use cases:

1. Applications without full blown rbac management. They might support role management, but only define a fixed set of operations, some of which might use predefined bizRules. (e.g. a CMS like joomla)
2. More dynamic applications, that allow full control over auth items including bizRules

While I still liked to have a possibility to execute predefined bizRules for the first kind of applications without using eval(), I have to think about how to handle things for the second kind of applications.

First question is how to store those new bizRules. Basically, there are only two options:

1. store a string and evaluate it
2. store php code and execute it

If there will be an ID => bizRule mapping, the bizRules are already decoupled from the auth items. So the rules don't necessarily need to be stored in DB, even when using a DB based rbac manager. Instead, they could directly be stored as PHP code as suggested by @creocoder. An alternative would be to store them in separate files, which might make it easier to manage them (CRUD).

Advantage over evaluating strings would be faster execution speed and the possibility to work on the rules using an IDE. Not sure if it worth the effort, though. Maybe only if the biz rules reach a certain size and complexity...

[creocoder]

How to maintain this file (insert, update, delete)? We should consider DB-based storage too.

I think you suggest to maintance this file manually. Since:

There should be an external map between the biz rule ID and the actual definition of the biz rule.

[creocoder]

@bwoester Lets not discuss calbacks VS events() since there is no real difference and we have real alternative to discuss. I'm about bizrule map and store only bizrule ids.

[creocoder]

@qiangxue I suggest for DbManager make auth_biz_rule table where we can store biz_rules like eval()'able strings. As alternative we can store biz_rule bodies in separate file. Same thing for PhpManager. We can introduce Manager::$separateBizRules property.

Also i think we should make this:

make auth_biz_rule table where we can store biz_rules like eval()'able strings.

Even if we not accept mapping approach. Just for db storage place economy and from db normalization point of view.

[klimov-paul]

I can not see any benefit from callback usage. Callbacks can not as flexible as plain PHP code processed by eval. This means they can not replace it fully.
If you do not want to provide the end user ability to create business rules, you can simply predefine the set of available rules and provide the interface, which allows only selection of them from drop down - view always can hide the actual implementation.

Speaking of security, business rule codes are stored either in files or database. To modify these data and thus place hazard code to the rules someone will need and unlimited access to such storage.
If some person, who is not an application developer, gains such access, your application is lost already no matter which approach you are using to store business rules.

[creocoder]

@klimov-paul I think its meaningless to discuss callback approach. What do you think about mapping approach? I'm about:

I think one possible solution we could take is to store an ID or name as a biz rule. There should be an external map between the biz rule ID and the actual definition of the biz rule. The map should only be maintained by 100% trusted users or developers.

[klimov-paul]

Mapping approach will not be as flexible as eval.
If we going this way we can simply abandon the "business rule" feature, moving it to the extra controller filters, which should be maintained by developers.

[bwoester]

Callbacks can not as flexible as plain PHP code processed by eval. This means they can not replace it fully.

Sure they can. All you need to do is defining one callback doing evaluation being used for dynamically generated bizRules. There's still no need to use eval() for bizRules you're writing when developing your app. Doing it would be the same as writing IFilter as a string being evaluated instead of using CInlineFilter.

I don't insist on callbacks though. If we really start using bizRule IDs, I would be fine having any alternative. Be it InlineBizRules, BizRuleClasses, callbacks or whatever. EvalBizRules or IncludeBizRules are required if an app allows someone to define his own bizRules. But since I never needed this, please don't force me to eval() my own code. 😨

[creocoder]

Mapping approach will not be as flexible as eval.

Seems you not read this issue carefully. Mapping approach can use eval() like current implementation approach or Closures as alternative.

[creocoder]

@klimov-paul

If we going this way we can simply abandon the "business rule" feature, moving it to the extra controller filters

Never seen action with 10 checkAccess()-es inside? Filters can solve only VERY simple RBAC tasks.

[qiangxue]

Reasons for using a biz rule map:

• Support reusing the same biz rule in different auth items.
• Easier maintenance of biz rules.
• Support both static (predefined, what @bwoester wanted to use) and dynamic biz rules.
• The evaluation of biz rules can use either eval() or call_user_func(), depending on how a biz rule is defined.

[creocoder]

@qiangxue

The evaluation of biz rules can use either eval() or call_user_func(), depending on how a biz rule is defined.

So you against biz rules as Closures?

[bwoester]

Should be possible to use a closure as static bizRule and have it executed through call_user_func(_array). 😌

[qiangxue]

@creocoder No, I'm not against using Closure. Closure can be executed via call_user_func().

I will close this PR and create a new issue specific for this biz rule map thing.