Re: [Rails-core] Nested Resource Route Helpers

[Ryan Bigg]

Hi Michael,

This is the Ruby on Rails Core list, used for discussions about the Rails framework itself.

For discussion about apps built using Rails, go to the rubyonrails-talk mailing list.

Thanks!

--
Ryan Bigg

On Tuesday, 5 June 2012 at 3:10, Michael Boutros wrote:

Hello all,

I'm working on a Rails app and I have resources nested three deep - let's call them user, project, and issues. The route helpers now look like user_project_issue_path(@user, @project, @issue). Would it make sense for Rails to guess the @user and @project relations from @issue? It just feels like a lot of redundant and non-DRY code. I'm willing to write the code myself (or die trying), but I just wanted to make sure this isn't by design.

- Michael Boutros

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/2jig2rKv1xMJ.
To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

[Ryan Bigg]

Oh, reading this again it seems I was wrong.

How would it guess the associations?

[Duncan Beevers]

On Mon, Jun 4, 2012 at 3:32 PM, Ryan Bigg <radarl...@gmail.com> wrote:
> Oh, reading this again it seems I was wrong.
>
> How would it guess the associations?

Indeed

The way that many apps deal with this pain is using shortcut urls like
/issues/5 that simply get the indicated record, look up the parent
records, and then figure out permissions based on the "implicit"
hierarchy.

[Prem Sichanugrist]

I think it make a lot of sense. I'd +1 on this.

I think since we already know what portion of the path is called, we can do something simple like:

if record.respond_to? :project
path_portion[1] = record.prefix
end

