Securing Models From a Controller versus ActsAsRowSecured

rubyonrails-talk
1

So, I'm wondering, what do all of you think of ActsAsRowSecured? I'm little wary because it appears to use a singleton UserContext and jigger that back into a SecurityContext class under ActiveRecord. i.e.

class UserContext   include Singleton

  @values = {}

  class << self     def get(key)       @values[key]     end

    def set(key, value)       @values[key] = value     end   end end

class AccessControlled < ActiveRecord::Base   def self.inherited(subclass)     subclass.send(:acts_as_row_secured, :conditions => { :user_id => SecurityContext.context_info(:user_id) })   end

Isn't it pure heresy to reference session data by proxy through Singletons with an ActiveRecord override? This seems like an egregious violation of MVC to me.

Isn't it better to simply apply a scope your AR models in ApplicationController as follows?

class ApplicationController   around_filter ScopedAccess::Filter.new(Posts, :accessible)

protected   def accessible     { :find => {:conditions => ["user_id = ?", session[:user_id]]},       :create => {:user_id => session[:user_id]} }   end

# Adopted from http://www.caboo.se/articles/2006/2/22/nested-with_scope

Can someone confirm my suspicions or allay my fears regarding acts_as_row_secure? Or, am I being a religious zealot by insisting on MVC purity here? Better yet, is there another idiom that's even more ironclad than the caboo.se ScopedAccess one?

Tony

2

Maybe I'm misunderstanding the context, but why not just secure your model through the has-many association? It's the most basic security idiom and is built-in.

I suggested this to my team lead originally. However, in our app there are a lot of models, and multiple tiers of access. So, it's a minor headache for us to manually secure everything that way. But, you're right, associations and lambda'd named_scopes are the least surprising way to accomplish this.

Thankfully, the ScopedAccess stuff is built-in to Rails and does the same thing as acts_as_row_secure in a standard way. So, it's just as viable as an association and a sane compromise. This idiom basically prevents us from having to type in accessible_by(@current_user) on nearly every model in our controllers.

BTW A pattern I seen if you really need to have the model access the current user is to use a class attribute accessor in a before filter.

That is a good point about using the User model cattr_accessor instead of a library class for current_user. Having the UserContext class serve as a session proxy doesn't really sit right with me either. That UserContext business is only needed for acts_as_row_secured because you can't really access a defined superclass of ActiveRecord when you're defining a mixin for ActiveRecord itself.

This is a big reason _not_ to use acts_as_row_secured. UserContext is essentially a dirty hack to sneak session data references into the ActiveRecord definition itself which violates MVC and forces you to have to set global constants when using a model anywhere outside of a controller. This technique is used in all over the place in ad hoc PHP apps which is why it seems wrong to me.

Tony

3

This is a big reason _not_ to use acts_as_row_secured. UserContext is essentially a dirty hack to sneak session data references into the ActiveRecord definition itself which violates MVC and forces you to have to set global constants when using a model anywhere outside of a controller. This technique is used in all over the place in ad hoc PHP apps which is why it seems wrong to me.

Let me play devil's advocate here and take the contrary position to this, giving reasons why you might want to break MVC in this particular way in this particular case:

First, by doing this with acts_as_row_secured you enforce that some UserContext is available during any model access. Although this gets in the way of some simple manual testing (requiring you to have contexts set all the time), by the same token, it enforces that you can't 'fake' your unit tests: You don't get the choice of wrapping a UserContext all the time -- if access security is paramount to you, this is a painful, but enforced way of making sure you have it at all time (including in all your testing).

Second, although this does break "classical" MVC (making the M reach out to the C for the UserContext, or, put the other way, invisibly injecting the UserContext from C down into M), this can be thought of very naturally if you just forget about UserContext altogether; just forget UserContext exits -- it's taken care of for you, by this hack, and it (UserContext) becomes part of the metaobject protocol. So, if you just forget that UserContext is a concept that you have to worry about anylonger, then MVC is no longer broken.

Finally, even if we grant that this breaks MVC (that is, you can't take my proposed stance of just forgetting about UserContext), hacking the metaobject protocol to take account of contextual factors is a well-accepted pattern in many (non-PHP) programming paradigms. One thing that RoR offers is a well encapsulated way of doing this; PHP does not, so, you're right that this is ad hoc and ugly in PHP, but the same thing can be done in RoR in this conceptually clean way. This is a very common, and very clean, pattern in Lisp engineering, where working with the metaobject protocol is a well accepted meta- pattern. This isn't to bias the issue, just to point out that just because a pattern is ugly in language A, it needn't be ugly in Language B, where B offers a clean way to do it.

'Jeff

[Full disclosures: a. I work with Tony, b. Although I'm not a highly experienced Rails engineer, I *am* a highly experienced engineer in many other paradigms OOP and classical paradigms, incl. Smalltalk and Lisp. I'm engaging in this discussion out of real constructive interest in learning; thus the devil's advocate stance.]

4

The problem is that acts_as_row_secured isn't really conceptually clean because you don't know the user_id at ActiveRecord model class creation time. That's why you have to create this mysterious context stuff to punch a hole in the abstraction layer.

It's a matter of pragmatism. Using acts_as_row_secured this way forces you to have to patch Rails itself and set a secret global variable to use unit tests, script/console, rake db:migrate. After that, you have to use a special disable flag to create a new user because there is no UserContext during sign-up to the site.

Using ScopedAccess filters from the controller generates the exact same SQL and doesn't break any of the Rails infrastructure in the process. So, why violate the principle of least surprise, break MVC, rewrite existing Rails features, and patch a bunch of infrastructure when you can accomplish the exact same functionality with two lines of code in ApplicationController?

Tony

5

I found this post from dhh in 2006:

http://groups.google.com/group/rubyonrails-core/browse_thread/thread/a8e60f340a682977/91ecfff96ae35de1

He disagrees with even tacking on a with_scope at the controller level. So, is the Rails way of doing this to explicitly call model functions or named_scopes from the controllers explicitly?

Tony

6

I got some more on this from the engineer who developed the other approach. He writes:

7

The Rails way according to dhh is "don't do that". I agree. Neither approach makes any sense as the main vector for MySQL vulnerabilities is an injection attack. Making the framework less general is both dumb and counterproductive and provides no additional security.

I'd argue that we should just use finder methods and named_scopes in the AR models rather than reinvent the wheel.

Tony

8

Tony Perrie wrote:

I'd argue that we should just use finder methods and named_scopes in the AR models rather than reinvent the wheel.

Tony

earlier you said:

Maybe I'm misunderstanding the context, but why not just secure your model through the has-many association? It's the most basic security idiom and is built-in.

I suggested this to my team lead originally. However, in our app there are a lot of models, and multiple tiers of access. So, it's a minor headache for us to manually secure everything that way. But, you're right, associations and lambda'd named_scopes are the least surprising way to accomplish this.

Minor headache is not a good excuse. You'll only have to write that sort of code once, and then copy it around to your other controllers.