Merge pull request #14 from flipboxfactory/login-cleanup

cleanup in the login process
flipboxfactory · Aug 17, 2018 · 53f93c0 · 53f93c0
2 parents 05a9d85 + ddf110d
commit 53f93c0
Show file tree

Hide file tree

Showing 5 changed files with 183 additions and 75 deletions.
diff --git a/composer.json b/composer.json
@@ -20,7 +20,7 @@
   },
   "require-dev": {
     "phpunit/phpunit": "^5.0",
-    "squizlabs/php_codesniffer": "^2.0"
+    "squizlabs/php_codesniffer": "^3.0"
   },
   "autoload": {
     "psr-4": {

diff --git a/src/controllers/LoginController.php b/src/controllers/LoginController.php
@@ -112,23 +112,13 @@ public function actionRequest()
             $authnRequest->getID()
         );
 
-        $relayState = null;
-        if (isset($_SERVER['HTTP_REFERER'])) {
-            $site = parse_url(Craft::$app->config->general->siteUrl);
-            $refer = parse_url($_SERVER['HTTP_REFERER']);
-            if ($site && $refer && isset($site['host']) && isset($refer['host'])
-                && $site['host'] == $refer['host']) {
-                $relayState = $_SERVER['HTTP_REFERER'];
-            }
-        }
-
         $authnRequest->setRelayState(
             SerializeHelper::toBase64(
                 Craft::$app->getUser()->getReturnUrl(
-                    /**
-                    * use refer here
-                    */
-                    $relayState
+                    Craft::$app->request->getParam(
+                        Saml::getInstance()->getSettings()->relayStateOverrideParam,
+                        null
+                    )
                 )
             )
         );

diff --git a/src/models/Settings.php b/src/models/Settings.php
@@ -122,4 +122,6 @@ class Settings extends AbstractSettings implements SettingsInterface
 
 
     ];
+
+    public $relayStateOverrideParam = 'RelayState';
 }
