Add Devise User

[Katee]

This creates a new model user which will be used to unify Admin, Artist and Voter. Users can register using devise at http://localhost:3000/users/sign_up (currently these pages are unstyled, that will happen in a future PR.)

I also added the letter_opener gem, in development emails will open up in the browser when they are "sent". To see this in action try running FactoryGirl.create(:user) in the rails console.

Gotchas:

• New values have been added to config/secrets.yml which will require new values on the production servers
• Devise stores confirmation_token, reset_password_token, etc directly in the database (passwords are salted/peppered and hashed). The previous tokens were created with BCrypt::Password.create(SecureRandom.urlsafe_base64)). I'm not sure if that behaviour is still required / what threat model pushed for that design choice.

Future PRs:

• Styling
• Use User model

Related issues:

• Use gem for authentication/account activation/password reset/etc #62
• Unify Artist, Admin, and Voter models #63
• implement resending activation tokens #15 (devise will resend activation tokens)

[ywwg]

are these secrets supposed to be stored in the repo? I thought the idea was that individual installations would have their own

[Katee]

Yes, secrets are required for running in test and development. If you read this file you'll notice that in the production section we use values from ENV exclusively. This means that production WILL NOT fall back to these values (I included a comment DO NOT use value in common as fallback to make this clear in the code).

If these secrets are not set in the ENV and rails is running in production then (at least for secret_key_base) rails will raise an error and refuse to run. (I think it will run if auth_secret_key is missing since devise will fallback to using the value for secret_key_base. I also think it will run if password_pepper is missing since that is optional.)

[ywwg]

oh I misread your comment as an instruction to the reader, telling them not to use the value as a fallback (I usually word things like: "This ensures that we don't fall back to common..."). OK I thought the idea was people had to make their own secrets for dev and test as well. I may litter some more comments around.

[Katee]

I usually don't require everyone to make their own secrets for test/dev. If they are binding to localhost it should never matter. If they aren't binding to localhost then someone who sees their dev/test server and knows about this repo could do various things depending on the secret in question (with secret_key_base they could fake sessions) but that seems really low risk.

When including values where there is no test/dev version (such as AWS credentials) usually I mock it out in test so that tests still run and provide instructions in the readme.

[Katee]

It is a command to the reader.

Often the pattern in secrets/config.yml is value: <%= ENV['NAME'] || 'default' %> but with these secrets using that pattern is unsafe as rails would happily work but would use a publicly known value for secret_key_base.

[Katee]

If you want to bikeshed on the comment I am not attached to any wording, feel free to contribute a change directly to this PR.

[ywwg]

We should have a conversation about The Future of FFAGC before you do too much major work, just so you have a sense of the roadmap I have in mind. There are assumptions that the current framework makes that I would like to upend at some point.

We will need the concept of user roles, which should be ORable (someone can be a voter and an admin, for instance). I assume that's something that can be added later?

[Katee]

That is my plan, please see #63.

I will either add a simple roles array to User so each user can be some subset of [:admin, :voter, :artist] or use a more intense solutions such as CanCanCan that more finely defines "abilities" for various users/roles.

[Katee]

I'm definitely interested in the future plans too. My current plan is to get this project in a better spot by adding reasonable validations, writing specs, and removing duplicate code.

[Katee]

This PR is probably easier to read commit by commit. Most of the lines come from the commit adding the devise views.

[ywwg]

I've been thinking about this PR and the general design plan to unify the user types, and unfortunately I think this addition of the new user type is the wrong place to start. If we do this first, then we have two user models at the same time and conflicting frontend choices, which means we need to maintain both versions (keep the deprecated one working while still improving the new one) until the new one is feature complete.

I think a better plan would be to start by writing a shim for the current three-table model so that various bits of the frontend can be rewritten so they can inquire about user permissions in a similar way to how the eventual backend model will work. So instead of "if admin_logged_in || voter_logged_in", there would be some sort of API like [totally pseudocode] "if user.permission_contains ['admin', 'voter']". The various calls can be replaced piecemeal and don't need to be done in one fell swoop. Once all the logic has been rewritten to use the shim API, then the backend model can be replaced with a simple swap -- replace the old model with the new and remove the shim.

Doing the migration that way will keep the codebase cleaner by avoiding having conflicting coexisting models, and will ensure that the master branch stays runnable and deployable.

Redoing the user model is a really heavy lift for this codebase because this transition will be hard to manage. But frankly it's low on my list of priorities compared with fixing some of the UX issues. I've tweaked some of the priority labels in the Issues list to give an idea of where I'd prefer to focus development effort right now.