Add option to respect Drupal's registration configuration when auto-registration is enabled

claudiu.cristea → made their first commit to this issue’s fork.

I think this is a legit request. Recently, we were spammed by tons of accounts being created in Drupal but we have no control over the CAS server. We would like to be able to moderate registration of the users that are auto-creating accounts. Trying to get some code.

• Needs tests
• BC issue: Some sites might have the Who can register accounts? set to _Visitors, but administrator approval is required_, knowing that, with CAS module, that setting have no effect. If we merge this change their new accounts will be suddenly moderated. Even their config is wrong, we have to deal with this but how? Should se provide an update script and set the option to _Visitors_? Then the behavior will bot change.

I think this is a legit request. Recently, we were spammed by tons of accounts being created in Drupal but we have no control over the CAS server.

- why do you have auto registration turned on?

> why do you have auto registration turned on?

Because we only allow CAS registration.

No, what I mean is, why do you have CAS configured to allow anyone with a CAS account to have an account auto created? Why don't you require that an administrator manually register users to the site that require access?

I'm just playing devil's advocate to verify we really need the additional complexity this adds

Everyone on Earth with a CAS account should be able to register. There's no admin pre-register. People are coming on the site and, if they want to login, the click the link, are redirected to the CAS server where they login ore register new CAS user. Then they are redirected back to our site. If they have a local account they are logging in. In the case there's no local account, we auto-creare one and log them in. But in some critical circumstances, admins want to moderate the registration. . This is a native Drupal feature. As I told, we cannot do that on CAS Server level as the server is a service that we don't control. We would like to turn the switch and CAS to honour that configuration.

Okay. I guess it makes sense to add this support for such cases. It's just a little odd of course, because the user already "logged in" via CAS, but is being denied an account on the site. But I understand the use case now.

For existing sites, the safest solution to to make this opt-in via a feature flag checkbox that's revealed if the user enables auto-registration. "Respect user moderation setting" or something? I think implementing this change without considering the number of existing sites that might break is not a good idea

Added tests.

For existing sites, the safest solution to to make this opt-in via a feature flag checkbox that's revealed if the user enables auto-registration. "Respect user moderation setting" or something?

That works when CAS is being configured. But how about the sites already configured with auto-registration and admin approval set?

Then the new feature flag we have set will be "off", thus disabling this new event subscriber, and behavior is unchanged. Basically I'm suggesting we have a checkbox to controls if this event subscriber is used or not.

@bkosborne, a kill-switch is a good idea, thanks

Nice work, MR solves the issue for us. It is RTBC.

Two things from my review:

Missing post-update hook

Can you add a post_update hook to set new config option in cas.settings.yml for existing sites? I understand it's false by default, and retrieving it from settings when it doesn't exist will return null (which is falsy), but I think it's best if we set the default config anyway on existing sites.

Confusing behavior

After a user tries logging in via CAS after the initial attempt, they are denied login, but there's no error message displayed. That's because you're returning an empty string for this type of login failure in ServiceController::getLoginErrorMessage(). I understand you did this because your event subscriber returns a message instead. However, it only does this when the account is initially being registered. After it's already registered, the event subscriber doesn't fire (which is good), but now the user gets no wording.

I think we need to do something for that use case. How about altering this:

    if ($this->isAdminApprovalNeeded() && $account->isBlocked()) {
      // Cannot log in until the admins are not approving the new account. Note
      // that CAS module provides, by default, the normal Drupal behavior by
      // showing a status message and, if configured, sending email
      // notifications to user and admins. This is achieved by listening to
      // ExternalAuthEvents::REGISTER event. Third-party may override this
      // behavior by providing a subscriber with a higher priority, implementing
      // their logic and stopping the event propagation.
      // @see \Drupal\cas\Subscriber\CasAdminApprovalRegistrationSubscriber
      $this->casHelper->log(LogLevel::DEBUG, 'Login denied as new account needs admin approval.');
      throw new CasLoginException("Cannot login, admin approval is required for new accounts", CasLoginException::ADMIN_APPROVAL_REQUIRED);
    }

So it only executes if it's a brand new account being registered (not for existing account logins).

Then, existing accounts that attempt to login but are already blocked will allow this existing code to execute:

    // Check if the retrieved user is blocked before moving forward.
    if (!$account->isActive()) {
      throw new CasLoginException(sprintf('The username %s has not been activated or is blocked.', $account->getAccountName()), CasLoginException::ACCOUNT_BLOCKED);
    }

Which will present the proper account blocked message to the user (which admins can configure).

In the previous commit the text fields were replaced with textareas, we need to adapt config schema for them.

Addressed remarks from #21. Ready for review.

Merged in the latest 2.x to this branch, and tests still pass. Looks good to me. Thanks all.

Automatically closed - issue fixed for 2 weeks with no activity.