Last active
March 21, 2019 00:05
-
-
Save Eloi/b81a5d54c655ca1be8b77e1ba19500a7 to your computer and use it in GitHub Desktop.
/app\controllers\todo_list_controller.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
class TodoListsController < ApplicationController | |
before_action :set_current_user | |
def index | |
sort_by = params[:sort] || 'created_at' | |
sort_order = (params[:asc] == 'false') ? 'ASC' : 'DESC' | |
# COMMENT: Feeding unsanitized query params as string into the database is always A VERY BAD IDEA. Luckily in this example | |
# the attack vector seems restricted to the sorting, but who knows if there is any bug in the ActiveRecord order method | |
# that allows to widen the scale of the attach to a full SQL injection code. | |
# | |
# The proper way to do it is what is done in the sort_order, where we always control what strings are fed to the database. | |
@lists = TodoList.where(user_id: User.current_user.id).order("#{sort_by} #{sort_order}") | |
end | |
def show | |
# We must check for user ownership, otherwise a malicious user could see all the other users TodoList items just changing | |
# the id in the url (something dead easy to do if the id is the typical sequential integer of most SQL databases). | |
@list = TodoList.find(params[:id]) | |
end | |
# Mark an item as done and return user back from whence they came! | |
def complete_item | |
# We must check for user ownership, otherwise a malicious user could complete the other users TodoList items just changing | |
# the id in the url (something dead easy to do if the id is the typical sequential integer of most SQL databases). | |
TodoItem.find(params[:id]).complete! | |
redirect_to :back | |
end | |
def new | |
@list = TodoList.new | |
end | |
def create | |
# I don't see any strong params here ;) Again, unsanitized input is a no-no. | |
@list = TodoList.new(params[:todo_list]) | |
if @list.save! | |
redirect_to todo_lists_url | |
else | |
flash[:notice] = "Error saving list" | |
redirect_to new_todo_list_url | |
end | |
end | |
def set_current_user | |
User.set_current_user(session[:user_id]) | |
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# COMMENT: From the complete! method, we could deduce that the database has a boolean "done" field for | |
# TodoItem records. | |
# | |
# I know that not defining the fields each model contains and define only the relations and validations is the recommended | |
# way to go in ActiveRecord, but it makes me frown every time I see it. | |
# | |
# The model is esentially a contract, to expose a domain model data to be manipulated from the rest of the code. Not knowing | |
# what its inside this model at first sight, and having to resort to look at methods or at another external tool (like a database | |
# schema dump) to know what fields is supposed to hold, which are allowed values and ranges, etc. makes zero sense IMHO. | |
# | |
# Luckily for me, most of the time I've been working in Mongoid backed apps, where is mandatory to define the fields (as Mongo | |
# is schemaless) :) | |
# | |
# This is just my opinion, but I would gladly have preferred that given the tendency of Rails core developers to love magic | |
# and convention over configuration, that the default would be something like Django South, where migrations are done | |
# automatically to reflect the changes you do in the model fields. | |
class TodoItem < ApplicationRecord | |
belongs_to :todo_list | |
validates :title, presence: true | |
# Completes to do item, saving to database. | |
def complete! | |
update!(done: true) | |
end | |
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
class TodoList < ApplicationRecord | |
has_many :todo_items | |
belongs_to :user | |
before_save :set_usr | |
# COMMENT: Assuming "title" is a TodoList field holding the title of the list, I don't see which value this method | |
# coud add. Probably the developer meant something along the lines of "todo_items.where(done: true).empty?" instead. | |
# And this will match for lists without any items in it, so an extra check to take in account that would be good to do. | |
def formatted_title | |
title + (todo_items.empty? ' (Complete)' : '') | |
end | |
# Assign this list to the currently signed in user when it's createdç | |
# | |
# COMMENT:Lots of thing wrong in the set_user method: | |
# | |
# 1 - Never use "save" in an before_save callback (in fact never user it un any database-related callback, except perhaps | |
# in after_create, and there are very few use cases where this would make sense, like bootstraping some data coming from | |
# some slow asynchronous source. | |
# | |
# 2 - Even ignoring the "save", this method does not make much sense to me. We are coupling the new todo_list to the current | |
# user, and what happens if we want to create todo_lists in some process in backend that is not tied to a user session? or | |
# if we want some admin to create todo_lists in behalf of another user? I don't like this method the slightest, we are | |
# coupling too much the model domain with some specific business logic that is bound to evolve over time. | |
# | |
# In my experience, would be better to skip this method and replace it for some tests that test that the controllers | |
# are injecting the current user when creating the todo_list, taking in acccount also that is the tests coverage is not | |
# enough and somebody slips and tries to enter a todo_list without user, the FK validations will raise an error and | |
# we will have a rollbar (or any other error reporting system in place... because you are using one, for sure ;) ) | |
# | |
# At the very least, I will change it to "self.user_id ||= User.current_user_id" to only set it as default if it was empty. | |
# | |
# 3 - As a minor hindrance, far below the other two, I woudl change the naming. No reason to abbreviate set_usr instead | |
# of set_user. If possible, its better to use the complete model or field names to be 100% sure what are we referring to. | |
def set_usr | |
self.user_id = User.current_user.id | |
save | |
end | |
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
class User < ApplicationRecord | |
has_many :todo_lists | |
# COMMENT: I have two problems with the chosen mechanism to store the current user: the first one conceptual, and the second | |
# one technical/practical. | |
# | |
# 1 - Conceptual problem: Current user does not belong to the User class. A current user it a current user inside a scope, being | |
# it a web request, some code in a job, or any other scope. | |
# It does not make sense conceptually to store it as a class instance variable that is shared by all the code that uses that model. | |
# Every scope should keep track of the current user on his own. The usual method in rails controllers is store it as an | |
# instance variable at controller level automatically on each request throuhg a before_action callback. | |
# | |
# If you want to avoid database hits between requests for the same user, you could use some caching mechanism like Redis | |
# to store the user (and say hi to the wonderful new world of errors and logic handling related to cache invalidation). | |
# | |
# 2 - Technical problem: Despite the conceptual problem, using a class instance variable to keep this data smells like problems | |
# to me. For sure it work depending on the setup, but if we are using multithreaded servers or other methods that access | |
# the same code at once, we will overwrite the current_user constantly and will create a mess as a result. I'm not 100% | |
# sure of what could trigger that behaviour, but I'm pretty confident that given time I could design an scenario where | |
# this approach will fail. | |
def self.set_current_user(id) | |
return if @_current_user.present? | |
@_current_user = User.find(id) | |
end | |
# Avoid a DB query each time we need the currently logged in user | |
def self.current_user | |
@_current_user | |
end | |
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<%-#COMMENT: Former app\views\todo_lists\list.erb, but that name was wrong if you want the Rails "magic" of convention over configuration | |
to happen here -%> | |
<%-# Display the current user's lists along with any uncompleted items -%> | |
<% @lists.each do |list| %> | |
<h2><%= list.formatted_title %></h2> | |
<ul> | |
<% list.todo_items.where(done: false).each do |item| %> | |
<li><% item.title %> - <%= link_to "Done!", complete_item_todo_lists(item), method: :put %></li> | |
<% end %> | |
</ul> | |
<% end %> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<h2><%= @list.formatted_title %></h2> | |
<ul> | |
<% @list.todo_items.where(done: false).each do |item| %> | |
<li><% item.title %> - <%= link_to "Done!", complete_item_todo_lists(item), method: :put %></li> | |
<% end %> | |
</ul> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment