0

Okay so I'm using AASM in my Rails app to manage workflows around my User model. In the view layer, I want to make a little dropdown that displays all the available transitions for a given user and perform that transition when clicked, which adjusts dynamically depending on what state the user is in. It works okay. Here's the basic implementation:

views/admin/users/index.html.erb:

<% user.available_transitions.each do |t| %>
  <%= link_to(t.to_s.humanize, eval("#{t}_admin_user_path(user)"), class: 'dropdown-item', :method => :post) %>
<% end %>

And in the routes file:

namespace :admin do
    ...
    resources :users do
      member do
        post 'apply'
        post 'invite_to_interview'
        post 'approve_for_training'
        ...
      end
    end
  end

Each one has a corresponding action in the controller. It isn't really worth listing each one, they just call the appropriate transition, such as @user.invite_to_interview! with some begin/rescues to catch invalid transitions due to guards, etc.

Brakeman is freaking out about the eval() method in the view. I'm not 100% sure why it's bothered by this - is it just treating all eval() methods as evil? The object that's being passed into the method (t) isn't a user input, it's based on the transitions made available by the state machine. I don't know how this is something that could be a vulnerability? Maybe this is my lack of understanding about some underlying concept though...

So two questions really:

  • Should I be worried and why? What's the potential issue here, and if there is a problem how should I go about fixing it?
  • In general, this feels like a cumbersome way of implementing things because every time you add a new state, transitions etc you have to create a controller action and route entry. Is there a better method for implementing this? Maybe some kind of generic transition action in the controller that has the transition passed in? Seems cleaner, interested to hear if anyone else has taken this approach.

Thank you! I appreciate the help.

stooshie45
  • 39
  • 7
  • 5
    The alternative to this is to change `eval` to `public_send("#{t}_admin_user_path",user)` – engineersmnky Dec 02 '19 at 18:38
  • 1
    I'm not sure what you mean by "freaking out", but if I were reviewing this code, I would definitely not let it pass. There is exactly 0 reason to use `eval` over `public_send` in this code. – Jörg W Mittag Dec 02 '19 at 18:59
  • Thanks, wasn't aware of `public_send` - will check it out. Still, interested in why `eval` is so evil? Especially in this context? – stooshie45 Dec 02 '19 at 20:28

1 Answers1

3

Is there a better method for implementing this? Maybe some kind of generic transition action in the controller that has the transition passed in? Seems cleaner, interested to hear if anyone else has taken this approach.

How about something along the lines of: 

<% user.available_transitions.each do |t| %>
  <%= link_to t.to_s.humanize, admin_user_path(user, transition: t), class: 'dropdown-item', method: :patch %>
<% end %>

Then, in the update method of your AdminUsersController (or wherever admin_user_path using the patch HTTP verb resolves to), you can test for the presence of the transition param and act accordingly. You can include, as you say, some begin/rescues to catch invalid transitions due to guards, etc.

That way, you have only one action for all of your links.

jvillian
  • 19,953
  • 5
  • 31
  • 44
  • Thanks! That seems like such a logical suggestion now you've pointed it out, and will definitely help me clean up a lot of this code. – stooshie45 Dec 02 '19 at 20:31