Add ECA condition plugin "is TFA setup"

Created on 30 November 2023, over 1 year ago

Problem/Motivation

I'm not sure if the maintainers of TFA have explored ECA, however my experience is this module is already at the Views v2 stage in terms of awesomeness.

So while D7 had a module tfa_rules , modern plugin architecture is pretty neat, and I'm wondering whether TFA could provide its own plugins for ECA either directly in the main module, or a submodule.

This would allow ECA like this:

Steps to reproduce

n/a

Proposed resolution

Here's an anonymised version of a plugin we're using - would you accept a pull request or would you see this going into another contrib project.
https://gist.github.com/simesy/7bfbca281cf1e1594f20e2d233b2186e

Remaining tasks

User interface changes

API changes

Data model changes

Feature request
Status

Active

Version

2.0

Component

Code

Created by

🇦🇺Australia sime Melbourne

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @sime
  • 🇦🇺Australia sime Melbourne
  • 🇦🇺Australia sime Melbourne
  • 🇺🇸United States cmlara

    I'm aware of ECA although to date I have not personally given it more than a cursory glance. Even though ECA's install count is small (around 2k reported at the moment) it does seem it might possibly become the replacement for Rules in the future (though I would be unable to opine on the strengths of either) especially because no D8+ stable release of Rules yet exists.

    Cursory discussions indicate there there may indeed be at least one use for such a plugin (redirect on login) and I could see that some ohter uses might exist (such as only allow TFA enabled accounts access to certain nodes). As such I can see value in such a plugin existing.

    The primary call isReady() is in a class marked @internal in 2.x. I would prefer to keep it so in order to allow TFA the flexibility to respond to changing needs in the future.

    Given the above I would be supportive of an MR to add this into the main repo.

  • Assigned to sime
  • Status changed to Needs work over 1 year ago
  • 🇦🇺Australia sime Melbourne

    That's good news.

  • 🇮🇳India bhanu951

    I would suggest to go with submodule in TFA instead of standalone plugin in the TFA module, as this plugin will have external dependency of ECA Module which won't be present in all sites which are planning to use this module.

  • 🇦🇺Australia sime Melbourne

    Yes i assume tfa_eca?

  • 🇺🇸United States cmlara

    as this plugin will have external dependency of ECA Module which won't be present in all sites which are planning to use this module.

    It is my understanding that plugins are scoped to the ecosystem they work with eg tfa\Plugins\ECA, and should only be loaded when the plugin manager for ECA is installed.

    There is 🐛 AttributeClassDiscovery fails while trying to include non valid plugin class Fixed in core, however that applies only to Attribute based plugins and to my understanding should only pose an issue if the ECA namespace is listed for discovery and the plugin has a dependency on yet another module that is not installed.

    No other code should attempt discovery for tfa\Plugins\ECA except for ECA, or some other module that provides all of ECA's API. So long as our plugin doesn't use source beyond that of TFA or ECA it shouldn't pose an issue.

    ECA will likely becomes a require-dev for testing however that is acceptable dependency as it doesn't impact end users.

  • Merge request !64Add ECA condition to test if TFA is setup. → (Open) created by sime
  • Status changed to Needs review over 1 year ago
  • 🇦🇺Australia sime Melbourne
  • 🇺🇸United States rtdean93

    Bumping to ask about the status of this?

    I am wanting to improve the UX for people by providing a Block which can be shown on pages when a TFA Status "is TFA setup" is False.

    Any ideas?

    Thank you.

  • 🇺🇸United States cmlara

    This one kinda fell through and was lost.

    Wording is still the biggest one. With a fresh set of eyes it may need to be something along the lines of "Account is known to have configured TFA" or similar as the isReady() method really is checking for a known configured plugin yet doesn't assert that it is properly operational.

    The entire 'check if a user needs to login with TFA' subsystem is seeing patch after patch as we find new bad assumptions (at the time my last comments were made https://www.drupal.org/sa-contrib-2024-003 was likely running through my mind).
    Eventually we can come back to firm it up to be more towards known operational.

    Needs work as it requires a rebase at a minimal. We also may want to look at the fact ECA 2.x is now out and see what impact this has, if both can be supported or not.

Production build 0.71.5 2024