(that's psudocode btw, the actual impl will be more complex. Just to get you the idea.)

- Prem

[John Mileham]

One reason that the code to generate all those URLs seems not to be DRY might be that any URL that can be programmatically deduced from the model at the end of the chain is itself non-DRY.  i.e. /users/1/projects/2/issues/3 adds no more information than /issues/3 would in that case.  Obviously in some cases you want nested routes to provide a more human-readable URL, but there are real security pitfalls to using nested routes if you don't unpack the URLs correctly and validate the associations in your receiving controller.

I wonder whether a bit of vinegar might actually be a good thing in this case?  If you really want deeply nested routes, you have to do a little more leg work, because the simpler and safer way is to avoid nesting.  Thoughts?

-john

[Michael Boutros]

John,

I feel like there definitely would be some security risk but I can't think of a real, solid example. Can you lay one out for me?

As for actually implementing this, my train of thought is to use ActiveRecord::Reflections to keep checking what a model belongs_to and then calling that association.

- Michael Boutros

-john

>> To post to this group, send email to rubyonrails-core@googlegroups.com.

>> To unsubscribe from this group, send email to

>> rubyonrails-core+unsubscribe@googlegroups.com.

>> For more options, visit this group at
>> http://groups.google.com/group/rubyonrails-core?hl=en.
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Ruby on Rails: Core" group.

>> To post to this group, send email to rubyonrails-core@googlegroups.com.

>> To unsubscribe from this group, send email to

>> rubyonrails-core+unsubscribe@googlegroups.com.

>> For more options, visit this group at
>> http://groups.google.com/group/rubyonrails-core?hl=en.
>
> --
> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.

> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com.

> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
>

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.

To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com.

[Piotr Sarnacki]

On Tue, Jun 5, 2012 at 4:28 AM, Michael Boutros <michael...@gmail.com> wrote:

As for actually implementing this, my train of thought is to use ActiveRecord::Reflections to keep checking what a model belongs_to and then calling that association.

There is no sane way to guess which association to use. "Issue" from your example may as well belong to category, author or other issue (and many other things). Also, such implementation would be rather slow, complicated and would depend on ActiveRecord's API, while we try to only depend on ActiveModel.

I believe that the implementation should look like the example provided by Prem. In such scenario developer would be responsible to set the prefix for given record, which is good from various reasons:

* we won't break any existing APIs

* it should be pretty fast considering that it will just add respond_to? call for people not using this feature

I have mixed feelings about that, as I don't use nested resources heavily, but I can see that it could make it easier to manage nestings in some of the apps.

 

To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/Zdw0HifJd64J.

To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

--
Piotr Sarnacki
http://piotrsarnacki.com

[Steve Klabnik]

It's been long best practice to not nest more than one deep:
http://weblog.jamisbuck.org/2007/2/5/nesting-resources

[John Mileham]

I feel like there definitely would be some security risk but I can't think of a real, solid example. Can you lay one out for me?

Hi Michael, here's one example.  Let's say that you must be the creator of a project to add issues to it (perhaps contrived in this case, but many access control use cases fit this kind of pattern).  This would be a vulnerable implementation of an issue create action because an attacker could set a different user_id in the url:

def create

  @user = User.find(params[:user_id])

  raise "alert!!" unless @user == current_user

  @project = Project.find(params[:project_id])

  @issue = @project.issues.new(params[:issue])

  if @issue.save

    head :no_content

  else

    head :unprocessable_entity

  end

end

It's easy to dismiss as an obvious mistake, but I've seen it done too many times to want to encourage deeply nested routes by automating the URL generation.  If your controller is forced find the user from the project in order to perform access control, then the odds of making a mistake are much lower:

dev create

  @project = Project.find(params[:project_id])

  raise "alert!!" unless @project.user == current_user

  @issue = @project.issues.new(params[:issue])

  if @issue.save

    head :no_content

  else

    head :unprocessable_entity

  end

end

-john

 

[Matt Jones]

On Jun 5, 2012, at 8:18 AM, John Mileham wrote:
>
> It's easy to dismiss as an obvious mistake, but I've seen it done too many times to want to encourage deeply nested routes by automating the URL generation. If your controller is forced find the user from the project in order to perform access control, then the odds of making a mistake are much lower:
>
> dev create
> @project = Project.find(params[:project_id])
> raise "alert!!" unless @project.user == current_user

Curious - is there a reason to prefer this code to the alternative:

@project = current_user.projects.find(params[:project_id])

which turns attempts to access other's projects into 404s?

--Matt Jones

[John Mileham]

Hi Matt, 

Nope, that would be the Correct way to do it, I was demonstrating a simplistic buggy implementation that I've seen people new to Rails use in the past.  I should reiterate that it's not impossible to write secure code that uses deeply nested routes (I've used them myself when appropriate), it just increases your attack surface by giving you more user input to validate, and is among the reasons that deeply nested routes aren't the best way to go for many use cases.

-john

[Andrew White]

The only thing I really have to add to this conversation is that your routes are not necessarily mapped one to one to your models. You might have a nested url that is actually a different association but there's also an association matching the url - how do you tell it not to use the automatically discovered one? Tying urls and models together means giving up a certain degree of flexibility.

Also the `resources` and `resource` definitions in routes.rb are just macros to reduce the amount of typing needed to define RESTful url patterns. This is perfectly illustrated by the form_for/singleton resource issue (#1769).


Andrew White

[Michael Boutros]

You guys are right. It would be too much work that would only work (accurately) if the three models had strictly one relationship. We could find ways to ensure a greater accuracy but it wouldn't be worth it. And like John said, it has the potential to be greatly misused and open a can of security worms. I'll just stick to what I'm doing now. Thanks :)

On Monday, June 4, 2012 1:10:50 PM UTC-4, Michael Boutros wrote:

Hello all,

I'm working on a Rails app and I have resources nested three deep - let's call them user, project, and issues. The route helpers now look like user_project_issue_path(@user, @project, @issue). Would it make sense for Rails to guess the @user and @project relations from @issue? It just feels like a lot of redundant and non-DRY code. I'm willing to write the code myself (or die trying), but I just wanted to make sure this isn't by design.

- Michael Boutros