Securing Models From a Controller versus ActsAsRowSecured

Rails
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

Tony P. wrote:

Better yet, is there another idiom that’s even more
ironclad than the caboo.se ScopedAccess one?

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.

class User < ActiveRecord::Base
has_many :posts
end

class ApplicationController < ActionController::Base
def …
current_user.posts.find …
current_user.posts.create …
end
end

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.

class User < ActiveRecord::Base
cattr_accessor :current_user
…
end

class ApplicationController
before_filter :set_current_user

protected

def set_current_user
User.current_user = current_user if logged_in?
end
end

3

On Nov 1, 9:44 am, David L. [email protected]
wrote:

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

4

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.]

5

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

6

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

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

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

The proposed method supports multi-tenancy with the explicit
requirement for role-based permissions for all data views. On the one
hand, the intellectual property of an individual or organization must
be guaranteed. On the other hand, it must be possible to share
information with an arbitrary granularity. All of this must be
accomplished with reasonable database performance. Our model now
assumes that some role is always current.

Enforcing read permission cannot be accomplished as a post-query
filtering operation. Not only would such a solution not scale, but it
would potentially result in incorrect results being returned because
of inappropriate intermediate results being incorporated as part of a
join. Thus, for both efficiency and semantic correctness, we have
engineered a role-based view as part of the data model, affecting
nearly every query. This is most conveniently addressed in the Rails
framework by appending a WHERE clause to the ActiveRecord :find
operator (using the :conditions hook).

To designate the classes under access control, I define an
acts_as_access_controlled mixin based on a plugin downloaded from
RubyForge. This has the advantage of being declarative, making it
obvious which classes are under access control. At run time, a
UserContext (we could call it something different) is dynamically
discovered and used to bind parameters in the prepared SQL statement.
This causes access control to be on by default, which I consider to be
desirable because it is now part of the data model and should be
routinely incorporated into queries during development and testing. An
all-seeing (‘root’) mode is also supported.

Tony has proposed an alternate mechanism to inject the same methods
(or perhaps methods with an SQL string not requiring parameter
binding)
into the models from the application controller. [See above for Tony’s
claims of the superiority of his method.]

Opinions?

9

Tony P. 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.