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

Enable multifactor authorization flows using Authenticator app and SMS #2274

Merged
merged 56 commits into from Nov 28, 2018

Conversation

Projects
None yet
2 participants
@kevinrobinson
Copy link
Contributor

kevinrobinson commented Nov 27, 2018

This is a replacement for #2248, since it batches in much more automated and manual testing.

Who is this PR for?

students, families, districtwide admin

What problem does this PR fix?

We don't have multifactor authentication enabled for any users yet, and it's hard to update the login flow since it's embedded in the core layout, and because changes here involve interop with Devise.

What does this PR do?

First, factors out the sign in form to its own page, outside of the navbar. This enables better isolation here, and some styling is improved and cleaned in the process.

Second, this updates the form and authentication setup in Devise and our strategy to allow an additional login_code parameter. For the standard login, this is set to a placeholder value on the form (since Devise requires all parameters are always passed).

Third, this adds JS to the sign in page that allows a multifactor login flow (see below). This PR does not enable this flow for any users.

Fourth, this adds server models and endpoints for two kinds of multifactor authentication flows: an using an authenticator app or SMS. The authenticator app is more secure but more friction, with the idea that it can be for developer accounts to start, while SMS could be enabled for project leads and districtwide admin. Migrating users will take a while and have to be done in phases; this PR assumes this is not enabled for anyone to start. These endpoints are protected by Rack::Attack and by ConsistentTiming.

Fifth, this adds more layers of tests for all files touched, including more edge cases to the authentication specs, specifically ones that test whether it guards against invalid upstream and downstream behavior. It also factors out the actual call to the LDAP server into LdapAuthenticator and adds another layers tests there as well.

EDIT: This PR also includes

  • Remove all logging for request parameters (part of #2274)
  • Scrub Rollbar.rb logging (part of #2274)

Screenshot (if adding a client-side feature)

Sign in styling, with "Use multifactor login" link

This form works the same as before, just also sending a placeholder login_code to keep the Devise API the same across both flows.
screen shot 2018-11-27 at 7 13 24 pm

Multifactor sign in, step 1

No server interaction.
screen shot 2018-11-27 at 7 13 30 pm

Multifactor sign in, step 2

screen shot 2018-11-27 at 7 13 50 pm

Updated invalid login message, same for all reasons, including multifactor required

screen shot 2018-11-27 at 7 14 09 pm

Required fields warnings across browsers

screen shot 2018-11-20 at 12 26 04 pm

screen shot 2018-11-20 at 12 25 44 pm

screen shot 2018-11-20 at 12 25 35 pm

Checklists

Which features or pages does this PR touch?

  • Sign in
  • Navbar
  • Core

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Included specs for changes
  • Improved specs for existing code in need of better test coverage
  • Manual testing made more sense here

Deploying requires some config changes as well. To deploy, I'll deploy to demo first and do more manual testing there as well before deploying this to any production sites. The first production deployments will also omit configuration that prevent SMS authentication altogether.

@studentinsights-bot

This comment has been minimized.

Copy link

studentinsights-bot commented Nov 27, 2018

@kevinrobinson, this looks like it might be worth double-checking! @kevinrobinson might be able to help.

kevinrobinson added some commits Nov 27, 2018

@kevinrobinson kevinrobinson force-pushed the feature/multifactor-enabled branch from 9739ad2 to b8655ef Nov 28, 2018

@kevinrobinson kevinrobinson temporarily deployed to somerville-teacher-tool-demo Nov 28, 2018 Inactive

@kevinrobinson kevinrobinson temporarily deployed to somerville-teacher-tool-demo Nov 28, 2018 Inactive

@kevinrobinson kevinrobinson temporarily deployed to somerville-teacher-tool-demo Nov 28, 2018 Inactive

@kevinrobinson kevinrobinson temporarily deployed to somerville-teacher-tool-demo Nov 28, 2018 Inactive

@kevinrobinson kevinrobinson temporarily deployed to somerville-teacher-tool-demo Nov 28, 2018 Inactive

@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Nov 28, 2018

🚢

@kevinrobinson kevinrobinson merged commit 32ff482 into master Nov 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kevinrobinson kevinrobinson deleted the feature/multifactor-enabled branch Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment