Fix authorization mass assignment

[jjb]

In my app I have accessible attributes set to empty globally, which forces each model to define its own whitelist.

ActiveRecord::Base.send(:attr_accessible, nil)

This is a pretty common practice AFAIK. Because of this, creating a new OAuth2::Model::Authorization in OAuth2::Model::Authorization.for_response_type was failing for me.

This patch accommodates such environments.

[jcoglan]

I'm not quite sure what this achieves. You make all attributes inaccessible for mass-assignment, and then work around that by essentially reimplementing find_or_create_by* to paper over the restriction. What value does this add? Do you need to do this for all the ActiveRecord libraries you use?

[jjb]

You make all attributes inaccessible for mass-assignment

yes

and then work around that by essentially reimplementing find_or_create_by* to paper over the restriction.

I'm not sure what you mean here.

Let me reexplain from the beginning. It's a very common, standard, recommended practice in Rails to make an initializer which does this:

ActiveRecord::Base.send(:attr_accessible, nil)

And then use attr_accessible in each and every model to specify the whitelist of attributes which may be mass-assigned (that is, the set of attributes that a user is allowed to control from, say, the create and edit web forms for a given resource).

So given that, there's a problem using oauth2-provider without this patch.

Does that make sense?

[jcoglan]

What I'm saying is that with the find_or_new method, you seem to have reimplemented find_or_create_by_owner_and_client in order to work around a security restriction you put in, and this makes the security restriction itself seem less valuable. The restriction is designed to make developers writing forms unable to expose fields the user should not be able to change, but if you provide an alternative API that works around this restriction the holes come back.

I also wonder whether you need to patch all the ActiveRecord libraries you use -- surely they don't all assume all the attributes have been locked down and do everything using the verbose model.attr = value syntax?

Have you tried doing this to make the fields writable:

class OAuth2::Model::Authorization
  attr_accessible :owner, :client
end

[jjb]

What I'm saying is that with the find_or_new method, you seem to have reimplemented find_or_create_by_owner_and_client in order to work around a security restriction you put in, and this makes the security restriction itself seem less valuable. The restriction is designed to make developers writing forms unable to expose fields the user should not be able to change, but if you provide an alternative API that works around this restriction the holes come back.

The restriction is so if any part of the system throws params into a create or update, the ownership aspects of the model can't be changed if a nefarious person puts in extra vars. So, no part of your library is an offender, but your library still has to exist within implementations which set ActiveRecord::Base.send(:attr_accessible, nil)

I also wonder whether you need to patch all the ActiveRecord libraries you use -- surely they don't all assume all the attributes have been locked down and do everything using the verbose model.attr = value syntax?

That's an interesting point... But yeah, I've never had this problem before.

Have you tried doing this to make the fields writable:

class OAuth2::Model::Authorization
 attr_accessible :owner, :client
end

If we did that then the model would be vulnerable to the params injection attack :-D

[jjb]

If we did that then the model would be vulnerable to the params injection attack :-D

Hmm actually, since they aren't _id that might be fine... hmm

[bugant]

The attr_accessible solution should prevent injection attack, right? That is, only owner and client attributes could be mass-assigned, but this should be fine.

[jcoglan]

This is now fixed by @pixeltrix. Sorry it took so long.