Merge pull request #791 from nextcloud/backport/788/stable-5.1

[stable-5.1] refactor(Controller): read parameter only once

Author: blizzz

.drone.yml

@@ -67,77 +67,7 @@ trigger:
     - pull_request
     - push
 
-type: docker
-
----
-kind: pipeline
-name: integration-tests-stable22
-
-clone:
-  depth: 1
-
-steps:
-  - name: integration-tests-stable22
-    image: ghcr.io/nextcloud/continuous-integration-user_saml_shibboleth-php7.3:latest
-    environment:
-      CORE_BRANCH: stable22
-    commands:
-      - /start.sh &
-      - /wait-for-services.sh
-      - rm -rf /var/www/html
-      - cd /var/www/
-      - git clone --depth 1 -b $CORE_BRANCH https://github.com/nextcloud/server html
-      - cd /var/www/html && git submodule update --init
-      # use local clone
-      - cp -r /drone/src /var/www/html/apps/user_saml
-      - scl enable rh-php73 "bash -c 'php /var/www/html/occ maintenance:install --database sqlite --admin-pass password; php /var/www/html/occ app:enable user_saml'"
-      - chown -R apache:apache /var/www/html/
-      - scl enable rh-php73  "bash -c 'cd /var/www/html/apps/user_saml/tests/integration && vendor/bin/behat'"
-
-trigger:
-  branch:
-    - master
-    - stable*
-  event:
-    - pull_request
-    - push
-
-type: docker
-
----
-kind: pipeline
-name: integration-tests-stable21
-
-clone:
-  depth: 1
-
-steps:
-  - name: integration-tests-stable21
-    image: ghcr.io/nextcloud/continuous-integration-user_saml_shibboleth-php7.3:latest
-    environment:
-      CORE_BRANCH: stable21
-    commands:
-      - /start.sh &
-      - /wait-for-services.sh
-      - rm -rf /var/www/html
-      - cd /var/www/
-      - git clone --depth 1 -b $CORE_BRANCH https://github.com/nextcloud/server html
-      - cd /var/www/html && git submodule update --init
-      # use local clone
-      - cp -r /drone/src /var/www/html/apps/user_saml
-      - scl enable rh-php73 "bash -c 'php /var/www/html/occ maintenance:install --database sqlite --admin-pass password; php /var/www/html/occ app:enable user_saml'"
-      - chown -R apache:apache /var/www/html/
-      - scl enable rh-php73  "bash -c 'cd /var/www/html/apps/user_saml/tests/integration && vendor/bin/behat'"
-
-trigger:
-  branch:
-    - master
-    - stable*
-  event:
-    - pull_request
-    - push
-
 type: docker
 ---
 kind: signature
-hmac: a52d842da41e201e20d05b1f9b79289f29d38347a02a0455e03223dc244856d5
+hmac: 68850e2cb40f63c6c75cd56498acc4706222cbd10c69ddf0d02f604fced2eb0e

.github/workflows/phpunit-sqlite.yml

@@ -44,7 +44,7 @@ jobs:
     strategy:
       matrix:
         php-versions: ['7.4', '8.0']
-        server-versions: ['stable21', 'stable22', 'stable23', 'stable24']
+        server-versions: ['stable23', 'stable24']
         include:
           - php-versions: '8.1'
             server-versions: 'stable24'

appinfo/info.xml

@@ -33,7 +33,7 @@ While theoretically any other authentication provider implementing either one of
 	<screenshot>https://raw.githubusercontent.com/nextcloud/user_saml/master/screenshots/1.png</screenshot>
 	<screenshot>https://raw.githubusercontent.com/nextcloud/user_saml/master/screenshots/2.png</screenshot>
 	<dependencies>
-		<nextcloud min-version="21" max-version="26" />
+		<nextcloud min-version="23" max-version="24" />
 	</dependencies>
 	<commands>
 		<command>OCA\User_SAML\Command\ConfigCreate</command>

lib/Controller/SAMLController.php

@@ -42,6 +42,7 @@
 use OCP\IURLGenerator;
 use OCP\IUserSession;
 use OCP\Security\ICrypto;
