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: make 2fa great again #26557

Merged
merged 25 commits into from
Dec 9, 2024
Merged

feat: make 2fa great again #26557

merged 25 commits into from
Dec 9, 2024

Conversation

zlwaterfield
Copy link
Contributor

@zlwaterfield zlwaterfield commented Nov 29, 2024

Changes

This PR provides a bunch of 2fa improvements - follow up to #26526

  • more consistent naming across variables, components, endpoints, etc.
  • pull together all modals into a single reusable one
  • improve styling of 2fa related ui
  • allows backup codes to be used
  • reduces back up codes down to 5

Questions

  • what security implications should we be thinking about?
    • blocking delete 2fa and generate/re-generate backup codes with the time sensitive auth check
    • sending emails when 2fa added, remove, or backup code used.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

It doesn't have an impact

How did you test this code?

Lots of manual testing + added tests

@zlwaterfield zlwaterfield self-assigned this Nov 29, 2024
@zlwaterfield zlwaterfield force-pushed the zach/implement-backup-codes branch from 5a86375 to a5d0ca5 Compare December 2, 2024 20:45
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Size Change: -30 B (0%)

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.11 MB -30 B (0%)

compressed-size-action

@zlwaterfield zlwaterfield marked this pull request as ready for review December 3, 2024 19:56
@zlwaterfield zlwaterfield changed the title chore: make 2fa great again feat: make 2fa great again Dec 3, 2024
@zlwaterfield zlwaterfield requested a review from a team December 4, 2024 20:47
Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

It's great. Not even again, more for the first time

frontend/src/layout/GlobalModals.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/authentication/TwoFactorSetupModal.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

One non blocking thing. This is awesome work.

posthog/api/test/test_authentication.py Outdated Show resolved Hide resolved
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

102 snapshot changes in total. 0 added, 102 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

18 snapshot changes in total. 0 added, 18 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@zlwaterfield zlwaterfield merged commit 58e51ff into master Dec 9, 2024
96 checks passed
@zlwaterfield zlwaterfield deleted the zach/implement-backup-codes branch December 9, 2024 23:36
Copy link

sentry-io bot commented Dec 10, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ KeyError: 'django_two_factor-hex' /api/users/{uuid}/two_factor_validate/ View Issue

Did you find this useful? React with a 👍 or 👎

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.

4 participants