zikula / core

Zikula Core Framework
GNU Lesser General Public License v3.0
238 stars 66 forks source link

[Users][Registration] Move all antispam/antibot features to hooks-antispam module #4070

Closed Kaik closed 1 year ago

Kaik commented 4 years ago

I think all anti spam related stuff should be in a separated module. This is because we could add more advanced antispam features or/and enhance current ones. UsersModule should only be responsible for users.

Apart from that(this should actually go to that antispam module features) additional enhancements would be:

Guite commented 4 years ago

This is already possible. User registration supports events (UserFormAwareEvent) and also UI hooks.

The core won't provide such an anti spam module though. This is something for a separate module/project.

:-1: for removing the antispam question from Users because with it the core has a very basic/limited functionality on board without requiring an additional module. If we would remove it there would be not even a simple function without a 3rd party module which could become unmaintained.

So IMO won't fix. Let's wait for what @craigh means though.

Guite commented 4 years ago

I'd prefer a separate module which provides different methods which a site admin can choose: e.g. a multilingual question, a math exercise, recaptcha, etc.

Kaik commented 4 years ago

@Guite
I still think that moving all anti spam related stuff to a separate core module, and then improving those features there would be more future proof and clean situation. It could provide a nice skeleton for additional features, just copy and alter hook provider to your needs. Additionally these could be used in more places keeping everything more consistent.

Now improving core antispam features means either hacking users module, adding more features there or creating another 3rd party module just for simple honeypot or any other antispam method that is simple to implement (recaptcha)...

How about moving antispam question from users module core functionality to users module hooks? So additional antispam things could be as easy as creating another hook provider and would not make users module core functionality cumbersome.

Guite commented 4 years ago

Where is the problem in creating another 3rd party module instead of creating another core module?

Kaik commented 4 years ago

Well I'm looking, at it from the core perspective, on one hand you have core module that is mixed with additional functionality, and you keep it there because at least it's something... improving it is messing with module core functionality itself.

Currently antispam question does not work at all, maybe for a few minutes/day then someone is providing bot with the right answer or bot is doing it by itself and that is it... apart from that there is no multilingual capability so this antispam feature is not really usable for someone that does not speak antispam question language.

Now the problem is that most of those "antispam" features are very simple in terms of the code, so creating module for each of them would be a wast of time.

You can do one 3rd party module and then I don't know... tell everyone if they want to add new antispam option they should add it there or something... I guess AntispamModule like ProfileModule which is maintained by Zikula could do that it does not have to be strictly core module...

The thing is that eventually, all of those antispam features will be hook providers so why not create them as such and thus provide, a "template/example provider" so others can just copy and change accordingly and then add as a new antispam feature.

Guite commented 4 years ago

https://github.com/Gregwar/CaptchaBundle

craigh commented 4 years ago

I agree that the simple anti-spam question is ineffective and probably implemented wrongly. Certainly, it would be better implemented as a hook or listener.

I'm not sure another core module is required for this. perhaps it could simply be moved to a listener in the same module or maybe SecurityCenter. My concern there is that it may lose some configurability/flexibility. But maybe I don't know what I'm talking about.

I also agree that a third party module is probably the best answer for a complete and thorough solution that can add new features as @Kaik described in the OP. Possibly with the implementation of the CaptchaBundle referenced above. I also had a Captcha module at one time https://github.com/craigh/Captcha that implemented the Google recaptcha. This was updated by @nmpetkov to use the newest tech from Google, but it is still a Core-1 era module. I'm not interested in maintaining that module any longer, but certainly the idea worth pursuing.

@Kaik this may be a great opportunity for you to contribute a module back that can be used as you describe. Perhaps the core needs to be adjusted to accommodate further needs.

Kaik commented 4 years ago

Ok so: I was thinking about SecurityCenter as a host for those hook providers as well...

My concern there is that it may lose some configurability/flexibility.

Well, all settings data is stored in module variables so hook can get to it.

I'm not sure... hmm the only think that bothers me is that some of those antispam methods fail quietly or even with success, so attacker thinks everything succeed (no learning)... with error he will know that he did something wrong and will go back and try again (honeypot) I'm not sure if hooks can simulate a quiet fail I guess not... (maybe listeners need to check code and see...)

At the moment I do not have time, like literally even those issues, I'm adding hacks to my own websites because I'm really busy, so whole new module I'm not going to be able to do it now.

I will test hooks and maybe those listeners. I do have a big problem with bots like 20 or even more per day, and I have google recapcha and some of them go right thru... antispam question would be good but with list of answer and questions not just only one (and of course multilingual as well) I heard about honeypot and it looks it can work as well...

Banning domains works as well actually I'm banning whole domains but banning only tlds like .website or .xyz would be great

So whole antispam game is more like a mix of different methods than just one solution. Anyway I will think about it and test some stuff like I'm about to try hook provider see how it works first it would be a step to move that antispam question too and then maybe improve it.

Kaik commented 4 years ago

I'm working on it here https://github.com/Kaik/KaikMediaAntispam Hopefully will have something to show soon at the moment it's just an empty module.

craigh commented 4 years ago

it won't be useful to the core if it is built for Core 2

Kaik commented 4 years ago

