Some of my thoughts regarding Security code

rxelizondo posted on Monday, November 02, 2009

Hello,

I finished going through the Security code this past Saturday and here are some thought regarding the subject. I hope they make sense and I didn’t totally miss the mark.

Thanks guys.

------

I am looking at the implementation of the RolesForProperty and RolesForType and can’t help but wonder why such similar classes don’t share the same design.

There are a couple of things here:

1) RolesForProperty declares all its backing fields variables at the beginning of the class while RolesForType declares them adjacent to its property get() implementation.

2) RolesForProperty implements code that check permissions while RolesForType implements to add roles.

3) Both RolesForProperty declares its member as public but RolesForType declares them as internal. Both classes are scoped as internal so I would personally scope all members as public but that is just me.

There is really nothing fundamentally wrong with the way things are implemented right now, but I would think that there is also nothing wrong with in having consistency in the code. And yes, I realize I am being anal but I am on my little quest to try to make it easier for people to understand the CSLA code and every bit matters to me.

------

Besides that, I would also move the:

public delegate bool IsInRoleProvider(IPrincipal principal, string role);

from the AuthorizationRulesManager class and put it in the AuthorizationRules and mark AuthorizationRulesManager as internal. I think that exposing the AuthorizationRulesManager class just to have access to the delegate declaration may be an overkill. By far, most (if not all) of the developer’s validation interactivity involves using AuthorizationRules so to me it only makes sense to put that delegate declaration there. Let’s give developers only one class to worry about when dealing with authorization issues.

------

I think I would also merge AuthorizationRulesManager and SharedAuthorizationRules into one class. I mean after all, it looks like SharedAuthorizationRules is nothing more that factory methods to obtain AuthorizationRulesManager instances so to me, all that functionality really belongs in only one class.

------

I also noticed that the AccessType enum does not appear to be in used unless I am missing something here. Anyone knows where that enum is used?

------

Also, PrincipalCache does not seems to be used anywhere either. A search in the e-book seems to say something regarding that the PrincipalCache is used for WCF purposes. If that is the case, should that class not be on a WCF folder or namespace?

RockfordLhotka replied on Monday, November 02, 2009

I appreciate your feedback, thank you.

You should keep in mind that this codebase has existed (at least some of it) since 1999 or 2000. Coding styles and preferences (at least mine) change over time. I suspect most people look back at things they wrote 6-12 months ago and see things they'd have done differently. Try doing that with code you wrote 7-9 years ago!

Of course this is production code, used by thousands of people around the world. So while I value code cleanliness and consistency (a lot), there's something to be said for leaving working code alone. Changing code purely for aestheic reasons is rarely a high enough bar to make it worth the time/effort/testing/etc.

And there are backward compatibility concerns. It is true, I might choose to organize some classes or types differently in retrospect. Or not, because some choices were made in anticipation of possible future features/enhancements/changes. But in any case, moving public members around from place to place is always a breaking change and so I try to avoid doing so when possible.

Finally there's the "fun factor". The framework is free, and sales of books and videos don't really cover my time. This is a labor of love as much as anything.

So when I sit down to work on CSLA I look at the wish list, and I tend to order my work based on a few priorities:

1. Is it a bug with no workaround?
2. Will it help a Magenic client/project?
3. Is it fun/interesting/intellectually stimulating to me?
4. Will it help make the lives of a reasonable number of users better?
5. Is it easy (low-hanging fruit)?
6. Does it make CSLA .NET more "complete"? (like the MVVM stuff)

And there are some anti-priorities:

1. Is it boring? Or worse, boring and time consuming?
2. Does it increase complexity without amazing payoff in productivity/flexibility?
3. Does it increase my testing/support surface area?
4. Does it focus on "legacy" technologies (now including Windows Forms, Remoting, asmx, Enterprise Services and maybe Web Forms)?
5. Does it solve a problem that's already been solved? (like ORM stuff or UI framework stuff)

These priorities are especially important during point releases, but they certainly factor into major releases (like 4.0) as well. Though the "fun factor" becomes a much bigger priority for major releases, and tactical things like bug fixes or specific Magenic client requirements are usually not as big an issue.