Refactored out the LDAP repo

app/Http/Controllers/Auth/LoginController.php

@@ -5,7 +5,6 @@ namespace BookStack\Http\Controllers\Auth;
 use BookStack\Exceptions\AuthException;
 use BookStack\Http\Controllers\Controller;
 use BookStack\Repos\UserRepo;
-use BookStack\Repos\LdapRepo;
 use BookStack\Services\LdapService;
 use BookStack\Services\SocialAuthService;
 use Illuminate\Contracts\Auth\Authenticatable;
@@ -38,18 +37,21 @@ class LoginController extends Controller
     protected $redirectAfterLogout = '/login';
 
     protected $socialAuthService;
+    protected $ldapService;
     protected $userRepo;
 
     /**
      * Create a new controller instance.
      *
      * @param SocialAuthService $socialAuthService
+     * @param LdapService $ldapService
      * @param UserRepo $userRepo
      */
-    public function __construct(SocialAuthService $socialAuthService, UserRepo $userRepo)
+    public function __construct(SocialAuthService $socialAuthService, LdapService $ldapService, UserRepo $userRepo)
     {
         $this->middleware('guest', ['only' => ['getLogin', 'postLogin']]);
         $this->socialAuthService = $socialAuthService;
+        $this->ldapService = $ldapService;
         $this->userRepo = $userRepo;
         $this->redirectPath = baseUrl('/');
         $this->redirectAfterLogout = baseUrl('/login');
@@ -98,13 +100,11 @@ class LoginController extends Controller
             auth()->login($user);
         }
 
