Lindsey - Pipes - media ranker

[VirtualLindsey]

No description provided.

[droberts-sea]

This is a step in the right direction, but it still falls short of our expectations.

The good:

1. Logging in via GitHub now works as expected
2. Authorization logic has moved to the controller where it belongs, allowing you to block access to non-GET actions

The bad:

1. Many controller actions are still not restricted based on who is logged in. For example, I was able to edit a work I did not own by typing /works/1/edit into my browser's URL bar.
2. The way you've implemented auth checking sets you up for vulnerabilities if you ever expand the application (see below)
3. Many of your tests are still failing, and you haven't added test coverage around the authorization story for Works. For example, for works#destroy you would need to add three things:
   • A test to make sure a not-logged-in user cannot destroy a work
   • A test to make sure a logged-in user who does not own the work cannot destroy it
   • Update the existing tests to include logging the user in before running the action

Security is a matter of doing a large number of little things right. It requires focus, patience and attention to detail. Testing is incredibly important, as is a constant awareness of the question "what happens when I'm wrong".

Please make the changes above and resubmit this project. The deadline remains Monday 11/13. You don't need to close and re-open the PR, git will automatically update this one when you push.

[droberts-sea]

This logic is repeated throughout this controller, presenting two problems:

1. The code is not DRY
2. When you add a new controller action, you need to remember to add this extra check.

Instead, you should define a controller filter using before_action. If defined in the ApplicationController it will be applied to all controller actions throughout the site, and you can mark individual actions as not requiring login using skip_before action only: [:action, :action]. Because it's applied to all new actions by default, this strategy makes you much less likely to leave a hole in your application by accident.

[droberts-sea]

While this strategy of recording the User's username works fine, it's a bit roundabout. The standard technique here would be to add a user_id column to the works table and a belongs_to / has_many relationship between Work and User. This has three advantages over tracking the username:

1. Since the user_id is stored in the session, you can check work ownership without making a query against the users table to translate ID to username.
2. Databases are designed with a very particular organization in mind, in this case that the foreign key in one table is the primary key of another. Matching this assumption will give you a nice performance boost with very little effort.
3. Using belongs_to / has_many gives you access to all the built-in ActiveRecord methods.

[droberts-sea]

I still don't understand what this file is doing here. Looks like a copy-paste problem - please remove it.

[droberts-sea]

This looks much better. Thank you for taking the time to go back and update this.