Skip to content
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

feat: email-based authentication codes for MFA/2FA #19008

Merged
merged 88 commits into from
Dec 9, 2024
Merged

Conversation

netroms
Copy link
Contributor

@netroms netroms commented Nov 1, 2024

Summary

This adds 2FA login with a code sent via email as a new feature.

NOTE: This PR removes the enforced 2FA enrollment with TOTP functionality and does not address this for email 2FA either. The requirements for enforced enrollment needs to be addressed in a separate JIRA ticket. Enforced enrollment for TOTP was never used, so it's safe to remove.

TODO:

  • Add rate limiting to email 2FA sending
  • Do sending of 2FA email async so we don't block UI
  • Enforced enrollment

Automatic tests

  • TwoFactorControllerTest
  • UserServiceTest
  • AuthenticationControllerTest
  • LoginTest (E2E)

Manual tests

  • Enable 2FA email in system settings: POST api/42/systemSettings/email2FAEnabled?value=true
  • Set up a working email SMTP server in settings
  • Verify your email: POST api/account/sendEmailVerification (email with code will be sent to your verified email)
  • Enroll in email 2FA: POST api/2fa/enrollEmail2FA
  • Enable email 2FA with code sent to your email: POST api/2fa/enable with {code:CODE_SENT_TO_EMAIL}
  • Try to login, code should be sent to email
  • Now login should show 2FA code input, enter the code and submit
  • Observe you are logged in

Jira:

DHIS2-13334

# Conflicts:
#	dhis-2/dhis-api/src/main/java/org/hisp/dhis/setting/SystemSettings.java
#	dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/SystemUser.java
#	dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetails.java
#	dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserDetailsImpl.java
#	dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/security/oidc/DhisOidcUser.java
#	dhis-2/dhis-services/dhis-service-setting/src/test/java/org/hisp/dhis/setting/SystemSettingsTest.java
Copy link
Contributor

@david-mackessy david-mackessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small comments left, good job 👍

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw you moved more (internal) APIs from using a user's UID to the username which I think is a good thing. As we discussed it seems to me that the username is our identifier when it comes to questions of authentication. 👍

The are a handful or so details to correct and some suggestions about the REST API endpoints.

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There a some comments left

  • on visibility and TX-annotations in services
  • on which HTTP method and response code to use for the new endpoints (session controller)

Copy link

sonarcloud bot commented Dec 9, 2024

@netroms netroms merged commit 80e3d05 into master Dec 9, 2024
16 checks passed
@netroms netroms deleted the DHIS2-13334_2 branch December 9, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants