✨ Is91/server-side implementation of 2FA (⚠️ devops) #3257
Conversation
pcrespov
requested review from
sanderegg,
GitHK,
Surfict and
mrnicegyu11
as code owners
services/web/server/src/simcore_service_webserver/login/_2fa.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/login/handlers.py
Outdated
Show resolved
Hide resolved
| @@ -21,6 +21,48 @@ paths: | |||
| default: | |||
| $ref: "#/components/responses/DefaultErrorResponse" | |||
|
|
|||
| /auth/verify-phone-number: | |||
There was a problem hiding this comment.
Should we maybe consider to rate limit this route? I think a rate limit by IP should be sufficient.
There was a problem hiding this comment.
yes, but I would like to do it with traefik
I wait to next iteration where we add the front-end. There we might have to change the paths again
packages/postgres-database/src/simcore_postgres_database/models/users.py
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/login/_2fa.py
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| def _generage_2fa_code() -> str: | ||
| return f"{1000 + secrets.randbelow(8999)}" # code between [1000, 9999) |
There was a problem hiding this comment.
Why not include some chars as well? Also when using numbers people usually go for XXX-XXX they display the code split in two groups of 3 which are simpler to memorise. I'd stick to these formats which are known to the users.
There was a problem hiding this comment.
@odeimaiz created it with numbers. I guess is twilio reqs? for a tmp code i do no see it makes a huge difference
| async def register_phone(request: web.Request): | ||
| """ | ||
| Submits phone registration | ||
| - sends a code | ||
| - registration is completed requesting to 'phone_confirmation' route with the code received | ||
| """ |
There was a problem hiding this comment.
If I get it correctly, this is used when a new account is created?
| ) | ||
| return response | ||
|
|
||
| assert user["phone"] # nosec |
There was a problem hiding this comment.
Since this can be None, could you check and raise an error here in that case.
Ideally there should be a validator for the phone number as well. But just a the above check is already better.
There was a problem hiding this comment.
it should NOT be None in this context. That is why i added assertion, to make sure the external context respects this
services/web/server/src/simcore_service_webserver/login/_2fa.py
Outdated
Show resolved
Hide resolved
|
|
||
| assert user["phone"] # nosec | ||
| try: | ||
| code = await set_2fa_code(request.app, user["email"]) |
There was a problem hiding this comment.
is there some way of preventing me from calling xXXXXX times this entrypoint?
There was a problem hiding this comment.
yep ... @GitHK also mentioned it. I think it is a good idea and i will add this in next iteration (remember that the config by default disables 2FA)
Do you have any input about how to do this in traeffik? i.e. selectively rate limit some entrypoints?
| "TWILIO_AUTH_TOKEN": "fake-token", | ||
| "TWILIO_MESSAGING_SID": "x" * 34, | ||
| } | ||
| # NOTE: apparently some session-based fixtures is settings these envs |
There was a problem hiding this comment.
The problem here is that these variables are somehow already defined at the start of the test (even if no fixture was added) ... for that reason i have to explicitly delenv them!
I say "apparently" because my guess is that monkeypatch_session and monkeypatch_module are somehow introducing these variables as a side-effect. Those are very dangerous and marked for deprecation ... so I am slowly removing them
pcrespov
force-pushed
the
is91/server-2FA
branch
from
a7eb8ad to
bee2eb9
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
What do these changes do?
Implements server-side of a first implementation of 2FA (two-factor-authentication)
The workflows with 2FA are the following
a. submit user email and password for registration (confirmation email sent)
b. user email confirmed by clicking confirmation email link
c. submit phone number for registration (confirmation SMS sent)
d. user phone confirmed by adding 2FA code received
a. submit user email and password (2FA SMS sent)
b. submit 2FA code
c. user successfully logs in
NOTE: this PR introduces server-side of 2FA functionality (still not front-end) and by default IS DISABLED.
Highlights of implementation
loginpluginredisplugin to store 2fa codes for confirmationExample of env-vars needed to enable 2FA
VERY IMPORTANT:
d67532cDevops should also consider:
redis-commanderservice in ops-stack. This service must append to environmentREDIS_HOSTS:,validation_codes:${REDIS_HOST}:${REDIS_PORT}:2Related issue/s
How to test
$ cd services/web/server $ make install-dev $ pytest -vv tests/unit/with_dbs/03/test_login_2fa.pyChecklist
services/web/server/tests/unit/with_dbs/03/test_login_2fa.pymake openapi-specs,git commit ...and thenmake version-*)cd packages/postgres-database,make setup-commit,sc-pg review -m "my changes"