OOPhoto: Implementing Security In An Object Oriented Application - Round I
The latest OOPhoto application can be experienced here.
The OOPhoto code for this post can be seen here.
Today, I took my initial thoughts on implementing security in an object oriented application and tried to apply them to OOPhoto, my first attempt at learning object oriented programming in ColdFusion. This was definitely a shaky first step; I am referring to this as "Round I" as I am quite convinced that I will want to refactor the code based on further exploration and hopefully some really great feedback from you guys.
In addition to being shaky, this step required a good amount of code. It's a bit complicated, so I will walk through this slowly. I think it would be best to start from the outside (the Controller) and work my way in. As I stated in my previous post, I am going to start with a security implementation that is fully Controller-initiated. By this, I mean that the Controller must explicitly check and test user permissions - the Model does not do this implicitly.
Prior to this, the Application didn't have any sense of session management. Therefore, the Application.cfc had to be updated to use sessions and to listen for the OnSessionStart() event. It is in this event - session start - that I am creating the new User object and caching it in the SESSION scope:
<cffunction
name="OnSessionStart"
access="public"
returntype="void"
output="false"
hint="I fire when the session starts or is manually reset.">
<!--- Define the local scope. --->
<cfset var LOCAL = {} />
<!---
Store the CFID and CFTOKEN values locally so we don't
lose them.
--->
<cfset LOCAL.CFID = SESSION.CFID />
<cfset LOCAL.CFTOKEN = SESSION.CFTOKEN />
<!---
Clear the session in case we are re-initializing rather
than booting up for the first time.
--->
<cfset StructClear( SESSION ) />
<!--- Move the CFID / CFTOKEN back into the session. --->
<cfset SESSION.CFID = LOCAL.CFID />
<cfset SESSION.CFTOKEN = LOCAL.CFTOKEN />
<!--- Create a user object in our session. --->
<cfset SESSION.User = APPLICATION.Factory
.Get( "SecurityService" )
.GetUser()
/>
<!--- Return out. --->
<cfreturn />
</cffunction>
As you can see, we are using a new service, SecurityService.cfc, to create the User object. While we are using a minimal amount of security in this application (it's more a proof of concept than a best practice), I am trying to put in some "better" practices by using the IP address and user agent of the client to help authenticate the user. Here is the SecurityService::GetUser() method:
<cffunction
name="GetUser"
access="public"
returntype="any"
output="false"
hint="I return a new user (either by creating a totally new one or building one based on existing data).">
<!--- Define the local scope. --->
<cfset var LOCAL = {} />
<!---
The user ID is just stored as a regular ID in the cookie.
Let's param the value to make sure we can refer to it.
--->
<cfparam name="COOKIE.ID" type="numeric" default="0" />
<!---
Now that we have the cookie paramed, let's try to
authenticate the user based on the COOKIE as well as
environmental variables.
--->
<cfset LOCAL.ID = THIS.Authenticate(
COOKIE.ID,
CGI.http_user_agent,
CGI.remote_addr
) />
<!---
Now that we have a user ID, let's load the user object.
We need to wrap this in a Try/Catch block since the
Load() method will raise an exception if the ID we
authenticated is zero.
--->
<cftry>
<cfset LOCAL.User = VARIABLES.UserService.Load( LOCAL.ID ) />
<!--- Catch load error. --->
<cfcatch>
<!---
The load did not work, so let's just create a
new user.
--->
<cfset LOCAL.User = VARIABLES.UserService.New() />
<!---
Since we created a new user, we need to store
some information manually as well as set the ID
in the COOKIE so we can refer to it later.
--->
<cfset LOCAL.User
.SetUserAgent( CGI.http_user_agent )
.SetIPAddress( CGI.remote_addr )
.SetDateCreated( NOW() )
/>
<!---
Now that we have populated the user, let's store
it. This will persist the data for the next
session and give us a new ID that we can be put
into the cookies.
--->
<cfset LOCAL.User.Save() />
<!---
Now that we have saved the user, store the new ID
in the cookie.
--->
<cfcookie
name="ID"
value="#LOCAL.User.GetID()#"
expires="never"
/>
</cfcatch>
</cftry>
<!---
At this point, we have created are user object.
Now, let's authorize the user object to participte in
our application.
--->
<cfset THIS.Authorize( LOCAL.User ) />
<!--- Return the initialized, authorized user. --->
<cfreturn LOCAL.User />
</cffunction>
I am not crazy about the violation of encapsulation that is occurring in this SecurityService.cfc method. I am referring to the COOKIE scope as well as the CGI scope from within a ColdFusion component that shouldn't need to know about them. But, I wasn't sure how to get around this. I didn't want the Application.cfc to have to pass in the CGI values with the method call and I certainly didn't want the Application.cfc to store the COOKIE values that came back.
The SecurityService.cfc uses the new UserService.cfc service object to load and create new User.cfc instances. These two objects (UserService.cfc and User.cfc) are just like all my other existing domain objects, so I won't go into them just yet. Rather, let's pop back up the chain to the top-level page flow. In the OnRequestStart() event method of the Application.cfc, I am taking the User instance and storing it in the Data bag:
<!---
Store the current user in the Data. This way, if we ever
need to change the way the User is implemented/stored, we
only have to do it here - going forward, it will be in the
Data bag.
--->
<cfset REQUEST.Data.User = SESSION.User />
I like this move because at this point, the rest of the application doesn't need to know about the page-to-page persistence caching mechanism of the User object. For each page request, I am copying the User reference to the REQUEST.Data object, which is all the rest of the page processing needs to know.
Ok, so now that we have created and stored our User object, let's use it. When it comes to access permissions, I feel there are two gestures that need to be done. On one hand, we want to be able to check to see if a user has permissions to do something:
<!---
Check to see if this user can edit this target object.
If so, then display the edit link.
--->
<cfif User.CheckPermissions( "edit", Target )>
<!--- .... display code .... --->
</cfif>
This kind of check can be used in the rendering logic of a view. Then, on the other hand, we need to enforce access permissions. In my mind, this type of action would either run quietly if the user has the proper authorization or it would raise an exception if the user failed authorization:
<!---
Check to see if this user can edit this target object.
If NOT, then the method will raise a security exception.
--->
<cfset User.ConfirmPermissions( "edit", Target ) />
So what we have is one method, CheckPermissions() that will return a boolean for acceptance, and another method, ConfirmPermissions() that will raise an exception if the security acceptance fails.
Now, if you recall from my previous post, I stated that there were two kinds of security concerns: Domain-based and Application-based security. The Application-based security is concerned with page-view-access and is not associated with any particular data. Because of the way that OOPhoto uses an action variable to define a unique view, this type of security check can be performed in one place, at the top of my Front Controller (index.cfm snippet):
<!---
Check to see if this user has permissions to view this
page in general based on the "DO" action.
--->
<cfset REQUEST.Data.User.ConfirmPermissions(
"do",
ArrayToList( REQUEST.Data.Do, "." )
) />
Here, the action variable array is converted to a string like "gallery.edit" and passed to ConfirmPermissions() method of the User which will raises an exception if the user does not have view-clearance. Of course, this application does not implement that at this point, but I put it in as a future hook.
When it comes to Domain-based security, the Controller checks permissions explicitly in the action files. The big move I wanted to make in this step was to say that users could only edit or delete the galleries that they authored. So, for example, when a user goes to Edit a gallery, I get the PhotoGallery.cfc instance and then check the access permissions in the action file:
<!--- Load or create new gallery. --->
<cfset LOCAL.Gallery = ARGUMENTS.Data.Cache.Factory
.Get( "PhotoGalleryFacade" )
.GetPhotoGallery( ARGUMENTS.Data.Attributes.id, true )
/>
<!--- Check the user's permissions to edit this gallery. --->
<cfset ARGUMENTS.Data.User.ConfirmPermissions(
"edit",
LOCAL.Gallery
) />
This type of exception-raising check happens at the top of both the Edit and Delete action pages to make sure that a user cannot modify a gallery that they did not create. That's basically the extent of the Controller-initiated security model. So now, let's go in a little deeper - let's take a close look at the Domain Model updates.
As with my other business objects (BO), the security methods CheckPermissions() and ConfirmPermissions() can be called directly on the User.cfc object, but the logic for these methods is located elsewhere. The User.cfc simply turns around and passes the message onto the SecurityService.cfc, including itself as one of the relayed arguments (User.cfc snippet):
<cffunction
name="CheckPermissions"
access="public"
returntype="boolean"
output="false"
hint="I return a boolean flag as to whether or not the user can perform the given action.">
<!--- Define arguments. --->
<cfargument
name="Action"
type="string"
required="true"
hint="I am the action that is being performed."
/>
<cfargument
name="Target"
type="any"
required="true"
hint="I am the target object upon which the action is being performed."
/>
<!--- Pass off to the security service. --->
<cfreturn VARIABLES.SecurityService.CheckPermissions(
THIS,
ARGUMENTS.Action,
ARGUMENTS.Target
) />
</cffunction>
<cffunction
name="ConfirmPermissions"
access="public"
returntype="void"
output="false"
hint="I check to see if the user can perform the given action. If not, I rase an exception.">
<!--- Define arguments. --->
<cfargument
name="Action"
type="string"
required="true"
hint="I am the action that is being performed."
/>
<cfargument
name="Target"
type="any"
required="true"
hint="I am the target object upon which the action is being performed."
/>
<!--- Pass off to the security service. --->
<cfset VARIABLES.SecurityService.ConfirmPermissions(
THIS,
ARGUMENTS.Action,
ARGUMENTS.Target
) />
<!--- Return out. --->
<cfreturn />
</cffunction>
Remember, I am doing this because I made it a rule previously that all business objects should know in a robust fashion as to how to use or leverage their own internal data.
Notice that no matter what the Action or Target values are, I am passing this message onto the SecurityService.cfc. I wanted to funnel all of the security checks through the security service because it hid the implementation from the User object. The User.cfc never has to think about which "service" to use for security checks, it just passes it along to one place. Along those same lines, I like the separation of concerns; I felt it was purely the Security Service's concern to handle all security-based logic. If need be, the security service could reference other service objects or change its internal implementation and the User.cfc would not be affected.
At this point, we have worked our way down the chain of command to the beefiest and by far, the weakest part of the application - the permission-checking methods of the SecurityService.cfc. The ConfirmPermissions() method is actually quite simple - it just calls the CheckPermissions() method and raises an exception if necessary:
<cffunction
name="ConfirmPermissions"
access="public"
returntype="void"
output="false"
hint="I check to see if the given user can perform the given action. If not, I rase an exception.">
<!--- Define arguments. --->
<cfargument
name="User"
type="any"
required="true"
hint="I am the user in question."
/>
<cfargument
name="Action"
type="string"
required="true"
hint="I am the action being performed."
/>
<cfargument
name="Target"
type="any"
required="true"
hint="I am the target object upon which the action is being performed."
/>
<!---
Check to see if the user can perform the given action
on the given target.
--->
<cfif THIS.CheckPermissions(
ARGUMENTS.User,
ARGUMENTS.Action,
ARGUMENTS.Target
)>
<!--- The user can perform the given action. --->
<cfreturn />
<cfelse>
<!---
The user cannot perform the given action on the given
target. Raise a security exception.
--->
<cfthrow
type="OOPhoto.AccessDenied"
message="You do not have permission to perform this action."
detail="You do not have permissions to perform the action, #ARGUMENTS.Action#, on the given object."
/>
</cfif>
</cffunction>
As you can see, I tried my best to factor out "gestures" into their own methods so that I wasn't repeating my logic. But, the CheckPermissions() method is where I really started to take shots in the dark. I had no idea what I was doing:
<cffunction
name="CheckPermissions"
access="public"
returntype="boolean"
output="false"
hint="I return a boolean flag as to whether or not the user can perform the given action.">
<!--- Define arguments. --->
<cfargument
name="User"
type="any"
required="true"
hint="I am the user in question."
/>
<cfargument
name="Action"
type="string"
required="true"
hint="I am the action being performed."
/>
<cfargument
name="Target"
type="any"
required="true"
hint="I am the target object upon which the action is being performed."
/>
<!--- Check to see which type of action we are checking. --->
<cfswitch expression="#ARGUMENTS.Action#">
<cfcase value="do">
<!---
In this application, there are no view
restriction, so we are not gonig to do anything.
But, if we needed to add DO-based permissions,
we could do it here.
--->
<cfreturn true />
</cfcase>
<cfcase value="delete">
<!--- Check to see what kind of object this is. --->
<cfswitch expression="#ListLast( GetMetaData( ARGUMENTS.Target ).Name, '.' )#">
<cfcase value="PhotoGallery">
<!---
When it comes to photo galleries, the user
can only edit galleries that they authored.
--->
<cfif (
ARGUMENTS.Target.GetID() AND
(
NOT (
(NOT IsSimpleValue( ARGUMENTS.Target.GetUser() )) AND
(ARGUMENTS.Target.GetUser().GetID() EQ ARGUMENTS.User.GetID())
)
))>
<!---
Either no user was set yet or the
current user is not the author of the
given photo gallery; access is not
allowed.
--->
<cfreturn false />
</cfif>
</cfcase>
</cfswitch>
</cfcase>
<cfcase value="edit">
<!--- Check to see what kind of object this is. --->
<cfswitch expression="#ListLast( GetMetaData( ARGUMENTS.Target ).Name, '.' )#">
<cfcase value="PhotoGallery">
<!---
When it comes to photo galleries, the user
can only edit galleries that they authored.
--->
<cfif (
ARGUMENTS.Target.GetID() AND
(
NOT (
(NOT IsSimpleValue( ARGUMENTS.Target.GetUser() )) AND
(ARGUMENTS.Target.GetUser().GetID() EQ ARGUMENTS.User.GetID())
)
))>
<!---
Either no user was set yet or the
current user is not the author of the
given photo gallery; access is not
allowed.
--->
<cfreturn false />
</cfif>
</cfcase>
</cfswitch>
</cfcase>
<cfdefaultcase>
<!---
If the given action could not be found, then we
have no reason to think the user shouldn't be
performing this action. Simply return out.
--->
<cfreturn true />
</cfdefaultcase>
</cfswitch>
<!---
If we have gotten this far without returning,
then just return true since we were not checking for
something explicitly.
--->
<cfreturn true />
</cffunction>
As you can see, the service first checks the type of action that is being tested. Then, based on the action, it checks to see what type of object the target is and checks permissions based on that.
Why I don't like this version of CheckPermissions():
It has the potential to explode in size. I have barely any permissions in my application right now and you can already see how big this method is getting.
There is no separation of finer-concerns. By this I mean that the SecurityService.cfc is handling all permissions validation. There is no passing the checks off to other services. I like the idea of funneling all initial checks through the SecurityService.cfc, but I am not sure if other services should be able to help out?
It compares the current user to the author of the PhotoGallery instance by comparing the IDs of the respective objects. I think this is an OOP "Class 101" concern - this type of comparison should be encapsulated in some sort of User.EqualTo( User ) method. Perhaps I can do that in the next iteration with very minimal effort.
So, that's what I've got so far. Not bad for a few hours of coding, but clearly there is a ton of room for improvement. I'd really love some help on this topic as I feel completely out of my element.
On a final note, I didn't talk about it above, but as part of the update, the PhotoGallery.cfc now has a User property. This property contains a User.cfc instance that is the author of the gallery. This step has created noticeable slowness on the home page where each Photo that I load must load the Gallery object which must load the User object. I can implement some more lazy loading, but I am beginning to see that all of these separate database calls are going to get exponentially expensive. I suppose that a complex caching mechanism can help out, but I'm not there yet.
Object Oriented Reality Check
This was a big step for me; we fundamentally changed the way the application works. We added session management, created new services, and have user-based permissions. Time to take a step back and evaluate our actions.
Was This Step Worth Implementing?
No question, this step was a step in the right direction. Almost all ColdFusion applications have some sort of security that needs to be enforced. As such, it is important that we learn how to take those security concerns and integrate them into an object oriented applications.
Is My Application Object Oriented?
The application is certainly no more object oriented than it was in our previous step - we just added more domain objects. In fact, we might argue that the application is slightly less object oriented than it was before. The reason I say this is because our Controller is initiating all security checks in the application. That sounds more like our old procedural style of programming than it does our OOP, rich-domain style of programming.
Want to use code from this post? Check out the license.
Reader Comments
Hey Ben. Couple of thoughts.
1. IMO permission checks belong in the controller. The view should be ignorant because permission checks are about complex access to the model, and that always rules out the view. As for having security in the model well... you can, but personally I don't think it belongs there... Or rather that the security model should be a separate model from the domain of what it's securing. Think of it like national security right? The president, the VP, whitehouse chief of staff, etc. all those guys have their own jobs. None of them ever work security because that's not their job. They may talk to members of the secret service, but they don't generally really know the security jobs - that's why they hire qualified security people who know what they're doing. And conversely the security people may be armchair politicians, but politics isn't their job so they're not likely to understand all the minutia of how bills are drafted, etc. Separation of concerns. What this will allow you in the long run is the ability to modify the security model (or potentially even completely replace it) without changing the model that runs the photos and such.
2. I would recommend against trying to load up an author object as a composed object for each picture. For that matter, I would recommend against loading up an individual picture object for each picture on the home page. There's really nothing wrong with just using a query... but if you just feel totally put off by using a query then use an "iterating business object" to display those images.
3. Some of the things you've described about your security model here are similar to the security model I implemented in the onTap framework's Members onTap plugin. Pointing to the user session in the request scope to make the rest of the application ignorant about its persistence method is something I've been doing for a long time in particular. And until now I really have not promoted this particular entry on my blog, but it floated up to the top of my stats on its own, which surprised me at first, but I'm also kind of proud of it. :) Anyway it's about security and you may find it useful, I don't know. I just thought I'd share it while you were on the subject. http://ontap.riaforge.org/blog/index.cfm/2008/5/11/Keep-It-Simple
Oh and 4. I've got pages where the controller needs to use complex logic to determine if a particular user should be allowed to access the page. That is, logic more complex than "does he have permission to this page?" In some cases they are similar to your requirement of only allowing the author to edit or delete an item. What I did in my security layer was to have two objects - an application security service singleton and then a separate request-security object. When permissions are checked, they're checked using the request object so that it can cache the results of each check which does three things.
First it makes subsequent checks for the same permission faster (since it short-circuits).
Second it prevents a given permission from changing in the middle of a request (for example if someone edited the user's roles while they were in the middle of performing some kind of action - this helps prevent some race conditions)
Lastly it allows me to override a given permission for a specific request.
So on those pages where I need to use complex logic to deny (or grant) access, I can perform my complex logic and then simply go to the request security object and say "set permission x to grant/deny for the current request", and then the rest of the request behaves normally.
Enjoyed the article. :)
@Ike,
I like your onTap article. I think I was trying to achieve a similar effect - allowing any type of task to be passed to the security check. Can I ask you how you keep your isPermitted() under control? I feel like mine would get exponentially larger as I add more security concerns. How do you keep that method well factored?
When you say the View should be totally ignorant to all security checks, I assume you are referring to my use of my demonstrated logic:
<cfif User.CheckPermissions( "edit", Target )>
. . . . <!--- .... display code .... --->
</cfif>
How do you handle things like that to keep the View ignorant? Do you do some sort of XFA-style maneuver where you perform the check in the Controller and then the View references that rather than the actual Model-based security. Something like:
In Controller
<cfset canEdit = User.CheckPermissions( "edit", Target ) />
In View
<cfif canEdit>
. . . . <!--- .... display code .... --->
</cfif>
I suppose this way the View is only bound to the Controller, not to the Model. Is that what you are going for?
As far as the homepage, for this project, I decided to try and go fully OOP for the first iteration. Once I feel that I have an OK handle on all of this OO stuff, I figured I could then go back and put in some query-based optimizations. I just wanted to learn to walk before I could run.
I like the idea of a request-based security object that can cache checks. I think you can only do that because you have a REQUEST-based onTap object. Currently, my security service is an APPLICATION-based singleton, so it can't have any sense of state. I'd have to completely change the way it works .... or rather, maybe I'd only have to make an intermediate REQUEST-based object which could cache or hand off to the singleton if need be.
One last thing - how would handle my scenario - author based permissions - with the IsPermitted() method? Does the "task" argument define both the task and the target object some how?
thanks for this Ben, been following the whole OOPhoto series and I've learned a thing or two...
i'm just getting into full-fledged OO (i was just kind of "faking" it before) and the whole concept of Security & Logging for an OO application will give me problems when i try to implement it into my next project as well, so I'll follow your follow-up comments closely ;D
@Chris,
Sounds good. I hope we can learn together.
@Ben - Now that you mention it I think I do remember reading you making that comment about coming back to optimize the home page before, so just ignore my #2 comment. :P
Glad you enjoyed the security article. :)
re: keeping isPermitted() under control. Well in my case two ways - 1 because all the permissions are stored in the db, so they're actually separate from the rest of the app and that means when I add new permissions it doesn't change the code any. 2 is that my permission system is really not designed to handle "special cases" in particular -- for that I bow out to controller-level logic and overriding the request permissions.
re: special cases - If I had some reason that I really wanted to try and get all the special cases inside my security layer, I would probably be asking a lot of the same questions you're asking here and I would probably also be headed in the direction of passing off the permission check to special use-case objects from within the main security service object for those conditions like you mentioned (in other words, having a single point of entry for permission checking). But as of yet I've just not headed down that road -- it seems to me like it might be getting into diminishing returns where there's a higher cost in complexity than I'm willing to pay to solve the problem.
In my case the special cases generally apply to a single page, so just putting the logic in the controller for that page doesn't create any duplication. If I started seeing duplication of a particular piece of logic, I might wrap that up into a method call on some object. Like in this case I might have something like user.isAuthor(photoid) -- that's probably a poor example because there's so little logic. I'm thinking more like if there were 2 or more conditions. Though in my case I've actually got a big XML-based "rule manager" component that I use sometimes for allowing users to configure complex rules for access to the items they've authored. And so at that point then I've got a whole other layer where I grab the rulemanager they configured and use it to test for an override. And I would guess that's a lot more work than is really needed here.
re: keeping the view ignorant - yep, that's pretty much how I do that. Although I will admit that I'm not the most religious person with regard to keeping my views clean. Though in my case I've also got an HTML abstraction layer that allows me to modify the view after-the-fact, which means that an unclean view can often be "corrected" prior to rendering. And yes the theory behind it is that then the controller can make decisions about whether a particular view should use the default behavior or do something different.
re: request-based short-circuit - yep, you're on the right track there... I've got 2 objects, one in application and one in request. So all you would really have to do is create an extra object in onRequestStart and then use that as a facade to pass the permission checks off to the application object (so it becomes your single point of entry). Then if it already has a cached answer, it doesn't have to bother the application object, it can just return the cached answer.
re: author-based permissions - in my security model the object isn't part of the question, just the task. I consider cases where the specific object is part of the security model as "special use cases" ... or you might say "edge cases". So the generic security model's only role is to understand tasks and their relationships to users/members. Anything more complex than that is relegated to 3rd-party logic which then determines whether or not I choose to override the default permissions.
You could in theory choose to assign a separate permission for editing your own stuff vs. editing someone else's - editing someone else's stuff could be a sub-permission underneath just plain editing. Of course, you'd still have to check that permission manually, but that would allow you to assign certain people (moderators) the ability to edit other people's entries. In the member plugin there are 2 different pages that update a member account - one for a member to update their own and one for an admin to update a member (and assign roles), so in that case as an example there are actually two separate "edit member" permissions. It's basically the same deal - can you edit yourself vs. can you edit someone else. But the way it's handled is by making the "edit my profile" page force the userid for that page to be the userid for whoever's logged in. I'm not sure that would work here though. I don't think so.
and one other thing I forgot to mention - in my security model I also short-circuit if the current user is a member of the "admin" role. So it doesn't even bother checking, it just says "oh you're an admin, yes, you can".
Anyway, I'm not so sure how coherent these comments are... I feel terribly distracted. :P Did I answer all the questions?
@Ike,
Thanks for answering all of my questions. Don't worry, that was all very coherent. You've given me some good stuff to think about. Gotta let it digest before I think about how to make my next move.
Cool, thanks. Glad you're able to make sense of it. :)