Below is the code for the “CanWriteProperty()” function. I copied and pasted the code here so that anyone interested can more easily fallow along my explanation of the issue. Please take a second to get familiar with this code.
-----------------------------------------------------
public virtual bool CanWriteProperty(Csla.Core.IPropertyInfo property)
{
bool result = true;
VerifyAuthorizationCache();
if (!_writeResultCache.TryGetValue(property.Name, out result))
{
result = BusinessRules.HasPermission(AuthorizationActions.WriteProperty, property);
if (BusinessRules.CachePermissionResult(AuthorizationActions.WriteProperty, property))
{
// store value in cache
_writeResultCache[property.Name] = result;
}
}
return result;
}
-----------------------------------------------------
As you can see from the code above, the “CanWriteProperty()” function internally calls the “BusinessRules.HasPermission()” function if the authorization result is not found in the cache.
If we now look inside the “BusinessRules.HasPermission()” function, we will find the following first couple of lines of code:
-----------------------------------------------------
public bool HasPermission(AuthorizationActions action, Csla.Core.IMemberInfo element)
{
if (_suppressRuleChecking)
return true;
...
...
-----------------------------------------------------
As you can see, when we call the "HasPermission()" function while we suppress rule checking (_suppressRuleChecking == true), the function immediately returns “true” which is not necessarily a bad thing except for what the “CanWriteProperty()” function does once control returns to it.
When control returns to the “CanWriteProperty()”, the next thing the function does is to make a call to the “BusinessRules.CachePermissionResult()” function and this is where things start to go in the wrong way. If the “BusinessRules.CachePermissionResult()” returns "true" then the authorization result will be cached and that is where the bug occurs.
The reason why this is not good behavior is because we never ran the authorization rule for the property. We are caching a value of "true" because that is what the "HasPermission()" function returned due to rule check suppressing and not because of the result of running the rule.
I am not too familiar with the CSLA code so perhaps I am wrong about this but I thought I would post what I think may be an issue.
Thanks.
Yes,
That looks like a bug.
We should have a TryHasPermission or move the check for supressRuleChecking into CanWriteProperty/CanReadProperty/CanExecuteMethod and only update cache when rule checking is not supressed.
Actually, the easiest fix is to change BusinessRules.CachePermissionResult to this:
public bool CachePermissionResult(AuthorizationActions action, Csla.Core.IMemberInfo element) { if (_suppressRuleChecking) return false;
as there is no point in allowing to cache the PermissionResult when SupressRuleChecking is True.
See: http://www.lhotka.net/cslabugs/edit_bug.aspx?id=975
Thanks Jonny,
Well, I wish I could offer an educated opinion regarding your proposed fix but my knowledge of the CSLA interworking is not good enough so I can’t!
However, I think there is at least one other ghosts hiding in the current implementation. Even after your proposed fix, the other issue I am seeing here is that if we don’t suppress rule checking then the rule will execute this time.... but is that really a good thing? ..... is really only a good thing if the rule runs at the appropriate time right?
For example, rules (at least authorization rules) should not be executed as a result of a property value being set inside a DataPortal Ceate or Fetch call. The reason for this is that during these two data portal calls, the object is on an indeterminate state (properties are being set during these data portal calls).
So for example, if an authorization rule is executed while setting the value of property “A” and the rule for property “A” requires the value of property “B” and property “B” has not been set yet, then we may get an error or worst we may end up with some undefined condition. You could run into the same issue even outside a DataPortal call but i am using the DataPortal call example as an easy to explain the problem scenario.
So I think we should defer running any rules until after the object is on a determined state. Kind of what we do with the DataPortal_Create, we call the “BusinessRules.CheckRules()” method after we know the state of the object is set.
So in short, I think we should get rid of running authorization rules on demand (as property values are being set), and move to a predictable and deterministic method where we can decide when authz rules are run, for example, via a “BusinessRules.CheckAuthzRules()” function call or something like that.
Again, I stopped messing around with the internal CSLA implementation code sometime ago so I may be talking nonsense here.
Thanks.
Hi,
The Authz rules that only check Roles (in IPrincipal) can be cached, can run whenever a user has been is logged in and the result will never change. Hence the result can be cached as it will never change for this instance of the BO..
Instance Authz rule that rely on the content of a BO should NOT be cached and should have CacheResult return false.
Caching is per instance of the BO.
For these situations you will either use LoadProperty or BypassPropertyChecks (and set property values) without calling the Authz rules. And any Authz rule that relies on the content of the BO (ex other properties) can NOT cache the result. These rules must always call the Execute method and have CacheResult set to false.
Thanks Jonny,
The fact that the code:
----------------------------------------
this.PropertyA = 123;
this.PorpertyB = 456;
----------------------------------------
And the reverse:
----------------------------------------
this.PorpertyB = 456;
this.PropertyA = 123;
----------------------------------------
Could potentially produce different cached authorization results without any clear and ovious indications to the developers seems wrong to me.
Imagine what would happen if a developer innocently moves the order of how the property values get set while refactoring some code. Not only could this lead to bugs that are difficult to trace, but also behaviors like this are (in my opinion) security holes waiting to be happen.
Without giving this issue too much thought and without having a good understanding of the interworking of the CSLA I would recommend the following changes:
The benefits of these changes would be:
Perhaps I am trying to solve some corner case scenario, I am not sure. But it looks to me like this is the right thing to do.
Thanks.
I've seen several cases where a property can only be set when certain conditions is met is a business requirement and will change as the user edits the fields of the BO.
And so the sequence _MAY_ be important unless you specifically want it to be otherwise in your code.
The simplest solution (inside the BO) is:
using (BypassPropertyChecks) { Country = "US"; State = "AZ"; } BusinessRules.CheckRules();
Another option that can be used on the outside of the BO (calling code):
var root = Root.NewEditableRoot(); var suppress = (Csla.Core.ICheckRules) root; suppress.SuppressRuleChecking(); // sequence does not matter root.Country = "US"; root.State = "AZ"; suppress.ResumeRuleChecking(); suppress.CheckRules();
(semantically - this is actually what the CslaModelBinder does in ASP.NET MVC to update the fields from http POST).
Your proposed changes would be breaking changes and I do not agree with them. You could just as easy cache Authz rules that should not be cached - only now it is in your code!!
Since all rules are now classes - I like to create several base classes and one of them is a PropertyAuthorizationRule that by default has CacheResult return false. You can create a number of helpful base classes for BusinessRules too. We wanted to keep the baseclasses within the framework as simple as possible and allow developers to create these (and share with the community).
public abstract class PropertyAuthorizationRule : Csla.Rules.AuthorizationRule { protected PropertyAuthorizationRule(AuthorizationActions action) : base(action) {} protected PropertyAuthorizationRule(AuthorizationActions action, IMemberInfo element) : base(action, element) {} public override bool CacheResult { get { return false; } } }
Copyright (c) Marimer LLC