Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentication #76

Merged
merged 12 commits into from
Dec 25, 2019
Merged

Authentication #76

merged 12 commits into from
Dec 25, 2019

Conversation

glassjoseph
Copy link
Collaborator

Resolves #71

Description

Adds Devise gem authentication to app.

Requires authentication for accessing and uploading data. Users cannot sign up on their own and need to be added by a developer. Mailer will need to be configured in production if we want Devise emails for password reset, etc.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update

@jcavena
Copy link
Collaborator

jcavena commented Oct 25, 2019

@glassjoseph I'd move the before_action :authenticate_user! out of the individual controllers and into the application controller. You can then use skip_before_action :authenticate_user! in the one controller/actions combo where you won't need it.

@glassjoseph
Copy link
Collaborator Author

Thanks for the suggestion! I'll do that.

@rafaltrojanowski
Copy link
Contributor

rafaltrojanowski commented Oct 27, 2019

@jcavena I'm not sure about this approach. It's DRY, yes, but won't be 'authenticate_user' method triggered for Devise controllers as well, ie. login, register and logout?
As far I know Devise's default parent controller is ApplicationController.

@glassjoseph
Copy link
Collaborator Author

@rafaltrojanowski I've moved the before_action :authenticate_user! into application_controller, and the login page, forgot password page, etc are all still accessible and working. I guess the devise controllers know how to handle authenticate_user! for those views.

@rafaltrojanowski
Copy link
Contributor

@glassjoseph Thanks for checking it. I've got to hand it to you, @jcavena :)

@jcavena
Copy link
Collaborator

jcavena commented Oct 30, 2019

@rafaltrojanowski Devise might do that automatically, like @glassjoseph found, but you can always skip callbacks in a particular controller for actions you don't want it to run on. Easier to skip the few exceptions than it is to remember to include in all new controllers down the line.

@jcavena
Copy link
Collaborator

jcavena commented Nov 6, 2019

@glassjoseph Any chance you can take a look at the merge conflicts? These may have shown up after your changes so not your fault.

@glassjoseph
Copy link
Collaborator Author

@jcavena Thanks, should be good to go now.

# Configure the e-mail address which will be shown in Devise::Mailer,
# note that it will be overwritten if you use your own mailer class
# with default "from" parameter.
config.mailer_sender = 'please-change-me-at-config-initializers-devise@example.com'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pollygee What email address should someone in Abalone use if they need to ask for Devise help? we should set the config to use that address.

@jtu0
Copy link
Collaborator

jtu0 commented Dec 25, 2019

@Thrillberg did some testing in his dev env (THANK YOU):

  • Ran bundle exec rspec, confirmed everything passes
  • Followed the new README directions, confirmed login appears to log in
  • Confirmed when logged out, gets redirected to a login page when attempting to visit /facilities, /reports, or /file_uploads/new routes

He ran into one problem:
Running rake db:seed throws this error:

rake aborted!                                                                                                                     
CsvImporter::InvalidCsvCategoryError: CsvImporter::InvalidCsvCategoryError

But the same behavior occurs in master as well.

@jtu0 jtu0 merged commit c4dff4b into master Dec 25, 2019
@jtu0 jtu0 deleted the authentication branch December 25, 2019 04:07
@jtu0 jtu0 mentioned this pull request Dec 30, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend/Frontend - Authentication
4 participants