-        // ldap groups refresh
+        // Sync LDAP groups if required
-        if (config('services.ldap.user_to_groups') !== false && $request->filled('username')) {
+        if ($this->ldapService->shouldSyncGroups()) {
-            $ldapRepo = new LdapRepo($this->userRepo, app(LdapService::class));
+            $this->ldapService->syncGroups($user);
-            $ldapRepo->syncGroups($user, $request->input('username'));
         }
 
-
         $path = session()->pull('url.intended', '/');
         $path = baseUrl($path, true);
         return redirect($path);
@@ -134,6 +134,7 @@ class LoginController extends Controller
      * Redirect to the relevant social site.
      * @param $socialDriver
      * @return \Symfony\Component\HttpFoundation\RedirectResponse
+     * @throws \BookStack\Exceptions\SocialDriverNotConfigured
      */
     public function getSocialLogin($socialDriver)
     {

app/Repos/LdapRepo.php

@@ -1,83 +0,0 @@
-<?php namespace BookStack\Repos;
-
-use BookStack\Services\LdapService;
-use BookStack\Role;
-
-class LdapRepo
-{
-
-    protected $ldap = null;
-    protected $ldapService = null;
-
-    protected $config;
-
-    /**
-     * LdapRepo constructor.
-     * @param \BookStack\Repos\UserRepo $userRepo
-     * @param LdapService $ldapService
-     */
-    public function __construct(UserRepo $userRepo, LdapService $ldapService)
-    {
-        $this->config = config('services.ldap');
-
-        if (config('auth.method') !== 'ldap') {
-            return false;
-        }
-
-        $this->ldapService = $ldapService;
-        $this->userRepo = $userRepo;
-    }
-
-    /**
-     * If there is no ldap connection, all methods calls to this library will return null
-     */
-    public function __call($method, $arguments)
-    {
-        if ($this->ldap === null) {
-            return null;
-        }
-
-        return call_user_func_array(array($this,$method), $arguments);
-    }
-
-    /**
-     * Sync the LDAP groups to the user roles for the current user
-     * @param \BookStack\User $user
-     * @param string $userName
-     * @throws \BookStack\Exceptions\NotFoundException
-     */
-    public function syncGroups($user, $userName)
-    {
-        $userLdapGroups = $this->ldapService->getUserGroups($userName);
-        $userLdapGroups = $this->groupNameFilter($userLdapGroups);
-        // get the ids for the roles from the names
-        $ldapGroupsAsRoles = Role::whereIn('name', $userLdapGroups)->pluck('id');
-        // sync groups
-        if ($this->config['remove_from_groups']) {
-            $user->roles()->sync($ldapGroupsAsRoles);
-            $this->userRepo->attachDefaultRole($user);
-        } else {
-            $user->roles()->syncWithoutDetaching($ldapGroupsAsRoles);
-        }
-
-        // make the user an admin?
-        if (in_array($this->config['admin'], $userLdapGroups)) {
-            $this->userRepo->attachSystemRole($user, 'admin');
-        }
-    }
-
-    /**
-     * Filter to convert the groups from ldap to the format of the roles name on BookStack
-     * Spaces replaced with -, all lowercase letters
-     * @param array $groups
-     * @return array
-     */
-    private function groupNameFilter($groups)
-    {
-        $return = [];
-        foreach ($groups as $groupName) {
-            $return[] = str_replace(' ', '-', strtolower($groupName));
-        }
-        return $return;
-    }
-}

app/Services/LdapService.php

@@ -1,6 +1,9 @@
 <?php namespace BookStack\Services;
 
 use BookStack\Exceptions\LdapException;
+use BookStack\Repos\UserRepo;
+use BookStack\Role;
+use BookStack\User;
 use Illuminate\Contracts\Auth\Authenticatable;
 
 /**
@@ -14,15 +17,29 @@ class LdapService
     protected $ldap;
     protected $ldapConnection;
     protected $config;
+    protected $userRepo;
+    protected $enabled;
 
     /**
      * LdapService constructor.
      * @param Ldap $ldap
+     * @param UserRepo $userRepo
      */
-    public function __construct(Ldap $ldap)
+    public function __construct(Ldap $ldap, UserRepo $userRepo)
     {
         $this->ldap = $ldap;
         $this->config = config('services.ldap');
+        $this->userRepo = $userRepo;
+        $this->enabled = config('auth.method') === 'ldap';
+    }
+
+    /**
+     * Check if groups should be synced.
+     * @return bool
+     */
+    public function shouldSyncGroups()
+    {
+        return $this->enabled && $this->config['user_to_groups'] !== false;
     }
 
     /**
@@ -185,6 +202,7 @@ class LdapService
      * Get the groups a user is a part of on ldap
      * @param string $userName
      * @return array|null
+     * @throws LdapException
      */
     public function getUserGroups($userName)
     {
@@ -205,6 +223,7 @@ class LdapService
      * @param array $groupsArray
      * @param array $checked
      * @return array
+     * @throws LdapException
      */
     private function getGroupsRecursive($groupsArray, $checked)
     {
@@ -231,6 +250,7 @@ class LdapService
      * Get the parent groups of a single group
      * @param string $groupName
      * @return array
+     * @throws LdapException
      */
     private function getGroupGroups($groupName)
     {
@@ -274,4 +294,48 @@ class LdapService
         }
         return $ldapGroups;
     }
+
+    /**
+     * Sync the LDAP groups to the user roles for the current user
+     * @param \BookStack\User $user
+     * @throws LdapException
+     * @throws \BookStack\Exceptions\NotFoundException
+     */
+    public function syncGroups(User $user)
+    {
+        $userLdapGroups = $this->getUserGroups($user->external_auth_id);
+        $userLdapGroups = $this->groupNameFilter($userLdapGroups);
+
+        // Get the ids for the roles from the names
+        $ldapGroupsAsRoles = Role::query()->whereIn('name', $userLdapGroups)->pluck('id');
+
+        // Sync groups
+        if ($this->config['remove_from_groups']) {
+            $user->roles()->sync($ldapGroupsAsRoles);
+            $this->userRepo->attachDefaultRole($user);
+        } else {
+            $user->roles()->syncWithoutDetaching($ldapGroupsAsRoles);
+        }
+
+        // make the user an admin?
+        // TODO - Remove
+        if (in_array($this->config['admin'], $userLdapGroups)) {
+            $this->userRepo->attachSystemRole($user, 'admin');
+        }
+    }
+
+    /**
+     * Filter to convert the groups from ldap to the format of the roles name on BookStack
+     * Spaces replaced with -, all lowercase letters
+     * @param array $groups
+     * @return array
+     */
+    private function groupNameFilter(array $groups)
+    {
+        $return = [];
+        foreach ($groups as $groupName) {
+            $return[] = str_replace(' ', '-', strtolower($groupName));
+        }
+        return $return;
+    }
 }