Auth: Refactored OIDC RP-logout PR code, Extracted logout

Extracted logout to the login service so the logic can be shared instead of re-implemented at each stage. For this, the SocialAuthService was split so the driver management is in its own class, so it can be used elsewhere without use (or circular dependencies) of the SocialAuthService. During review of #4467
BookStackApp · Dec 6, 2023 · bba7dcc · bba7dcc
1 parent cc10d1d
commit bba7dcc
Show file tree

Hide file tree

Showing 17 changed files with 263 additions and 288 deletions.
diff --git a/.env.example.complete b/.env.example.complete
@@ -273,11 +273,8 @@ OIDC_USER_TO_GROUPS=false
 OIDC_GROUPS_CLAIM=groups
 OIDC_REMOVE_FROM_GROUPS=false
 OIDC_EXTERNAL_ID_CLAIM=sub
-
-# OIDC Logout Feature: Its value should be value of end_session_endpoint from <issuer>/.well-known/openid-configuration 
 OIDC_END_SESSION_ENDPOINT=null
 
-
 # Disable default third-party services such as Gravatar and Draw.IO
 # Service-specific options will override this option
 DISABLE_EXTERNAL_SERVICES=false

diff --git a/app/Access/Controllers/LoginController.php b/app/Access/Controllers/LoginController.php
@@ -3,7 +3,7 @@
 namespace BookStack\Access\Controllers;
 
 use BookStack\Access\LoginService;
-use BookStack\Access\SocialAuthService;
+use BookStack\Access\SocialDriverManager;
 use BookStack\Exceptions\LoginAttemptEmailNeededException;
 use BookStack\Exceptions\LoginAttemptException;
 use BookStack\Facades\Activity;
