MFA v1 - TOTP #2830

yash-learner · 2025-02-12T21:19:31Z

Proposed Changes

This pull request introduces a new feature for Two-Factor Authentication (2FA) using Time-based One-Time Password (TOTP). The changes include updates to various files to support TOTP setup, verification, and login processes.

New TOTP Feature Implementation:

care/emr/api/viewsets/totp.py: Added a new TOTPViewSet class to handle TOTP setup, verification, and login. This includes methods for generating backup codes, encrypting/decrypting TOTP secrets, and handling TOTP-related requests.
care/users/migrations/0022_user_mfa_settings_user_totp_secret.py: Added a migration to include mfa_settings and totp_secret fields in the User model.
care/users/models.py: Updated the User model to include totp_secret and mfa_settings fields.
care/utils/encryption.py: Added utility functions for encrypting and decrypting strings using Fernet.
config/api_router.py: Registered the new TOTPViewSet in the API router. [1] [2]

Configuration and Dependencies:

.env.example: Added TOTP_SECRET_ENCRYPTION_KEY to the environment variables.
Pipfile: Added the pyotp library as a dependency for TOTP functionality.
config/settings/base.py: Added TOTP_SECRET_ENCRYPTION_KEY to the settings.
docker/.local.env: Added TOTP_SECRET_ENCRYPTION_KEY to the local environment variables.
docker/.prebuilt.env: Added TOTP_SECRET_ENCRYPTION_KEY to the prebuilt environment variables.

Authentication Updates:

config/auth_views.py: Updated the TokenObtainPairSerializer and TokenRefreshSerializer to handle temporary tokens for TOTP verification during login. [1] [2] [3]

Associated Issue

Add Backend Support for Two-Factor Authentication (2FA) #2614

TOTP Feature Implementation:

care/emr/api/viewsets/totp.py: Added a new TOTPViewSet class with endpoints for TOTP setup, verification, login, disabling, and backup code management.
care/emr/tasks/totp.py: Added tasks to send email notifications when TOTP is enabled or disabled.
care/templates/email/totp_enabled.html: Created an email template for TOTP enabled notifications.
care/templates/email/totp_disabled.html: Created an email template for TOTP disabled notifications.

User Model and Migration:

care/users/models.py: Added totp_secret and mfa_settings fields to the User model.
care/users/migrations/0022_user_mfa_settings_user_totp_secret.py: Created a migration to add the new fields to the User model.

Encryption Utilities:

care/utils/encryption.py: Added utility functions for encrypting and decrypting strings using Fernet.

Configuration and Environment Variables:

.env.example, docker/.local.env, docker/.prebuilt.env, config/settings/base.py: Added TOTP_SECRET_ENCRYPTION_KEY to environment variables and settings. [1] [2] [3] [4]

API Router and Authentication:

config/api_router.py: Registered the new TOTPViewSet with the API router. [1] [2]
config/auth_views.py: Updated token serializers to handle temporary tokens for TOTP verification during login. [1] [2] [3]

Dependency Updates:

Pipfile: Added pyotp dependency for TOTP functionality.

Merge Checklist

Tests added/fixed
Update docs in /docs
Linting Complete
Any other necessary step

Only PR's with test cases included and passing lint and test pipelines will be reviewed

@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins

…el migration

… store encrypted secrets

coderabbitai · 2025-02-12T21:19:40Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

codecov · 2025-02-12T21:27:37Z

Codecov Report

Attention: Patch coverage is 41.66667% with 112 lines in your changes missing coverage. Please review.

Project coverage is 56.01%. Comparing base (41877f1) to head (c027542).
Report is 9 commits behind head on develop.

Files with missing lines	Patch %	Lines
care/emr/api/viewsets/totp.py	40.25%	92 Missing ⚠️
care/emr/tasks/totp.py	47.36%	10 Missing ⚠️
config/auth_views.py	11.11%	8 Missing ⚠️
care/utils/encryption.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2830      +/-   ##
===========================================
- Coverage    56.24%   56.01%   -0.23%     
===========================================
  Files          215      218       +3     
  Lines        10231    10404     +173     
  Branches      1038     1052      +14     
===========================================
+ Hits          5754     5828      +74     
- Misses        4461     4560      +99     
  Partials        16       16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…unctionality

care/emr/api/viewsets/totp.py

vigneshhari · 2025-02-18T05:03:19Z

.env.example

@@ -18,3 +18,5 @@ BUCKET_ENDPOINT=http://localhost:4566
 BUCKET_EXTERNAL_ENDPOINT=http://localhost:4566
 FILE_UPLOAD_BUCKET=patient-bucket
 FACILITY_S3_BUCKET=facility-bucket
+
+TOTP_SECRET_ENCRYPTION_KEY=your-encryption-key-here


We already have a secret key, lets reuse that.

I can't find one in this https://github.com/ohcnetwork/care/blob/develop/.env.example, by any chance is it django_secret_key ?

vigneshhari · 2025-02-18T05:06:06Z

care/emr/api/viewsets/totp.py

+        return Response({"backup_codes": backup_codes})
+
+    @staticmethod
+    def _check_totp_enabled(mfa_settings: dict):


This function should return either true or false and let the implementer write the exception.

This function should return either true or false and let the implementer write the exception.

Yeah, I thought this was the expected way when writting this method but wrote the exception inside the_check_totp_enabled as the exception is same for all three methods which are calling this so trying to not repeat the code made me write this way and also this will make understanding the code little tough and also misleading with its method name.

Will fix it 👍

vigneshhari · 2025-02-18T05:12:41Z

care/emr/api/viewsets/totp.py

