Why it’s good to break the MVC pattern
Bruce Perens hit on a really good thing when he wrote a package for Ruby on Rails called Model Security. It’s too bad the project is gathering dust. But even if you don’t use the whole thing (I haven’t been able to) there are some really valuable ideas and chunks of code in there.
The idea behind Model Security is to centralize security rules in the model classes. Certain objects can only be accessed by certain users. Perens talks about multi-layered security. But in my mind the real benefit is that you can just write the basic rules in one place and not worry about it everywhere else.
An apparent problem with this strategy is that it violates the encapsulation of the MVC pattern. The only way to put security into the Model part of the pattern is for the Model to know who is trying to access it. The concept of the user is generally localized to the controllers in an MVC pattern. Maybe the view. But definitely not the model. In MVC, the model is supposed to stand entirely on its own and not depend on anything except maybe the persistence mechanism (i.e. the database). So in this way Model Security violates the basic MVC pattern. Violating well-known design patterns is bad, right?
Absolutely not! In this case it’s actually a really good thing. Developers who blindly follow the MVC pattern end up copying and pasting the same security code all over their controllers. Every place that could possibly modify data needs to check security rights. Any place the developer forgets to do this represents a security hole. By putting security rules in your models, you know everything is secure against hackers. Then in your controllers you just need to worry about preventing your users from accidentally seeing security exceptions that would confuse and distress them. The result is cleaner, more maintainable, more secure code.
Unit tests
"What about unit tests?" I hear you cry. Good
question! For good reasons, we like having unit tests that run on the
models without the web framework in place. But with ModelSecurity, the models depend on the user object, which is generally a part of the web session. So we’re kinda stuck. Encapsulation is broken, and thus follow our unit tests. The easy answer is to use a
global configuration setting that turns the model security checking on
and off. When you’re processing a web request, turn it on. When
you’re running unit tests, leave it off. I’m thinking this should be pretty easily done in application.rb. Or perhaps through an IOC method in the tests themselves. But I haven’t actually revived the unit tests in this project so I couldn’t tell you for sure. Sloppy, I know, but it’s a lot easier to justify when there’s only one coder on the project. I’ll post an update when I dive back into this project.
Problems with Peren’s ModelSecurity gem
I’ve experienced some bizarre interactions with FCGI at least on dreamhost. The ModelSecurity subsystem seems to crash at some point and then opens everything up to allow free access for everybody until I restart the FCGI process. This is absolutely not acceptable. On a somewhat similar note, sometimes basic functions will fail on first execution claiming things like "NoMethodError" but will work fine on subsequent reloads. Having very little interest in debugging this interaction, I have given up on using Perens’ fine-grained rules. The ModelSecurity allows you to specify very carefully which data fields can be accessed by which users under which conditions.
In my app, and many others I can imagine, it’s enough to set security at the row or object level. This is relatively straightforward with ActiveRecord’s own callbacks.
class SecureObject < ActiveRecord::Base has_one :user
#Implement model-based security before_save :check_is_me def after_find # For performance reasons, you have to explicitly define an after_find method. # You can't link it in with "after_find :check_is_me" like other AR callbacks. check_is_me end
def check_is_me if !is_me? raise "Security exception. Not your object!" end end
def is_me? return (User.current) && (User.current.id == self.user_id) end
I still use a valuable construct from the ModelSecurity package,
which is the User.current class method which keeps track of who is
currently logged in thread local storage. This global variable is what enables us to break the MVC pattern by giving the Model access to information about the User from the Controller. Here’s a relevant snippet from Perens’ user_controller.rb:
def User.current # This does not refer to the session because the application has set # this from the session in user_setup. Thread.current[:user] end
def User.current=(u) Thread.current[:user] = u
session = Thread.current[:session]
if session.nil? message = "Programming error: Please add \"before_filter :user_setup\" to your application controller. See the ModelSecurity documentation."
raise RuntimeError.new(message) end
# Don't cause a session store unnecessarily if session[:user] != u session[:user] = u end end
The missing ModelSecurity migration
Another problem is the lack of a migration to add the tables required by Perens’ code. Fortunately, it’s not hard to reverse-engineer using schema.rb and the .sql files that Perens provides. Here’s db/migrate/###_add_modelsecurity_tables.rb: (the filename is important — read on)
class AddModelsecurityTables < ActiveRecord::Migration def self.up create_table "user_configurations", :force => true do |t| t.column "email_confirmation", :integer, :limit => 3, :default => 1, :null => false t.column "email_sender", :text, :default => "", :null => false t.column "created_on", :timestamp t.column "updated_on", :timestamp end
create_table "users", :force => true do |t| t.column "login", :string, :limit => 40, :default => "", :null => false t.column "name", :string, :limit => 128, :default => "", :null => false t.column "admin", :integer, :limit => 1, :default => 0, :null => false t.column "activated", :integer, :limit => 1, :default => 0, :null => false t.column "email", :string, :limit => 80, :default => "", :null => false t.column "cypher", :text, :default => "", :null => false t.column "salt", :string, :limit => 40, :default => "", :null => false t.column "token", :string, :limit => 10, :default => "", :null => false t.column "token_expiry", :timestamp t.column "created_on", :timestamp t.column "updated_on", :timestamp t.column "lock_version", :integer, :default => 0, :null => false end
add_index "users", ["login"], :name => "login" add_index "users", ["email"], :name => "email" end
def self.down drop_table :users drop_table :user_configurations endend
In classically annoying Rails style, if the class name of your migration doesn’t perfectly "match" the filename then rake migrate will fail mysteriously with an unhelpful error message and a mile-long stack-trace with none of your code in it. E.g. if you name the above file 005_add_model_security_tables.rb (note the extra underscore between "model" and "security") you’ll get an error message like this:
rake aborted!uninitialized constant AddModelSecurityTables
or if you run rake migrate --trace you’ll get this stack trace:
** Invoke migrate (first_time)** Invoke db:migrate (first_time)** Invoke environment (first_time)** Execute environment** Execute db:migraterake aborted!uninitialized constant AddModelSecurityTables/usr/lib/ruby/gems/1.8/gems/activesupport-1.4.1/lib/active_support/dependencies.rb:266:in `load_missing_constant'/usr/lib/ruby/gems/1.8/gems/activesupport-1.4.1/lib/active_support/dependencies.rb:452:in `const_missing'/usr/lib/ruby/gems/1.8/gems/activesupport-1.4.1/lib/active_support/dependencies.rb:464:in `const_missing'/usr/lib/ruby/gems/1.8/gems/activesupport-1.4.1/lib/active_support/inflector.rb:250:in `constantize'/usr/lib/ruby/gems/1.8/gems/activesupport-1.4.1/lib/active_support/core_ext/string/inflections.rb:148:in `constantize'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/migration.rb:366:in `migration_class'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/migration.rb:346:in `migration_classes'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/connection_adapters/mysql_adapter.rb:248:in `inject'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/migration.rb:342:in `each'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/migration.rb:342:in `inject'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/migration.rb:342:in `migration_classes'/usr/lib/ruby/gems/1.8/gems/activerecord-1.15.2/lib/active_record/migration.rb:330:in `migrate'
Then you might grep your code for "AddModelSecurityTables" and find that it’s not there because you have "AddModelsecurityTables" (difference in upper- vs. lower-case S). This kind of thing is why Rails is still a bad choice for complex projects — a small hard-to-see typo results in the system not running and providing almost no useful feedback about what’s wrong. And yet we keep trying to use Rails. Because it seems to have so much potential.
on website scalability points out, raw performance does not matter if you can add more servers, which you can with RoR. But as I learned yesterday, Ruby on Rails does not scale well in terms of complexity. A friend once aptly pointed out that