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: TOTP authenticator #2731

Merged
merged 11 commits into from
Nov 16, 2024
Merged

feat: TOTP authenticator #2731

merged 11 commits into from
Nov 16, 2024

Conversation

sheensantoscapadngan
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan commented Nov 13, 2024

Description 📣

This PR adds support for new MFA method: time-based one-time password

This has been tested with Duo and Google authenticator, but current integration should be compatible with other authenticator apps as well

New screens:
TOTP MFA screen after login:
image

Personal settings screen with registered TOTP
image

Personal settings screen without registered TOTP
image

Organization MFA settings:
image

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

@sheensantoscapadngan sheensantoscapadngan changed the title feat: initial implementation for totp authenticator WIP feat: initial implementation for totp authenticator Nov 13, 2024
@sheensantoscapadngan sheensantoscapadngan changed the title WIP feat: initial implementation for totp authenticator feat: TOTP authenticator Nov 13, 2024
@sheensantoscapadngan sheensantoscapadngan marked this pull request as ready for review November 13, 2024 18:42
@dangtony98
Copy link
Collaborator

dangtony98 commented Nov 14, 2024

For the "TOTP MFA screen after login," would it be possible to keep this as the cleaner 6-digit input similar to the email confirmation code input component?

This might be cleaner but is dependent on if the code is always the same digit length or not.

Copy link
Contributor

@scott-ray-wilson scott-ray-wilson left a comment

Choose a reason for hiding this comment

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

This is awesome! You really thought out all the edge cases, great work - a few code comments and then left some UI/flow feedback/thoughts in this notion doc: https://www.notion.so/infisical/MFA-Feedback-3a95fe28840847f594c76d7da7eb42c3?pvs=4

backend/src/ee/routes/v1/saml-router.ts Show resolved Hide resolved
backend/src/server/routes/v1/user-router.ts Show resolved Hide resolved
backend/src/server/routes/v1/user-router.ts Outdated Show resolved Hide resolved
backend/src/services/totp/totp-service.ts Outdated Show resolved Hide resolved
backend/src/services/user/user-service.ts Show resolved Hide resolved
frontend/src/components/mfa/TotpRegistration.tsx Outdated Show resolved Hide resolved
frontend/src/views/Login/Mfa.tsx Outdated Show resolved Hide resolved
@scott-ray-wilson
Copy link
Contributor

@dangtony98

For the "TOTP MFA screen after login," would it be possible to keep this as the cleaner 6-digit input similar to the email confirmation code input component?

This might be cleaner but is dependent on if the code is always the same digit length or not.

The recovery codes are longer than the verify codes :(

Copy link
Contributor

@scott-ray-wilson scott-ray-wilson left a comment

Choose a reason for hiding this comment

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

Should also probably include docs update for mfa: https://infisical.com/docs/documentation/platform/mfa

Copy link
Contributor

@scott-ray-wilson scott-ray-wilson left a comment

Choose a reason for hiding this comment

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

Looks great - few nits

backend/src/services/totp/totp-service.ts Show resolved Hide resolved
frontend/src/views/Login/Mfa.tsx Outdated Show resolved Hide resolved
frontend/src/views/Login/Mfa.tsx Outdated Show resolved Hide resolved
@sheensantoscapadngan sheensantoscapadngan merged commit ba89491 into main Nov 16, 2024
8 checks passed
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.

3 participants