+            )
+
+        secret = random_base32()
+        encrypted_secret = encrypt_string(secret)


I didn't really understand how encrypting this string has a security advantage, for someone to hack around this, they need access to the database and not the application, but if you have access to the database then we have already lost right?
And the backup codes are not encrypted, that already undoes all this work.

And the backup codes are not encrypted, that already undoes all this work.

Backup codes are hashed and stored

care/care/emr/api/viewsets/totp.py

Line 128 in f46573c

"code": make_password(code),

So even some get's access to DB. they can't use it

but if you have access to the database then we have already lost right?

Yes

but

If it is plain text they can generate the TOTP codes and if it is encrypted, one cannot generate, and also it will save during accidental exposure

🤔

vigneshhari · 2025-02-18T05:13:37Z

care/emr/api/viewsets/totp.py

+        user = request.user
+
+        if not user.totp_secret:
+            return Response(


raise a validation error

vigneshhari · 2025-02-18T05:18:29Z

care/emr/api/viewsets/totp.py

+    def login(self, request):
+        request_data = TOTPLoginRequest(**request.data)
+
+        token = RefreshToken(request_data.temp_token)


Need exception handling around this, if an invalid token is given it will raise an error

vigneshhari · 2025-02-18T05:19:50Z

care/emr/api/viewsets/totp.py

+        if totp.verify(request_data.code):
+            refresh = RefreshToken.for_user(user)
+
+            try:


No need to blacklist, you can let this token exist, this token can only be used with an MFA option.

vigneshhari · 2025-02-18T05:20:39Z

care/emr/api/viewsets/totp.py

+        user = User.objects.get(external_id=token["user_id"])
+        totp = TOTP(decrypt_string(user.totp_secret))
+
+        if totp.verify(request_data.code):


How is the backup code used here? the backup codes are maintained by us or the package?

By us, pyotp does not provide out of the box

vigneshhari · 2025-02-18T05:22:37Z

care/emr/api/viewsets/totp.py

+        permission_classes=[],
+        authentication_classes=[],
+    )
+    def backup_login(self, request):


Why cant we just use the same endpoint and check the backup codes if the others dont match?

We can, but pyotp does not provide out of the box backup_codes support, and it is implemented by us, so keeping both in one endpoint will make the method bulkier 😁 and having separate endpoint provides clear separation

but a lot of shared code between two login and backup_login will think whether I can reduce some duplicity of code and will think what will it look like both in single endpoint

Do you have reasons to make it in a single endpoint ?

vigneshhari · 2025-02-18T05:23:09Z

care/emr/api/viewsets/totp.py

+        permission_classes=[],
+        authentication_classes=[],
+    )
+    def login(self, request):


We need ratelimiting, the token should expire within 5 mins as well.

yash-learner added 15 commits February 11, 2025 02:17

Add pyotp dependency to Pipfile

74907af

Add TOTP secret and MFA settings to User model

5c3964e

Add make migratons

f560363

Add TOTP setup and verification endpoints for MFA

173b4d3

Register TOTPViewSet in API router

0f25d2d

Refactor TOTPViewSet to use Pydantic models

023b721

fix and improve logic

418eaf7

Add backup code generation

8efcd17

Change TOTP secret field type from CharField to TextField in user mod…

2527506

…el migration

Implement TOTP secret encryption and decryption; update user model to…

ca0e9dc

… store encrypted secrets

Update TOTP route to use 'mfa/totp' for improved clarity

e7a24ae

WIP: integrate MFA into login flow

901ba9d

WIP: integrate MFA into login flow

ea3d27d

simply flow

622fa57

Refactor TOTP login method and clean up MFA token handling

5eef7d7

yash-learner added 5 commits February 14, 2025 01:17

Add email notification for TOTP activation

d5aeab4

Add TOTP disable functionality and refactor login request handling

d45a732

Refactor TOTP setup and error handling; add regenerate backup codes f…

fac7eef

…unctionality

Add endpoint backup_login and wip:refactor

edcc411

Add mailer for TOTP disable and tiny fixes

f46573c

vigneshhari requested changes Feb 18, 2025

View reviewed changes

WIP: Fixing review comments and playing with rate limit

c027542

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MFA v1 - TOTP #2830

MFA v1 - TOTP #2830

yash-learner commented Feb 12, 2025 •

edited

Loading

coderabbitai bot commented Feb 12, 2025

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

codecov bot commented Feb 12, 2025 •

edited

Loading

vigneshhari Feb 18, 2025

yash-learner Feb 18, 2025

vigneshhari Feb 18, 2025

yash-learner Feb 18, 2025

vigneshhari Feb 18, 2025

yash-learner Feb 18, 2025

yash-learner Feb 18, 2025

vigneshhari Feb 18, 2025

vigneshhari Feb 18, 2025

vigneshhari Feb 18, 2025

vigneshhari Feb 18, 2025

yash-learner Feb 18, 2025

vigneshhari Feb 18, 2025

yash-learner Feb 18, 2025

vigneshhari Feb 18, 2025

MFA v1 - TOTP #2830

Are you sure you want to change the base?

MFA v1 - TOTP #2830

Conversation

yash-learner commented Feb 12, 2025 • edited Loading

Proposed Changes

Associated Issue

TOTP Feature Implementation:

User Model and Migration:

Encryption Utilities:

Configuration and Environment Variables:

API Router and Authentication:

Dependency Updates:

Merge Checklist

coderabbitai bot commented Feb 12, 2025

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yash-learner commented Feb 12, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

codecov bot commented Feb 12, 2025 •

edited

Loading