From 76a021d74493c6a5da1d5ec68b2a5508c182018e Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Tue, 16 Jan 2024 11:41:27 +0100 Subject: [PATCH 1/5] make sure the user_oidc login does not redirect to logout (when allow_multiple_user_backends == 0) Signed-off-by: Julien Veyssier --- lib/AppInfo/Application.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 8862be06..49660a59 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -98,6 +98,12 @@ private function registerRedirect(IRequest $request, IURLGenerator $urlGenerator // in case any errors happen when checking for the path do not apply redirect logic as it is only needed for the login } if ($isDefaultLogin && !$settings->getAllowMultipleUserBackEnds() && count($providers) === 1) { + // To avoid login/logout loop if the IdP session is still alive: + // if the login page's redirect_url GET param is the logout page, just use the base URL instead + $logoutUrl = $urlGenerator->linkToRoute('core.login.logout'); + if (strpos($redirectUrl, $logoutUrl) !== false) { + $redirectUrl = $urlGenerator->getBaseUrl(); + } $targetUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.login', [ 'providerId' => $providers[0]->getId(), 'redirectUrl' => $redirectUrl From 751a1971d630c133cbdbb3e9ec984f6a115b3a96 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Tue, 16 Jan 2024 13:18:22 +0100 Subject: [PATCH 2/5] also avoid login redirect to user_oidc's single logout Signed-off-by: Julien Veyssier --- lib/AppInfo/Application.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 49660a59..bfa8c31f 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -101,7 +101,8 @@ private function registerRedirect(IRequest $request, IURLGenerator $urlGenerator // To avoid login/logout loop if the IdP session is still alive: // if the login page's redirect_url GET param is the logout page, just use the base URL instead $logoutUrl = $urlGenerator->linkToRoute('core.login.logout'); - if (strpos($redirectUrl, $logoutUrl) !== false) { + $userOidcLogoutUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.singleLogoutService'); + if (strpos($redirectUrl, $logoutUrl) !== false || strpos($redirectUrl, $userOidcLogoutUrl) !== false) { $redirectUrl = $urlGenerator->getBaseUrl(); } $targetUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.login', [ From 7faff3f42649722eb7f8946e27113d6c09610e59 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Tue, 16 Jan 2024 13:29:26 +0100 Subject: [PATCH 3/5] add comment about improving when registerRedirect is called Signed-off-by: Julien Veyssier --- lib/AppInfo/Application.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index bfa8c31f..4f0d6f86 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -87,6 +87,10 @@ public function boot(IBootContext $context): void { } private function registerRedirect(IRequest $request, IURLGenerator $urlGenerator, SettingsService $settings, ProviderMapper $providerMapper): void { + // TODO when min supported version is >=28 : + // run this in a listener of OCP\AppFramework\Http\Events\BeforeLoginTemplateRenderedEvent + // to avoid doing useless stuff on data requests and template requests that are not the login page + $providers = $this->getCachedProviders($providerMapper); $redirectUrl = $request->getParam('redirect_url'); From 531107012c70a7622dfbb55e315010fae81d0695 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Wed, 17 Jan 2024 00:21:11 +0100 Subject: [PATCH 4/5] fix sso redirect when redirectUrl is null Signed-off-by: Julien Veyssier --- lib/AppInfo/Application.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 4f0d6f86..8188aaff 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -106,7 +106,10 @@ private function registerRedirect(IRequest $request, IURLGenerator $urlGenerator // if the login page's redirect_url GET param is the logout page, just use the base URL instead $logoutUrl = $urlGenerator->linkToRoute('core.login.logout'); $userOidcLogoutUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.singleLogoutService'); - if (strpos($redirectUrl, $logoutUrl) !== false || strpos($redirectUrl, $userOidcLogoutUrl) !== false) { + if ( + $redirectUrl + && (strpos($redirectUrl, $logoutUrl) !== false || strpos($redirectUrl, $userOidcLogoutUrl) !== false) + ) { $redirectUrl = $urlGenerator->getBaseUrl(); } $targetUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.login', [ From 17d7e7f5d3daa91ff7859b5246b2bf171e0d057b Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Thu, 18 Jan 2024 15:26:06 +0100 Subject: [PATCH 5/5] be more defensive about redirecting to logout in login controller Signed-off-by: Julien Veyssier --- lib/Controller/LoginController.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index 8e84c3fd..13a793ba 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -209,6 +209,16 @@ private function buildProtocolErrorResponse(?bool $throttle = null): TemplateRes * @return DataDisplayResponse|RedirectResponse|TemplateResponse */ public function login(int $providerId, string $redirectUrl = null) { + // to be safe, avoid redirecting to logout or single-logout + $logoutUrl = $this->urlGenerator->linkToRoute('core.login.logout'); + $userOidcLogoutUrl = $this->urlGenerator->linkToRoute(Application::APP_ID . '.login.singleLogoutService'); + if ( + $redirectUrl + && (strpos($redirectUrl, $logoutUrl) !== false || strpos($redirectUrl, $userOidcLogoutUrl) !== false) + ) { + $redirectUrl = $this->urlGenerator->getBaseUrl(); + } + if ($this->userSession->isLoggedIn()) { return new RedirectResponse($redirectUrl); }