Skip to content

Conversation

@jj2bw
Copy link
Contributor

@jj2bw jj2bw commented Aug 17, 2025

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

enabled: true
totp:
enabled: true # If TOTP authentication should be enabled, default false
server_name: bewelcome.com # Server name used in QR code
Copy link
Contributor

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.

Copy link
Contributor Author

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'
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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).

Copy link
Contributor Author

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" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is lacking translations.

Copy link
Contributor Author

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.

Copy link
Contributor

@thisismeonmounteverest thisismeonmounteverest left a 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.

@jj2bw
Copy link
Contributor Author

jj2bw commented Aug 22, 2025

Thanks for working on this. But needs more work to be able to be pulled.

This is expected as Work In Progress.
Thanks for the feedback

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.

2 participants