diff --git a/src/services/Login.php b/src/services/Login.php
@@ -26,7 +26,15 @@
 class Login extends Component
 {
 
+    /**
+     * Use before or after now
+     * @deprecated
+     */
     const EVENT_RESPONSE_TO_USER = 'eventResponseToUser';
+    const EVENT_BEFORE_RESPONSE_TO_USER = 'eventBeforeResponseToUser';
+    const EVENT_AFTER_RESPONSE_TO_USER = 'eventAfterResponseToUser';
+
+    protected $isAssertionDecrypted = false;
 
     /**
      * @param SamlResponse $response
@@ -45,10 +53,20 @@ public function login(SamlResponse $response)
         Saml::getInstance()->getResponse()->isValidTimeAssertion($assertion);
         Saml::getInstance()->getResponse()->isValidAssertion($assertion);
 
+        /**
+         * Get User
+         */
+        $user = $this->getUserByResponse($response);
+
         /**
          * Sync User
          */
-        $identity = $this->syncUser($response);
+        $this->syncUser($user, $response);
+
+        /**
+         * Get Identity
+         */
+        $identity = $this->getIdentityByUserAndResponse($user, $response);
 
         /**
          * Log user in
@@ -69,6 +87,10 @@ public function login(SamlResponse $response)
         return $identity;
     }
 
+    /**
+     * @param KeyChainRecord $keyChainRecord
+     * @param SamlResponse $response
+     */
     protected function decryptAssertions(KeyChainRecord $keyChainRecord, \LightSaml\Model\Protocol\Response $response)
     {
         Saml::getInstance()->getResponse()->decryptAssertions(
@@ -87,11 +109,18 @@ public function getFirstAssertion(\LightSaml\Model\Protocol\Response $response)
 
         $ownProvider = Saml::getInstance()->getProvider()->findOwn();
 
-        if ($ownProvider->keychain && $response->getFirstEncryptedAssertion()) {
+        if ($ownProvider->keychain &&
+            $response->getFirstEncryptedAssertion() &&
+            $this->isAssertionDecrypted === false
+        ) {
             $this->decryptAssertions(
                 $ownProvider->keychain,
                 $response
             );
+            /**
+             * try to only do this once
+             */
+            $this->isAssertionDecrypted = true;
         }
 
         $assertions = $response->getAllAssertions();
@@ -107,14 +136,12 @@ public function getFirstAssertion(\LightSaml\Model\Protocol\Response $response)
 
     /**
      * @param SamlResponse $response
-     * @return \flipbox\saml\sp\records\ProviderIdentityRecord
+     * @return User
      * @throws InvalidMessage
      * @throws UserException
      * @throws \Throwable
-     * @throws \craft\errors\ElementNotFoundException
-     * @throws \yii\base\Exception
      */
-    protected function syncUser(\LightSaml\Model\Protocol\Response $response)
+    protected function getUserByResponse(\LightSaml\Model\Protocol\Response $response)
     {
 
         $assertion = $this->getFirstAssertion($response);
@@ -126,83 +153,82 @@ protected function syncUser(\LightSaml\Model\Protocol\Response $response)
          */
         $username = $assertion->getSubject()->getNameID()->getValue();
 
-        $idpProvider = Saml::getInstance()->getProvider()->findByEntityId(
-            $response->getIssuer()->getValue()
-        );
+        return $this->getUser($username);
+    }
 
+    /**
+     * @param User $user
+     * @param SamlResponse $response
+     * @throws UserException
+     * @throws \Throwable
+     * @throws \craft\errors\ElementNotFoundException
+     * @throws \yii\base\Exception
+     */
+    protected function syncUser(User $user, \LightSaml\Model\Protocol\Response $response)
+    {
         /**
-         * Get Identity
+         * Before user save
          */
-        $identity = $this->forceGetIdentity($username, $idpProvider);
+        $event = new Event();
+        $event->sender = $response;
+        $event->data = $user;
+
+        $this->trigger(
+            static::EVENT_BEFORE_RESPONSE_TO_USER,
+            $event
+        );
 
         /**
-         * Ger User
+         * enable and transform the user
          */
-        if (! $user = $identity->getUser()) {
-            $user = $this->forceGetUser($username);
-        }
+        $this->constructUser($user, $response);
 
         /**
-         * Is User Active?
+         * Save
          */
-        if (! UserHelper::isUserActive($user)) {
-            if (! Saml::getInstance()->getSettings()->enableUsers) {
-                throw new UserException('User access denied.');
-            }
-            UserHelper::enableUser($user);
-        }
-
-        if ($assertion->getFirstAttributeStatement()) {
-            /**
-             *
-             */
-            $this->transformToUser($response, $user);
-
-        } else {
+        $this->saveUser($user);
 
-            /**
-             * There doesn't seem to be any attribute statements.
-             * Try and use username for the email and move on.
-             */
-            \Craft::warning(
-                'No attribute statements found! Trying to assign username as the email.',
-                Saml::getInstance()->getHandle()
-            );
-            $user->email = $user->username;
-        }
+        /**
+         * Sync groups depending on the plugin setting.
+         */
+        $this->syncUserGroupsByAssertion($user, $this->getFirstAssertion($response));
 
         /**
-         * Before user save
+         * after user save
          */
         $event = new Event();
         $event->sender = $response;
         $event->data = $user;
 
         $this->trigger(
-            static::EVENT_RESPONSE_TO_USER,
+            static::EVENT_AFTER_RESPONSE_TO_USER,
             $event
         );
+    }
 
-        if (! \Craft::$app->getElements()->saveElement($user)) {
-            Saml::error(
-                'User save failed: ' . json_encode($user->getErrors())
-            );
-            throw new UserException("User save failed: " . json_encode($user->getErrors()));
-        }
+    /**
+     * @param User $user
+     * @param SamlResponse $response
+     * @return ProviderIdentityRecord
+     * @throws UserException
+     */
+    protected function getIdentityByUserAndResponse(User $user, \LightSaml\Model\Protocol\Response $response)
+    {
 
+        $idpProvider = Saml::getInstance()->getProvider()->findByEntityId(
+            $response->getIssuer()->getValue()
+        );
         /**
-         * Sync groups depending on the plugin setting.
+         * Get Identity
          */
-        if (Saml::getInstance()->getSettings()->syncGroups) {
-            $this->syncUserGroupsByAssertion($user, $assertion);
-        }
+        $identity = $this->forceGetIdentity($user->username, $idpProvider);
 
         /**
          * Get Session
          */
         $sessionIndex = null;
-        if ($assertion->hasAnySessionIndex()) {
-            $sessionIndex = $assertion->getFirstAuthnStatement()->getSessionIndex();
+        if ($response->getFirstAssertion()->hasAnySessionIndex()) {
+            $sessionIndex = $response->getFirstAssertion()->getFirstAuthnStatement()->getSessionIndex();
         }
 
         /**
@@ -215,7 +241,76 @@ protected function syncUser(\LightSaml\Model\Protocol\Response $response)
     }
 
     /**
-     * @param $nameId
+     * @param User $user
+     * @return bool
+     * @throws UserException
+     * @throws \Throwable
+     * @throws \craft\errors\ElementNotFoundException
+     * @throws \yii\base\Exception
+     */
+    protected function saveUser(User $user)
+    {
+        if (! \Craft::$app->getElements()->saveElement($user)) {
+            Saml::error(
+                'User save failed: ' . json_encode($user->getErrors())
+            );
+            throw new UserException("User save failed: " . json_encode($user->getErrors()));
+        }
+
+        return true;
+    }
+
+    /**
+     * @param $username
+     * @return User
+     * @throws UserException
+     */
+    protected function getUser($username)
+    {
+        return $this->forceGetUser($username);
+    }
+
+    /**
+     * @param User $user
+     * @param SamlResponse $response
+     * @throws UserException
+     * @throws \Throwable
+     */
+    protected function constructUser(User $user, \LightSaml\Model\Protocol\Response $response)
+    {
+        /**
+         * Is User Active?
+         */
+        if (! UserHelper::isUserActive($user)) {
+            if (! Saml::getInstance()->getSettings()->enableUsers) {
+                throw new UserException('User access denied.');
+            }
+            UserHelper::enableUser($user);
+        }
+
+        $assertion = $this->getFirstAssertion($response);
+
+        if ($assertion->getFirstAttributeStatement()) {
+            /**
+             *
+             */
+            $this->transformToUser($response, $user);
+        } else {
+
+            /**
+             * There doesn't seem to be any attribute statements.
+             * Try and use username for the email and move on.
+             */
+            \Craft::warning(
+                'No attribute statements found! Trying to assign username as the email.',
+                Saml::getInstance()->getHandle()
+            );
+            $user->email = $user->username;
+        }
+    }
+
+    /**
+     * @param string $nameId
      * @param ProviderInterface $provider
      * @return ProviderIdentityRecord
      * @throws UserException
@@ -318,15 +413,33 @@ protected function getUserByUsernameOrEmail($usernameOrEmail, bool $archived = f
      */
     protected function syncUserGroupsByAssertion(User $user, Assertion $assertion)
     {
+        /**
+         * Nothing to do, move on
+         */
+        if (false === Saml::getInstance()->getSettings()->syncGroups) {
+            return true;
+        }
+
         $groupNames = Saml::getInstance()->getSettings()->groupAttributeNames;
         $groups = [];
         /**
          * Make sure there is an attribute statement
          */
-        if(!$assertion->getFirstAttributeStatement()) {
+        if (! $assertion->getFirstAttributeStatement()) {
+            Saml::debug(
+                'No attribute statement found, moving on.'
+            );
             return true;
         }
+
         foreach ($assertion->getFirstAttributeStatement()->getAllAttributes() as $attribute) {
+            Saml::debug(
+                sprintf(
+                    'Is attribute group? "%s" in %s',
+                    $attribute->getName(),
+                    json_encode($groupNames)
+                )
+            );
             /**
              * Is there a group name match?
              * Match the attribute name to the specified name in the plugin settings
@@ -346,9 +459,14 @@ protected function syncUserGroupsByAssertion(User $user, Assertion $assertion)
                  *           </saml2:AttributeValue>
                  * </saml2:Attribute>
                  */
-
                 foreach ($attribute->getAllAttributeValues() as $value) {
                     if ($group = $this->findOrCreateUserGroup($value)) {
+                        Saml::debug(
+                            sprintf(
+                                'Assigning group: %s',
+                                $group->name
+                            )
+                        );
                         $groups[] = $group->id;
                     }
                 }
@@ -413,7 +531,7 @@ protected function transformToUser(\LightSaml\Model\Protocol\Response $response,
     }
 
     /**
-     * @param $groupName
+     * @param string $groupName
      * @return UserGroup
      * @throws UserException
      * @throws \craft\errors\WrongEditionException