-
Notifications
You must be signed in to change notification settings - Fork 60
feat: Initial 2FA implementation with Scheb\TwoFactorBundle #365
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
base: develop
Are you sure you want to change the base?
Conversation
config/packages/scheb_2fa.yaml
Outdated
| enabled: true | ||
| totp: | ||
| enabled: true # If TOTP authentication should be enabled, default false | ||
| server_name: bewelcome.com # Server name used in QR code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. The server would be bewelcome.org. But I suppose it is better to use a parameter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now switched to .org
not sure where variable should be defined.
| leeway: 60 # Acceptable time drift in seconds, must be less or equal than the TOTP period | ||
| parameters: # Additional parameters added in the QR code | ||
| # image: 'https://my-service/img/logo.png' | ||
| image: 'https://miro.medium.com/v2/resize:fill:128:128/1*65AfOY_oNSTe2G1bFMwQ4A.jpeg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be served through our own server. Could this be done relative to the base url?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to know which file. I got this one while doing image search for a small logo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduced new columns for the member entity but there is no migration. Additionally the two factor auth should be optional which means that there needs to be a preference to enable it and the columns should be in a table of their own (we already have way too many columns on member).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho, that should come after validating things are functional. more database optimisation.
| @@ -0,0 +1,39 @@ | |||
| {% extends "base.html.twig" %} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is lacking translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similary, after validating things are functional (and not breaking anything) as this initial work and there will certainly be polishing later. But no point to polish something not working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. But needs more work to be able to be pulled.
This is expected as Work In Progress. |
Goal: support 2FA/MFA as any website containing personal information should, even if no financial/health data.
Implementation
Following https://symfony.com/bundles/SchebTwoFactorBundle/7.x/installation.html
Reviewed https://symfony.com/bundles/SchebTwoFactorBundle/current/troubleshooting.html
Work In Progress
Limited testing as issues with local docker and CI (separate PR/issues)
Current CI results with ci changes: https://github.com/jj2bw/rox/actions/runs/17011983649
Would address #360