Refactor for codestyle

diff --git a/app/Auth/Access/ExternalAuthService.php b/app/Auth/Access/ExternalAuthService.php
new file mode 100644 (file)
index 0000000..b1c0360
--- /dev/null
+++ b/app/Auth/Access/ExternalAuthService.php
@@ -0,0 +1,75 @@
+<?php namespace BookStack\Auth\Access;
+
+use BookStack\Auth\Role;
+use BookStack\Auth\User;
+
+class ExternalAuthService
+{
+    /**
+     * Check a role against an array of group names to see if it matches.
+     * Checked against role 'external_auth_id' if set otherwise the name of the role.
+     * @param \BookStack\Auth\Role $role
+     * @param array $groupNames
+     * @return bool
+     */
+    protected function roleMatchesGroupNames(Role $role, array $groupNames)
+    {
+        if ($role->external_auth_id) {
+            $externalAuthIds = explode(',', strtolower($role->external_auth_id));
+            foreach ($externalAuthIds as $externalAuthId) {
+                if (in_array(trim($externalAuthId), $groupNames)) {
+                    return true;
+                }
+            }
+            return false;
+        }
+
+        $roleName = str_replace(' ', '-', trim(strtolower($role->display_name)));
+        return in_array($roleName, $groupNames);
+    }
+
+    /**
+     * Match an array of group names to BookStack system roles.
+     * Formats group names to be lower-case and hyphenated.
+     * @param array $groupNames
+     * @return \Illuminate\Support\Collection
+     */
+    protected function matchGroupsToSystemsRoles(array $groupNames)
+    {
+        foreach ($groupNames as $i => $groupName) {
+            $groupNames[$i] = str_replace(' ', '-', trim(strtolower($groupName)));
+        }
+
+        $roles = Role::query()->where(function (Builder $query) use ($groupNames) {
+            $query->whereIn('name', $groupNames);
+            foreach ($groupNames as $groupName) {
+                $query->orWhere('external_auth_id', 'LIKE', '%' . $groupName . '%');
+            }
+        })->get();
+
+        $matchedRoles = $roles->filter(function (Role $role) use ($groupNames) {
+            return $this->roleMatchesGroupNames($role, $groupNames);
+        });
+
+        return $matchedRoles->pluck('id');
+    }
+
+    /**
+     * Sync the groups to the user roles for the current user
+     * @param \BookStack\Auth\User $user
+     * @param array $samlAttributes
+     */
+    public function syncWithGroups(User $user, array $userGroups)
+    {
+        // Get the ids for the roles from the names
+        $samlGroupsAsRoles = $this->matchGroupsToSystemsRoles($userSamlGroups);
+
+        // Sync groups
+        if ($this->config['remove_from_groups']) {
+            $user->roles()->sync($samlGroupsAsRoles);
+            $this->userRepo->attachDefaultRole($user);
+        } else {
+            $user->roles()->syncWithoutDetaching($samlGroupsAsRoles);
+        }
+    }
+}

diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php
index c7415e1f738e723bd18e98ea9a3f5c6a217bb697..3111ea9faf11f4c572a2ceac0dcb221be20a00ee 100644 (file)
--- a/app/Auth/Access/LdapService.php
+++ b/app/Auth/Access/LdapService.php
@@ -1,7 +1,6 @@
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\Access;
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\Access;

-use BookStack\Auth\Role;

 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\LdapException;
 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\LdapException;


@@ -13,7 +12,7 @@ use Illuminate\Database\Eloquent\Builder;
  * Handles any app-specific LDAP tasks.
  * @package BookStack\Services
  */
  * Handles any app-specific LDAP tasks.
  * @package BookStack\Services
  */