@@ -17,19 +17,19 @@ class LoginController extends Controller
 {
     use ThrottlesLogins;
 
-    protected SocialAuthService $socialAuthService;
+    protected SocialDriverManager $socialDriverManager;
     protected LoginService $loginService;
 
     /**
      * Create a new controller instance.
      */
-    public function __construct(SocialAuthService $socialAuthService, LoginService $loginService)
+    public function __construct(SocialDriverManager $driverManager, LoginService $loginService)
     {
         $this->middleware('guest', ['only' => ['getLogin', 'login']]);
         $this->middleware('guard:standard,ldap', ['only' => ['login']]);
         $this->middleware('guard:standard,ldap,oidc', ['only' => ['logout']]);
 
-        $this->socialAuthService = $socialAuthService;
+        $this->socialDriverManager = $driverManager;
         $this->loginService = $loginService;
     }
 
@@ -38,7 +38,7 @@ public function __construct(SocialAuthService $socialAuthService, LoginService $
      */
     public function getLogin(Request $request)
     {
-        $socialDrivers = $this->socialAuthService->getActiveDrivers();
+        $socialDrivers = $this->socialDriverManager->getActive();
         $authMethod = config('auth.method');
         $preventInitiation = $request->get('prevent_auto_init') === 'true';
 
@@ -101,15 +101,9 @@ public function login(Request $request)
     /**
      * Logout user and perform subsequent redirect.
      */
-    public function logout(Request $request)
+    public function logout()
     {
-        Auth::guard()->logout();
-        $request->session()->invalidate();
-        $request->session()->regenerateToken();
-
-        $redirectUri = $this->shouldAutoInitiate() ? '/login?prevent_auto_init=true' : '/';
-
-        return redirect($redirectUri);
+        return redirect($this->loginService->logout());
     }
 
     /**
@@ -218,16 +212,4 @@ protected function updateIntendedFromPrevious(): void
 
         redirect()->setIntendedUrl($previous);
     }
-
-    /**
-     * Check if login auto-initiate should be valid based upon authentication config.
-     */
-    protected function shouldAutoInitiate(): bool
-    {
-        $socialDrivers = $this->socialAuthService->getActiveDrivers();
-        $authMethod = config('auth.method');
-        $autoRedirect = config('auth.auto_initiate');
-
-        return $autoRedirect && count($socialDrivers) === 0 && in_array($authMethod, ['oidc', 'saml2']);
-    }
 }
diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php
@@ -11,9 +11,6 @@ class OidcController extends Controller
 {
     protected OidcService $oidcService;
 
-    /**
-     * OpenIdController constructor.
-     */
     public function __construct(OidcService $oidcService)
     {
         $this->oidcService = $oidcService;
@@ -65,16 +62,10 @@ public function callback(Request $request)
     }
 
     /**
-     * OIDC Logout Feature: Start the authorization logout flow via OIDC.
+     * Log the user out then start the OIDC RP-initiated logout process.
      */
     public function logout()
     {
-        try {
-            return $this->oidcService->logout();
-        } catch (OidcException $exception) {
-            $this->showErrorNotification($exception->getMessage());
-            return redirect('/logout');
-        }
+        return redirect($this->oidcService->logout());
     }
-
 }
diff --git a/app/Access/Controllers/RegisterController.php b/app/Access/Controllers/RegisterController.php
@@ -4,7 +4,7 @@
 
 use BookStack\Access\LoginService;
 use BookStack\Access\RegistrationService;
-use BookStack\Access\SocialAuthService;
+use BookStack\Access\SocialDriverManager;
 use BookStack\Exceptions\StoppedAuthenticationException;
 use BookStack\Exceptions\UserRegistrationException;
 use BookStack\Http\Controller;
@@ -15,22 +15,22 @@
 
 class RegisterController extends Controller
 {
-    protected SocialAuthService $socialAuthService;
+    protected SocialDriverManager $socialDriverManager;
     protected RegistrationService $registrationService;
     protected LoginService $loginService;
 
     /**
      * Create a new controller instance.
      */
     public function __construct(
-        SocialAuthService $socialAuthService,
+        SocialDriverManager $socialDriverManager,
         RegistrationService $registrationService,
         LoginService $loginService
     ) {
         $this->middleware('guest');
         $this->middleware('guard:standard');
 
-        $this->socialAuthService = $socialAuthService;
+        $this->socialDriverManager = $socialDriverManager;
         $this->registrationService = $registrationService;
         $this->loginService = $loginService;
     }
@@ -43,7 +43,7 @@ public function __construct(
     public function getRegister()
     {
         $this->registrationService->ensureRegistrationAllowed();
-        $socialDrivers = $this->socialAuthService->getActiveDrivers();
+        $socialDrivers = $this->socialDriverManager->getActive();
 
         return view('auth.register', [
             'socialDrivers' => $socialDrivers,

diff --git a/app/Access/Controllers/SocialController.php b/app/Access/Controllers/SocialController.php
@@ -79,7 +79,7 @@ public function callback(Request $request, string $socialDriver)
             try {
                 return $this->socialAuthService->handleLoginCallback($socialDriver, $socialUser);
             } catch (SocialSignInAccountNotUsed $exception) {
-                if ($this->socialAuthService->driverAutoRegisterEnabled($socialDriver)) {
+                if ($this->socialAuthService->drivers()->isAutoRegisterEnabled($socialDriver)) {
                     return $this->socialRegisterCallback($socialDriver, $socialUser);
                 }
 
@@ -114,7 +114,7 @@ protected function socialRegisterCallback(string $socialDriver, SocialUser $soci
     {
         $socialUser = $this->socialAuthService->handleRegistrationCallback($socialDriver, $socialUser);
         $socialAccount = $this->socialAuthService->newSocialAccount($socialDriver, $socialUser);
-        $emailVerified = $this->socialAuthService->driverAutoConfirmEmailEnabled($socialDriver);
+        $emailVerified = $this->socialAuthService->drivers()->isAutoConfirmEmailEnabled($socialDriver);
 
         // Create an array of the user data to create a new user instance
         $userData = [

diff --git a/app/Access/LoginService.php b/app/Access/LoginService.php
@@ -16,13 +16,11 @@ class LoginService
 {
     protected const LAST_LOGIN_ATTEMPTED_SESSION_KEY = 'auth-login-last-attempted';
 
-    protected $mfaSession;
-    protected $emailConfirmationService;
-
-    public function __construct(MfaSession $mfaSession, EmailConfirmationService $emailConfirmationService)
-    {
-        $this->mfaSession = $mfaSession;
-        $this->emailConfirmationService = $emailConfirmationService;
+    public function __construct(
+        protected MfaSession $mfaSession,
+        protected EmailConfirmationService $emailConfirmationService,
+        protected SocialDriverManager $socialDriverManager,
+    ) {
     }
 
     /**
@@ -163,4 +161,29 @@ public function attempt(array $credentials, string $method, bool $remember = fal
 
         return $result;
     }
+
+    /**
+     * Logs the current user out of the application.
+     * Returns an app post-redirect path.
+     */
+    public function logout(): string
+    {
+        auth()->logout();
+        session()->invalidate();
+        session()->regenerateToken();
+
+        return $this->shouldAutoInitiate() ? '/login?prevent_auto_init=true' : '/';
+    }
+
+    /**
+     * Check if login auto-initiate should be valid based upon authentication config.
+     */
+    protected function shouldAutoInitiate(): bool
+    {
+        $socialDrivers = $this->socialDriverManager->getActive();
+        $authMethod = config('auth.method');
+        $autoRedirect = config('auth.auto_initiate');
+
+        return $autoRedirect && count($socialDrivers) === 0 && in_array($authMethod, ['oidc', 'saml2']);
+    }
 }
diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
@@ -217,11 +217,7 @@ protected function processAccessTokenCallback(OidcAccessToken $accessToken, Oidc
             $settings->keys,
         );
 
-        // OIDC Logout Feature: Temporarily save token in session 
-        $access_token_for_logout = $idTokenText;
-        session()->put("oidctoken", $access_token_for_logout);
-
-
+        session()->put("oidc_id_token", $idTokenText);
 
         $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [
             'access_token' => $accessToken->getToken(),
@@ -291,36 +287,24 @@ protected function shouldSyncGroups(): bool
         return $this->config()['user_to_groups'] !== false;
     }
 
-
     /**
-     * OIDC Logout Feature: Initiate a logout flow.
-     *
-     * @throws OidcException
-     *
-     * @return string
+     * Start the RP-initiated logout flow if active, otherwise start a standard logout flow.
+     * Returns a post-app-logout redirect URL.
+     * Reference: https://openid.net/specs/openid-connect-rpinitiated-1_0.html
      */
-    public function logout() {
-
-        $config = $this->config();
-        $app_url = env('APP_URL', '');
-        $end_session_endpoint = $config["end_session_endpoint"];
-
-        $oidctoken = session()->get("oidctoken");
-        session()->invalidate();
-
-        if (str_contains($app_url, 'https://')) { 
-             $protocol = 'https://';
-        } else {
-             $protocol = 'http://';
-        }
-
-
+    public function logout(): string
+    {
+        $endSessionEndpoint = $this->config()["end_session_endpoint"];
 
-        return redirect($end_session_endpoint.'?id_token_hint='.$oidctoken."&post_logout_redirect_uri=".$protocol.$_SERVER['HTTP_HOST']."/");
+        // TODO - Add autodiscovery and false/null config value support.
 
+        $oidcToken = session()->pull("oidc_id_token");
+        $defaultLogoutUrl = url($this->loginService->logout());
+        $endpointParams = [
+            'id_token_hint' => $oidcToken,
+            'post_logout_redirect_uri' => $defaultLogoutUrl,
+        ];
 
+        return $endSessionEndpoint . '?' . http_build_query($endpointParams);
     }
-
-
-
 }
diff --git a/app/Access/Saml2Service.php b/app/Access/Saml2Service.php
@@ -71,8 +71,7 @@ public function logout(User $user): array
                 throw $error;
             }
 
-            $this->actionLogout();
-            $url = '/';
+            $url = $this->loginService->logout();
             $id = null;
         }
 
@@ -140,20 +139,11 @@ public function processSlsResponse(?string $requestId): ?string
             );
         }
 
-        $this->actionLogout();
+        $this->loginService->logout();
 
         return $redirect;
     }
 
-    /**
-     * Do the required actions to log a user out.
-     */
-    protected function actionLogout()
-    {
-        auth()->logout();
-        session()->invalidate();
-    }
-
     /**
      * Get the metadata for this service provider.
      *