On Friday, March 9, 2012 8:07:44 AM UTC-8, Jamie Hodge wrote: > > Hi Jeremy, > > I am working on implementing role-based authorization with sequel > (along with warden). I have a sketch here: > > https://github.com/jamiehodge/sinatra-warden-roles > > The sketch is currently using a yaml file. I would like to move over > to a solution that forgoes serialization and uses a more natural > chaining of the subsets. It should also support limiting fields for > updates. > > I am struggling to decide where the responsibility for this control > lies. I was hoping that you be willing to share some insight into how > to best use subsets and set_fields or set_only and whether this logic > should lie in the model classes, in a separate module or somewhere > else entirely. >
I reviewed the code, and I think your current design is OK. Pushing access control down to the model class level only makes sense in rare cases. In most cases, I think it rightly belongs at the controller level. My rules for dealing with user input (i.e. web forms/API): * Anytime you have a fixed input (where the input fields should always be the same), use #set_fields/#update_fields. * Anytime you don't have a fixed input, but know the superset of fields that are allowed, use #set_only/#update_only. Using .create/.new/#set/#update means that users can abuse mass assignment to set fields you didn't expect unless you have specifically set the allowed/restricted attributes at the model class level (ala the recent GitHub vulnerability). Setting allowed attributes at the class level does make sense in certain cases (and isn't a bad fallback for the paranoid), but I prefer just using #set_fields/#update_fields or #set_only/#update_only to deal with user input, as that gives granular control on a case by case basis. My main beef against setting allowed attributes at the model class level is that allowed attributes is generally an authorization issue, and belongs at the controller level. The classic example of this is an app with a user area and an admin area, where the admin can change fields the user cannot. ActiveRecord 3.1+ deals with this using mass-assignment roles, which makes simple cases fairly easy (i.e. simple user/admin split), but is a bad idea in complex cases IMO. If you have 10 forms in your app that each modify a given model in different ways (with different inputs on the form), do you really want to have 10 lines in your model file to list the allowed attributes, one for each form? And every time you add a new web form that modifies a model in a different way, you need to another line? Basically, on your web forms, you usually are defining specific fields. The controller code that handles the form submission should operate on those fields and those fields alone, and #set_fields/#update_fields are designed for that case. For APIs, usually you have a set of allowed fields which may or may not be present, which is the case that #set_only/#update_only is designed for. FWIW, I don't really like the implementation of #set_only/#update_only. It shares code with #set/#update, just changing the allowed fields for the specific call, and it iterates over the input hash instead of over the allowed fields. For a while I've considered expanding #set_fields/#update_fields to take an options hash so that you could pass options like :missing=>:ignore or :skip (don't set missing fields to nil) and :missing=>:raise (raise error on missing fields), so that it can handle more cases (i.e. the API case could just use :missing=>:skip). I think the only reason I haven't done it yet is missing is hard to determine. Using has_key? doesn't deal with symbols when the hash uses string keys (with a default_proc that checks for a string key if given a symbol key). Checking for nil means that if the entry is in the hash (with a nil value), missing wouldn't be accurate terminology. The best compromise I can think of currently is has_key?(field) || has_key?(field.to_s). I'm not thrilled with that either, and this isn't a high-priority issue for me, so it's been on the back-burner. If anyone has thoughts on this issue, I'd certainly like some input. To bring this back to your code, in your post '/' action, you are using .create (which calls .new, which calls #set). I would switch that to .new.update_fields and list the specifically allowed fields (or update_only if that makes more sense). Thanks, Jeremy -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To view this discussion on the web visit https://groups.google.com/d/msg/sequel-talk/-/dISbwKqaHZcJ. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/sequel-talk?hl=en.