-class LdapService
+class LdapService extends Access\ExternalAuthService

 {
 
     protected $ldap;
 {
 
     protected $ldap;


@@ -351,65 +350,6 @@ class LdapService
     public function syncGroups(User $user, string $username)
     {
         $userLdapGroups = $this->getUserGroups($username);
     public function syncGroups(User $user, string $username)
     {
         $userLdapGroups = $this->getUserGroups($username);

-
-        // Get the ids for the roles from the names
-        $ldapGroupsAsRoles = $this->matchLdapGroupsToSystemsRoles($userLdapGroups);
-
-        // Sync groups
-        if ($this->config['remove_from_groups']) {
-            $user->roles()->sync($ldapGroupsAsRoles);
-            $this->userRepo->attachDefaultRole($user);
-        } else {
-            $user->roles()->syncWithoutDetaching($ldapGroupsAsRoles);
-        }
-    }
-
-    /**
-     * Match an array of group names from LDAP to BookStack system roles.
-     * Formats LDAP group names to be lower-case and hyphenated.
-     * @param array $groupNames
-     * @return \Illuminate\Support\Collection
-     */
-    protected function matchLdapGroupsToSystemsRoles(array $groupNames)
-    {
-        foreach ($groupNames as $i => $groupName) {
-            $groupNames[$i] = str_replace(' ', '-', trim(strtolower($groupName)));
-        }
-
-        $roles = Role::query()->where(function (Builder $query) use ($groupNames) {
-            $query->whereIn('name', $groupNames);
-            foreach ($groupNames as $groupName) {
-                $query->orWhere('external_auth_id', 'LIKE', '%' . $groupName . '%');
-            }
-        })->get();
-
-        $matchedRoles = $roles->filter(function (Role $role) use ($groupNames) {
-            return $this->roleMatchesGroupNames($role, $groupNames);
-        });
-
-        return $matchedRoles->pluck('id');
-    }
-
-    /**
-     * Check a role against an array of group names to see if it matches.
-     * Checked against role 'external_auth_id' if set otherwise the name of the role.
-     * @param \BookStack\Auth\Role $role
-     * @param array $groupNames
-     * @return bool
-     */
-    protected function roleMatchesGroupNames(Role $role, array $groupNames)
-    {
-        if ($role->external_auth_id) {
-            $externalAuthIds = explode(',', strtolower($role->external_auth_id));
-            foreach ($externalAuthIds as $externalAuthId) {
-                if (in_array(trim($externalAuthId), $groupNames)) {
-                    return true;
-                }
-            }
-            return false;
-        }
-
-        $roleName = str_replace(' ', '-', trim(strtolower($role->display_name)));
-        return in_array($roleName, $groupNames);
+        $this->syncWithGroups($user, $userLdapGroups);

     }
 }
     }
 }

diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index 0b6cbe80594b1669b2b77820a24d19010c75cee9..95049efd2ec1152b0ee6cd686af907218fccea44 100644 (file)
--- a/app/Auth/Access/Saml2Service.php
+++ b/app/Auth/Access/Saml2Service.php
@@ -1,13 +1,11 @@
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\Access;
 <?php namespace BookStack\Auth\Access;
 
 use BookStack\Auth\Access;

-use BookStack\Auth\Role;

 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\SamlException;
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Database\Eloquent\Builder;
 use BookStack\Auth\User;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\SamlException;
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Database\Eloquent\Builder;

-use Illuminate\Support\Facades\Auth;

 use Illuminate\Support\Facades\Log;
 
 
 use Illuminate\Support\Facades\Log;
 
 


@@ -16,7 +14,7 @@ use Illuminate\Support\Facades\Log;
  * Handles any app-specific SAML tasks.
  * @package BookStack\Services
  */
  * Handles any app-specific SAML tasks.
  * @package BookStack\Services
  */

-class Saml2Service
+class Saml2Service extends Access\ExternalAuthService

 {
     protected $config;
     protected $userRepo;
 {
     protected $config;
     protected $userRepo;


@@ -44,25 +42,14 @@ class Saml2Service
         return $this->enabled && $this->config['user_to_groups'] !== false;
     }
 
         return $this->enabled && $this->config['user_to_groups'] !== false;
     }
 

-    /**
-     * Extract the details of a user from a SAML response.
-     * @param $samlID
-     * @param $samlAttributes
-     * @return array
+    /** Calculate the display name
+     *  @param array $samlAttributes
+     *  @param string $defaultValue
+     *  @return string

      */
      */

