Problem/Motivation

When a pre login subscriber (acting on CasHelper::EVENT_PRE_LOGIN event) is cancelling the login, there's a site-wide message shown as an status error message to the user:

You do not have access to log in to this website. Please contact a site administrator if you believe you should have access.

True, the message is configurable but on the same site, depending on the site's business logic there can be more than one reasons why a login could be cancelled. With the actual implementation the same message will be displayed, regardless of the cancellation source.

Proposed resolution

  • When cancelling the login from subscriber, allow the subscriber to set a cancellation reason.
  • If set, the cancellation reason message, will take precedence over the site-wide message.

Remaining tasks

None.

User interface changes

User may see different error messages when a subscriber cancels a login process.

API changes

  • The reason can be set only when cancelling the login. For this reason, CasPreLoginEvent::setAllowLogin() was split in ::allowLogin() and ::cancelLogin($reason)
  • CasPreLoginEvent::setAllowLogin() is deprecated in favour the new methods.
  • A new ::getCancelLoginReason() has been added.
  • New CasLoginException methods: ::setSubscriberCancelReason() and ::getSubscriberCancelReason().

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#2 3111073-2.patch14.47 KBclaudiu.cristea

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review
StatusFileSize
new 3111073-2.patch14.47 KB

Patch.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Reviewed & tested by the community

I am on the same project as @claudiu.cristea and the real life use case for this is that we currently moving from classic Drupal login to CAS and have both systems active at the same time during the transition period. We need to show a custom message to a CAS user if they change their email address to an address that is already used by an existing Drupal user. See https://github.com/ec-europa/joinup-dev/pull/1982/commits/3b456caa76f849...

I have reviewed the code, the approach looks good, it allows to (optionally) set a custom reason on the event. Since a reason is only appropriate when cancelling and not when allowing a login the existing ::setAllowLogin($allow_login) has been deprecated and replaced by two dedicated methods ::allowLogin() and ::cancelLogin($reason = NULL).

The optional reason is passed on through the CasLoginException by setting it on a dedicated property. A more simple approach could have been to use the exception message as the reason, but I like the suggested approach since the exception message is intended as technical information for developers and the reason is for end users.

Also nice to see some reusable test code being split off to a trait.

The tests are not running due to a Composer error but I ran them locally and the whole test suite is passing.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Thanks for the contribution! I'll take a look soon.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Reviewed & tested by the community » Fixed

Okay, this makes sense to me. Later we can extend this to some of the other flow-control exceptions (like registration denial) as well. Thank you for the contribution!

  1. +++ b/src/Controller/ServiceController.php
@@ -356,6 +356,11 @@ class ServiceController implements ContainerInjectionInterface {
+        if ($message = $e->getSubscriberCancelReason()) {
+          return $message;
+        }

    trying to stay away from this code style. Would rather have the var definition on a separate line. 

  2. +++ b/src/Exception/CasLoginException.php
@@ -37,4 +37,36 @@ class CasLoginException extends \Exception {
+    if ($this->getCode() == self::SUBSCRIBER_DENIED_LOGIN) {
+      $this->subscriberCancelReason = $reason;
+    }

    Should use === here

I'll fix both these minors on commit.

Status: Fixed » Closed (fixed)

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