I need it for Core 2 and I have to learn what has changed in Core 3 so I can make core 3 version as well.

craigh commented 4 years ago

https://github.com/zikula/core/blob/master/docs/Development/General/Refactoring_3.md

Kaik commented 4 years ago

hmm looks like I can't use form aware hook with users module and I have to go for a listener (like in profile module)

craigh commented 4 years ago

why do you say that?

Kaik commented 4 years ago

Unless I'm missing something there is no FormAwareHookSubscriber on UsersModule side so my FormAwareHookProvider won't be able to connect. It's actually saying that in hooks menu. Screenshot from 2020-08-12 16-05-38

craigh commented 4 years ago

ok. I'm not certain of the intended behavior there. I would have to look into it further. Help is welcome.

Kaik commented 4 years ago

I had a quick look into registration code and hook form aware functionality is present there, only actual hook definitionFormAwareHookSubscriber is missing, so FormAwareHookProvider can't be connected.

There are two modules that are "adding" fields to register/admin user form and that is ProfileModule and LegalModule - both are using Listeners instead of Hooks.

Actually now I remember complaing about that here https://github.com/zikula-modules/Profile/issues/116 - there is no way to turn off profile fields via web interface for a specyfic area. For example turn off additional fields in registration form while keep them in admin area. It is because this module does not utilise hooks but it is hardwired via listeners.

So what I think would be right thing to do is:

  1. Add more areas (so registration area and admin area separately - now both seems to be connected, at least via events)
  2. Add FormAwareHookSubscriber for those areas
  3. Refactor LegalModule and ProfileModule to use Hooks instead of Listener.

At the moment I will work on what I have so I will use events to test anti-spam features on a live system then I will see what I can do about above, there might be a reason why it is the way it is.

Kaik commented 4 years ago

I don't think you will like the way I think solution should look like...

So the problem is that the registration form is considered to be user form so both hooks and events on registration form are kind of mixed with user form in the system. Apart from that there are actually two registration forms ReEntrant and NonReEntrant.

So to support all I have created additional events and hook subscribers and in code it looks like this

        $formEvent = new RegistrationFormAwareEvent($form);
        $formHook = new RegistrationFormAwareHook($form);

        $hasReEntrantListeners = $dispatcher->hasListeners(RegistrationEvents::RE_ENTRANT_EDIT_FORM);
        $hasReEntrantHookBindings = $hookDispatcher->getBindingsFor('users.form_aware_hook.reentrant_registration.edit');

        if ($authenticationMethod instanceof ReEntrantAuthenticationMethodInterface 
                && !empty($userData) 
                && !$hasReEntrantListeners 
                && 0 == count($hasReEntrantHookBindings)
            ) {
            // skip form display and process immediately.
            $userData['_token'] = $this->get('security.csrf.token_manager')->getToken($form->getName())->getValue();
            $userData['submit'] = true;
            $form->submit($userData);
        } else {
            if ($authenticationMethod instanceof ReEntrantAuthenticationMethodInterface ) {
                // re entrant registration form events and hooks
                $dispatcher->dispatch(RegistrationEvents::RE_ENTRANT_EDIT_FORM, $formEvent);
                $hookDispatcher->dispatch(ReEntrantRegistrationFormAwareSubscriber::RE_ENTRANT_REGISTRATION_TYPE_EDIT, $formHook);
            } elseif ($authenticationMethod instanceof NonReEntrantAuthenticationMethodInterface ) {
                // non re entrant registration form events and hooks
                $dispatcher->dispatch(RegistrationEvents::NON_RE_ENTRANT_EDIT_FORM, $formEvent);
                $hookDispatcher->dispatch(NonReEntrantRegistrationFormAwareSubscriber::NON_RE_ENTRANT_REGISTRATION_TYPE_EDIT, $formHook);
            }

            $form->handleRequest($request);
        }

Both RegistrationFormAwareEvent and RegistrationFormAwareHook are basically the same things apart from that hooks can be configured dynamically in admin.

This solves all the problems I had.

I have added one more event at the beginning (event name is just for testing)

 $dispatcher->dispatch(RegistrationEvents::REGISTRATION_INITIATED, new GenericEvent());

instead of

        // $this->throwExceptionForBannedUserAgents($request);

So it can be moved into listener and other exceptions may be added as listeners like banned IP addresses etc...

this maybe, too much and BC break and maybe overthinked... I will do more tests on how it works with different antispam methods

Kaik commented 4 years ago

It might be that I'm wrong with form aware hook category and ui_hook is better for this. I'm pretty sure about having registration functionality different from user management in terms of events and hooks.

Kaik commented 4 years ago

~Looks like I was wrong and Ui hooks are actually better for this.~ I'm not so sure about that now with form aware I can target additional fields with error message with ui hooks not really.

Module is in beta/MVP stage more will come later. Above problems still persists (I made required changes in my installation)

  1. distinction between registration form and user form - you do not need antispam in admin
  2. distinction between NonReEntrant and ReEntrant as you don't need antispam when registration is performed via facebook or google.

ATM it's core 2x module will bring it to 3x someday for sure... have to learn strict php first ;)

https://github.com/Kaik/KaikMediaAntispam