🎨 Two-factor-auth per user (🗃️ )

[pcrespov]

What do these changes do?

Currently the activate of 2FA is defined as a property of the app and/or product (due to requirements) denoted LOGIN_2FA_REQUIRED. This PR now adds an analogous property at user level denoted two_factor_enabled which default to true.

First, we determine whether 2fa is active in the product. For that we resolve resolve app.LOGIN_2FA_REQUIRED which can be overriden by product.LOGIN_2FA_REQUIRED i.e.

app.LOGIN_2FA_REQUIRED	product.LOGIN_2FA_REQUIRED	Active?
true	-	true
false	-	false
*	true	true
*	false	false
where * is any value and - is undefined.

Then, if the 2fa is activated in the product (and only in that case* then the flag in the user determines whether to require 2fa to a user or not is user.two_factor_enabled. The latter is by default True to preserve the current policy.

Why this change?

• requirements of 2FA at a app/product level is not practical in many use cases
  • e.g. we would like to allow selected tester users (e.g. puppeteers) to skip this step
  • 2fa requires consumption of SMS etc ...and might be deactivated for some internal users
  • some users in Asia have been having problems with phone sms or big delays in emails
• Avoids old solution of disabling 2fa to privilege users (i.e. roles>=TESTER did not have 2fa) in the code
• Anticipating the unavoidable: this option is typically selected by the user and not imposed by the app. We expect this to happen sooner than later.

Related issue/s

• @elisabettai pointed out that PO did not 2FA

How to test

cd services/web/server
make install-dev
pytest tests/unit/**/test_*2fa*.py

Dev Checklist

• No ENV changes or I properly updated ENV (read the instruction)

DevOps Checklist

• 🗃️ Adds users.two_factor_enabled (migration defaults all to True)

[codecov]

Codecov Report

Merging #5061 (49206df) into master (d459139) will increase coverage by 1.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #5061      +/-   ##
=========================================
+ Coverage    87.0%   88.1%    +1.0%     
=========================================
  Files        1212     563     -649     
  Lines       50712   28558   -22154     
  Branches     1083     198     -885     
=========================================
- Hits        44149   25173   -18976     
+ Misses       6324    3335    -2989     
+ Partials      239      50     -189     

Flag	Coverage Δ
integrationtests	63.5% <100.0%> (-1.5%)	⬇️
unittests	86.5% <100.0%> (+1.2%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...c/simcore_service_webserver/login/handlers_auth.py	97.8% <100.0%> (-0.1%)	⬇️

... and 672 files with indirect coverage changes

[jsaq007]

👍🏼

[codeclimate]

Code Climate has analyzed commit 49206df and detected 0 issues on this pull request.

View more on Code Climate.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.4% Duplication

[elisabettai]

Thanks for the great explanation and adding 2FA at the user level!

I didn't know there was 2FA at the app level (not sure I understand what that means 😉), at the product level is clear.

Maybe an issue to link is this one about saving a default 2FA method: ITISFoundation/osparc-issues#1053

It is of course not going to be solved in this PR, but maybe something to think about now. E.g. if that issue had in mind users from Asia, allowing them to disable 2FA would solver

[sanderegg]

discussed