-    public function getUserDetails($samlID, $samlAttributes)
+    protected function getUserDisplayName(array $samlAttributes, string $defaultValue)

     {
     {

-        $emailAttr = $this->config['email_attribute'];

         $displayNameAttr = $this->config['display_name_attribute'];
         $displayNameAttr = $this->config['display_name_attribute'];

-        $userNameAttr = $this->config['user_name_attribute'];
-
-        $email = $this->getSamlResponseAttribute($samlAttributes, $emailAttr, null);
-
-        if ($userNameAttr === null) {
-          $userName = $samlID;
-        } else {
-          $userName = $this->getSamlResponseAttribute($samlAttributes, $userNameAttr, $samlID);
-        }

 
         $displayName = [];
         foreach ($displayNameAttr as $dnAttr) {
 
         $displayName = [];
         foreach ($displayNameAttr as $dnAttr) {


@@ -73,16 +60,43 @@ class Saml2Service
         }
 
         if (count($displayName) == 0) {
         }
 
         if (count($displayName) == 0) {

-          $displayName = $userName;
+          $displayName = $defaultValue;

         } else {
           $displayName = implode(' ', $displayName);
         }
 
         } else {
           $displayName = implode(' ', $displayName);
         }
 

+        return $displayName;
+    }
+
+    protected function getUserName(array $samlAttributes, string $defaultValue)
+    {
+        $userNameAttr = $this->config['user_name_attribute'];
+
+        if ($userNameAttr === null) {
+            $userName = $defaultValue;
+        } else {
+            $userName = $this->getSamlResponseAttribute($samlAttributes, $userNameAttr, $defaultValue);
+        }
+
+        return $userName;
+    }
+
+    /**
+     * Extract the details of a user from a SAML response.
+     * @param $samlID
+     * @param $samlAttributes
+     * @return array
+     */
+    public function getUserDetails($samlID, $samlAttributes)
+    {
+        $emailAttr = $this->config['email_attribute'];
+        $userName = $this->getUserName($samlAttributes, $samlID);
+

         return [
             'uid'   => $userName,
         return [
             'uid'   => $userName,

-            'name'  => $displayName,
+            'name'  => $this->getUserDisplayName($samlAttributes, $userName),

             'dn'    => $samlID,
             'dn'    => $samlID,

-            'email' => $email,
+            'email' => $this->getSamlResponseAttribute($samlAttributes, $emailAttr, null),

         ];
     }
 
         ];
     }
 


@@ -115,22 +129,28 @@ class Saml2Service
     {
         if (isset($samlAttributes[$propertyKey])) {
             $data = $samlAttributes[$propertyKey];
     {
         if (isset($samlAttributes[$propertyKey])) {
             $data = $samlAttributes[$propertyKey];

-            if (!is_array($data)) {
-              return $data;
-            } else if (count($data) == 0) {
-              return $defaultValue;
-            } else if (count($data) == 1) {
-              return $data[0];
-            } else {
-              return $data;
+            if (is_array($data)) {
+              if (count($data) == 0) {
+                $data = $defaultValue;
+              } else if (count($data) == 1) {
+                $data = $data[0];
+              }

             }
             }

+        } else {
+          $data = $defaultValue;

         }
 
         }
 

-        return $defaultValue;
+        return $data;

     }
 
     }
 