+use OCP\Security\ITrustedDomainHelper;
 use OneLogin\Saml2\Auth;
 use OneLogin\Saml2\Error;
 use OneLogin\Saml2\Settings;
@@ -74,6 +75,10 @@ class SAMLController extends Controller {
 	 * @var ICrypto
 	 */
 	private $crypto;
+	/**
+	 * @var ITrustedDomainHelper
+	 */
+	private $trustedDomainHelper;
 
 	/**
 	 * @param string $appName
@@ -100,7 +105,8 @@ public function __construct(
 		IL10N         $l,
 		UserResolver  $userResolver,
 		UserData      $userData,
-		ICrypto       $crypto
+		ICrypto       $crypto,
+		ITrustedDomainHelper $trustedDomainHelper
 	) {
 		parent::__construct($appName, $request);
 		$this->session = $session;
@@ -114,6 +120,7 @@ public function __construct(
 		$this->userResolver = $userResolver;
 		$this->userData = $userData;
 		$this->crypto = $crypto;
+		$this->trustedDomainHelper = $trustedDomainHelper;
 	}
 
 	/**
@@ -203,11 +210,17 @@ protected function assertGroupMemberships(): void {
 	 * @throws \Exception
 	 */
 	public function login(int $idp = 1) {
+		$originalUrl = (string)$this->request->getParam('originalUrl', '');
+		if (!$this->trustedDomainHelper->isTrustedUrl($originalUrl)) {
+			$originalUrl = '';
+		}
+
 		$type = $this->config->getAppValue($this->appName, 'type');
 		switch ($type) {
 			case 'saml':
 				$auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp));
-				$returnUrl = $this->request->getParam('originalUrl', $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login'));
+
+				$returnUrl = $originalUrl ?: $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login');
 				$ssoUrl = $auth->login($returnUrl, [], false, false, true);
 				$response = new Http\RedirectResponse($ssoUrl);
 
@@ -226,7 +239,7 @@ public function login(int $idp = 1) {
 				// Pack data as JSON so we can properly extract it later
 				$data = json_encode([
 					'AuthNRequestID' => $auth->getLastRequestID(),
-					'OriginalUrl' => $this->request->getParam('originalUrl', ''),
+					'OriginalUrl' => $originalUrl,
 					'Idp' => $idp,
 					'flow' => $flowData,
 				]);
@@ -240,7 +253,7 @@ public function login(int $idp = 1) {
 				$response->addCookie('saml_data', $data, null, 'None');
 				break;
 			case 'environment-variable':
-				$ssoUrl = $this->request->getParam('originalUrl', '');
+				$ssoUrl = $originalUrl;
 				if (empty($ssoUrl)) {
 					$ssoUrl = $this->urlGenerator->getAbsoluteURL('/');
 				}

tests/unit/Controller/SAMLControllerTest.php

@@ -39,6 +39,7 @@
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
+use OCP\Security\ITrustedDomainHelper;
 use PHPUnit\Framework\MockObject\MockObject;
 use OCP\Security\ICrypto;
 use Test\TestCase;
@@ -70,6 +71,8 @@ class SAMLControllerTest extends TestCase {
 	private $crypto;
 	/** @var SAMLController */
 	private $samlController;
+	/** @var ITrustedDomainHelper|MockObject */
+	private $trustedDomainController;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -86,6 +89,7 @@ protected function setUp(): void {
 		$this->userResolver = $this->createMock(UserResolver::class);
 		$this->userData = $this->createMock(UserData::class);
 		$this->crypto = $this->createMock(ICrypto::class);
+		$this->trustedDomainController = $this->createMock(ITrustedDomainHelper::class);
 
 		$this->l->expects($this->any())->method('t')->willReturnCallback(
 			function ($param) {
@@ -111,7 +115,8 @@ function ($param) {
 			$this->l,
 			$this->userResolver,
 			$this->userData,
-			$this->crypto
+			$this->crypto,
+			$this->trustedDomainController
 		);
 	}