DARIAEngineering / dcaf_case_management

Rails-based case management system for abortion funds
MIT License
87 stars 245 forks source link

Add multifactor authentication #2003

Closed colinxfleming closed 1 year ago

colinxfleming commented 4 years ago

Thanks for creating an issue! Please fill out this form so we can be sure to have all the information we need, and to minimize back and forth.

Up to this point we've taken advantage of googly account security. I looked pretty hard and we unfortunately can't make people have 2fa on their google accounts. We can require general 2fa for all accounts, though, or require it for password entry, or just have it in general. (We should start by having it in general probably.)

Better app security since we're handling so much pii!

very carefully! @BintLopez has been doing some thinking about this over in lockbox town, so hopefully she'll have some insight to add here. (Step 1 of this process should be @BintLopez writing up their implementation on Lockbox and giving us some sense of what was easy, what sucked, and how their userbase handled it.)

I think our ideal outcome is:

I think that should square us away and put us in a good place?

not currently!

BintLopez commented 4 years ago

This is the gem that I'm using and working through: https://github.com/twilio/authy-devise

colinxfleming commented 1 year ago

okay, a writeup of the outcomes here is overdue from me, I think. Here it am:

This ticket should be considered done when:

Other notes:

djohnston32 commented 1 year ago

Unfortunately the Authy API is being deprecated in favor of the new Twilio Verify API, so authy-devise is no longer an option. The authy-devise readme suggests a similar verify-devise plugin was once in development, but it's unclear if that's still the case. I reached out to Twilio support to confirm. Without that gem, there doesn't appear to be a well-maintained option that gives us all of 1) integrating well with devise, 2) providing robust support for a variety of channel types, 3) naturally supporting the most common two-step UI for MFA.

If the pricing is acceptable, Twilio Verify seems like the best option even if a helper gem isn't in development. We’d have to handle integrating it with devise ourselves, but it seems to make everything else relatively easy. It handles generating/storing codes, rate limiting, and other things itself and the documentation seems solid at a glance.

devise-two-factor is another option. It gives you a lot less out of the box, but it doesn't incur any costs on its own and it looks relatively straightforward to set up with app-based authentication. The downside is it looks like it takes quite a bit of wrangling to get it to do a standard two-step login process (see this example), and with SMS we'd have to handle generating and sending codes ourselves. It still gets occasional maintenance updates but given that it seems most other devise two factor gems appear to have stopped development, I'm not sure how long that will last.

colinxfleming commented 1 year ago

I think I'm willing to throw money at this if it does yield a less-maintenance or better experience/result (or if they manage more of it). Our userbase isn't big so 5c isn't problematic (though it is annoying, ha). I think if that seems like an effective path, let me know and I'll start a service - we have an org twilio account we can use for this I believe.

djohnston32 commented 1 year ago

I definitely lean towards Verify. One question I think will help: do you think it's likely you'll want to add any other channels beyond app-based (TOTP) in the future?

I think devise-two-factor would be faster and cheaper to get set up initially for app-based 2FA only (after more research I think I overstated the UI challenges above). But Verify has a clear advantage if you think it's likely you'll want to add any other channels. devise-two-factor is not much help with SMS for example. devise-two-factor also has all of 2 commits in the last 6 months, so the risk of maintenance problems in the future seems higher.

colinxfleming commented 1 year ago

I think my ideal world is that we offer people sms/email/app, though frankly I could live without app! I think most of our userbase is most likely to be SMS/email friendly but might be less comfortable with an app 2fa thing, though I would like to offer that option. I think based on that analysis twilio verify seems like an increasingly good option.

@lomky do you have a strong opinion here? does that seem right to me?

lomky commented 1 year ago

Concur it feels like a trivial cost for our volume. I have the same instincts for the channels - our user base will be familiar with SMS & email MFA from their banks but plausibly won't have an app, but I would want an app!

djohnston32 commented 1 year ago

Cool! In the meantime I've been putting together a rough outline:

UI

Registration

Models

Twilio handles code generation and secret storage, so we would just have to keep track of which users have which factors enabled. I'm thinking a model/table like this:

Code

Registration is pretty straightforward. We can add a new controller and views for various actions on authentication_factors.

Validation on sign in is trickier since we have to override some devise behavior. This example overrides the devise SessionsController's create action to redirect to a new MFA controller/views, using the session to keep track of the user id. If tokens are valid once the user has gone through the 2FA process on that page, this new controller signs in the user. I was able to get a more generic version of that working locally and it seems fairly simple and effective. I'd want to test more edge cases for security concerns though.

djohnston32 commented 1 year ago

I think it'll be best to implement one channel type at a time, so I'm going to start with SMS unless there's a strong preference for something else first. No given type seems way easier or harder than another so no strong feelings on my side.

If you feel good about Verify, can you make a Verify Service for me to test with at this link? No rush though, I can make progress with mocked versions of all the calls for now.

colinxfleming commented 1 year ago

the model change looks right to me. we don't store phones for CMs in our db currently; bonus if that column is scrambled. (I feel like rails 7 might have some nice stuff around this though I can't remember super clearly.)

The UI/controller/view changes also seem right to me but @lomky has thought about this harder than I have so that opinion goes!

I spun up a verify service; I'll send you the details for it offthread. thanks @djohnston32 !

lomky commented 1 year ago

I think that flow looks great for a first implementation! A couple edge case and follow on flows, all non-blocking for getting started.

  1. If we're supporting multiple factor methods per user, which I like, what would the flow look like for a user who has set up multiple to disable just one of them? Or to change one of the factors to a new SMS/email/app?
    • Totally chill if the flow is just "User must disable all and then re-enable with the new preferred setup" - but if that's the case we should make sure we have user messaging explaining it.
  2. Do we have thoughts on what the Administrator access to this might be like?
    • I think at a minimum we want to support an administrator disabling the active MFA factor for a specific user (someone has lost their phone, for example), pretty early in the roll out for this feature. Without that administering MFA issues would fall to our team, and we're definitely not best qualified to assess a fund's case manager's access.
  3. Definitely a later follow on: what does the flow look like for the use case of an administrator turning on "require MFA" on behalf of a user / for the fund?
    • In a perfect world the admins have told the users to expect this, but we should presume a user is seeing it without expecting it on signing in.
    • How do we keep the user sandboxed until they have successfull setup MFA?
djohnston32 commented 1 year ago

The first one is the one I have immediate thoughts on:

colinxfleming commented 1 year ago

I think for a v1 of this the more we err on the side of simplicity the better. So 'only one active option' + 'deleting = disabling' sounds good to me if we think that's simpler from a user perspective? Agree with @lomky that the rest of this sounds great. Let us know if you need to resolve anything else to get cooking!

djohnston32 commented 1 year ago

Sounds good! Work is in progress focusing just on SMS. I got the basic skeleton of controllers/views in place last week with all the API calls mocked. I've been using the wicked gem for multi-step forms and I like the way that's feeling. This week I'm adding in more validations and error handling, etc, then I'll plug in the real API. Aiming to have a PR up by the end of the week.

djohnston32 commented 1 year ago

Uploaded my changes so far to a remote branch. If you run locally you can see the rough registration and login flows with SMS-only. All the Twilio API calls are mocked so you just have to put in '111111' as the verification code whenever asked and it works. 2FA registration can be found on the Edit Profile page.

No pressure to look now though, especially not to review in depth at this point since there will be an actual PR coming later. I still have to add the actual API, refine the UI styles and text, add some more robust error handling, add tests, etc. to get it production ready. Just wanted to put it out there.

xmunoz commented 1 year ago

I think we can declare victory on this.