-    protected function registerUser($userDetails) {
-
+    /**
+     *  Register a user that is authenticated but not
+     *  already registered.
+     *  @param array $userDetails
+     *  @return User
+     */
+    protected function registerUser($userDetails)
+    {

         // Create an array of the user data to create a new user instance
         $userData = [
             'name' => $userDetails['name'],
         // Create an array of the user data to create a new user instance
         $userData = [
             'name' => $userDetails['name'],


@@ -146,96 +166,47 @@ class Saml2Service
         return $user;
     }
 
         return $user;
     }
 

-    public function processLoginCallback($samlID, $samlAttributes) {
-
-        $userDetails = $this->getUserDetails($samlID, $samlAttributes);
+    /**
+     * Get the user from the database for the specified details.
+     * @param array $userDetails
+     * @return User|null
+     */
+    protected function getOrRegisterUser($userDetails)
+    {
+        $isRegisterEnabled = config('services.saml.auto_register') === true;

         $user = $this->user
         $user = $this->user

-            ->where('external_auth_id', $userDetails['uid'])
-            ->first();
+          ->where('external_auth_id', $userDetails['uid'])
+          ->first();

 
 

-        $isLoggedIn = auth()->check();
-
-        if (!$isLoggedIn) {
-            if ($user === null && config('services.saml.auto_register') === true) {
-                $user = $this->registerUser($userDetails);
-            }
-
-            if ($user !== null) {
-                auth()->login($user);
-            }
+        if ($user === null && $isRegisterEnabled) {
+            $user = $this->registerUser($userDetails);

         }
 
         return $user;
     }
 
     /**
         }
 
         return $user;
     }
 
     /**

-     * Sync the SAML groups to the user roles for the current user
-     * @param \BookStack\Auth\User $user
-     * @param array $samlAttributes
+     *  Process the SAML response for a user. Login the user when
+     *  they exist, optionally registering them automatically.
+     *  @param string $samlID
+     *  @param array $samlAttributes

      */
      */

-    public function syncGroups(User $user, array $samlAttributes)
+    public function processLoginCallback($samlID, $samlAttributes)

     {
     {

-        $userSamlGroups = $this->getUserGroups($samlAttributes);
-
-        // Get the ids for the roles from the names
-        $samlGroupsAsRoles = $this->matchSamlGroupsToSystemsRoles($userSamlGroups);
+        $userDetails = $this->getUserDetails($samlID, $samlAttributes);
+        $isLoggedIn = auth()->check();

 
 

-        // Sync groups
-        if ($this->config['remove_from_groups']) {
-            $user->roles()->sync($samlGroupsAsRoles);
-            $this->userRepo->attachDefaultRole($user);
+        if ($isLoggedIn) {
+            logger()->error("Already logged in");

         } else {
         } else {

-            $user->roles()->syncWithoutDetaching($samlGroupsAsRoles);
-        }
-    }
-
-    /**
-     * Match an array of group names from SAML to BookStack system roles.
-     * Formats group names to be lower-case and hyphenated.
-     * @param array $groupNames
-     * @return \Illuminate\Support\Collection
-     */
-    protected function matchSamlGroupsToSystemsRoles(array $groupNames)
-    {
-        foreach ($groupNames as $i => $groupName) {
-            $groupNames[$i] = str_replace(' ', '-', trim(strtolower($groupName)));
-        }
-
-        $roles = Role::query()->where(function (Builder $query) use ($groupNames) {
-            $query->whereIn('name', $groupNames);
-            foreach ($groupNames as $groupName) {
-                $query->orWhere('external_auth_id', 'LIKE', '%' . $groupName . '%');
-            }
-        })->get();
-
-        $matchedRoles = $roles->filter(function (Role $role) use ($groupNames) {
-            return $this->roleMatchesGroupNames($role, $groupNames);
-        });
-
-        return $matchedRoles->pluck('id');
-    }
-
-    /**
-     * Check a role against an array of group names to see if it matches.
-     * Checked against role 'external_auth_id' if set otherwise the name of the role.
-     * @param \BookStack\Auth\Role $role
-     * @param array $groupNames
-     * @return bool
-     */
-    protected function roleMatchesGroupNames(Role $role, array $groupNames)
-    {
-        if ($role->external_auth_id) {
-            $externalAuthIds = explode(',', strtolower($role->external_auth_id));
-            foreach ($externalAuthIds as $externalAuthId) {
-                if (in_array(trim($externalAuthId), $groupNames)) {
-                    return true;
-                }
+            $user = $this->getOrRegisterUser($userDetails);
+            if ($user === null) {
+                logger()->error("User does not exist");
+            } else {
+                auth()->login($user);

             }
             }

-            return false;

         }
 
         }
 

-        $roleName = str_replace(' ', '-', trim(strtolower($role->display_name)));
-        return in_array($roleName, $groupNames);
+        return $user;

     }
     }

-

 }
 }