This issue is addressed to these files in the ProjectTracker.Library project:
Project.cs (up to rev.1722)
Resource.cs (up to rev.1722)
Admin\Roles.cs (up to rev.1320)
I wish to talk about the moving any authorization code from
the Factory methods (which runs on the client side) to the Data Access methods
(which runs on the server).
It looks everybody here knows about this issue in the ProjectTracker.
And there are nearly everyweek-questions from the CSLA-newbies (as I am) about it.
I think these changes should be placed into the svn at last.
The changes to Project.cs (based on current rev.1722):
#region Factory Methods
public static Project NewProject()
{
//if (!CanAddObject())
// throw new System.Security.SecurityException(
// "User not authorized to add a project");
return DataPortal.Create<Project>();
}
public static Project GetProject(Guid id)
{
//if (!CanGetObject())
// throw new System.Security.SecurityException(
// "User not authorized to view a project");
return DataPortal.Fetch<Project>(new Criteria(id));
}
public static void DeleteProject(Guid id)
{
//if (!CanDeleteObject())
// throw new System.Security.SecurityException(
// "User not authorized to remove a project");
DataPortal.Delete(new Criteria(id));
}
private Project()
{
_resources = ProjectResources.NewProjectResources();
_resources.ListChanged += new System.ComponentModel.ListChangedEventHandler(_resources_ListChanged);
}
void _resources_ListChanged(object sender, System.ComponentModel.ListChangedEventArgs e)
{
OnPropertyChanged(null);
}
// public override Project Save()
// {
// if (IsDeleted && !CanDeleteObject())
// throw new System.Security.SecurityException(
// "User not authorized to remove a project");
// else if (IsNew && !CanAddObject())
// throw new System.Security.SecurityException(
// "User not authorized to add a project");
// else if (!CanEditObject())
// throw new System.Security.SecurityException(
// "User not authorized to update a project");
//
// return base.Save();
// }
#endregion
#region Data Access
[Serializable()]
private class Criteria
{
private Guid _id;
public Guid Id
{
get { return _id; }
}
public Criteria(Guid id)
{ _id = id; }
}
[RunLocal()]
protected override void DataPortal_Create()
{
if (!CanAddObject())
throw new System.Security.SecurityException(
"User not authorized to add a project");
_id = Guid.NewGuid();
_started.Date = DateTime.Today;
ValidationRules.CheckRules();
}
private void DataPortal_Fetch(Criteria criteria)
{
if (!CanGetObject())
throw new System.Security.SecurityException(
"User not authorized to view a project");
using (SqlConnection cn = new SqlConnection(Database.PTrackerConnection))
{
cn.Open();
using (SqlCommand cm = cn.CreateCommand())
{
cm.CommandType = CommandType.StoredProcedure;
cm.CommandText = "getProject";
cm.Parameters.AddWithValue("@id", criteria.Id);
using (SafeDataReader dr = new SafeDataReader(cm.ExecuteReader()))
{
dr.Read();
_id = dr.GetGuid("Id");
_name = dr.GetString("Name");
_started = dr.GetSmartDate("Started", _started.EmptyIsMin);
_ended = dr.GetSmartDate("Ended", _ended.EmptyIsMin);
_description = dr.GetString("Description");
dr.GetBytes("LastChanged", 0, _timestamp, 0, 8);
// load child objects
dr.NextResult();
_resources.ListChanged -= new System.ComponentModel.ListChangedEventHandler(_resources_ListChanged);
_resources = ProjectResources.GetProjectResources(dr);
_resources.ListChanged += new System.ComponentModel.ListChangedEventHandler(_resources_ListChanged);
}
}
}
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_Insert()
{
if (!CanAddObject())
throw new System.Security.SecurityException(
"User not authorized to add a project");
using (SqlConnection cn = new SqlConnection(Database.PTrackerConnection))
{
cn.Open();
using (SqlCommand cm = cn.CreateCommand())
{
cm.CommandText = "addProject";
DoInsertUpdate(cm);
}
}
// update child objects
_resources.Update(this);
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_Update()
{
if (!CanEditObject())
throw new System.Security.SecurityException(
"User not authorized to update a project");
if (base.IsDirty)
{
using (SqlConnection cn = new SqlConnection(Database.PTrackerConnection))
{
cn.Open();
using (SqlCommand cm = cn.CreateCommand())
{
cm.CommandText = "updateProject";
cm.Parameters.AddWithValue("@lastChanged", _timestamp);
DoInsertUpdate(cm);
}
}
}
// update child objects
_resources.Update(this);
}
private void DoInsertUpdate(SqlCommand cm)
{
cm.CommandType = CommandType.StoredProcedure;
cm.Parameters.AddWithValue("@id", _id);
cm.Parameters.AddWithValue("@name", _name);
cm.Parameters.AddWithValue("@started", _started.DBValue);
cm.Parameters.AddWithValue("@ended", _ended.DBValue);
cm.Parameters.AddWithValue("@description", _description);
SqlParameter param =
new SqlParameter("@newLastChanged", SqlDbType.Timestamp);
param.Direction = ParameterDirection.Output;
cm.Parameters.Add(param);
cm.ExecuteNonQuery();
_timestamp = (byte[])cm.Parameters["@newLastChanged"].Value;
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_DeleteSelf()
{
DataPortal_Delete(new Criteria(_id));
}
[Transactional(TransactionalTypes.TransactionScope)]
private void DataPortal_Delete(Criteria criteria)
{
if (!CanDeleteObject())
throw new System.Security.SecurityException(
"User not authorized to remove a project");
using (SqlConnection cn = new SqlConnection(Database.PTrackerConnection))
{
cn.Open();
using (SqlCommand cm = cn.CreateCommand())
{
cm.CommandType = CommandType.StoredProcedure;
cm.CommandText = "deleteProject";
cm.Parameters.AddWithValue("@id", criteria.Id);
cm.ExecuteNonQuery();
}
}
}
protected override void OnDeserialized(System.Runtime.Serialization.StreamingContext context)
{
_resources.ListChanged += new System.ComponentModel.ListChangedEventHandler(_resources_ListChanged);
}
#endregion
These changes should be applied to Resource.cs and to Roles.cs also.
Rules to make changes for Resource.cs are (move authorization code from ... to ...):
1. NewResource() -> DataPortal_Create() and DataPortal_Insert()
2. DeleteResource() -> DataPortal_Delete()
3. GetResource() -> DataPortal_Fetch()
4. "if (!CanEditObject()) throw ..." from Save() -> DataPortal_Update()
5. remove Save().
code for Roles.cs:
public override Roles Save()
{
//// see if save is allowed
//if (!CanEditObject())
// throw new System.Security.SecurityException(
// "User not authorized to save roles");
// do the save
Roles result;
result = base.Save();
// this runs on the client and invalidates
// the RoleList cache
RoleList.InvalidateCache();
return result;
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_Update()
{
// see if save is allowed
if (!CanEditObject())
throw new System.Security.SecurityException(
"User not authorized to save roles");
this.RaiseListChangedEvents = false;
...
}
With these changes the ProjectTracker improves its security against the hacking the server side with the hacker's version of the client application.
Of course there are little benefits also:
1. The factory methods for the Project object are as simple as:
public static Project NewProject()
{
return DataPortal.Create<Project>();
}
public static Project GetProject(Guid id)
{
return DataPortal.Fetch<Project>(new Criteria(id));
}
public static void DeleteProject(Guid id)
{
DataPortal.Delete(new Criteria(id));
}
2. There is no more requirements to override the Save() method in the Project and Resource business objects.
3. After putting these changes to the svn, Rocky and others can answer to a questions about this issue with just a "look at current code" sentence ;)
---
Regards,
Oleg
I recall doing something like this a while ago. I am pretty sure I did not end up keeping it this way though. As I recall there is a reason that code was in the Shared Factory methods. I can't quite recall what it is though. Maybe someone else can chime in here.
Joe
Well, I don't know if this was ever an "official stated reason", but I can see performance as one issue. Checking the security on the client side potentially eliminates a hop to an application server. The object graph, and all the associated context, doesn't have to be serialized and sent to the server, just to serialize an exception and send it back to the client. Yes, it's entirely possible that your security checks have to hit the database anyway. But that would be done within the context of a (presumably much smaller) specific security object.
As for the security aspect - there have been a number of discussions lately concerning security and CSLA, and they all seem to center around how to protect yourself if a hacker manages to get into your client application. While I will agree that could be a concern, I always come back to this point:
If a hacker has managed to infiltrate your client application to the point that they can reverse-engineer something, you have bigger issues that you will have a very hard time protecting yourself against. Moving the authorization code to the server does not, in my humble opinion, help you much in that situation. The reason is that they can simply reverse-engineer the application and fake whatever credentials they need to pass your security checks. Chances are, in order to even get to your application, they'll need to have hacked a network account anyway, and presumably said user already has some kind of access to your application. So, armed with appropriate credential information, it's not a big step to take to fake access. Everything else they can get from the code.
As has been mentioned before, you can encapsulate the DP_ code in a separate assembly that it not distributed to the client, so that even if they manage to hack your code, they won't be able to replicate the server calls. That - again, in my humble opinion - is about as secure as you're going to get from a coding perspective.
There are obviously other technologies that come into play - SSL, certificates, strong-naming - any of which will significantly add to the levels of security of your application, if they are needed. If your hacker can get past all of those, then hacking the code is the least of the issues remaining. I'm not saying that you shouldn't include defensive programming techniques - but realize what it buys you.
The only other option is to assume that no tier of your application is safe, and treat them all like an external resource. That will significantly impact your overall application design, diluting some of the advantages of CSLA, and will likely significantly decrease the performance of your appliction as well.
Copyright (c) Marimer LLC