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: expand 2FA support #1254

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

CosDiabos
Copy link
Contributor

Although Shield currently offers 2FA, its support is somewhat limited. Like this discussion #1120, I also feel that it could benefit from a more robust system, like supporting multiple 2FA systems, allowing global/per user 2FA, or setting custom 2FA actions per user group, like suggested in discussions.

I would love to know what you think.

Description
This PR expands support of 2FA actions for Shield. It allows having multiple active 2FA methods, per-user or site-wide 2FA, per-group custom 2FA action, and a default. The settings $Mfa, $forceMfa, $actionsMfa, $defaultMfa and $matrixMfa are introduced to the Auth config file to control these settings. The per-user 2FA is achieved through a new column named mfa in the user table acting as a flag. The User Entity introduces the isMfaActive() :bool method for easy access to the property.

Within the Authenticators/Session.php:511, currently, the auth_action_message is being assigned the extra field directly. Expanding the 2FA actions, that extra field may be useful to store data related to the identity, so the ActionInterface introduces the getActionMessage() :string method to get this auth_action_message value from the action.

These changes introduce breaking changes to past versions.

Implements #1120

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@datamweb datamweb added breaking change Pull requests that may break existing functionalities enhancement New feature or request labels Feb 25, 2025
@datamweb
Copy link
Collaborator

Thank you for submitting PR.
To be honest, I’ve actually needed such features in several projects, but I’ve implemented them in a custom way using custom actions. In my opinion, Shield should have provided a simpler solution for setting these things up, but for some reason, it didn’t. Now, we are at a stage where I’m not eager to introduce a breaking change that could potentially affect many projects.

You can keep the PR open to gather feedback from the community, or it might be better to inform them on the forum or Slack.

@datamweb
Copy link
Collaborator

With all due respect, given that your PRs are quite large, I kindly suggest using the atomic commit approach. This will make the review process easier and faster for others. Smaller, more focused commits allow the team to review changes more efficiently.

Additionally, if possible, please consider writing your commit messages in a way that provides a clear and concise explanation of the changes.
https://www.conventionalcommits.org/en/v1.0.0/
https://gist.github.com/joshbuchea/6f47e86d2510bce28f8e7f42ae84c716

@michalsn
Copy link
Member

I'm afraid that a breaking change at this stage of the library (in v1) is not an option. We can improve the way we handle actions, but it must be an evolution, not a revolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants