Merge pull request #195 from owncloud/fix/openid-public-link-upload

fix: only register the alternative login method on login page and GET…
owncloud · Dec 13, 2021 · c34e878 · c34e878
2 parents 45393e4 + 4105de1
commit c34e878
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 18 deletions.
diff --git a/lib/LoginPageBehaviour.php b/lib/LoginPageBehaviour.php
@@ -53,6 +53,17 @@ public function handleLoginPageBehaviour(array $openIdConfig): void {
 		if ($this->userSession->isLoggedIn()) {
 			return;
 		}
+		# only GET requests are of interest
+		if ($this->request->getMethod() !== 'GET') {
+			return;
+		}
+		# only requests on the login page are of interest
+		$components = \parse_url($this->request->getRequestUri());
+		/** @phan-suppress-next-line PhanTypePossiblyInvalidDimOffset */
+		$uri = $components['path'];
+		if (\substr($uri, -6) !== '/login') {
+			return;
+		}
 
 		// register alternative login
 		$loginName = $openIdConfig['loginButtonName'] ?? 'OpenID Connect';
@@ -63,15 +74,11 @@ public function handleLoginPageBehaviour(array $openIdConfig): void {
 		if (!$autoRedirectOnLoginPage) {
 			return;
 		}
-		$components = \parse_url($this->request->getRequestUri());
-		/** @phan-suppress-next-line PhanTypePossiblyInvalidDimOffset */
-		$uri = $components['path'];
-		if (\substr($uri, -6) === '/login') {
-			$req = $this->request->getRequestUri();
-			$this->logger->debug("Redirecting to IdP - request url: $req");
-			$loginUrl = $this->urlGenerator->linkToRoute('openidconnect.loginFlow.login', $this->request->getParams());
-			$this->redirect($loginUrl);
-		}
+
+		$req = $this->request->getRequestUri();
+		$this->logger->debug("Redirecting to IdP - request url: $req");
+		$loginUrl = $this->urlGenerator->linkToRoute('openidconnect.loginFlow.login', $this->request->getParams());
+		$this->redirect($loginUrl);
 	}
 
 	/**

diff --git a/tests/unit/LoginPageBehaviourTest.php b/tests/unit/LoginPageBehaviourTest.php
@@ -32,10 +32,6 @@
 
 class LoginPageBehaviourTest extends TestCase {
 
-	/**
-	 * @var MockObject | Logger
-	 */
-	private $logger;
 	/**
 	 * @var MockObject | LoginPageBehaviour
 	 */
@@ -55,14 +51,14 @@ class LoginPageBehaviourTest extends TestCase {
 
 	protected function setUp(): void {
 		parent::setUp();
-		$this->logger = $this->createMock(Logger::class);
+		$logger = $this->createMock(Logger::class);
 		$this->userSession = $this->createMock(IUserSession::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->request = $this->createMock(IRequest::class);
 
 		$this->loginPageBehaviour = $this->getMockBuilder(LoginPageBehaviour::class)
-			->setConstructorArgs([$this->logger, $this->userSession, $this->urlGenerator, $this->request])
-			->setMethods(['registerAlternativeLogin', 'redirect'])
+			->setConstructorArgs([$logger, $this->userSession, $this->urlGenerator, $this->request])
+			->onlyMethods(['registerAlternativeLogin', 'redirect'])
 			->getMock();
 	}
 
@@ -74,13 +70,15 @@ public function testLoggedIn(): void {
 
 	public function testNotLoggedInNoAutoRedirect(): void {
 		$this->userSession->method('isLoggedIn')->willReturn(false);
-		$this->request->expects(self::never())->method('getRequestUri');
+		$this->request->expects(self::once())->method('getMethod')->willReturn('GET');
+		$this->request->method('getRequestUri')->willReturn('https://example.com/login');
 		$this->loginPageBehaviour->expects(self::once())->method('registerAlternativeLogin')->with('foo');
 		$this->loginPageBehaviour->handleLoginPageBehaviour(['loginButtonName' => 'foo']);
 	}
 
 	public function testNotLoggedInAutoRedirect(): void {
 		$this->userSession->method('isLoggedIn')->willReturn(false);
+		$this->request->method('getMethod')->willReturn('GET');
 		$this->request->method('getRequestUri')->willReturn('https://example.com/login');
 		$this->urlGenerator->method('linkToRoute')->willReturn('https://example.com/openid/redirect');
 		$this->loginPageBehaviour->expects(self::once())->method('registerAlternativeLogin')->with('OpenID Connect');
@@ -90,9 +88,20 @@ public function testNotLoggedInAutoRedirect(): void {
 
 	public function testNotLoggedInAutoRedirectNoLoginPage(): void {
 		$this->userSession->method('isLoggedIn')->willReturn(false);
+		$this->request->method('getMethod')->willReturn('GET');
 		$this->request->method('getRequestUri')->willReturn('https://example.com/apps/files');
 		$this->urlGenerator->method('linkToRoute')->willReturn('https://example.com/openid/redirect');
-		$this->loginPageBehaviour->expects(self::once())->method('registerAlternativeLogin')->with('OpenID Connect');
+		$this->loginPageBehaviour->expects(self::never())->method('registerAlternativeLogin')->with('OpenID Connect');
+		$this->loginPageBehaviour->expects(self::never())->method('redirect')->with('https://example.com/openid/redirect');
+		$this->loginPageBehaviour->handleLoginPageBehaviour(['autoRedirectOnLoginPage' => true]);
+	}
+
+	public function testPUT(): void {
+		$this->userSession->method('isLoggedIn')->willReturn(false);
+		$this->request->method('getMethod')->willReturn('PUT');
+		$this->request->method('getRequestUri')->willReturn('https://example.com/apps/files');
+		$this->urlGenerator->method('linkToRoute')->willReturn('https://example.com/openid/redirect');
+		$this->loginPageBehaviour->expects(self::never())->method('registerAlternativeLogin')->with('OpenID Connect');
 		$this->loginPageBehaviour->expects(self::never())->method('redirect')->with('https://example.com/openid/redirect');
 		$this->loginPageBehaviour->handleLoginPageBehaviour(['autoRedirectOnLoginPage' => true]);
 	}